[PATCH v1 0/5] usbnet: avoiding access auto-suspended device

[PATCH v1 0/5] usbnet: avoiding access auto-suspended device

[Ming Lei]

Thip patchset avoids accessing auto-suspended device in ioctl path,
which is generally triggered by some network utility(ethtool, ifconfig,
...)

Most of network devices have the problem, but as discussed in the
thread:

        http://marc.info/?t=135054860600003&r=1&w=2

the problem should be solved inside driver.

Considered that only smsc75xx and smsc95xx calls usbnet_read_cmd()
and usbnet_write_cmd() inside its resume and suspend callback, the
patcheset introduce the nopm version of the two functions which
should be called only in the resume and suspend callback. So we
can solve the problem by runtime resuming device before doing
control message things.

The patchset is against 3.7.0-rc3-next-20121102, and has been tested
OK on smsc95xx usbnet device.

Change logs:
V1:
	- rebased on 3.7.0-rc3-next-20121102, only patch 4/5 changed
	- fix one memory leak during smsc95xx_suspend, patch 3/5 added

 drivers/net/usb/smsc75xx.c |  133 +++++++++++++++++++++++++---------------
 drivers/net/usb/smsc95xx.c |  146 ++++++++++++++++++++++++++++----------------
 drivers/net/usb/usbnet.c   |   72 ++++++++++++++++++++--
 include/linux/usb/usbnet.h |    4 ++
 4 files changed, 248 insertions(+), 107 deletions(-)


Thanks,
--
Ming Lei

[PATCH v1 1/5] usbnet: introduce usbnet_read[write]_cmd_nopm

[Ming Lei]

This patch introduces the below two helpers to prepare for solving
the usbnet runtime PM problem, which may cause some network utilities
(ifconfig, ethtool,...) touch a suspended device.

	usbnet_read_cmd_nopm()
	usbnet_write_cmd_nopm()

The above two helpers should be called by usbnet resume/suspend
callback to avoid deadlock.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/usbnet.c   |   62 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/usb/usbnet.h |    4 +++
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 09ea47a..a7fb074 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1614,8 +1614,8 @@ void usbnet_device_suggests_idle(struct usbnet *dev)
 EXPORT_SYMBOL(usbnet_device_suggests_idle);
 
 /*-------------------------------------------------------------------------*/
-int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
-		    u16 value, u16 index, void *data, u16 size)
+static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+			     u16 value, u16 index, void *data, u16 size)
 {
 	void *buf = NULL;
 	int err = -ENOMEM;
@@ -1639,10 +1639,10 @@ int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 out:
 	return err;
 }
-EXPORT_SYMBOL_GPL(usbnet_read_cmd);
 
-int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
-		     u16 value, u16 index, const void *data, u16 size)
+static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+			      u16 value, u16 index, const void *data,
+			      u16 size)
 {
 	void *buf = NULL;
 	int err = -ENOMEM;
@@ -1665,8 +1665,56 @@ int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 out:
 	return err;
 }
+
+/*
+ * The function can't be called inside suspend/resume callback,
+ * otherwise deadlock will be caused.
+ */
+int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+		    u16 value, u16 index, void *data, u16 size)
+{
+	return __usbnet_read_cmd(dev, cmd, reqtype, value, index,
+				 data, size);
+}
+EXPORT_SYMBOL_GPL(usbnet_read_cmd);
+
+/*
+ * The function can't be called inside suspend/resume callback,
+ * otherwise deadlock will be caused.
+ */
+int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+		     u16 value, u16 index, const void *data, u16 size)
+{
+	return __usbnet_write_cmd(dev, cmd, reqtype, value, index,
+				  data, size);
+}
 EXPORT_SYMBOL_GPL(usbnet_write_cmd);
 
