[PATCH 0/2] smsc75xx enhancements

[PATCH 0/2] smsc75xx enhancements

[Steve Glendinning]

This patchset implements wake on PHY (link up or link down) for
smsc75xx, please consider for net-next.

Steve Glendinning (2):
  smsc75xx: refactor entering suspend modes
  smsc75xx: support PHY wakeup source

 drivers/net/usb/smsc75xx.c |  224 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 186 insertions(+), 38 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

This patch splits out the logic for entering suspend modes
to separate functions, to reduce the complexity of the
smsc75xx_suspend function.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc75xx.c |   62 +++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 953c4f4..4655c01 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -1213,6 +1213,42 @@ static int smsc75xx_write_wuff(struct usbnet *dev, int filter, u32 wuf_cfg,
 	return 0;
 }
 
+static int smsc75xx_enter_suspend0(struct usbnet *dev)
+{
+	u32 val;
+	int ret;
+
+	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
+	check_warn_return(ret, "Error reading PMT_CTL\n");
+
+	val &= (~(PMT_CTL_SUS_MODE | PMT_CTL_PHY_RST));
+	val |= PMT_CTL_SUS_MODE_0 | PMT_CTL_WOL_EN | PMT_CTL_WUPS;
+
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
+	check_warn_return(ret, "Error writing PMT_CTL\n");
+
+	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
+
+	return 0;
+}
+
+static int smsc75xx_enter_suspend2(struct usbnet *dev)
+{
+	u32 val;
+	int ret;
+
+	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
+	check_warn_return(ret, "Error reading PMT_CTL\n");
+
+	val &= ~(PMT_CTL_SUS_MODE | PMT_CTL_WUPS | PMT_CTL_PHY_RST);
+	val |= PMT_CTL_SUS_MODE_2;
+
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
+	check_warn_return(ret, "Error writing PMT_CTL\n");
+
+	return 0;
+}
+
 static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct usbnet *dev = usb_get_intfdata(intf);
@@ -1244,17 +1280,7 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL\n");
 
-		/* enter suspend2 mode */
-		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
-		check_warn_return(ret, "Error reading PMT_CTL\n");
-
-		val &= ~(PMT_CTL_SUS_MODE | PMT_CTL_WUPS | PMT_CTL_PHY_RST);
-		val |= PMT_CTL_SUS_MODE_2;
-
-		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
-		check_warn_return(ret, "Error writing PMT_CTL\n");
-
-		return 0;
+		return smsc75xx_enter_suspend2(dev);
 	}
 
 	if (pdata->wolopts & (WAKE_MCAST | WAKE_ARP)) {
@@ -1368,19 +1394,7 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 
 	/* some wol options are enabled, so enter SUSPEND0 */
 	netdev_info(dev->net, "entering SUSPEND0 mode\n");
-
-	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
-	check_warn_return(ret, "Error reading PMT_CTL\n");
-
-	val &= (~(PMT_CTL_SUS_MODE | PMT_CTL_PHY_RST));
-	val |= PMT_CTL_SUS_MODE_0 | PMT_CTL_WOL_EN | PMT_CTL_WUPS;
-
-	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
-	check_warn_return(ret, "Error writing PMT_CTL\n");
-
-	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
-
-	return 0;
+	return smsc75xx_enter_suspend0(dev);
 }
 
 static int smsc75xx_resume(struct usb_interface *intf)
-- 
1.7.10.4

[PATCH 2/2] smsc75xx: support PHY wakeup source

[Steve Glendinning]

This patch enables LAN7500 family devices to wake from suspend
on either link up or link down events.

It also adds _nopm versions of mdio access functions, so we can
safely call them from suspend and resume functions

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc75xx.c |  168 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 151 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 4655c01..8f92d81 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -54,7 +54,7 @@
 #define USB_PRODUCT_ID_LAN7500		(0x7500)
 #define USB_PRODUCT_ID_LAN7505		(0x7505)
 #define RXW_PADDING			2
-#define SUPPORTED_WAKE			(WAKE_UCAST | WAKE_BCAST | \
+#define SUPPORTED_WAKE			(WAKE_PHY | WAKE_UCAST | WAKE_BCAST | \
 					 WAKE_MCAST | WAKE_ARP | WAKE_MAGIC)
 
 #define check_warn(ret, fmt, args...) \
@@ -185,14 +185,15 @@ static int smsc75xx_clear_feature(struct usbnet *dev, u32 feature)
 
 /* Loop until the read is completed with timeout
  * called with phy_mutex held */
-static int smsc75xx_phy_wait_not_busy(struct usbnet *dev)
+static __must_check int __smsc75xx_phy_wait_not_busy(struct usbnet *dev,
+						     int in_pm)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
 	int ret;
 
 	do {
-		ret = smsc75xx_read_reg(dev, MII_ACCESS, &val);
+		ret = __smsc75xx_read_reg(dev, MII_ACCESS, &val, in_pm);
 		check_warn_return(ret, "Error reading MII_ACCESS\n");
 
 		if (!(val & MII_ACCESS_BUSY))
@@ -202,7 +203,8 @@ static int smsc75xx_phy_wait_not_busy(struct usbnet *dev)
 	return -EIO;
 }
 
