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

[Mahesh Bandewar <maheshb@google.com>]

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.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>> Changes since v3:
>>  (a) Corrected the condition (|| => &&) where loopback capability is added.
>>  (b) set_features() always returns 0.
>>  (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.
>>
>> Changes since v2:
>>  Implemtned Joe Perches's style change.
>>
>> Changes since v1:
>>  Implemented Matt Carlson's suggestions.
>>  (a) Enable this capability on the devices which are capable of MAC-loopback
>>      mode.
>>  (b) check if the device is running before making changes.
>>  (c) check bits before making changes.
>>
>>  drivers/net/tg3.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 69 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index 7c7c9a8..7f7a290 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
>> @@ -6309,6 +6309,44 @@ dma_error:
>>        return NETDEV_TX_OK;
>>  }
>>
>> +static int tg3_set_loopback(struct net_device *dev, u32 features)
>> +{
>> +       struct tg3 *tp = netdev_priv(dev);
>> +       u32 cur_mode = 0;
>> +       int retval = 0;
>
> void? You always return zero and don't check it in callers.
>
> [...]
>> @@ -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).

> 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?

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

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