| Message ID | 1464228407.5421.34.camel@googlemail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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.
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?
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?
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.
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 */
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(+)