-static int smsc75xx_mdio_read(struct net_device *netdev, int phy_id, int idx)
+static int __smsc75xx_mdio_read(struct net_device *netdev, int phy_id, int idx,
+				int in_pm)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	u32 val, addr;
@@ -211,7 +213,7 @@ static int smsc75xx_mdio_read(struct net_device *netdev, int phy_id, int idx)
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = smsc75xx_phy_wait_not_busy(dev);
+	ret = __smsc75xx_phy_wait_not_busy(dev, in_pm);
 	check_warn_goto_done(ret, "MII is busy in smsc75xx_mdio_read\n");
 
 	/* set the address, index & direction (read from PHY) */
@@ -220,13 +222,13 @@ static int smsc75xx_mdio_read(struct net_device *netdev, int phy_id, int idx)
 	addr = ((phy_id << MII_ACCESS_PHY_ADDR_SHIFT) & MII_ACCESS_PHY_ADDR)
 		| ((idx << MII_ACCESS_REG_ADDR_SHIFT) & MII_ACCESS_REG_ADDR)
 		| MII_ACCESS_READ | MII_ACCESS_BUSY;
-	ret = smsc75xx_write_reg(dev, MII_ACCESS, addr);
+	ret = __smsc75xx_write_reg(dev, MII_ACCESS, addr, in_pm);
 	check_warn_goto_done(ret, "Error writing MII_ACCESS\n");
 
-	ret = smsc75xx_phy_wait_not_busy(dev);
+	ret = __smsc75xx_phy_wait_not_busy(dev, in_pm);
 	check_warn_goto_done(ret, "Timed out reading MII reg %02X\n", idx);
 
-	ret = smsc75xx_read_reg(dev, MII_DATA, &val);
+	ret = __smsc75xx_read_reg(dev, MII_DATA, &val, in_pm);
 	check_warn_goto_done(ret, "Error reading MII_DATA\n");
 
 	ret = (u16)(val & 0xFFFF);
@@ -236,8 +238,8 @@ done:
 	return ret;
 }
 
-static void smsc75xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
-				int regval)
+static void __smsc75xx_mdio_write(struct net_device *netdev, int phy_id,
+				  int idx, int regval, int in_pm)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	u32 val, addr;
@@ -246,11 +248,11 @@ static void smsc75xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = smsc75xx_phy_wait_not_busy(dev);
+	ret = __smsc75xx_phy_wait_not_busy(dev, in_pm);
 	check_warn_goto_done(ret, "MII is busy in smsc75xx_mdio_write\n");
 
 	val = regval;
-	ret = smsc75xx_write_reg(dev, MII_DATA, val);
+	ret = __smsc75xx_write_reg(dev, MII_DATA, val, in_pm);
 	check_warn_goto_done(ret, "Error writing MII_DATA\n");
 
 	/* set the address, index & direction (write to PHY) */
@@ -259,16 +261,39 @@ static void smsc75xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
 	addr = ((phy_id << MII_ACCESS_PHY_ADDR_SHIFT) & MII_ACCESS_PHY_ADDR)
 		| ((idx << MII_ACCESS_REG_ADDR_SHIFT) & MII_ACCESS_REG_ADDR)
 		| MII_ACCESS_WRITE | MII_ACCESS_BUSY;
-	ret = smsc75xx_write_reg(dev, MII_ACCESS, addr);
+	ret = __smsc75xx_write_reg(dev, MII_ACCESS, addr, in_pm);
 	check_warn_goto_done(ret, "Error writing MII_ACCESS\n");
 
-	ret = smsc75xx_phy_wait_not_busy(dev);
+	ret = __smsc75xx_phy_wait_not_busy(dev, in_pm);
 	check_warn_goto_done(ret, "Timed out writing MII reg %02X\n", idx);
 
 done:
 	mutex_unlock(&dev->phy_mutex);
 }
 
+static int smsc75xx_mdio_read_nopm(struct net_device *netdev, int phy_id,
+				   int idx)
+{
+	return __smsc75xx_mdio_read(netdev, phy_id, idx, 1);
+}
+
+static void smsc75xx_mdio_write_nopm(struct net_device *netdev, int phy_id,
+				     int idx, int regval)
+{
+	__smsc75xx_mdio_write(netdev, phy_id, idx, regval, 1);
+}
+
+static int smsc75xx_mdio_read(struct net_device *netdev, int phy_id, int idx)
+{
+	return __smsc75xx_mdio_read(netdev, phy_id, idx, 0);
+}
+
+static void smsc75xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
+				int regval)
+{
+	__smsc75xx_mdio_write(netdev, phy_id, idx, regval, 0);
+}
+
 static int smsc75xx_wait_eeprom(struct usbnet *dev)
 {
 	unsigned long start_time = jiffies;
@@ -1232,6 +1257,32 @@ static int smsc75xx_enter_suspend0(struct usbnet *dev)
 	return 0;
 }
 
+static int smsc75xx_enter_suspend1(struct usbnet *dev)
+{
+	u32 val;
+	int ret;
+
+	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
+	check_warn_return(ret, "Error reading PMT_CTL");
+
+	val &= ~(PMT_CTL_SUS_MODE | PMT_CTL_WUPS | PMT_CTL_PHY_RST);
+	val |= PMT_CTL_SUS_MODE_1;
+
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
+	check_warn_return(ret, "Error writing PMT_CTL");
+
+	/* clear wol status, enable energy detection */
+	val &= ~PMT_CTL_WUPS;
+	val |= (PMT_CTL_WUPS_ED | PMT_CTL_ED_EN);
+
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
+	check_warn_return(ret, "Error writing PMT_CTL");
+
+	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
+
+	return 0;
+}
+
 static int smsc75xx_enter_suspend2(struct usbnet *dev)
 {
 	u32 val;
@@ -1249,18 +1300,61 @@ static int smsc75xx_enter_suspend2(struct usbnet *dev)
 	return 0;
 }
 