+/*
+ * The function can be called inside suspend/resume callback safely
+ * and should only be called by suspend/resume callback generally.
+ */
+int usbnet_read_cmd_nopm(struct usbnet *dev, u8 cmd, u8 reqtype,
+			  u16 value, u16 index, void *data, u16 size)
+{
+	return __usbnet_read_cmd(dev, cmd, reqtype, value, index,
+				 data, size);
+}
+EXPORT_SYMBOL_GPL(usbnet_read_cmd_nopm);
+
+/*
+ * The function can be called inside suspend/resume callback safely
+ * and should only be called by suspend/resume callback generally.
+ */
+int usbnet_write_cmd_nopm(struct usbnet *dev, u8 cmd, u8 reqtype,
+			  u16 value, u16 index, const void *data,
+			  u16 size)
+{
+	return __usbnet_write_cmd(dev, cmd, reqtype, value, index,
+				  data, size);
+}
+EXPORT_SYMBOL_GPL(usbnet_write_cmd_nopm);
+
 static void usbnet_async_cmd_cb(struct urb *urb)
 {
 	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
@@ -1680,6 +1728,10 @@ static void usbnet_async_cmd_cb(struct urb *urb)
 	usb_free_urb(urb);
 }
 
+/*
+ * The caller must make sure that device can't be put into suspend
+ * state until the control URB completes.
+ */
 int usbnet_write_cmd_async(struct usbnet *dev, u8 cmd, u8 reqtype,
 			   u16 value, u16 index, const void *data, u16 size)
 {
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 4410e416..9bbeabf 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -167,6 +167,10 @@ extern int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 		    u16 value, u16 index, void *data, u16 size);
 extern int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 		    u16 value, u16 index, const void *data, u16 size);
+extern int usbnet_read_cmd_nopm(struct usbnet *dev, u8 cmd, u8 reqtype,
+		    u16 value, u16 index, void *data, u16 size);
+extern int usbnet_write_cmd_nopm(struct usbnet *dev, u8 cmd, u8 reqtype,
+		    u16 value, u16 index, const void *data, u16 size);
 extern int usbnet_write_cmd_async(struct usbnet *dev, u8 cmd, u8 reqtype,
 		    u16 value, u16 index, const void *data, u16 size);
 
-- 
1.7.9.5

[PATCH v1 2/5] usbnet: smsc75xx: apply the introduced usbnet_read[write]_cmd_nopm

[Ming Lei]

This patch applies the introduced usbnet_read_cmd_nopm() and
usbnet_write_cmd_nopm() in the callback of resume and suspend
to avoid deadlock if USB runtime PM is considered into
usbnet_read_cmd() and usbnet_write_cmd().

Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/smsc75xx.c |  133 +++++++++++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 50 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 1baa53a..d4d6d31 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -82,18 +82,23 @@ static bool turbo_mode = true;
 module_param(turbo_mode, bool, 0644);
 MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
 
-static int __must_check smsc75xx_read_reg(struct usbnet *dev, u32 index,
-					  u32 *data)
+static int __must_check __smsc75xx_read_reg(struct usbnet *dev, u32 index,
+					    u32 *data, int in_pm)
 {
 	u32 buf;
 	int ret;
+	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
 
 	BUG_ON(!dev);
 
-	ret = usbnet_read_cmd(dev, USB_VENDOR_REQUEST_READ_REGISTER,
-			      USB_DIR_IN | USB_TYPE_VENDOR |
-			      USB_RECIP_DEVICE,
-			      0, index, &buf, 4);
+	if (!in_pm)
+		fn = usbnet_read_cmd;
+	else
+		fn = usbnet_read_cmd_nopm;
+
+	ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
+		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 0, index, &buf, 4);
 	if (unlikely(ret < 0))
 		netdev_warn(dev->net,
 			"Failed to read reg index 0x%08x: %d", index, ret);
@@ -104,21 +109,26 @@ static int __must_check smsc75xx_read_reg(struct usbnet *dev, u32 index,
 	return ret;
 }
 
