mcs7830: Fix link state detection
Subsystems (3)
PATCH 1/1 mcs7830: Fix link state detection
The device had an undocumented "feature": it can provide a sequence of spurious link-down status data even if the link is up all the time. A sequence of 10 was seen so update the link state only after the device reports the same link state 20 times.
--- drivers/net/usb/mcs7830.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
drivers/net/usb/mcs7830.c +1 -0
@@ -117,6 +117,7 @@ enum {struct mcs7830_data {u8 multi_filter[8 ];u8 config; +u8 link_counter; };static const char driver_name[] ="MOSCHIP usb-ethernet driver" ;
@@ -633,19 +634,25 @@ static void mcs7830_status(struct usbnet *dev, struct
urb *urb)
{
u8 *buf = urb->transfer_buffer;
bool link;
+ struct mcs7830_data *data = mcs7830_get_data(dev);
if (urb->actual_length < 16)
return;
link = !(buf[1] & 0x20);
if (netif_carrier_ok(dev->net) != link) {
- if (link) {
- netif_carrier_on(dev->net);
- usbnet_defer_kevent(dev, EVENT_LINK_RESET);
- } else
- netif_carrier_off(dev->net);
- netdev_dbg(dev->net, "Link Status is: %d\n", link);
- }
+ data->link_counter++;
+ if (data->link_counter > 20) {
+ data->link_counter = 0;
+ if (link) {
+ netif_carrier_on(dev->net);
+ usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+ } else
+ netif_carrier_off(dev->net);
+ netdev_dbg(dev->net, "Link Status is: %d\n", link);
+ }
+ } else
+ data->link_counter = 0;
}
static const struct driver_info moschip_info = {
Signature · 1 line
Ondrej Zary
Hi,
Review
Quoted 6 lines · Ondrej Zary
(quoted diff) +1
-0u8 *buf = urb->transfer_buffer; bool link; + struct mcs7830_data *data = mcs7830_get_data(dev); if (urb->actual_length < 16) return;
Alternatively could do *data = NULL; and then actually assign after the conditional. But since the conditional most likely is coldpath
I think your chosen implementation is best.
Review
Quoted 3 lines
(quoted diff) +0
-0link = !(buf[1] & 0x20); if (netif_carrier_ok(dev->net) != link) {
I usually like to introduce helper bools to clearly express the intention behind things (source code should be readable like a book, yet it all too often is everything but...).
I.e.
bool link_state_change_detected = (netif_carrier_ok......); if (bool)
Might be an idea here, too.
The logic behind your counter implementation seems solid to me (short explanatory comment "track link state several times, to guard against transient erroneous link state of (some versions of?) this chip" might have been useful though).
Thank you very much for your patch!
Andreas Mohr
David Miller
maintainerFrom: Ondrej Zary <[email protected]>
Date: Thu, 11 Oct 2012 12:33:28 +0200
Quoted 8 lines
The device had an undocumented "feature": it can provide a sequence of spurious link-down status data even if the link is up all the time. A sequence of 10 was seen so update the link state only after the device reports the same link state 20 times.
Signed-off-by Ondrej Zary <[email protected]>Reported-by Michael Leun <[email protected]>Tested-by Michael Leun <[email protected]>
You've been given feedback on this patch, and also it doesn't apply cleanly. Perhaps it was corrupted by your email client.