+static int smsc75xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
+{
+	struct mii_if_info *mii = &dev->mii;
+	int ret;
+
+	netdev_dbg(dev->net, "enabling PHY wakeup interrupts");
+
+	/* read to clear */
+	ret = smsc75xx_mdio_read_nopm(dev->net, mii->phy_id, PHY_INT_SRC);
+	check_warn_return(ret, "Error reading PHY_INT_SRC");
+
+	/* enable interrupt source */
+	ret = smsc75xx_mdio_read_nopm(dev->net, mii->phy_id, PHY_INT_MASK);
+	check_warn_return(ret, "Error reading PHY_INT_MASK");
+
+	ret |= mask;
+
+	smsc75xx_mdio_write_nopm(dev->net, mii->phy_id, PHY_INT_MASK, ret);
+
+	return 0;
+}
+
+static int smsc75xx_link_ok_nopm(struct usbnet *dev)
+{
+	struct mii_if_info *mii = &dev->mii;
+	int ret;
+
+	/* first, a dummy read, needed to latch some MII phys */
+	ret = smsc75xx_mdio_read_nopm(dev->net, mii->phy_id, MII_BMSR);
+	check_warn_return(ret, "Error reading MII_BMSR");
+
+	ret = smsc75xx_mdio_read_nopm(dev->net, mii->phy_id, MII_BMSR);
+	check_warn_return(ret, "Error reading MII_BMSR");
+
+	return !!(ret & BMSR_LSTATUS);
+}
+
 static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct usbnet *dev = usb_get_intfdata(intf);
 	struct smsc75xx_priv *pdata = (struct smsc75xx_priv *)(dev->data[0]);
+	u32 val, link_up;
 	int ret;
-	u32 val;
 
 	ret = usbnet_suspend(intf, message);
 	check_warn_return(ret, "usbnet_suspend error\n");
 