-static int __must_check smsc75xx_write_reg(struct usbnet *dev, u32 index,
-					   u32 data)
+static int __must_check __smsc75xx_write_reg(struct usbnet *dev, u32 index,
+					     u32 data, int in_pm)
 {
 	u32 buf;
 	int ret;
+	int (*fn)(struct usbnet *, u8, u8, u16, u16, const void *, u16);
 
 	BUG_ON(!dev);
 
+	if (!in_pm)
+		fn = usbnet_write_cmd;
+	else
+		fn = usbnet_write_cmd_nopm;
+
 	buf = data;
 	cpu_to_le32s(&buf);
 
-	ret = usbnet_write_cmd(dev, USB_VENDOR_REQUEST_WRITE_REGISTER,
-			       USB_DIR_OUT | USB_TYPE_VENDOR |
-			       USB_RECIP_DEVICE,
-			       0, index, &buf, 4);
+	ret = fn(dev, USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT
+		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 0, index, &buf, 4);
 	if (unlikely(ret < 0))
 		netdev_warn(dev->net,
 			"Failed to write reg index 0x%08x: %d", index, ret);
@@ -126,16 +136,38 @@ static int __must_check smsc75xx_write_reg(struct usbnet *dev, u32 index,
 	return ret;
 }
 
+static int __must_check smsc75xx_read_reg_nopm(struct usbnet *dev, u32 index,
+					       u32 *data)
+{
+	return __smsc75xx_read_reg(dev, index, data, 1);
+}
+
+static int __must_check smsc75xx_write_reg_nopm(struct usbnet *dev, u32 index,
+						u32 data)
+{
+	return __smsc75xx_write_reg(dev, index, data, 1);
+}
+
+static int __must_check smsc75xx_read_reg(struct usbnet *dev, u32 index,
+					  u32 *data)
+{
+	return __smsc75xx_read_reg(dev, index, data, 0);
+}
+
+static int __must_check smsc75xx_write_reg(struct usbnet *dev, u32 index,
+					   u32 data)
+{
+	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;
 
-	cpu_to_le32s(&feature);
-
-	return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, feature, 0, NULL, 0,
-		USB_CTRL_SET_TIMEOUT);
+	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)
@@ -143,11 +175,9 @@ static int smsc75xx_clear_feature(struct usbnet *dev, u32 feature)
 	if (WARN_ON_ONCE(!dev))
 		return -EINVAL;
 
-	cpu_to_le32s(&feature);
-
-	return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE, feature, 0, NULL, 0,
-		USB_CTRL_SET_TIMEOUT);
+	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
@@ -793,13 +823,16 @@ static int smsc75xx_set_features(struct net_device *netdev,
 	return 0;
 }
 
