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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 5 May 2011 16:16:14 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Michał Mirosław <mirqus@...il.com>
Cc:	Matt Carlson <mcarlson@...adcom.com>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Michael Chan <mchan@...adcom.com>,
	Tom Herbert <therbert@...gle.com>
Subject: Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

On Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@...il.com> wrote:
> W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar
> <maheshb@...gle.com> napisał:
>> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@...il.com> wrote:
>>> 2011/5/5 Mahesh Bandewar <maheshb@...gle.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.

>>> 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).

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?


>>> BTW, similar problems (also like in previous versions) are in
>>> forcedeth implementation.
>> Yes, whatever we decide about the state of the wanted_features, I'll
>> implement similarly for forcedeth. Which previous problem you are
>> referring to? Is it the return value? There is a different kind of
>> failure (error while writing the register). Since update_features()
>> cant handle return value, I'm ignoring the return value. As far as the
>> correct return code is concerned, I wasn't sure what is appropriate
>> return code here (may be PHY_ERROR would be appropriate there) but
>> again I could ignore it just the way rest of the code is ignoring.
>
> Let's wait for this threads points to be resolved. You will probably
> change the forcedeth's implementation then anyway. :-)
>
agreed.

Thanks,
--mahesh..
> Best Regards,
> Michał Mirosław
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html