usbnet: smsc95xx: fix link detection for disabled autonegotiation

usbnet: smsc95xx: fix link detection for disabled autonegotiation

[Christoph Fritz]

To detect link status up/down for connections where autonegotiation is
explicitly disabled, we don't get an irq but need to poll the status
register for link up/down detection.
This patch adds a workqueue to poll for link status.

Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
---
 drivers/net/usb/smsc95xx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index d9d2806..dc989a8 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -61,6 +61,8 @@
 #define SUSPEND_ALLMODES		(SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
 					 SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
 
+#define CARRIER_CHECK_DELAY (2 * HZ)
+
 struct smsc95xx_priv {
 	u32 mac_cr;
 	u32 hash_hi;
@@ -69,6 +71,9 @@ struct smsc95xx_priv {
 	spinlock_t mac_cr_lock;
 	u8 features;
 	u8 suspend_flags;
+	bool link_ok;
+	struct delayed_work carrier_check;
+	struct usbnet *dev;
 };
 
 static bool turbo_mode = true;
@@ -624,6 +629,44 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
 			    intdata);
 }
 
+static void set_carrier(struct usbnet *dev, bool link)
+{
+	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+
+	if (pdata->link_ok == link)
+		return;
+
+	pdata->link_ok = link;
+
+	if (link)
+		usbnet_link_change(dev, 1, 0);
+	else
+		usbnet_link_change(dev, 0, 0);
+}
+
+static void check_carrier(struct work_struct *work)
+{
+	struct smsc95xx_priv *pdata = container_of(work, struct smsc95xx_priv,
+						carrier_check.work);
+	struct usbnet *dev = pdata->dev;
+	int ret;
+
+	if (pdata->suspend_flags != 0)
+		return;
+
+	ret = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, MII_BMSR);
+	if (ret < 0) {
+		netdev_warn(dev->net, "Failed to read MII_BMSR\n");
+		return;
+	}
+	if (ret & BMSR_LSTATUS)
+		set_carrier(dev, 1);
+	else
+		set_carrier(dev, 0);
+
+	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);
+}
+
 /* Enable or disable Tx & Rx checksum offload engines */
 static int smsc95xx_set_features(struct net_device *netdev,
 	netdev_features_t features)
@@ -1165,13 +1208,20 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->net->flags |= IFF_MULTICAST;
 	dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM;
 	dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