-static int smsc75xx_wait_ready(struct usbnet *dev)
+static int smsc75xx_wait_ready(struct usbnet *dev, int in_pm)
 {
 	int timeout = 0;
 
 	do {
 		u32 buf;
-		int ret = smsc75xx_read_reg(dev, PMT_CTL, &buf);
+		int ret;
+
+		ret = __smsc75xx_read_reg(dev, PMT_CTL, &buf, in_pm);
+
 		check_warn_return(ret, "Failed to read PMT_CTL: %d", ret);
 
 		if (buf & PMT_CTL_DEV_RDY)
@@ -821,7 +854,7 @@ static int smsc75xx_reset(struct usbnet *dev)
 
 	netif_dbg(dev, ifup, dev->net, "entering smsc75xx_reset");
 
-	ret = smsc75xx_wait_ready(dev);
+	ret = smsc75xx_wait_ready(dev, 0);
 	check_warn_return(ret, "device not ready in smsc75xx_reset");
 
 	ret = smsc75xx_read_reg(dev, HW_CFG, &buf);
@@ -1158,30 +1191,30 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 		netdev_info(dev->net, "entering SUSPEND2 mode");
 
 		/* disable energy detect (link up) & wake up events */
-		ret = smsc75xx_read_reg(dev, WUCSR, &val);
+		ret = smsc75xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val &= ~(WUCSR_MPEN | WUCSR_WUEN);
 
-		ret = smsc75xx_write_reg(dev, WUCSR, val);
+		ret = smsc75xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 
-		ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
 		check_warn_return(ret, "Error reading PMT_CTL");
 
 		val &= ~(PMT_CTL_ED_EN | PMT_CTL_WOL_EN);
 
-		ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL");
 
 		/* enter suspend2 mode */
-		ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+		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_2;
 
-		ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL");
 
 		return 0;
@@ -1189,17 +1222,17 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (pdata->wolopts & WAKE_MAGIC) {
 		/* clear any pending magic packet status */
-		ret = smsc75xx_read_reg(dev, WUCSR, &val);
+		ret = smsc75xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val |= WUCSR_MPR;
 
-		ret = smsc75xx_write_reg(dev, WUCSR, val);
+		ret = smsc75xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 	}
 
 	/* enable/disable magic packup wake */
-	ret = smsc75xx_read_reg(dev, WUCSR, &val);
+	ret = smsc75xx_read_reg_nopm(dev, WUCSR, &val);
 	check_warn_return(ret, "Error reading WUCSR");
 
 	if (pdata->wolopts & WAKE_MAGIC) {
@@ -1210,47 +1243,47 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 		val &= ~WUCSR_MPEN;
 	}
 
-	ret = smsc75xx_write_reg(dev, WUCSR, val);
+	ret = smsc75xx_write_reg_nopm(dev, WUCSR, val);
 	check_warn_return(ret, "Error writing WUCSR");
 
 	/* enable wol wakeup source */
-	ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
 	check_warn_return(ret, "Error reading PMT_CTL");
 
 	val |= PMT_CTL_WOL_EN;
 
-	ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 	check_warn_return(ret, "Error writing PMT_CTL");
 
 	/* enable receiver */
-	ret = smsc75xx_read_reg(dev, MAC_RX, &val);
+	ret = smsc75xx_read_reg_nopm(dev, MAC_RX, &val);
 	check_warn_return(ret, "Failed to read MAC_RX: %d", ret);
 
 	val |= MAC_RX_RXEN;
 
-	ret = smsc75xx_write_reg(dev, MAC_RX, val);
+	ret = smsc75xx_write_reg_nopm(dev, MAC_RX, val);
 	check_warn_return(ret, "Failed to write MAC_RX: %d", ret);
 
 	/* some wol options are enabled, so enter SUSPEND0 */
 	netdev_info(dev->net, "entering SUSPEND0 mode");
 
-	ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+	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_0;
 
-	ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 	check_warn_return(ret, "Error writing PMT_CTL");
 
 	/* clear wol status */
 	val &= ~PMT_CTL_WUPS;
 	val |= PMT_CTL_WUPS_WOL;
-	ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+	ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 	check_warn_return(ret, "Error writing PMT_CTL");
 
 	/* read back PMT_CTL */
-	ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+	ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
 	check_warn_return(ret, "Error reading PMT_CTL");
 
 	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
@@ -1271,36 +1304,36 @@ static int smsc75xx_resume(struct usb_interface *intf)
 		smsc75xx_clear_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
 
 		/* Disable magic packup wake */
-		ret = smsc75xx_read_reg(dev, WUCSR, &val);
+		ret = smsc75xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val &= ~WUCSR_MPEN;
 
-		ret = smsc75xx_write_reg(dev, WUCSR, val);
+		ret = smsc75xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 
 		/* clear wake-up status */
-		ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
 		check_warn_return(ret, "Error reading PMT_CTL");
 
 		val &= ~PMT_CTL_WOL_EN;
 		val |= PMT_CTL_WUPS;
 
-		ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL");
 	} else {
 		netdev_info(dev->net, "resuming from SUSPEND2");
 
-		ret = smsc75xx_read_reg(dev, PMT_CTL, &val);
+		ret = smsc75xx_read_reg_nopm(dev, PMT_CTL, &val);
 		check_warn_return(ret, "Error reading PMT_CTL");
 
 		val |= PMT_CTL_PHY_PWRUP;
 
-		ret = smsc75xx_write_reg(dev, PMT_CTL, val);
+		ret = smsc75xx_write_reg_nopm(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL");
 	}
 
-	ret = smsc75xx_wait_ready(dev);
+	ret = smsc75xx_wait_ready(dev, 1);
 	check_warn_return(ret, "device not ready in smsc75xx_resume");
 
 	return usbnet_resume(intf);
-- 
1.7.9.5

Re: [PATCH v1 2/5] usbnet: smsc75xx: apply the introduced usbnet_read[write]_cmd_nopm

[David Miller]

From: Ming Lei <ming.lei@canonical.com>
Date: Sun,  4 Nov 2012 09:29:51 +0800

> This patch applies the introduced usbnet_read_cmd_nopm() and
> usbnet_write_cmd_nopm() in the callback of resume and suspend
> to avoid deadlock if USB runtime PM is considered into
> usbnet_read_cmd() and usbnet_write_cmd().
> 
> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

This gets rejects, you didn't have your net-next tree uptodate
enough.

Re: [PATCH v1 2/5] usbnet: smsc75xx: apply the introduced usbnet_read[write]_cmd_nopm

[Ming Lei]

On Sun, Nov 4, 2012 at 1:58 PM, David Miller <davem@davemloft.net> wrote:
>
> This gets rejects, you didn't have your net-next tree uptodate
> enough.

Sorry, I just rebased that on 3.7.0-rc3-next-20121102.

The new patchset which is against the latest net-next has been sent out.

Thanks,
--
Ming Lei

[PATCH v1 3/5] usbnet: smsc95xx: fix memory leak in smsc95xx_suspend

[Ming Lei]

This patch fixes memory leak in smsc95xx_suspend.

Also, it isn't necessary to bother mm to allocate 8bytes/16byte,
and we can use stack variable safely.

Cc: Steve Glendinning <steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/smsc95xx.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 46cd784..f1f473f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1070,11 +1070,15 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
 		u32 *filter_mask = kzalloc(32, GFP_KERNEL);
-		u32 *command = kzalloc(2, GFP_KERNEL);
-		u32 *offset = kzalloc(2, GFP_KERNEL);
-		u32 *crc = kzalloc(4, GFP_KERNEL);
+		u32 command[2];
+		u32 offset[2];
+		u32 crc[4];
 		int i, filter = 0;
 
+		memset(command, 0, sizeof(command));
+		memset(offset, 0, sizeof(offset));
+		memset(crc, 0, sizeof(crc));
+
 		if (pdata->wolopts & WAKE_BCAST) {
 			const u8 bcast[] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
 			netdev_info(dev->net, "enabling broadcast detection");
@@ -1131,6 +1135,8 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 			check_warn_return(ret, "Error writing WUFF");
 		}
 
+		kfree(filter_mask);
+
 		for (i = 0; i < (pdata->wuff_filter_count / 4); i++) {
 			ret = smsc95xx_write_reg(dev, WUFF, command[i]);
 			check_warn_return(ret, "Error writing WUFF");
-- 
1.7.9.5

--
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

[PATCH v1 4/5] usbnet: smsc95xx: apply the introduced usbnet_read[write]_cmd_nopm

[Ming Lei]

This patch applies the introduced usbnet_read_cmd_nopm() and
usbnet_write_cmd_nopm() in the callback of resume and suspend
to avoid deadlock if USB runtime PM is considered into
usbnet_read_cmd() and usbnet_write_cmd().

Cc: Steve Glendinning <steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/smsc95xx.c |  134 ++++++++++++++++++++++++++++----------------
 1 file changed, 85 insertions(+), 49 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index f1f473f..0241db4 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -73,20 +73,26 @@ static bool turbo_mode = true;
 module_param(turbo_mode, bool, 0644);
 MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
 
-static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
-					  u32 *data)
+static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
+					    u32 *data, int in_pm)
 {
 	u32 buf;
 	int ret;
+	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
 
 	BUG_ON(!dev);
 
-	ret = usbnet_read_cmd(dev, USB_VENDOR_REQUEST_READ_REGISTER,
-			      USB_DIR_IN | USB_TYPE_VENDOR |
-			      USB_RECIP_DEVICE,
-			      0, index, &buf, 4);
+	if (!in_pm)
+		fn = usbnet_read_cmd;
+	else
+		fn = usbnet_read_cmd_nopm;
+
+	ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
+		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 0, index, &buf, 4);
 	if (unlikely(ret < 0))
-		netdev_warn(dev->net, "Failed to read register index 0x%08x\n", index);
+		netdev_warn(dev->net,
+			"Failed to read reg index 0x%08x: %d", index, ret);
 
 	le32_to_cpus(&buf);
 	*data = buf;
@@ -94,35 +100,64 @@ static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
 	return ret;
 }
 