-	/* if no wol options set, enter lowest power SUSPEND2 mode */
-	if (!(pdata->wolopts & SUPPORTED_WAKE)) {
+	/* determine if link is up using only _nopm functions */
+	link_up = smsc75xx_link_ok_nopm(dev);
+
+	/* if no wol options set, or if link is down and we're not waking on
+	 * PHY activity, enter lowest power SUSPEND2 mode
+	 */
+	if (!(pdata->wolopts & SUPPORTED_WAKE) ||
+		!(link_up || (pdata->wolopts & WAKE_PHY))) {
 		netdev_info(dev->net, "entering SUSPEND2 mode\n");
 
 		/* disable energy detect (link up) & wake up events */
@@ -1283,6 +1377,33 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 		return smsc75xx_enter_suspend2(dev);
 	}
 
+	if (pdata->wolopts & WAKE_PHY) {
+		ret = smsc75xx_enable_phy_wakeup_interrupts(dev,
+			(PHY_INT_MASK_ANEG_COMP | PHY_INT_MASK_LINK_DOWN));
+		check_warn_return(ret, "error enabling PHY wakeup ints");
+
+		/* if link is down then configure EDPD and enter SUSPEND1,
+		 * otherwise enter SUSPEND0 below
+		 */
+		if (!link_up) {
+			struct mii_if_info *mii = &dev->mii;
+			netdev_info(dev->net, "entering SUSPEND1 mode");
+
+			/* enable energy detect power-down mode */
+			ret = smsc75xx_mdio_read_nopm(dev->net, mii->phy_id,
+				PHY_MODE_CTRL_STS);
+			check_warn_return(ret, "Error reading PHY_MODE_CTRL_STS");
+
+			ret |= MODE_CTRL_STS_EDPWRDOWN;
+
+			smsc75xx_mdio_write_nopm(dev->net, mii->phy_id,
+				PHY_MODE_CTRL_STS, ret);
+
+			/* enter SUSPEND1 mode */
+			return smsc75xx_enter_suspend1(dev);
+		}
+	}
+
 	if (pdata->wolopts & (WAKE_MCAST | WAKE_ARP)) {
 		int i, filter = 0;
 
@@ -1349,6 +1470,19 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 	ret = smsc75xx_write_reg_nopm(dev, WUCSR, val);
 	check_warn_return(ret, "Error writing WUCSR\n");
 
+	if (pdata->wolopts & WAKE_PHY) {
+		netdev_info(dev->net, "enabling PHY wakeup\n");
+		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
+		check_warn_return(ret, "Error reading PMT_CTL");
+
+		/* clear wol status, enable energy detection */
+		val &= ~PMT_CTL_WUPS;
+		val |= (PMT_CTL_WUPS_ED | PMT_CTL_ED_EN);
+
+		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
+		check_warn_return(ret, "Error writing PMT_CTL");
+	}
+
 	if (pdata->wolopts & WAKE_MAGIC) {
 		netdev_info(dev->net, "enabling magic packet wakeup\n");
 		ret = smsc75xx_read_reg_nopm(dev, WUCSR, &val);
-- 
1.7.10.4

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Bjørn Mork]

I believe the drivers/net/usb patches should be CCed to linux-usb for
review, because they often touch USB specific things.  So I added that
CC and did not strip any of the quoted text.


Steve Glendinning <steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org> writes:

> This patch splits out the logic for entering suspend modes
> to separate functions, to reduce the complexity of the
> smsc75xx_suspend function.
>
> Signed-off-by: Steve Glendinning <steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
> ---
>  drivers/net/usb/smsc75xx.c |   62 +++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
> index 953c4f4..4655c01 100644
> --- a/drivers/net/usb/smsc75xx.c
> +++ b/drivers/net/usb/smsc75xx.c
> @@ -1213,6 +1213,42 @@ static int smsc75xx_write_wuff(struct usbnet *dev, int filter, u32 wuf_cfg,
>  	return 0;
>  }
>  
> +static int smsc75xx_enter_suspend0(struct usbnet *dev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
> +	check_warn_return(ret, "Error reading PMT_CTL\n");
> +
> +	val &= (~(PMT_CTL_SUS_MODE | PMT_CTL_PHY_RST));
> +	val |= PMT_CTL_SUS_MODE_0 | PMT_CTL_WOL_EN | PMT_CTL_WUPS;
> +
> +	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
> +	check_warn_return(ret, "Error writing PMT_CTL\n");
> +
> +	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);

As mentioned in another comment to the smsc95xx driver: This is weird.
Do you really need to do that?

This is an USB interface driver.  The USB device is handled by the
generic "usb" driver, which will do the right thing.  See 
drivers/usb/generic.c and drivers/usb/core/hub.c


generic_suspend() calls usb_port_suspend() which does:

        /* enable remote wakeup when appropriate; this lets the device
         * wake up the upstream hub (including maybe the root hub).
         *
         * NOTE:  OTG devices may issue remote wakeup (or SRP) even when
         * we don't explicitly enable it here.
         */
        if (udev->do_remote_wakeup) {
                if (!hub_is_superspeed(hub->hdev)) {
                        status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
                                        USB_REQ_SET_FEATURE, USB_RECIP_DEVICE,
                                        USB_DEVICE_REMOTE_WAKEUP, 0,
                                        NULL, 0,
                                        USB_CTRL_SET_TIMEOUT);
                } else {
                        /* Assume there's only one function on the USB 3.0
                         * device and enable remote wake for the first
                         * interface. FIXME if the interface association
                         * descriptor shows there's more than one function.
                         */
                        status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
                                        USB_REQ_SET_FEATURE,
                                        USB_RECIP_INTERFACE,
                                        USB_INTRF_FUNC_SUSPEND,
                                        USB_INTRF_FUNC_SUSPEND_RW |
                                        USB_INTRF_FUNC_SUSPEND_LP,
                                        NULL, 0,
                                        USB_CTRL_SET_TIMEOUT);
                }




So you should not need to touch the USB device feature directly from your
interface driver.


> +
> +	return 0;
> +}
> +
> +static int smsc75xx_enter_suspend2(struct usbnet *dev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
> +	check_warn_return(ret, "Error reading PMT_CTL\n");
> +
> +	val &= ~(PMT_CTL_SUS_MODE | PMT_CTL_WUPS | PMT_CTL_PHY_RST);
> +	val |= PMT_CTL_SUS_MODE_2;
> +
> +	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
> +	check_warn_return(ret, "Error writing PMT_CTL\n");
> +
> +	return 0;
> +}
> +
>  static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>  	struct usbnet *dev = usb_get_intfdata(intf);
> @@ -1244,17 +1280,7 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
>  		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
>  		check_warn_return(ret, "Error writing PMT_CTL\n");
>  
> -		/* enter suspend2 mode */
> -		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
> -		check_warn_return(ret, "Error reading PMT_CTL\n");
> -
> -		val &= ~(PMT_CTL_SUS_MODE | PMT_CTL_WUPS | PMT_CTL_PHY_RST);
> -		val |= PMT_CTL_SUS_MODE_2;
> -
> -		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
> -		check_warn_return(ret, "Error writing PMT_CTL\n");
> -
> -		return 0;
> +		return smsc75xx_enter_suspend2(dev);
>  	}
>  
>  	if (pdata->wolopts & (WAKE_MCAST | WAKE_ARP)) {
> @@ -1368,19 +1394,7 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
>  
>  	/* some wol options are enabled, so enter SUSPEND0 */
>  	netdev_info(dev->net, "entering SUSPEND0 mode\n");
> -
> -	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
> -	check_warn_return(ret, "Error reading PMT_CTL\n");
> -
> -	val &= (~(PMT_CTL_SUS_MODE | PMT_CTL_PHY_RST));
> -	val |= PMT_CTL_SUS_MODE_0 | PMT_CTL_WOL_EN | PMT_CTL_WUPS;
> -
> -	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
> -	check_warn_return(ret, "Error writing PMT_CTL\n");
> -
> -	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
> -
> -	return 0;
> +	return smsc75xx_enter_suspend0(dev);
>  }
>  
>  static int smsc75xx_resume(struct usb_interface *intf)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] smsc75xx: support PHY wakeup source

[Joe Perches]

