Patchwork mcs7830: Fix link state detection

login
register
mail settings
Submitter Ondrej Zary
Date Oct. 11, 2012, 10:33 a.m.
Message ID <201210111233.28652.linux@rainbow-software.org>
Download mbox | patch
Permalink /patch/190867/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Ondrej Zary - Oct. 11, 2012, 10:33 a.m.
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 <linux@rainbow-software.org>
Reported-by: Michael Leun <lkml20120218@newton.leun.net>
Tested-by: Michael Leun <lkml20120218@newton.leun.net>
---
 drivers/net/usb/mcs7830.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

 static const struct driver_info moschip_info = {
Andreas Mohr - Oct. 11, 2012, 6:37 p.m.
Hi,

On Thu, Oct 11, 2012 at 12:33:28PM +0200, Ondrej Zary wrote:
>  	u8 *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.


>  
>  	link = !(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
--
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
David Miller - Oct. 11, 2012, 7:20 p.m.
From: Ondrej Zary <linux@rainbow-software.org>
Date: Thu, 11 Oct 2012 12:33:28 +0200

> 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 <linux@rainbow-software.org>
> Reported-by: Michael Leun <lkml20120218@newton.leun.net>
> Tested-by: Michael Leun <lkml20120218@newton.leun.net>

You've been given feedback on this patch, and also it doesn't apply
cleanly.  Perhaps it was corrupted by your email client.
--
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

Patch

diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index 03c2d8d..3a02a5d 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -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;
 }