-static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
-					   u32 data)
+static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
+					     u32 data, int in_pm)
 {
 	u32 buf;
 	int ret;
+	int (*fn)(struct usbnet *, u8, u8, u16, u16, const void *, u16);
 
 	BUG_ON(!dev);
 
+	if (!in_pm)
+		fn = usbnet_write_cmd;
+	else
+		fn = usbnet_write_cmd_nopm;
+
 	buf = data;
 	cpu_to_le32s(&buf);
 
-
-	ret = usbnet_write_cmd(dev, USB_VENDOR_REQUEST_WRITE_REGISTER,
-			       USB_DIR_OUT | USB_TYPE_VENDOR |
-			       USB_RECIP_DEVICE,
-			       0, index, &buf, 4);
+	ret = fn(dev, USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT
+		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 0, index, &buf, 4);
 	if (unlikely(ret < 0))
-		netdev_warn(dev->net, "Failed to write register index 0x%08x\n", index);
+		netdev_warn(dev->net,
+			"Failed to write reg index 0x%08x: %d", index, ret);
 
 	return ret;
 }
 
+static int __must_check smsc95xx_read_reg_nopm(struct usbnet *dev, u32 index,
+					       u32 *data)
+{
+	return __smsc95xx_read_reg(dev, index, data, 1);
+}
+
+static int __must_check smsc95xx_write_reg_nopm(struct usbnet *dev, u32 index,
+						u32 data)
+{
+	return __smsc95xx_write_reg(dev, index, data, 1);
+}
+
+static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
+					  u32 *data)
+{
+	return __smsc95xx_read_reg(dev, index, data, 0);
+}
+
+static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
+					   u32 data)
+{
+	return __smsc95xx_write_reg(dev, index, data, 0);
+}
 static int smsc95xx_set_feature(struct usbnet *dev, u32 feature)
 {
 	if (WARN_ON_ONCE(!dev))
 		return -EINVAL;
 
-	return usbnet_write_cmd(dev, USB_REQ_SET_FEATURE,
-				USB_RECIP_DEVICE, feature, 0, NULL, 0);
+	return usbnet_write_cmd_nopm(dev, USB_REQ_SET_FEATURE,
+				     USB_RECIP_DEVICE, feature, 0,
+				     NULL, 0);
 }
 
 static int smsc95xx_clear_feature(struct usbnet *dev, u32 feature)