On Tue, 2012-11-27 at 14:28 +0000, Steve Glendinning wrote:
> This patch enables LAN7500 family devices to wake from suspend
> on either link up or link down events.
[]
> diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
[]
> +static int smsc75xx_enter_suspend1(struct usbnet *dev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
> +	check_warn_return(ret, "Error reading PMT_CTL");

Hi Steve, can you please add newlines to these new
check_warn_<foo> messages and the netdev_<level> ones too?
It helps avoid message interleaving.

> +		if (!link_up) {
> +			struct mii_if_info *mii = &dev->mii;
> +			netdev_info(dev->net, "entering SUSPEND1 mode");

etc..

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

Hi Bjorn,

>> +     smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
>
> As mentioned in another comment to the smsc95xx driver: This is weird.
> Do you really need to do that?
>
> This is an USB interface driver.  The USB device is handled by the
> generic "usb" driver, which will do the right thing.  See
> drivers/usb/generic.c and drivers/usb/core/hub.c

Thanks, I've tested removing all these calls from the driver and
wakeup functionality seems to still work.

I'll resubmit my smsc75xx enhancement patchset with this change once
I've done some more testing.

David: please discard this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] smsc75xx: support PHY wakeup source

[Steve Glendinning]

Hi Joe,

On 27 November 2012 15:56, Joe Perches <joe@perches.com> wrote:
> On Tue, 2012-11-27 at 14:28 +0000, Steve Glendinning wrote:
>> +     ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
>> +     check_warn_return(ret, "Error reading PMT_CTL");
>
> Hi Steve, can you please add newlines to these new
> check_warn_<foo> messages and the netdev_<level> ones too?
> It helps avoid message interleaving.

Sorry about that, I did see your earlier patch to fix existing instances.

David: please discard this patch too, I'll resubmit.

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

Hi Bjorn,

On 27 November 2012 17:21, Steve Glendinning <steve-nksJyM/082jR7s880joybQ@public.gmane.org> wrote:
> Hi Bjorn,
>
>>> +     smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
>>
>> As mentioned in another comment to the smsc95xx driver: This is weird.
>> Do you really need to do that?
>>
>> This is an USB interface driver.  The USB device is handled by the
>> generic "usb" driver, which will do the right thing.  See
>> drivers/usb/generic.c and drivers/usb/core/hub.c
>
> Thanks, I've tested removing all these calls from the driver and
> wakeup functionality seems to still work.
>
> I'll resubmit my smsc75xx enhancement patchset with this change once
> I've done some more testing.

Further testing shows that removing these calls stop wakeup from
system suspend working (although don't appear to impact runtime
autosuspend).  Have I missed a flag or somewhere that causes
udev->do_remote_wakeup to be set in the code you posted?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Bjørn Mork]

Steve Glendinning <steve-nksJyM/082jR7s880joybQ@public.gmane.org> writes:

> Hi Bjorn,
>
> On 27 November 2012 17:21, Steve Glendinning <steve-nksJyM/082jR7s880joybQ@public.gmane.org> wrote:
>> Hi Bjorn,
>>
>>>> +     smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
>>>
>>> As mentioned in another comment to the smsc95xx driver: This is weird.
>>> Do you really need to do that?
>>>
>>> This is an USB interface driver.  The USB device is handled by the
>>> generic "usb" driver, which will do the right thing.  See
>>> drivers/usb/generic.c and drivers/usb/core/hub.c
>>
>> Thanks, I've tested removing all these calls from the driver and
>> wakeup functionality seems to still work.
>>
>> I'll resubmit my smsc75xx enhancement patchset with this change once
>> I've done some more testing.
>
> Further testing shows that removing these calls stop wakeup from
> system suspend working (although don't appear to impact runtime
> autosuspend).  Have I missed a flag or somewhere that causes
> udev->do_remote_wakeup to be set in the code you posted?

udev->do_remote_wakeup is set in choose_wakeup() in
drivers/usb/core/driver.c.  AFAICS it is always set as long as
device_may_wakeup(&udev->dev) is true.

But I am just trying to get a grasp of this code.  Others on the
linux-usb list will know these things much better than me...

Just a thought: Did you remember to remove the clear_feature you have in
resume?


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Alan Stern]

On Tue, 27 Nov 2012, Bjørn Mork wrote:

> Steve Glendinning <steve@shawell.net> writes:
> 
> > Hi Bjorn,
> >
> > On 27 November 2012 17:21, Steve Glendinning <steve@shawell.net> wrote:
> >> Hi Bjorn,
> >>
> >>>> +     smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
> >>>
> >>> As mentioned in another comment to the smsc95xx driver: This is weird.
> >>> Do you really need to do that?
> >>>
> >>> This is an USB interface driver.  The USB device is handled by the
> >>> generic "usb" driver, which will do the right thing.  See
> >>> drivers/usb/generic.c and drivers/usb/core/hub.c
> >>
> >> Thanks, I've tested removing all these calls from the driver and
> >> wakeup functionality seems to still work.
> >>
> >> I'll resubmit my smsc75xx enhancement patchset with this change once
> >> I've done some more testing.
> >
> > Further testing shows that removing these calls stop wakeup from
> > system suspend working (although don't appear to impact runtime
> > autosuspend).  Have I missed a flag or somewhere that causes
> > udev->do_remote_wakeup to be set in the code you posted?
> 
> udev->do_remote_wakeup is set in choose_wakeup() in
> drivers/usb/core/driver.c.  AFAICS it is always set as long as
> device_may_wakeup(&udev->dev) is true.

That's right.  But is device_may_wakeup(&udev->dev) true?

By default it wouldn't be.  The normal way to set it is for the user or 
a program to do:

	echo enabled >/sys/bus/usb/devices/.../power/wakeup

