Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

["Michał Mirosław" <mirqus@gmail.com>]

W dniu 6 maja 2011 01:16 użytkownik Mahesh Bandewar
<maheshb@google.com> napisał:
> On Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@gmail.com> wrote:
>> W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar
>> <maheshb@google.com> napisał:
>>> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>>>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>:
>>>>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>>>>> capability is added for the devices which support internal MAC loopback mode.
>>>>> So when enabled, it enables internal-MAC loopback.
>> [...]
>>>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>>>>>
>>>>>        netif_tx_start_all_queues(dev);
>>>>>
>>>>> +       /*
>>>>> +        * Reset loopback feature if it was turned on while the device was down
>>>>> +        * to avoid and any discrepancy in features reporting.
>>>>> +        */
>>>>> +       if (dev->features & NETIF_F_LOOPBACK) {
>>>>> +               dev->features &= ~NETIF_F_LOOPBACK;
>>>>> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
>>>>> +       }
>>>>> +
>>>>
>>>> if (dev->features & NETIF_F_LOOPBACK)
>>>>  tg3_set_loopback(dev, dev->features);
>>>>
>>> Unfortunately at this stage device will not be able to set-loopback so
>>> I resorted to clearing the bit(s).
>> ndo_fix_features+ndo_set_features might be caled before ndo_open - the
>> first might be just from register_netdev() so even before ndo_open
>> callback.
> ndo_open is the triggering event, so anything before that point does
> not really help.

You're right - I forgot that netif_running() becomes true just before
ndo_open is called. I have a hard time to convince myself that you
can't program the hardware for loopback just at the end of ndo_open
callback.

>>>> Whatever you do, don't modify wanted_features in drivers.
>>> Since just clearing the 'features' would leave wanted_features in
>>> discrepant state, I thought this will bring it to a sane state. So
>>> what is a preferred way?
>> If you really can't do other way, driver should keep additional state
>> that is checked in ndo_fix_features callback.
> ndo_fix_features callback is called just before calling
> ndo_set_features callback. Also this is called during update_features,
> so having the state maintained is not really going to help in this
> scenario since that is not happening when device reaches ready state
> (to remove this discrepancy in the feature-set).

You can call netdev_update_features() at the end of ndo_open. But that
would be the same in the end as just enabling the loopback there.

> There must be a reason (that you did not explain!) why driver code
> should not alter wanted_feature set. So in this scenario, just leaving
> the wanted_features as it is and clearing the features bit is correct
> / enough?

wanted_features reflects a set of features the user wants. I assume
that no code can change the user himself, so changing wanted_features
instead would break this connection. You can view wanted_features as a
cache of user's intention. ;-)

Best Regards,
Michał Mirosław