@@ -130,8 +165,9 @@ static int smsc95xx_clear_feature(struct usbnet *dev, u32 feature)
 	if (WARN_ON_ONCE(!dev))
 		return -EINVAL;
 
-	return usbnet_write_cmd(dev, USB_REQ_CLEAR_FEATURE,
-				USB_RECIP_DEVICE, feature, 0, NULL, 0);
+	return usbnet_write_cmd_nopm(dev, USB_REQ_CLEAR_FEATURE,
+				     USB_RECIP_DEVICE, feature,
+				     0, NULL, 0);
 }
 
 /* Loop until the read is completed with timeout
@@ -708,7 +744,7 @@ static int smsc95xx_start_tx_path(struct usbnet *dev)
 }
 
 /* Starts the Receive path */
-static int smsc95xx_start_rx_path(struct usbnet *dev)
+static int smsc95xx_start_rx_path(struct usbnet *dev, int in_pm)
 {
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	unsigned long flags;
@@ -718,7 +754,7 @@ static int smsc95xx_start_rx_path(struct usbnet *dev)
 	pdata->mac_cr |= MAC_CR_RXEN_;
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
-	ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	ret = __smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr, in_pm);
 	check_warn_return(ret, "Failed to write MAC_CR: %d\n", ret);
 
 	return 0;
@@ -937,7 +973,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 	ret = smsc95xx_start_tx_path(dev);
 	check_warn_return(ret, "Failed to start TX path");
 
-	ret = smsc95xx_start_rx_path(dev);
+	ret = smsc95xx_start_rx_path(dev, 0);
 	check_warn_return(ret, "Failed to start RX path");
 
 	netif_dbg(dev, ifup, dev->net, "smsc95xx_reset, return 0\n");
@@ -1039,30 +1075,30 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		netdev_info(dev->net, "entering SUSPEND2 mode");
 
 		/* disable energy detect (link up) & wake up events */