Of course, a driver could disregard the user's choice and set the flag 
by itself.

Alan Stern

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

Hi Alan,

>> udev->do_remote_wakeup is set in choose_wakeup() in
>> drivers/usb/core/driver.c.  AFAICS it is always set as long as
>> device_may_wakeup(&udev->dev) is true.
>
> That's right.  But is device_may_wakeup(&udev->dev) true?
>
> By default it wouldn't be.  The normal way to set it is for the user or
> a program to do:
>
>         echo enabled >/sys/bus/usb/devices/.../power/wakeup
>
> Of course, a driver could disregard the user's choice and set the flag
> by itself.

If I set that from userspace the system is able to resume, but I can't
work out how to successfully set this from the driver.  I believe the
driver should be overriding this as if the user has asked for the
device to wake on lan they're expecting this to resume the system.

I've tried placing various combinations of device_set_wakeup_capable
and device_set_wakeup_enable in different places (bind, suspend), but
it still doesn't allow the device to resume from suspend.  How should
I do this?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:

>>> udev->do_remote_wakeup is set in choose_wakeup() in
>>> drivers/usb/core/driver.c.  AFAICS it is always set as long as
>>> device_may_wakeup(&udev->dev) is true.
>>
>> That's right.  But is device_may_wakeup(&udev->dev) true?
>>
>> By default it wouldn't be.  The normal way to set it is for the user or
>> a program to do:
>>
>>         echo enabled >/sys/bus/usb/devices/.../power/wakeup
>>
>> Of course, a driver could disregard the user's choice and set the flag
>> by itself.
>
> If I set that from userspace the system is able to resume, but I can't
> work out how to successfully set this from the driver.  I believe the
> driver should be overriding this as if the user has asked for the
> device to wake on lan they're expecting this to resume the system.
>
> I've tried placing various combinations of device_set_wakeup_capable
> and device_set_wakeup_enable in different places (bind, suspend), but
> it still doesn't allow the device to resume from suspend.  How should
> I do this?

I may be completely wrong here, but this is how I believe it is supposed
to work...  The device can be suspended for two possible reasons:

1) system suspend.  If the user want the device to wake the system, then
   (s)he will do

       echo enabled >/sys/bus/usb/devices/.../power/wakeup

   If this isn't set, then there is no reason for the driver to request
   remote wakeup while the system is suspended.

2) autosuspend.  Any interface driver needing remote wakeup will set
   intf->needs_remote_wakeup, which makes autosuspend_check() set
   udev->do_remote_wakeup


If all my guesses and assumptions are right, then you want to set
intf->needs_remote_wakeup unconditionally.  This will make the USB core
enable remote wakeup on autosuspend.

Remote wakeup will not be enabled on system suspend unless the user (or
a userspace program on the users behalf) has requested it.


Bjørn

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

Hi Bjorn,

On 28 November 2012 09:31, Bjørn Mork <bjorn@mork.no> wrote:
>
> Remote wakeup will not be enabled on system suspend unless the user (or
> a userspace program on the users behalf) has requested it.

If a user types "ethtool -s eth2 wol p" they *are* explicitly
requesting the ethernet device to bring the system out of suspend, so
I think the ethernet driver should set the feature automatically.

from drivers/base/power/wakeup.c:

 * By default, most devices should leave wakeup disabled.  The exceptions are
 * devices that everyone expects to be wakeup sources: keyboards, power buttons,
 * possibly network interfaces, etc.

Steve

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:

> Hi Bjorn,
>
> On 28 November 2012 09:31, Bjørn Mork <bjorn@mork.no> wrote:
>>
>> Remote wakeup will not be enabled on system suspend unless the user (or
>> a userspace program on the users behalf) has requested it.
>
> If a user types "ethtool -s eth2 wol p" they *are* explicitly
> requesting the ethernet device to bring the system out of suspend, so
> I think the ethernet driver should set the feature automatically.
>
> from drivers/base/power/wakeup.c:
>
>  * By default, most devices should leave wakeup disabled.  The exceptions are
>  * devices that everyone expects to be wakeup sources: keyboards, power buttons,
>  * possibly network interfaces, etc.

Right.  That seems logical.  But the ethtool setting should still
probably be reflected in the device attributes so that the user can see
them there?

Just doing a simple test of what other ethernet drivers does, I tried
this on the e1000e adapter in my laptop.  Initially:

nemi:/tmp# grep .  /sys/bus/pci/devices/0000:00:19.0/power/*
/sys/bus/pci/devices/0000:00:19.0/power/async:enabled
grep: /sys/bus/pci/devices/0000:00:19.0/power/autosuspend_delay_ms: Input/output error
/sys/bus/pci/devices/0000:00:19.0/power/control:auto
/sys/bus/pci/devices/0000:00:19.0/power/runtime_active_kids:0
/sys/bus/pci/devices/0000:00:19.0/power/runtime_active_time:266378772
/sys/bus/pci/devices/0000:00:19.0/power/runtime_enabled:enabled
/sys/bus/pci/devices/0000:00:19.0/power/runtime_status:active
/sys/bus/pci/devices/0000:00:19.0/power/runtime_suspended_time:568333816
/sys/bus/pci/devices/0000:00:19.0/power/runtime_usage:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup:disabled
nemi:/tmp# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 2
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: off
        Supports Wake-on: pumbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes


Enabling WOL:

