W dniu 1 września 2011 10:37 użytkownik Vladislav Zolotarov
[off-list ref] napisał:

quoted

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-
owner@vger.kernel.org] On Behalf Of Michal Miroslaw
Sent: Wednesday, August 31, 2011 9:05 PM
To: Vladislav Zolotarov
Cc: Michal Schmidt; netdev@vger.kernel.org; Dmitry Kravkov; Eilon
Greenstein
Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

2011/8/31 Vladislav Zolotarov [off-list ref]:

quoted

quoted

-----Original Message-----
From: Michal Schmidt [mailto:mschmidt@redhat.com]
Sent: Wednesday, August 31, 2011 6:59 PM
To: Vladislav Zolotarov
Cc: netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein;
mirqus@gmail.com
Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:

quoted

On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:

quoted

  if (bnx2x_reload) {
-         if (bp->recovery_state == BNX2X_RECOVERY_DONE)
+         if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
+                 /*
+                  * Cheat! Normally dev->features will be
set after we
+                  * return, but that's too late. We need to
know how to
+                  * configure the NIC when reloading it, so
update
+                  * the features early.
+                  */
+                 dev->features = features;
                  return bnx2x_reload_if_running(dev);

NACK

This is bogus - what if bnx2x_reload_if_running(dev)
(bnx2x_nic_load()) failes? The original dev->features would be
lost...

Well, yes, but since the NIC would be now not working, do we really
care about its features? :-)

U r kidding, right? ;)
We care about the consistency in the netdev features state - if we

failed

quoted

to configure the requested feature and returned an error on e.g.

"ethtool -K ethX lro on"

quoted

call, it's expected that a subsequent ethtool -k ethX call won't

report the requested

quoted

feature (LRO) as set.

If bnx2x_reload_if_running() failure means that NIC is disabled, then
Michal is right that there's no point in caring about dev->features,
sice 'load' operation (NIC configuration) needs to be done again
anyway.

Michal, it's a matter of a consistent semantics/behavior of the ethtool callbacks just as I described above.
As long as dev->features may be queried both when device is down I'm afraid I can't agree with u.

If I understand correctly, bnx2x_reload_if_running() failure does not
mean exactly that features change failed. It means that device failed
to initialize, possibly because of other (transient?) conditions. So
unless reverting features change and retrying the initialization is
known to allow the device to be brought back, there's no difference
whether old or new dev->features value is kept and which set is
reported by ethtool.

Best Regards,
Michał Mirosław