-		ret = smsc95xx_read_reg(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val &= ~(WUCSR_MPEN_ | WUCSR_WAKE_EN_);
 
-		ret = smsc95xx_write_reg(dev, WUCSR, val);
+		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 
-		ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 		check_warn_return(ret, "Error reading PM_CTRL");
 
 		val &= ~(PM_CTL_ED_EN_ | PM_CTL_WOL_EN_);
 
-		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 		check_warn_return(ret, "Error writing PM_CTRL");
 
 		/* enter suspend2 mode */
-		ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 		check_warn_return(ret, "Error reading PM_CTRL");
 
 		val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
 		val |= PM_CTL_SUS_MODE_2;
 
-		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 		check_warn_return(ret, "Error writing PM_CTRL");
 
 		return 0;
@@ -1131,50 +1167,50 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 
 		for (i = 0; i < (pdata->wuff_filter_count * 4); i++) {
-			ret = smsc95xx_write_reg(dev, WUFF, filter_mask[i]);
+			ret = smsc95xx_write_reg_nopm(dev, WUFF, filter_mask[i]);
 			check_warn_return(ret, "Error writing WUFF");
 		}
 
 		kfree(filter_mask);
 
 		for (i = 0; i < (pdata->wuff_filter_count / 4); i++) {
-			ret = smsc95xx_write_reg(dev, WUFF, command[i]);
+			ret = smsc95xx_write_reg_nopm(dev, WUFF, command[i]);
 			check_warn_return(ret, "Error writing WUFF");
 		}
 
 		for (i = 0; i < (pdata->wuff_filter_count / 4); i++) {
-			ret = smsc95xx_write_reg(dev, WUFF, offset[i]);
+			ret = smsc95xx_write_reg_nopm(dev, WUFF, offset[i]);
 			check_warn_return(ret, "Error writing WUFF");
 		}
 
 		for (i = 0; i < (pdata->wuff_filter_count / 2); i++) {
-			ret = smsc95xx_write_reg(dev, WUFF, crc[i]);
+			ret = smsc95xx_write_reg_nopm(dev, WUFF, crc[i]);
 			check_warn_return(ret, "Error writing WUFF");
 		}
 
 		/* clear any pending pattern match packet status */
-		ret = smsc95xx_read_reg(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val |= WUCSR_WUFR_;
 
-		ret = smsc95xx_write_reg(dev, WUCSR, val);
+		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 	}
 
 	if (pdata->wolopts & WAKE_MAGIC) {
 		/* clear any pending magic packet status */
-		ret = smsc95xx_read_reg(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val |= WUCSR_MPR_;
 
-		ret = smsc95xx_write_reg(dev, WUCSR, val);
+		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 	}
 
 	/* enable/disable wakeup sources */
-	ret = smsc95xx_read_reg(dev, WUCSR, &val);
+	ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 	check_warn_return(ret, "Error reading WUCSR");
 
 	if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
@@ -1193,41 +1229,41 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 		val &= ~WUCSR_MPEN_;
 	}
 
-	ret = smsc95xx_write_reg(dev, WUCSR, val);
+	ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 	check_warn_return(ret, "Error writing WUCSR");
 
 	/* enable wol wakeup source */
-	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 	check_warn_return(ret, "Error reading PM_CTRL");
 
 	val |= PM_CTL_WOL_EN_;
 
-	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 	check_warn_return(ret, "Error writing PM_CTRL");
 
 	/* enable receiver to enable frame reception */
-	smsc95xx_start_rx_path(dev);
+	smsc95xx_start_rx_path(dev, 1);
 
 	/* some wol options are enabled, so enter SUSPEND0 */
 	netdev_info(dev->net, "entering SUSPEND0 mode");
 
-	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 	check_warn_return(ret, "Error reading PM_CTRL");
 
 	val &= (~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_));
 	val |= PM_CTL_SUS_MODE_0;
 
-	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 	check_warn_return(ret, "Error writing PM_CTRL");
 
 	/* clear wol status */
 	val &= ~PM_CTL_WUPS_;
 	val |= PM_CTL_WUPS_WOL_;
-	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+	ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 	check_warn_return(ret, "Error writing PM_CTRL");
 
 	/* read back PM_CTRL */
-	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+	ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 	check_warn_return(ret, "Error reading PM_CTRL");
 
 	smsc95xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
@@ -1248,22 +1284,22 @@ static int smsc95xx_resume(struct usb_interface *intf)
 		smsc95xx_clear_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
 
 		/* clear wake-up sources */
-		ret = smsc95xx_read_reg(dev, WUCSR, &val);
+		ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
 		check_warn_return(ret, "Error reading WUCSR");
 
 		val &= ~(WUCSR_WAKE_EN_ | WUCSR_MPEN_);
 
-		ret = smsc95xx_write_reg(dev, WUCSR, val);
+		ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
 		check_warn_return(ret, "Error writing WUCSR");
 
 		/* clear wake-up status */
-		ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+		ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
 		check_warn_return(ret, "Error reading PM_CTRL");
 
 		val &= ~PM_CTL_WOL_EN_;
 		val |= PM_CTL_WUPS_;
 