nemi:/tmp# ethtool -s eth0 wol p
nemi:/tmp# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 2
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: off
        Supports Wake-on: pumbg
        Wake-on: p
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes
nemi:/tmp# grep .  /sys/bus/pci/devices/0000:00:19.0/power/*
/sys/bus/pci/devices/0000:00:19.0/power/async:enabled
grep: /sys/bus/pci/devices/0000:00:19.0/power/autosuspend_delay_ms: Input/output error
/sys/bus/pci/devices/0000:00:19.0/power/control:auto
/sys/bus/pci/devices/0000:00:19.0/power/runtime_active_kids:0
/sys/bus/pci/devices/0000:00:19.0/power/runtime_active_time:266414488
/sys/bus/pci/devices/0000:00:19.0/power/runtime_enabled:enabled
/sys/bus/pci/devices/0000:00:19.0/power/runtime_status:active
/sys/bus/pci/devices/0000:00:19.0/power/runtime_suspended_time:568333816
/sys/bus/pci/devices/0000:00:19.0/power/runtime_usage:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup:enabled
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_abort_count:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_active:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_active_count:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_count:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_expire_count:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_last_time_ms:834745779
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_max_time_ms:0
/sys/bus/pci/devices/0000:00:19.0/power/wakeup_total_time_ms:0


So this driver does set wakeup:enabled, and if this had been a USB
device then the USB core would have set the Remote Wakeup feature on
suspend without the driver having to do anything special in its
driver->suspend function.

Looking at the different ethernet drivers, the normal way do do this
seems to be something like this in their .set_wol implementation:

        device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);


where "adapter" is a netdev_priv private struct, "pdev" is a pci device
and "wol" is an u32.  I don't see any problem doing the same for USB
network devices implementing ethtool "set_wol".

Note that according to Documentation/power/devices.txt:

"Device drivers, however, are not supposed to call device_set_wakeup_enable()
 directly in any case."

which I guess really means that wakeup:enabled is supposed to be user
controlled and not driver controlled.  I assume the ethtool userspace
interface make the above void, as drivers implementing the ethtool
interface will have to call device_set_wakeup_enable() to syncronize
ethtool and sysfs settings.

Does this make sense?


Bjørn

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

> Looking at the different ethernet drivers, the normal way do do this
> seems to be something like this in their .set_wol implementation:
>
>         device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
>
> where "adapter" is a netdev_priv private struct, "pdev" is a pci device
> and "wol" is an u32.  I don't see any problem doing the same for USB
> network devices implementing ethtool "set_wol".

Ahh, good spot.  I've implemented this (patch below for reference) and
it still doesn't work:


$ cat /sys/bus/usb/devices/2-1.2/power/wakeup
disabled

$ sudo ethtool -s eth2 wol p

[ 1607.237767] smsc75xx 2-1.2:1.0 eth2: set_wol before
device_can_wakeup=1 device_may_wakeup=0
[ 1607.237772] smsc75xx 2-1.2:1.0 eth2: set_wol after
device_can_wakeup=1 device_may_wakeup=1

$ cat /sys/bus/usb/devices/2-1.2/power/wakeup
disabled


Huh?!  My debugging printk statements tell me I've succesfully set it,
but then the sysfs entry is disabled when I read it.  Is something
else setting this back?

My testing patch for reference (note this is against my tree with a
few to-be submitted patches so won't apply cleanly!):


diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index d8fa649..4c17849 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -61,7 +61,6 @@
 #define SUSPEND_SUSPEND1 (0x02)
 #define SUSPEND_SUSPEND2 (0x04)
 #define SUSPEND_SUSPEND3 (0x08)
-#define SUSPEND_REMOTEWAKE (0x10)
 #define SUSPEND_ALLMODES (SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
  SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)

@@ -172,26 +171,6 @@ static int __must_check smsc75xx_write_reg(struct
usbnet *dev, u32 index,
  return __smsc75xx_write_reg(dev, index, data, 0);
 }

-static int smsc75xx_set_feature(struct usbnet *dev, u32 feature)
-{
- if (WARN_ON_ONCE(!dev))
- return -EINVAL;
-
- return usbnet_write_cmd_nopm(dev, USB_REQ_SET_FEATURE,
-     USB_DIR_OUT | USB_RECIP_DEVICE,
-     feature, 0, NULL, 0);
-}
-
-static int smsc75xx_clear_feature(struct usbnet *dev, u32 feature)
-{
- if (WARN_ON_ONCE(!dev))
- return -EINVAL;
-
- return usbnet_write_cmd_nopm(dev, USB_REQ_CLEAR_FEATURE,
-     USB_DIR_OUT | USB_RECIP_DEVICE,
-     feature, 0, NULL, 0);
-}
-
 /* Loop until the read is completed with timeout
  * called with phy_mutex held */
 static __must_check int __smsc75xx_phy_wait_not_busy(struct usbnet *dev,
@@ -674,8 +653,19 @@ static int smsc75xx_ethtool_set_wol(struct net_device *net,
 {
  struct usbnet *dev = netdev_priv(net);
  struct smsc75xx_priv *pdata = (struct smsc75xx_priv *)(dev->data[0]);
+ int ret;
+
+ netdev_info(dev->net, "set_wol before device_can_wakeup=%d
device_may_wakeup=%d\n",
+ device_can_wakeup(&net->dev), device_may_wakeup(&net->dev));

  pdata->wolopts = wolinfo->wolopts & SUPPORTED_WAKE;
+
+ ret = device_set_wakeup_enable(&net->dev, pdata->wolopts);
+ check_warn_return(ret, "device_set_wakeup_enable error %d\n", ret);
+
+ netdev_info(dev->net, "set_wol after device_can_wakeup=%d
device_may_wakeup=%d\n",
+ device_can_wakeup(&net->dev), device_may_wakeup(&net->dev));
+
  return 0;
 }

@@ -1197,12 +1187,17 @@ static int smsc75xx_bind(struct usbnet *dev,
struct usb_interface *intf)

  /* Init all registers */
  ret = smsc75xx_reset(dev);
+ check_warn_return(ret, "smsc75xx_reset error %d\n", ret);

  dev->net->netdev_ops = &smsc75xx_netdev_ops;
  dev->net->ethtool_ops = &smsc75xx_ethtool_ops;
  dev->net->flags |= IFF_MULTICAST;
  dev->net->hard_header_len += SMSC75XX_TX_OVERHEAD;
  dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
+
+ ret = device_init_wakeup(&dev->net->dev, 1);
+ check_warn_return(ret, "device_init_wakeup error %d\n", ret);
+
  return 0;
 }

@@ -1262,9 +1257,7 @@ static int smsc75xx_enter_suspend0(struct usbnet *dev)
  ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
  check_warn_return(ret, "Error writing PMT_CTL\n");

- smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
-
- pdata->suspend_flags |= SUSPEND_SUSPEND0 | SUSPEND_REMOTEWAKE;
+ pdata->suspend_flags |= SUSPEND_SUSPEND0;

  return 0;
 }
