[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 5 May 2011 10:47:10 -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 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.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@...gle.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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
Powered by blists - more mailing lists