-		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+		ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
 		check_warn_return(ret, "Error writing PM_CTRL");
 	}
 
-- 
1.7.9.5

--
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

[PATCH v1 5/5] usbnet: make device out of suspend before calling usbnet_read[write]_cmd

[Ming Lei]

This patche gets the runtime PM reference count before calling
usbnet_read[write]_cmd, and puts it after completion of the
usbnet_read[write]_cmd, so that the usb control message can always
be sent to one active device in the non-PM context.

Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/usbnet.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index a7fb074..74caa67 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1673,8 +1673,13 @@ out:
 int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 		    u16 value, u16 index, void *data, u16 size)
 {
-	return __usbnet_read_cmd(dev, cmd, reqtype, value, index,
-				 data, size);
+	int ret;
+	if (usb_autopm_get_interface(dev->intf) < 0)
+		return -ENODEV;
+	ret = __usbnet_read_cmd(dev, cmd, reqtype, value, index,
+				data, size);
+	usb_autopm_put_interface(dev->intf);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usbnet_read_cmd);
 
@@ -1685,8 +1690,13 @@ EXPORT_SYMBOL_GPL(usbnet_read_cmd);
 int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 		     u16 value, u16 index, const void *data, u16 size)
 {
-	return __usbnet_write_cmd(dev, cmd, reqtype, value, index,
-				  data, size);
+	int ret;
+	if (usb_autopm_get_interface(dev->intf) < 0)
+		return -ENODEV;
+	ret = __usbnet_write_cmd(dev, cmd, reqtype, value, index,
+				 data, size);
+	usb_autopm_put_interface(dev->intf);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usbnet_write_cmd);
 
-- 
1.7.9.5

--
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 v1 0/5] usbnet: avoiding access auto-suspended device

[Oliver Neukum]

On Sunday 04 November 2012 09:29:49 Ming Lei wrote:
> Thip patchset avoids accessing auto-suspended device in ioctl path,
> which is generally triggered by some network utility(ethtool, ifconfig,
> ...)
> 
> Most of network devices have the problem, but as discussed in the
> thread:
> 
>         http://marc.info/?t=135054860600003&r=1&w=2
> 
> the problem should be solved inside driver.
> 
> Considered that only smsc75xx and smsc95xx calls usbnet_read_cmd()
> and usbnet_write_cmd() inside its resume and suspend callback, the
> patcheset introduce the nopm version of the two functions which
> should be called only in the resume and suspend callback. So we
> can solve the problem by runtime resuming device before doing
> control message things.

Hi,

I am happy with these patches.
Dave, do you have a principal objection regarding these patches, too?

	Regards
		Oliver

--
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 v1 0/5] usbnet: avoiding access auto-suspended device

[David Miller]

From: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
Date: Mon, 05 Nov 2012 10:24:57 +0100

> On Sunday 04 November 2012 09:29:49 Ming Lei wrote:
>> Thip patchset avoids accessing auto-suspended device in ioctl path,
>> which is generally triggered by some network utility(ethtool, ifconfig,
>> ...)
>> 
>> Most of network devices have the problem, but as discussed in the
>> thread:
>> 
>>         http://marc.info/?t=135054860600003&r=1&w=2
>> 
>> the problem should be solved inside driver.
>> 
>> Considered that only smsc75xx and smsc95xx calls usbnet_read_cmd()
>> and usbnet_write_cmd() inside its resume and suspend callback, the
>> patcheset introduce the nopm version of the two functions which
>> should be called only in the resume and suspend callback. So we
>> can solve the problem by runtime resuming device before doing
>> control message things.
> 
> Hi,
> 
> I am happy with these patches.
> Dave, do you have a principal objection regarding these patches, too?

There were review comments and the most recently posted series
needs to be reposted with that feedback incorporated.
--
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 v1 0/5] usbnet: avoiding access auto-suspended device

[Ming Lei]

Hi David,

On Tue, Nov 6, 2012 at 12:50 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> I am happy with these patches.
>> Dave, do you have a principal objection regarding these patches, too?
>
> There were review comments and the most recently posted series
> needs to be reposted with that feedback incorporated.

The comment is just about commit log and code style, and I have fixed
that in V3.

Thanks,
--
Ming Lei
--
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