+
+	pdata->dev = dev;
+	INIT_DELAYED_WORK(&pdata->carrier_check, check_carrier);
+	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);
+
 	return 0;
 }
 
 static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+
 	if (pdata) {
+		cancel_delayed_work(&pdata->carrier_check);
 		netif_dbg(dev, ifdown, dev->net, "free pdata\n");
 		kfree(pdata);
 		pdata = NULL;
@@ -1695,6 +1745,7 @@ static int smsc95xx_resume(struct usb_interface *intf)
 
 	/* do this first to ensure it's cleared even in error case */
 	pdata->suspend_flags = 0;
+	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);
 
 	if (suspend_flags & SUSPEND_ALLMODES) {
 		/* clear wake-up sources */
-- 
2.1.4

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[Andrew Lunn]

On Thu, May 26, 2016 at 04:06:47AM +0200, Christoph Fritz wrote:
> To detect link status up/down for connections where autonegotiation is
> explicitly disabled, we don't get an irq but need to poll the status
> register for link up/down detection.
> This patch adds a workqueue to poll for link status.

Did you consider using the phylib? It probably does the needed polling
already, and it looks like the functions needed to implement an MDIO
bus are already in place.

    Andrew

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[Christoph Fritz]

On Thu, 2016-05-26 at 04:31 +0200, Andrew Lunn wrote:
> On Thu, May 26, 2016 at 04:06:47AM +0200, Christoph Fritz wrote:
> > To detect link status up/down for connections where autonegotiation is
> > explicitly disabled, we don't get an irq but need to poll the status
> > register for link up/down detection.
> > This patch adds a workqueue to poll for link status.
> 
> Did you consider using the phylib? It probably does the needed polling
> already, and it looks like the functions needed to implement an MDIO
> bus are already in place.

smsc95xx supports a relative wide range of PHYs which I don't have
access to in regard of testing. So I prefer the least invasive one (with
this patch) as mostly all of the other usbnet drivers do.

A merge to phylib while paying attention to all the suspend modes and
testing the wide range of PHYs would surely be the right thing to do.

Any thoughts on that?

Thanks
  -- Christoph

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[Florian Fainelli]

On 05/26/2016 04:01 AM, Christoph Fritz wrote:
> On Thu, 2016-05-26 at 04:31 +0200, Andrew Lunn wrote:
>> On Thu, May 26, 2016 at 04:06:47AM +0200, Christoph Fritz wrote:
>>> To detect link status up/down for connections where autonegotiation is
>>> explicitly disabled, we don't get an irq but need to poll the status
>>> register for link up/down detection.
>>> This patch adds a workqueue to poll for link status.
>>
>> Did you consider using the phylib? It probably does the needed polling
>> already, and it looks like the functions needed to implement an MDIO
>> bus are already in place.
> 
> smsc95xx supports a relative wide range of PHYs which I don't have
> access to in regard of testing. So I prefer the least invasive one (with
> this patch) as mostly all of the other usbnet drivers do.

My reading of the driver is that it only supports its internal PHY, so
it should be pretty straightforward to extend drivers/net/phy/smsc.c to
support it?

> 
> A merge to phylib while paying attention to all the suspend modes and
> testing the wide range of PHYs would surely be the right thing to do.

Yes, the suspend stuff could be a little tricky, but not impossible, the
microchip lan78xx is an user of PHYLIB and it seems to work okay.
-- 
Florian

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[David Miller]

From: Christoph Fritz <chf.fritz@googlemail.com>
Date: Thu, 26 May 2016 04:06:47 +0200

> @@ -1695,6 +1745,7 @@ static int smsc95xx_resume(struct usb_interface *intf)
>  
>  	/* do this first to ensure it's cleared even in error case */
>  	pdata->suspend_flags = 0;
> +	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);

Why are you not cancelling this delayed work in the suspend routine of
the driver?

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[Christoph Fritz]

On Sun, 2016-05-29 at 22:30 -0700, David Miller wrote:
> From: Christoph Fritz <chf.fritz@googlemail.com>
> Date: Thu, 26 May 2016 04:06:47 +0200
> 
> > @@ -1695,6 +1745,7 @@ static int smsc95xx_resume(struct usb_interface *intf)
> >  
> >  	/* do this first to ensure it's cleared even in error case */
> >  	pdata->suspend_flags = 0;
> > +	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);
> 
> Why are you not cancelling this delayed work in the suspend routine of
> the driver?

I'm doing this:
+       if (pdata->suspend_flags != 0)
+               return;

inside the "worker-function" so that schedule_delayed_work() is not
called again.

Should I explicitly cancel_delayed_work() inside suspend() too?

Re: usbnet: smsc95xx: fix link detection for disabled autonegotiation

[David Miller]

From: Christoph Fritz <chf.fritz@googlemail.com>
Date: Mon, 30 May 2016 22:07:42 +0200

> On Sun, 2016-05-29 at 22:30 -0700, David Miller wrote:
>> From: Christoph Fritz <chf.fritz@googlemail.com>
>> Date: Thu, 26 May 2016 04:06:47 +0200
>> 
>> > @@ -1695,6 +1745,7 @@ static int smsc95xx_resume(struct usb_interface *intf)
>> >  
>> >  	/* do this first to ensure it's cleared even in error case */
>> >  	pdata->suspend_flags = 0;
>> > +	schedule_delayed_work(&pdata->carrier_check, CARRIER_CHECK_DELAY);
>> 
>> Why are you not cancelling this delayed work in the suspend routine of
>> the driver?
> 
> I'm doing this:
> +       if (pdata->suspend_flags != 0)
> +               return;
> 
> inside the "worker-function" so that schedule_delayed_work() is not
> called again.

Aha, I didn't catch that.

Applied, thanks.