@@ -1291,9 +1284,7 @@ static int smsc75xx_enter_suspend1(struct usbnet *dev)
  ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
  check_warn_return(ret, "Error writing PMT_CTL\n");

- smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
-
- pdata->suspend_flags |= SUSPEND_SUSPEND1 | SUSPEND_REMOTEWAKE;
+ pdata->suspend_flags |= SUSPEND_SUSPEND1;

  return 0;
 }
@@ -1348,9 +1339,7 @@ static int smsc75xx_enter_suspend3(struct usbnet *dev)
  ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
  check_warn_return(ret, "Error writing PMT_CTL\n");

- smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
-
- pdata->suspend_flags |= SUSPEND_SUSPEND3 | SUSPEND_REMOTEWAKE;
+ pdata->suspend_flags |= SUSPEND_SUSPEND3;

  return 0;
  }
@@ -1650,11 +1639,6 @@ static int smsc75xx_resume(struct usb_interface *intf)
  /* do this first to ensure it's cleared even in error case */
  pdata->suspend_flags = 0;

- if (suspend_flags & SUSPEND_REMOTEWAKE) {
- ret = smsc75xx_clear_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
- check_warn_return(ret, "Error disabling remote wakeup\n");
- }

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:

>> Looking at the different ethernet drivers, the normal way do do this
>> seems to be something like this in their .set_wol implementation:
>>
>>         device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>>
>>
>> where "adapter" is a netdev_priv private struct, "pdev" is a pci device
>> and "wol" is an u32.  I don't see any problem doing the same for USB
>> network devices implementing ethtool "set_wol".
>
> Ahh, good spot.  I've implemented this (patch below for reference) and
> it still doesn't work:
>
>
> $ cat /sys/bus/usb/devices/2-1.2/power/wakeup
> disabled
>
> $ sudo ethtool -s eth2 wol p
>
> [ 1607.237767] smsc75xx 2-1.2:1.0 eth2: set_wol before
> device_can_wakeup=1 device_may_wakeup=0
> [ 1607.237772] smsc75xx 2-1.2:1.0 eth2: set_wol after
> device_can_wakeup=1 device_may_wakeup=1
>
> $ cat /sys/bus/usb/devices/2-1.2/power/wakeup
> disabled
>
>
> Huh?!  My debugging printk statements tell me I've succesfully set it,
> but then the sysfs entry is disabled when I read it.  Is something
> else setting this back?
>
> My testing patch for reference (note this is against my tree with a
> few to-be submitted patches so won't apply cleanly!):

[..]

> @@ -674,8 +653,19 @@ static int smsc75xx_ethtool_set_wol(struct net_device *net,
>  {
>   struct usbnet *dev = netdev_priv(net);
>   struct smsc75xx_priv *pdata = (struct smsc75xx_priv *)(dev->data[0]);
> + int ret;
> +
> + netdev_info(dev->net, "set_wol before device_can_wakeup=%d
> device_may_wakeup=%d\n",
> + device_can_wakeup(&net->dev), device_may_wakeup(&net->dev));
>
>   pdata->wolopts = wolinfo->wolopts & SUPPORTED_WAKE;
> +
> + ret = device_set_wakeup_enable(&net->dev, pdata->wolopts);

You are touching the network device here.  That should have been the USB
device.  Try something like

 ret = device_set_wakeup_enable(&dev->udev->dev, pdata->wolopts);

instead.



Bjørn

Re: [PATCH 1/2] smsc75xx: refactor entering suspend modes

[Steve Glendinning]

>> +
>> + ret = device_set_wakeup_enable(&net->dev, pdata->wolopts);
>
> You are touching the network device here.  That should have been the USB
> device.  Try something like
>
>  ret = device_set_wakeup_enable(&dev->udev->dev, pdata->wolopts);

Perfect, this works exactly as expected now.  Patch included in
resubmitted patchset, thanks Bjorn!