[PATCH 00/12] usbnet: usb_control_msg cleanup

[PATCH 00/12] usbnet: usb_control_msg cleanup

[Ming Lei]

Hi,

This patch set introduces 3 helpers for handling usb read, write
and write_async command, and replaces the low level's implemention
with the generic ones.

Firstly, it is a cleanup and about 300 lines code can be saved.

Secondly, the patch fixes DMA on the buffer which is embedded inside
one dynamic allocated buffer, and such usages can be found
in cdc-ncm, sierra_net, mcs7830 and asix drivers.

Finally, almost all low level drivers don't consider runtime
PM situation when reading/writing control command to device via
usb_control_message. For example, 'ethtool' may touch a suspended
device and kind of below message will be dumped:

	[tom@tom-pandaboard ~]$ ethtool eth0
	Settings for eth[  117.084411] smsc95xx 1-1.1:1.0 eth0: Failed to read register index 0x00000114
	0:
	[  117.093139] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
	[  117.099395] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read

This patch fixes the problem above by holding runtime PM referece
count before calling usb_control_msg.

 drivers/net/usb/asix_common.c |  117 ++++-------------------------------
 drivers/net/usb/cdc_ncm.c     |   73 +++++++---------------
 drivers/net/usb/dm9601.c      |  107 +++++---------------------------
 drivers/net/usb/int51x1.c     |   52 +---------------
 drivers/net/usb/mcs7830.c     |   85 ++-----------------------
 drivers/net/usb/net1080.c     |  110 +++++++++------------------------
 drivers/net/usb/plusb.c       |   11 ++--
 drivers/net/usb/sierra_net.c  |   45 +++++---------
 drivers/net/usb/smsc75xx.c    |   39 +++++-------
 drivers/net/usb/smsc95xx.c    |  115 ++++++++--------------------------
 drivers/net/usb/usbnet.c      |  137 +++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usbnet.h    |    6 ++
 12 files changed, 295 insertions(+), 602 deletions(-)


Thanks,
--
Ming Lei

[PATCH 03/12] usbnet: cdc-ncm: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/cdc_ncm.c |   73 ++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 50 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4cd582a..429a2ad 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -159,16 +159,16 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 	u8 iface_no;
 	int err;
 	u16 ntb_fmt_supported;
+	struct usbnet *dev = netdev_priv(ctx->netdev);
 
 	iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
 
-	err = usb_control_msg(ctx->udev,
-				usb_rcvctrlpipe(ctx->udev, 0),
+	err = usbnet_read_cmd(dev,
 				USB_CDC_GET_NTB_PARAMETERS,
 				USB_TYPE_CLASS | USB_DIR_IN
 				 | USB_RECIP_INTERFACE,
 				0, iface_no, &ctx->ncm_parm,
-				sizeof(ctx->ncm_parm), 10000);
+				sizeof(ctx->ncm_parm));
 	if (err < 0) {
 		pr_debug("failed GET_NTB_PARAMETERS\n");
 		return 1;
@@ -217,40 +217,23 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 	if (ctx->rx_max != le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)) {
 
 		if (flags & USB_CDC_NCM_NCAP_NTB_INPUT_SIZE) {
-			struct usb_cdc_ncm_ndp_input_size *ndp_in_sz;
+			struct usb_cdc_ncm_ndp_input_size ndp_in_sz;
 
-			ndp_in_sz = kzalloc(sizeof(*ndp_in_sz), GFP_KERNEL);
-			if (!ndp_in_sz) {
-				err = -ENOMEM;
-				goto size_err;
-			}
-
-			err = usb_control_msg(ctx->udev,
-					usb_sndctrlpipe(ctx->udev, 0),
+			memset(&ndp_in_sz, 0, sizeof(ndp_in_sz));
+			err = usbnet_write_cmd(dev,
 					USB_CDC_SET_NTB_INPUT_SIZE,
 					USB_TYPE_CLASS | USB_DIR_OUT
 					 | USB_RECIP_INTERFACE,
-					0, iface_no, ndp_in_sz, 8, 1000);
-			kfree(ndp_in_sz);
+					0, iface_no, &ndp_in_sz, 8);
 		} else {
-			__le32 *dwNtbInMaxSize;
-			dwNtbInMaxSize = kzalloc(sizeof(*dwNtbInMaxSize),
-					GFP_KERNEL);
-			if (!dwNtbInMaxSize) {
-				err = -ENOMEM;
-				goto size_err;
-			}
-			*dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
+			__le32 dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
 
-			err = usb_control_msg(ctx->udev,
-					usb_sndctrlpipe(ctx->udev, 0),
+			err = usbnet_write_cmd(dev,
 					USB_CDC_SET_NTB_INPUT_SIZE,
 					USB_TYPE_CLASS | USB_DIR_OUT
 					 | USB_RECIP_INTERFACE,
-					0, iface_no, dwNtbInMaxSize, 4, 1000);
-			kfree(dwNtbInMaxSize);
+					0, iface_no, &dwNtbInMaxSize, 4);
 		}
-size_err:
 		if (err < 0)
 			pr_debug("Setting NTB Input Size failed\n");
 	}
@@ -306,23 +289,23 @@ size_err:
 
 	/* set CRC Mode */
 	if (flags & USB_CDC_NCM_NCAP_CRC_MODE) {
-		err = usb_control_msg(ctx->udev, usb_sndctrlpipe(ctx->udev, 0),
+		err = usbnet_write_cmd(dev,
 				USB_CDC_SET_CRC_MODE,
 				USB_TYPE_CLASS | USB_DIR_OUT
 				 | USB_RECIP_INTERFACE,
 				USB_CDC_NCM_CRC_NOT_APPENDED,
-				iface_no, NULL, 0, 1000);
+				iface_no, NULL, 0);
 		if (err < 0)
 			pr_debug("Setting CRC mode off failed\n");
 	}
 
 	/* set NTB format, if both formats are supported */
 	if (ntb_fmt_supported & USB_CDC_NCM_NTH32_SIGN) {
-		err = usb_control_msg(ctx->udev, usb_sndctrlpipe(ctx->udev, 0),
+		err = usbnet_write_cmd(dev,
 				USB_CDC_SET_NTB_FORMAT, USB_TYPE_CLASS
 				 | USB_DIR_OUT | USB_RECIP_INTERFACE,
 				USB_CDC_NCM_NTB16_FORMAT,
-				iface_no, NULL, 0, 1000);
+				iface_no, NULL, 0);
 		if (err < 0)
 			pr_debug("Setting NTB format to 16-bit failed\n");
 	}
@@ -331,28 +314,21 @@ size_err:
 
 	/* set Max Datagram Size (MTU) */
 	if (flags & USB_CDC_NCM_NCAP_MAX_DATAGRAM_SIZE) {
-		__le16 *max_datagram_size;
+		__le16 max_datagram_size;
 		u16 eth_max_sz = le16_to_cpu(ctx->ether_desc->wMaxSegmentSize);
 
-		max_datagram_size = kzalloc(sizeof(*max_datagram_size),
-				GFP_KERNEL);
-		if (!max_datagram_size) {
-			err = -ENOMEM;
-			goto max_dgram_err;
-		}
-
-		err = usb_control_msg(ctx->udev, usb_rcvctrlpipe(ctx->udev, 0),
+		err = usbnet_write_cmd(dev,
 				USB_CDC_GET_MAX_DATAGRAM_SIZE,
 				USB_TYPE_CLASS | USB_DIR_IN
 				 | USB_RECIP_INTERFACE,
-				0, iface_no, max_datagram_size,
-				2, 1000);
+				0, iface_no, &max_datagram_size,
+				2);
 		if (err < 0) {
 			pr_debug("GET_MAX_DATAGRAM_SIZE failed, use size=%u\n",
 						CDC_NCM_MIN_DATAGRAM_SIZE);
 		} else {
 			ctx->max_datagram_size =
-				le16_to_cpu(*max_datagram_size);
+				le16_to_cpu(max_datagram_size);
 			/* Check Eth descriptor value */
 			if (ctx->max_datagram_size > eth_max_sz)
 					ctx->max_datagram_size = eth_max_sz;
@@ -367,23 +343,20 @@ size_err:
 
 			/* if value changed, update device */
 			if (ctx->max_datagram_size !=
-					le16_to_cpu(*max_datagram_size)) {
-				err = usb_control_msg(ctx->udev,
-						usb_sndctrlpipe(ctx->udev, 0),
+					le16_to_cpu(max_datagram_size)) {
+				err = usbnet_write_cmd(dev,
 						USB_CDC_SET_MAX_DATAGRAM_SIZE,
 						USB_TYPE_CLASS | USB_DIR_OUT
 						 | USB_RECIP_INTERFACE,
 						0,
-						iface_no, max_datagram_size,
-						2, 1000);
+						iface_no, &max_datagram_size,
+						2);
 				if (err < 0)
 					pr_debug("SET_MAX_DGRAM_SIZE failed\n");
 			}
 		}
-		kfree(max_datagram_size);
 	}
 
-max_dgram_err:
 	if (ctx->netdev->mtu != (ctx->max_datagram_size - ETH_HLEN))
 		ctx->netdev->mtu = ctx->max_datagram_size - ETH_HLEN;
 
-- 
1.7.9.5

[PATCH 04/12] usbnet: dm9601: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/dm9601.c |  107 +++++++---------------------------------------
 1 file changed, 15 insertions(+), 92 deletions(-)

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index e0433ce..3f554c1 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -56,27 +56,12 @@
 
 static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
 {
-	void *buf;
-	int err = -ENOMEM;
-
-	netdev_dbg(dev->net, "dm_read() reg=0x%02x length=%d\n", reg, length);
-
-	buf = kmalloc(length, GFP_KERNEL);
-	if (!buf)
-		goto out;
-
-	err = usb_control_msg(dev->udev,
-			      usb_rcvctrlpipe(dev->udev, 0),
-			      DM_READ_REGS,
-			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-			      0, reg, buf, length, USB_CTRL_SET_TIMEOUT);
-	if (err == length)
-		memcpy(data, buf, length);
-	else if (err >= 0)
+	int err;
+	err = usbnet_read_cmd(dev, DM_READ_REGS,
+			       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			       0, reg, data, length);
+	if(err != length && err >= 0)
 		err = -EINVAL;
-	kfree(buf);
-
- out:
 	return err;
 }
 
@@ -87,91 +72,29 @@ static int dm_read_reg(struct usbnet *dev, u8 reg, u8 *value)
 
 static int dm_write(struct usbnet *dev, u8 reg, u16 length, void *data)
 {
-	void *buf = NULL;
-	int err = -ENOMEM;
-
-	netdev_dbg(dev->net, "dm_write() reg=0x%02x, length=%d\n", reg, length);
+	int err;
+	err = usbnet_write_cmd(dev, DM_WRITE_REGS,
+				USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				0, reg, data, length);
 
-	if (data) {
-		buf = kmemdup(data, length, GFP_KERNEL);
-		if (!buf)
-			goto out;
-	}
-
-	err = usb_control_msg(dev->udev,
-			      usb_sndctrlpipe(dev->udev, 0),
-			      DM_WRITE_REGS,
-			      USB_DIR_OUT | USB_TYPE_VENDOR |USB_RECIP_DEVICE,
-			      0, reg, buf, length, USB_CTRL_SET_TIMEOUT);
-	kfree(buf);
 	if (err >= 0 && err < length)
 		err = -EINVAL;
- out:
 	return err;
 }
 
 static int dm_write_reg(struct usbnet *dev, u8 reg, u8 value)
 {
-	netdev_dbg(dev->net, "dm_write_reg() reg=0x%02x, value=0x%02x\n",
-		   reg, value);
-	return usb_control_msg(dev->udev,
-			       usb_sndctrlpipe(dev->udev, 0),
-			       DM_WRITE_REG,
-			       USB_DIR_OUT | USB_TYPE_VENDOR |USB_RECIP_DEVICE,
-			       value, reg, NULL, 0, USB_CTRL_SET_TIMEOUT);
-}
-
-static void dm_write_async_callback(struct urb *urb)
-{
-	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
-	int status = urb->status;
-
-	if (status < 0)
-		printk(KERN_DEBUG "dm_write_async_callback() failed with %d\n",
-		       status);
-
-	kfree(req);
-	usb_free_urb(urb);
+	return usbnet_write_cmd(dev, DM_WRITE_REGS,
+				USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				value, reg, NULL, 0);
 }
 
 static void dm_write_async_helper(struct usbnet *dev, u8 reg, u8 value,
 				  u16 length, void *data)
 {
-	struct usb_ctrlrequest *req;
-	struct urb *urb;
-	int status;
-
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		netdev_err(dev->net, "Error allocating URB in dm_write_async_helper!\n");
-		return;
-	}
-
-	req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
-	if (!req) {
-		netdev_err(dev->net, "Failed to allocate memory for control request\n");
-		usb_free_urb(urb);
-		return;
-	}
-
-	req->bRequestType = USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE;
-	req->bRequest = length ? DM_WRITE_REGS : DM_WRITE_REG;
-	req->wValue = cpu_to_le16(value);
-	req->wIndex = cpu_to_le16(reg);
-	req->wLength = cpu_to_le16(length);
-
-	usb_fill_control_urb(urb, dev->udev,
-			     usb_sndctrlpipe(dev->udev, 0),
-			     (void *)req, data, length,
-			     dm_write_async_callback, req);
-
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status < 0) {
-		netdev_err(dev->net, "Error submitting the control message: status=%d\n",
-			   status);
-		kfree(req);
-		usb_free_urb(urb);
-	}
+	usbnet_write_cmd_async(dev, DM_WRITE_REGS,
+			       USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			       value, reg, data, length);
 }
 
 static void dm_write_async(struct usbnet *dev, u8 reg, u16 length, void *data)
-- 
1.7.9.5

[PATCH 05/12] usbnet: int51x1: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/int51x1.c |   52 +++------------------------------------------
 1 file changed, 3 insertions(+), 49 deletions(-)

diff --git a/drivers/net/usb/int51x1.c b/drivers/net/usb/int51x1.c
index 8de6417..ace9e74 100644
--- a/drivers/net/usb/int51x1.c
+++ b/drivers/net/usb/int51x1.c
@@ -116,23 +116,8 @@ static struct sk_buff *int51x1_tx_fixup(struct usbnet *dev,
 	return skb;
 }
 
-static void int51x1_async_cmd_callback(struct urb *urb)
-{
-	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
-	int status = urb->status;
-
-	if (status < 0)
-		dev_warn(&urb->dev->dev, "async callback failed with %d\n", status);
-
-	kfree(req);
-	usb_free_urb(urb);
-}
-
 static void int51x1_set_multicast(struct net_device *netdev)
 {
-	struct usb_ctrlrequest *req;
-	int status;
-	struct urb *urb;
 	struct usbnet *dev = netdev_priv(netdev);
 	u16 filter = PACKET_TYPE_DIRECTED | PACKET_TYPE_BROADCAST;
 
@@ -149,40 +134,9 @@ static void int51x1_set_multicast(struct net_device *netdev)
 		netdev_dbg(dev->net, "receive own packets only\n");
 	}
 
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		netdev_warn(dev->net, "Error allocating URB\n");
-		return;
-	}
-
-	req = kmalloc(sizeof(*req), GFP_ATOMIC);
-	if (!req) {
-		netdev_warn(dev->net, "Error allocating control msg\n");
-		goto out;
-	}
-
-	req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
-	req->bRequest = SET_ETHERNET_PACKET_FILTER;
-	req->wValue = cpu_to_le16(filter);
-	req->wIndex = 0;
-	req->wLength = 0;
-
-	usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		(void *)req, NULL, 0,
-		int51x1_async_cmd_callback,
-		(void *)req);
-
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status < 0) {
-		netdev_warn(dev->net, "Error submitting control msg, sts=%d\n",
-			    status);
-		goto out1;
-	}
-	return;
-out1:
-	kfree(req);
-out:
-	usb_free_urb(urb);
+	usbnet_write_cmd_async(dev, SET_ETHERNET_PACKET_FILTER,
+			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			       filter, 0, NULL, 0);
 }
 
 static const struct net_device_ops int51x1_netdev_ops = {
-- 
1.7.9.5

[PATCH 06/12] usbnet: mcs7830: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/mcs7830.c |   85 ++++-----------------------------------------
 1 file changed, 6 insertions(+), 79 deletions(-)

diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index 03c2d8d..db46a68 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -123,93 +123,20 @@ static const char driver_name[] = "MOSCHIP usb-ethernet driver";
 
 static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data)
 {
-	struct usb_device *xdev = dev->udev;
-	int ret;
-	void *buffer;
-
-	buffer = kmalloc(size, GFP_NOIO);
-	if (buffer == NULL)
-		return -ENOMEM;
-
-	ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
-			      MCS7830_RD_BMREQ, 0x0000, index, buffer,
-			      size, MCS7830_CTRL_TIMEOUT);
-	memcpy(data, buffer, size);
-	kfree(buffer);
-
-	return ret;
+	return usbnet_read_cmd(dev, MCS7830_RD_BREQ, MCS7830_RD_BMREQ,
+				0x0000, index, data, size);
 }
 
 static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, const void *data)
 {
-	struct usb_device *xdev = dev->udev;
-	int ret;
-	void *buffer;
-
-	buffer = kmemdup(data, size, GFP_NOIO);
-	if (buffer == NULL)
-		return -ENOMEM;
-
-	ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
-			      MCS7830_WR_BMREQ, 0x0000, index, buffer,
-			      size, MCS7830_CTRL_TIMEOUT);
-	kfree(buffer);
-	return ret;
-}
-
-static void mcs7830_async_cmd_callback(struct urb *urb)
-{
-	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
-	int status = urb->status;
-
-	if (status < 0)
-		printk(KERN_DEBUG "%s() failed with %d\n",
-		       __func__, status);
-
-	kfree(req);
-	usb_free_urb(urb);
+	return usbnet_write_cmd(dev, MCS7830_WR_BREQ, MCS7830_WR_BMREQ,
+				0x0000, index, data, size);
 }
 
 static void mcs7830_set_reg_async(struct usbnet *dev, u16 index, u16 size, void *data)
 {
-	struct usb_ctrlrequest *req;
-	int ret;
-	struct urb *urb;
-
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		dev_dbg(&dev->udev->dev,
-			"Error allocating URB in write_cmd_async!\n");
-		return;
-	}
-
-	req = kmalloc(sizeof *req, GFP_ATOMIC);
-	if (!req) {
-		dev_err(&dev->udev->dev,
-			"Failed to allocate memory for control request\n");
-		goto out;
-	}
-	req->bRequestType = MCS7830_WR_BMREQ;
-	req->bRequest = MCS7830_WR_BREQ;
-	req->wValue = 0;
-	req->wIndex = cpu_to_le16(index);
-	req->wLength = cpu_to_le16(size);
-
-	usb_fill_control_urb(urb, dev->udev,
-			     usb_sndctrlpipe(dev->udev, 0),
-			     (void *)req, data, size,
-			     mcs7830_async_cmd_callback, req);
-
-	ret = usb_submit_urb(urb, GFP_ATOMIC);
-	if (ret < 0) {
-		dev_err(&dev->udev->dev,
-			"Error submitting the control message: ret=%d\n", ret);
-		goto out;
-	}
-	return;
-out:
-	kfree(req);
-	usb_free_urb(urb);
+	usbnet_write_cmd_async(dev, MCS7830_WR_BREQ, MCS7830_WR_BMREQ,
+				0x0000, index, data, size);
 }
 
 static int mcs7830_hif_get_mac_address(struct usbnet *dev, unsigned char *addr)
-- 
1.7.9.5

[PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

This patch introduces the below 3 usb command helpers:

	usbnet_read_cmd / usbnet_write_cmd / usbnet_write_cmd_async

so that each low level driver doesn't need to implement them
by itself, and the dma buffer allocation for usb transfer and
runtime PM things can be handled just in one place.

Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usbnet.h |    6 ++
 2 files changed, 139 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index fc9f578..3b51554 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1592,6 +1592,139 @@ int usbnet_resume (struct usb_interface *intf)
 }
 EXPORT_SYMBOL_GPL(usbnet_resume);
 
+/*-------------------------------------------------------------------------*/
+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;
+
+	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
+		   " value=0x%04x index=0x%04x size=%d\n",
+		   cmd, reqtype, value, index, size);
+
+	if (data) {
+		buf = kmalloc(size, GFP_KERNEL);
+		if (!buf)
+			goto out;
+	}
+
+	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
+			      cmd, reqtype, value, index, buf, size,
+			      USB_CTRL_GET_TIMEOUT);
+	if (err > 0 && err <= size)
+		memcpy(data, buf, err);
+	kfree(buf);
+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)
+{
+	void *buf = NULL;
+	int err = -ENOMEM;
+
+	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
+		   " value=0x%04x index=0x%04x size=%d\n",
+		   cmd, reqtype, value, index, size);
+
+	if (data) {
+		buf = kmemdup(data, size, GFP_KERNEL);
+		if (!buf)
+			goto out;
+	}
+
+	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
+			      cmd, reqtype, value, index, buf, size,
+			      USB_CTRL_SET_TIMEOUT);
+	kfree(buf);
+
+out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(usbnet_write_cmd);
+
+static void usbnet_async_cmd_cb(struct urb *urb)
+{
+	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
+	int status = urb->status;
+
+	if (status < 0)
+		dev_dbg(&urb->dev->dev, "%s failed with %d",
+			__func__, status);
+
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+int usbnet_write_cmd_async(struct usbnet *dev, u8 cmd, u8 reqtype,
+			   u16 value, u16 index, const void *data, u16 size)
+{
+	struct usb_ctrlrequest *req = NULL;
+	struct urb *urb;
+	int err = -ENOMEM;
+	void *buf = NULL;
+
+	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
+		   " value=0x%04x index=0x%04x size=%d\n",
+		   cmd, reqtype, value, index, size);
+
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		netdev_err(dev->net, "Error allocating URB in"
+			   " %s!\n", __func__);
+		goto fail;
+	}
+
+	if (data) {
+		buf = kmemdup(data, size, GFP_ATOMIC);
+		if (!buf) {
+			netdev_err(dev->net, "Error allocating buffer"
+				   " in %s!\n", __func__);
+			goto fail_free;
+		}
+	}
+
+	req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
+	if (!req) {
+		netdev_err(dev->net, "Failed to allocate memory for %s\n",
+			   __func__);
+		goto fail_free_buf;
+	}
+
+	req->bRequestType = reqtype;
+	req->bRequest = cmd;
+	req->wValue = cpu_to_le16(value);
+	req->wIndex = cpu_to_le16(index);
+	req->wLength = cpu_to_le16(size);
+
+	usb_fill_control_urb(urb, dev->udev,
+			     usb_sndctrlpipe(dev->udev, 0),
+			     (void *)req, buf, size,
+			     usbnet_async_cmd_cb, req);
+	urb->transfer_flags |= URB_FREE_BUFFER;
+
+	err = usb_submit_urb(urb, GFP_ATOMIC);
+	if (err < 0) {
+		netdev_err(dev->net, "Error submitting the control"
+			   " message: status=%d\n", err);
+		goto fail_free;
+	}
+	return 0;
+
+fail_free_buf:
+	kfree(buf);
+fail_free:
+	kfree(req);
+	usb_free_urb(urb);
+fail:
+	return err;
+
+}
+EXPORT_SYMBOL_GPL(usbnet_write_cmd_async);
+
 
 /*-------------------------------------------------------------------------*/
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index f87cf62..32a57dd 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -161,6 +161,12 @@ extern int usbnet_suspend(struct usb_interface *, pm_message_t);
 extern int usbnet_resume(struct usb_interface *);
 extern void usbnet_disconnect(struct usb_interface *);
 
+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_write_cmd_async(struct usbnet *dev, u8 cmd, u8 reqtype,
+		    u16 value, u16 index, const void *data, u16 size);
 
 /* Drivers that reuse some of the standard USB CDC infrastructure
  * (notably, using multiple interfaces according to the CDC
-- 
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 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Tuesday 02 October 2012 14:51:12 Ming Lei wrote:
> This patch introduces the below 3 usb command helpers:
> 
> 	usbnet_read_cmd / usbnet_write_cmd / usbnet_write_cmd_async
> 
> so that each low level driver doesn't need to implement them
> by itself, and the dma buffer allocation for usb transfer and
> runtime PM things can be handled just in one place.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/net/usb/usbnet.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/usbnet.h |    6 ++
>  2 files changed, 139 insertions(+)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index fc9f578..3b51554 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1592,6 +1592,139 @@ int usbnet_resume (struct usb_interface *intf)
>  }
>  EXPORT_SYMBOL_GPL(usbnet_resume);
>  
> +/*-------------------------------------------------------------------------*/
> +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;
> +
> +	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
> +		   " value=0x%04x index=0x%04x size=%d\n",
> +		   cmd, reqtype, value, index, size);
> +
> +	if (data) {
> +		buf = kmalloc(size, GFP_KERNEL);

Using GFP_KERNEL you preclude using those in resume() and error handling.
Please pass a gfp_t parameter.

	Regards
		Oliver

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Tue, Oct 9, 2012 at 4:47 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
> Using GFP_KERNEL you preclude using those in resume() and error handling.
> Please pass a gfp_t parameter.

IMO, it is not a big deal because generally only several bytes are to be
allocated inside these helpers.

If you still think the problem should be considered, another candidate fix
is to take GFP_NOIO during system suspend/resume, and take GFP_KERNEL
in other situations.

Thanks,
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Wednesday 10 October 2012 11:19:09 Ming Lei wrote:
> On Tue, Oct 9, 2012 at 4:47 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> >
> > Using GFP_KERNEL you preclude using those in resume() and error handling.
> > Please pass a gfp_t parameter.
> 
> IMO, it is not a big deal because generally only several bytes are to be
> allocated inside these helpers.

Any allocation can do it, if the VM layer decides to block.

> If you still think the problem should be considered, another candidate fix
> is to take GFP_NOIO during system suspend/resume, and take GFP_KERNEL
> in other situations.

No, the problem is autoresume.

Suppose we have a device with two interface. Interface A be usbnet; interface B
something you page on. Now consider that you can only resume both interfaces
and this is (and needs to be) done synchronously.

Now we can have this code path:

autoresume of device -> resume() -> kmalloc(..., GFP_KERNEL) ->
VM layer decides to start paging out -> IO to interface B -> autoresume of device
--> DEADLOCK

We need to use GFP_NOIO in situations the helper cannot know about.
Please add a gfp_t parameter. Then the caller will solve that.

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> No, the problem is autoresume.
>
> Suppose we have a device with two interface. Interface A be usbnet; interface B
> something you page on. Now consider that you can only resume both interfaces
> and this is (and needs to be) done synchronously.
>
> Now we can have this code path:
>
> autoresume of device -> resume() -> kmalloc(..., GFP_KERNEL) ->
> VM layer decides to start paging out -> IO to interface B -> autoresume of device
> --> DEADLOCK

OK, thanks for your detailed explanation.

> We need to use GFP_NOIO in situations the helper cannot know about.
> Please add a gfp_t parameter. Then the caller will solve that.

Considered that most of drivers call the helpers in different context, I think
it is better to switch the gpf_t flag runtime inside helpers, like below:

           if (dev->power.runtime_status == RPM_RESUMING)
                   gfp = GFP_NOIO;
          else
                   gfp = GFP_KERNEL;

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

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Wednesday 10 October 2012 16:17:25 Ming Lei wrote:
> On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:

> > We need to use GFP_NOIO in situations the helper cannot know about.
> > Please add a gfp_t parameter. Then the caller will solve that.
> 
> Considered that most of drivers call the helpers in different context, I think
> it is better to switch the gpf_t flag runtime inside helpers, like below:
> 
>            if (dev->power.runtime_status == RPM_RESUMING)
>                    gfp = GFP_NOIO;
>           else
>                    gfp = GFP_KERNEL;

You are admirably persistent ;-)

If you extended the check to RPM_SUSPENDING it might work,
but still the problem with error handling exists.

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 4:39 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> On Wednesday 10 October 2012 16:17:25 Ming Lei wrote:
>> On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> > We need to use GFP_NOIO in situations the helper cannot know about.
>> > Please add a gfp_t parameter. Then the caller will solve that.
>>
>> Considered that most of drivers call the helpers in different context, I think
>> it is better to switch the gpf_t flag runtime inside helpers, like below:
>>
>>            if (dev->power.runtime_status == RPM_RESUMING)
>>                    gfp = GFP_NOIO;
>>           else
>>                    gfp = GFP_KERNEL;
>
> You are admirably persistent ;-)

I am only trying to solve the problem more generally, :-)

> If you extended the check to RPM_SUSPENDING it might work,
> but still the problem with error handling exists.

Could you describe the error handling case in a bit detail so
that callers of these helpers can know when GFP_KERNEL
is to be passed and when GFP_NOIO is taken if the gfp
patamerer has to be added?

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

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Wednesday 10 October 2012 17:48:54 Ming Lei wrote:
> On Wed, Oct 10, 2012 at 4:39 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > On Wednesday 10 October 2012 16:17:25 Ming Lei wrote:
> >> On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum@suse.de> wrote:
> >
> >> > We need to use GFP_NOIO in situations the helper cannot know about.
> >> > Please add a gfp_t parameter. Then the caller will solve that.
> >>
> >> Considered that most of drivers call the helpers in different context, I think
> >> it is better to switch the gpf_t flag runtime inside helpers, like below:
> >>
> >>            if (dev->power.runtime_status == RPM_RESUMING)
> >>                    gfp = GFP_NOIO;
> >>           else
> >>                    gfp = GFP_KERNEL;
> >
> > You are admirably persistent ;-)
> 
> I am only trying to solve the problem more generally, :-)

The most generic solution is passing the parameter.

> > If you extended the check to RPM_SUSPENDING it might work,
> > but still the problem with error handling exists.
> 
> Could you describe the error handling case in a bit detail so
> that callers of these helpers can know when GFP_KERNEL
> is to be passed and when GFP_NOIO is taken if the gfp
> patamerer has to be added?

A reset always applies to the whole device. Resets are used in error
handling of block devices (storage and uas). If you reset a device,
pre_reset() and post_reset() of all interfaces need to be called. So they
are part of the SCSI error handler. SCSI error handlers can allocate memory
only with GFP_NOIO (or GFP_ATOMIC) because any IO for paging
can cause the SCSI layer to wait for the error handling to finish. The error
handling can only finish when pre/post_reset() have finished. Catch-22

So any control messages in block error handling need to use GFP_NOIO.
If you look at the control message helpers in usbcore you will find a lot
of GFP_NOIO. That is the reason.

	Regards
		Oliver

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 6:08 PM, Oliver Neukum <oneukum@suse.de> wrote:

>
> A reset always applies to the whole device. Resets are used in error
> handling of block devices (storage and uas). If you reset a device,
> pre_reset() and post_reset() of all interfaces need to be called. So they
> are part of the SCSI error handler. SCSI error handlers can allocate memory
> only with GFP_NOIO (or GFP_ATOMIC) because any IO for paging
> can cause the SCSI layer to wait for the error handling to finish. The error
> handling can only finish when pre/post_reset() have finished. Catch-22

IMO, it is not practical to obey the rule for drivers, because driver may
call many other kernel component API which may allocate memory
via GFP_KERNEL in the path easily.

Same with runtime PM case.

>
> So any control messages in block error handling need to use GFP_NOIO.
> If you look at the control message helpers in usbcore you will find a lot
> of GFP_NOIO. That is the reason.

If one driver has no .pre_reset or .post_reset, usb_reset_device() will
try to unbind&bind the interface, so you mean these usb drivers have
to use GFP_NOIO to allocate memory in its probe() and disconnect()?
Unfortunately, that is not true, and no way to make sure all memory
allocations in the path via GFP_NOIO, IMO.

Also, only very few drivers have implemented .pre_reset and .post_reset.

Thanks,
--
Ming Lei

RE: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[David Laight]

> On Wed, Oct 10, 2012 at 6:08 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > A reset always applies to the whole device. Resets are used in error
> > handling of block devices (storage and uas). If you reset a device,
> > pre_reset() and post_reset() of all interfaces need to be called. So they
> > are part of the SCSI error handler. SCSI error handlers can allocate memory
> > only with GFP_NOIO (or GFP_ATOMIC) because any IO for paging
> > can cause the SCSI layer to wait for the error handling to finish. The error
> > handling can only finish when pre/post_reset() have finished. Catch-22
> 
> IMO, it is not practical to obey the rule for drivers, because driver may
> call many other kernel component API which may allocate memory
> via GFP_KERNEL in the path easily.

What about the error handler/sleep/resume code calling into the
memory allocator to indicate that all allocates be GFP_NOIO until
it calls back to indicate that the restricted path is complete.
Might be a per-cpu count?

	David



--
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 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Wednesday 10 October 2012 12:25:58 David Laight wrote:
> > On Wed, Oct 10, 2012 at 6:08 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> > > A reset always applies to the whole device. Resets are used in error
> > > handling of block devices (storage and uas). If you reset a device,
> > > pre_reset() and post_reset() of all interfaces need to be called. So they
> > > are part of the SCSI error handler. SCSI error handlers can allocate memory
> > > only with GFP_NOIO (or GFP_ATOMIC) because any IO for paging
> > > can cause the SCSI layer to wait for the error handling to finish. The error
> > > handling can only finish when pre/post_reset() have finished. Catch-22
> > 
> > IMO, it is not practical to obey the rule for drivers, because driver may
> > call many other kernel component API which may allocate memory
> > via GFP_KERNEL in the path easily.
> 
> What about the error handler/sleep/resume code calling into the
> memory allocator to indicate that all allocates be GFP_NOIO until
> it calls back to indicate that the restricted path is complete.

This seems to be a very complex scheme.

> Might be a per-cpu count?

No. The handlers may sleep and switch CPUs.

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 7:25 PM, David Laight <David.Laight@aculab.com> wrote:

>
> What about the error handler/sleep/resume code calling into the
> memory allocator to indicate that all allocates be GFP_NOIO until
> it calls back to indicate that the restricted path is complete.
> Might be a per-cpu count?

IMO, it might be a per-task variable, but unfortunately no such
mechanism is provided by current kernel.

Thanks,
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> No, the problem is autoresume.
>
> Suppose we have a device with two interface. Interface A be usbnet; interface B
> something you page on. Now consider that you can only resume both interfaces
> and this is (and needs to be) done synchronously.
>
> Now we can have this code path:
>
> autoresume of device -> resume() -> kmalloc(..., GFP_KERNEL) ->
> VM layer decides to start paging out -> IO to interface B -> autoresume of device
> --> DEADLOCK

Currently scsi disk can only be runtime suspended when the device is not
opened, so are you sure that the paging out above can cause IO on a suspend
usb mass storage disk which is not mounted or opened by utility now?


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

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Thursday 11 October 2012 11:18:22 Ming Lei wrote:
> On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> >
> > No, the problem is autoresume.
> >
> > Suppose we have a device with two interface. Interface A be usbnet; interface B
> > something you page on. Now consider that you can only resume both interfaces
> > and this is (and needs to be) done synchronously.
> >
> > Now we can have this code path:
> >
> > autoresume of device -> resume() -> kmalloc(..., GFP_KERNEL) ->
> > VM layer decides to start paging out -> IO to interface B -> autoresume of device
> > --> DEADLOCK
> 
> Currently scsi disk can only be runtime suspended when the device is not
> opened, so are you sure that the paging out above can cause IO on a suspend
> usb mass storage disk which is not mounted or opened by utility now?

We definitely do not wish to keep it that way. People at Intel are currently working
on better power management for sd, which would allow full autosuspend.

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Thu, Oct 11, 2012 at 12:11 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Thursday 11 October 2012 11:18:22 Ming Lei wrote:

>> Currently scsi disk can only be runtime suspended when the device is not
>> opened, so are you sure that the paging out above can cause IO on a suspend
>> usb mass storage disk which is not mounted or opened by utility now?
>
> We definitely do not wish to keep it that way. People at Intel are currently working
> on better power management for sd, which would allow full autosuspend.

OK, got it.

For auto-resume situation, it can be solved with switching the gpf_t flag
runtime inside helper, but I think it is better to do it after the sd's
full autosuspend is seen in -next tree.

For error handling case, it is inevitably for usbnet to allocate memory
with GFP_KERNEL because no usbnet drivers have implemented
.pre_reset and .post_reset callback, and no such actual problems
have been reported until now, so it should be OK to not consider the
case now.

So could we merge the patch set[1-11] first?

Thanks,
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Thursday 11 October 2012 16:14:02 Ming Lei wrote:
> On Thu, Oct 11, 2012 at 12:11 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > On Thursday 11 October 2012 11:18:22 Ming Lei wrote:
> 
> >> Currently scsi disk can only be runtime suspended when the device is not
> >> opened, so are you sure that the paging out above can cause IO on a suspend
> >> usb mass storage disk which is not mounted or opened by utility now?
> >
> > We definitely do not wish to keep it that way. People at Intel are currently working
> > on better power management for sd, which would allow full autosuspend.
> 
> OK, got it.
> 
> For auto-resume situation, it can be solved with switching the gpf_t flag
> runtime inside helper, but I think it is better to do it after the sd's
> full autosuspend is seen in -next tree.

That depends on whether an API change would be necessary.
Changing the code only when necessary is no problem. But the
API I want to do right from the beginning if that is possible.

This depends on whether you get in your extension to task_struct.
If you do, we can certainly use it also for the suspend case.

> For error handling case, it is inevitably for usbnet to allocate memory
> with GFP_KERNEL because no usbnet drivers have implemented
> .pre_reset and .post_reset callback, and no such actual problems
> have been reported until now, so it should be OK to not consider the
> case now.
> 
> So could we merge the patch set[1-11] first?

Given the solution for error handling you came up with, yes, we can.
Would you resubmit?

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Thu, Oct 11, 2012 at 5:05 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> That depends on whether an API change would be necessary.
> Changing the code only when necessary is no problem. But the
> API I want to do right from the beginning if that is possible.

For the auto-resume situation, the current API is OK even without
changes to task_struct.

>
> This depends on whether you get in your extension to task_struct.
> If you do, we can certainly use it also for the suspend case.

I will do it.

>
>> For error handling case, it is inevitably for usbnet to allocate memory
>> with GFP_KERNEL because no usbnet drivers have implemented
>> .pre_reset and .post_reset callback, and no such actual problems
>> have been reported until now, so it should be OK to not consider the
>> case now.
>>
>> So could we merge the patch set[1-11] first?
>
> Given the solution for error handling you came up with, yes, we can.
> Would you resubmit?

OK, will do it.

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

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Alan Stern]

On Thu, 11 Oct 2012, Oliver Neukum wrote:

> On Thursday 11 October 2012 11:18:22 Ming Lei wrote:
> > On Wed, Oct 10, 2012 at 1:51 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > >
> > > No, the problem is autoresume.
> > >
> > > Suppose we have a device with two interface. Interface A be usbnet; interface B
> > > something you page on. Now consider that you can only resume both interfaces
> > > and this is (and needs to be) done synchronously.
> > >
> > > Now we can have this code path:
> > >
> > > autoresume of device -> resume() -> kmalloc(..., GFP_KERNEL) ->
> > > VM layer decides to start paging out -> IO to interface B -> autoresume of device
> > > --> DEADLOCK
> > 
> > Currently scsi disk can only be runtime suspended when the device is not
> > opened, so are you sure that the paging out above can cause IO on a suspend
> > usb mass storage disk which is not mounted or opened by utility now?
> 
> We definitely do not wish to keep it that way. People at Intel are currently working
> on better power management for sd, which would allow full autosuspend.

It's worse than you may realize.  When a SCSI disk is suspended, all of
its ancestor devices may be suspended too.  Pages can't be read in from
the drive until all those ancestors are resumed.  This means that all
runtime resume code paths for all drivers that could be bound to an
ancestor of a block device must avoid GFP_KERNEL.  In practice it's
probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
before calling any runtime_resume method.

Or at least, this will be true when sd supports nontrivial autosuspend.

Alan Stern

--
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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Thu, Oct 11, 2012 at 10:36 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

>
> It's worse than you may realize.  When a SCSI disk is suspended, all of
> its ancestor devices may be suspended too.  Pages can't be read in from
> the drive until all those ancestors are resumed.  This means that all
> runtime resume code paths for all drivers that could be bound to an
> ancestor of a block device must avoid GFP_KERNEL.  In practice it's

Exactly, so several subsystems(for example, pci, usb, scsi) will be involved,
and converting GFP_KERNEL in runtime PM path to GFP_NOIO becomes
more difficult.

> probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
> before calling any runtime_resume method.

Yes, it might be done in usb runtime resume context because all
usb device might include a mass storage interface. But, in fact,
we can find if there is one mass storage interface on the current
configuration easily inside usb_runtime_resume().

Also, we can loose the constraint in runtime PM core, before calling
runtime_resume callback for one device, the current context is marked
as ~GFP_IOFS only if it is a block device or there is one block device
descendant. But the approach becomes a bit complicated because
device tree traversing is involved.


Thanks,
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Alan Stern]

On Fri, 12 Oct 2012, Ming Lei wrote:

> > probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
> > before calling any runtime_resume method.
> 
> Yes, it might be done in usb runtime resume context because all
> usb device might include a mass storage interface. But, in fact,
> we can find if there is one mass storage interface on the current
> configuration easily inside usb_runtime_resume().
> 
> Also, we can loose the constraint in runtime PM core, before calling
> runtime_resume callback for one device, the current context is marked
> as ~GFP_IOFS only if it is a block device or there is one block device
> descendant. But the approach becomes a bit complicated because
> device tree traversing is involved.

Exactly.  It's much easier just to do it always.

Alan Stern

--
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 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Thursday 11 October 2012 10:36:22 Alan Stern wrote:

> It's worse than you may realize.  When a SCSI disk is suspended, all of
> its ancestor devices may be suspended too.  Pages can't be read in from
> the drive until all those ancestors are resumed.  This means that all
> runtime resume code paths for all drivers that could be bound to an
> ancestor of a block device must avoid GFP_KERNEL.  In practice it's
> probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
> before calling any runtime_resume method.
> 
> Or at least, this will be true when sd supports nontrivial autosuspend.

Up to now, I've found three driver for which tsk_set_allowd_gfp() wouldn't
do the job. They boil down into two types of errors. That is surprisingly good.

First we have workqueues. bas-gigaset is a good example.
The driver kills a scheduled work in pre_reset(). If this is done synchronously
the driver may need to wait for a memory allocation inside the work.
In principle we could provide a workqueue limited to GFP_NOIO. Is that worth
it, or do we just check?

Second there is a problem just like priority inversion with realtime tasks.
usb-skeleton and ati_remote2
They take mutexes which are also taken in other code paths. So the error
handler may need to wait for a mutex to be dropped which can only happen
if a memory allocation succeeds, which is waiting for the error handler.

usb-skeleton is even worse, as it does copy_to_user(). I guess copy_to/from_user
must simply not be done under such a mutex.

I am afraid there is no generic solution in the last two cases. What do you think?

	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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Fri, Oct 12, 2012 at 9:51 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Thursday 11 October 2012 10:36:22 Alan Stern wrote:
>
>> It's worse than you may realize.  When a SCSI disk is suspended, all of
>> its ancestor devices may be suspended too.  Pages can't be read in from
>> the drive until all those ancestors are resumed.  This means that all
>> runtime resume code paths for all drivers that could be bound to an
>> ancestor of a block device must avoid GFP_KERNEL.  In practice it's
>> probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
>> before calling any runtime_resume method.
>>
>> Or at least, this will be true when sd supports nontrivial autosuspend.
>
> Up to now, I've found three driver for which tsk_set_allowd_gfp() wouldn't
> do the job. They boil down into two types of errors. That is surprisingly good.

Looks all are very good examples, :-)

>
> First we have workqueues. bas-gigaset is a good example.
> The driver kills a scheduled work in pre_reset(). If this is done synchronously
> the driver may need to wait for a memory allocation inside the work.
> In principle we could provide a workqueue limited to GFP_NOIO. Is that worth
> it, or do we just check?

The easiest way is to always call tsk_set_allowd_gfp(~GFP_IOFS) in the
start of work function under the situation, and restore the flag in the end
of the work function.

>
> Second there is a problem just like priority inversion with realtime tasks.
> usb-skeleton and ati_remote2
> They take mutexes which are also taken in other code paths. So the error
> handler may need to wait for a mutex to be dropped which can only happen
> if a memory allocation succeeds, which is waiting for the error handler.

Suppose mutex_lock(A) is called in pre_reset(), one solution is that
always calling tsk_set_allowd_gfp(~GFP_IOFS) before each mutex_lock(A).
We can do it only for devices with storage interface in current
configuration.


Thanks，
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Fri, Oct 12, 2012 at 11:17 PM, Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:

> Suppose mutex_lock(A) is called in pre_reset(), one solution is that
> always calling tsk_set_allowd_gfp(~GFP_IOFS) before each mutex_lock(A).
> We can do it only for devices with storage interface in current
> configuration.

The problem will become quite complicated if a 3rd, even 4th,... context
is involved in the task dependency.

But I am wondering if there are such practical examples wrt. usb
mass storage.

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

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Alan Stern]

On Fri, 12 Oct 2012, Oliver Neukum wrote:

> On Thursday 11 October 2012 10:36:22 Alan Stern wrote:
> 
> > It's worse than you may realize.  When a SCSI disk is suspended, all of
> > its ancestor devices may be suspended too.  Pages can't be read in from
> > the drive until all those ancestors are resumed.  This means that all
> > runtime resume code paths for all drivers that could be bound to an
> > ancestor of a block device must avoid GFP_KERNEL.  In practice it's
> > probably easiest for the runtime PM core to use tsk_set_allowd_gfp()
> > before calling any runtime_resume method.
> > 
> > Or at least, this will be true when sd supports nontrivial autosuspend.
> 
> Up to now, I've found three driver for which tsk_set_allowd_gfp() wouldn't
> do the job. They boil down into two types of errors. That is surprisingly good.
> 
> First we have workqueues. bas-gigaset is a good example.
> The driver kills a scheduled work in pre_reset(). If this is done synchronously
> the driver may need to wait for a memory allocation inside the work.
> In principle we could provide a workqueue limited to GFP_NOIO. Is that worth
> it, or do we just check?

The work routine could set the GFP mask upon entry and exit.  Then a 
separate workqueue wouldn't be needed.

> Second there is a problem just like priority inversion with realtime tasks.
> usb-skeleton and ati_remote2
> They take mutexes which are also taken in other code paths. So the error
> handler may need to wait for a mutex to be dropped which can only happen
> if a memory allocation succeeds, which is waiting for the error handler.
> 
> usb-skeleton is even worse, as it does copy_to_user(). I guess copy_to/from_user
> must simply not be done under such a mutex.

Right.

> I am afraid there is no generic solution in the last two cases. What do you think?

The other contexts must also set the GFP mask.  Unfortunately, this has 
to be done case-by-case.

Alan Stern

--
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 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Friday 12 October 2012 11:29:49 Alan Stern wrote:
> On Fri, 12 Oct 2012, Oliver Neukum wrote:

> > First we have workqueues. bas-gigaset is a good example.
> > The driver kills a scheduled work in pre_reset(). If this is done synchronously
> > the driver may need to wait for a memory allocation inside the work.
> > In principle we could provide a workqueue limited to GFP_NOIO. Is that worth
> > it, or do we just check?
> 
> The work routine could set the GFP mask upon entry and exit.  Then a 
> separate workqueue wouldn't be needed.

Well, yes. But if we have to touch the code we might just as well use GFP-NOIO

> > I am afraid there is no generic solution in the last two cases. What do you think?
> 
> The other contexts must also set the GFP mask.  Unfortunately, this has 
> to be done case-by-case.

This raises a question. If we do the port-power-off stuff, does reset_resume() of every
device under the depowered port have to be called? If so, we cannot exclude vendor
specific drivers from the audit, can we?

	Regards
		Oliver

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Alan Stern]

On Mon, 15 Oct 2012, Oliver Neukum wrote:

> On Friday 12 October 2012 11:29:49 Alan Stern wrote:
> > On Fri, 12 Oct 2012, Oliver Neukum wrote:
> 
> > > First we have workqueues. bas-gigaset is a good example.
> > > The driver kills a scheduled work in pre_reset(). If this is done synchronously
> > > the driver may need to wait for a memory allocation inside the work.
> > > In principle we could provide a workqueue limited to GFP_NOIO. Is that worth
> > > it, or do we just check?
> > 
> > The work routine could set the GFP mask upon entry and exit.  Then a 
> > separate workqueue wouldn't be needed.
> 
> Well, yes. But if we have to touch the code we might just as well use GFP-NOIO

Depends on what the code does.  If it does very little requiring memory 
allocation then yes, you could simply use GFP_NOIO.  But if it calls 
lots of other routines that all do their own allocation, setting the 
mask will be better.

> > > I am afraid there is no generic solution in the last two cases. What do you think?
> > 
> > The other contexts must also set the GFP mask.  Unfortunately, this has 
> > to be done case-by-case.
> 
> This raises a question. If we do the port-power-off stuff, does reset_resume() of every
> device under the depowered port have to be called?

Certainly.

>  If so, we cannot exclude vendor
> specific drivers from the audit, can we?

True.  But hasn't that always been the case?  A device could need a 
vendor-specific driver for one interface while another interface uses a 
standard mass-storage protocol.

Alan Stern

--
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 01/12] usbnet: introduce usbnet 3 command helpers

[Ming Lei]

On Wed, Oct 10, 2012 at 11:19 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, Oct 9, 2012 at 4:47 PM, Oliver Neukum <oneukum@suse.de> wrote:
>>
>> Using GFP_KERNEL you preclude using those in resume() and error handling.
>> Please pass a gfp_t parameter.
>
> IMO, it is not a big deal because generally only several bytes are to be
> allocated inside these helpers.

Also pm_restrict_gfp_mask()/pm_restore_gfp_mask() have been introduced
to address the problem for 2 years, looks the gfp_t isn't needed, doesn't it?

Thanks,
--
Ming Lei

Re: [PATCH 01/12] usbnet: introduce usbnet 3 command helpers

[Oliver Neukum]

On Wednesday 10 October 2012 13:56:16 Ming Lei wrote:
> On Wed, Oct 10, 2012 at 11:19 AM, Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> > On Tue, Oct 9, 2012 at 4:47 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> >>
> >> Using GFP_KERNEL you preclude using those in resume() and error handling.
> >> Please pass a gfp_t parameter.
> >
> > IMO, it is not a big deal because generally only several bytes are to be
> > allocated inside these helpers.
> 
> Also pm_restrict_gfp_mask()/pm_restore_gfp_mask() have been introduced
> to address the problem for 2 years, looks the gfp_t isn't needed, doesn't it?

No, absolutely not. Introducing them was a mistake and is hiding errors.
Those helpers solve the problem only for the case of _system_ suspend/resume.
However the runtime case has the same problem. So in addition to not solving
the problem, we now have two code paths.
Frankly, those functions should be removed.

Secondly, in this case a similar deadlock exists with error handling.
Again take a device with network and storage functions (a.k.a. cell phone).
The storage function does a reset. And the deadlock happens like this:

reset storage -> pre_reset() -> physical reset -> post_reset() -> net interface
does a control message -> kmalloc(..., GFP_KERNEL) -> VM layer decide
to page out -> IO to storage function -> SCSI layer waits for error handler --> DEADLOCK

Believe me, you won't find a fancy solution for this. Just pass the gfp_t.

	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

[PATCH 02/12] usbnet: asix: apply introduced usb command APIs

[Ming Lei]

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

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 774d9ce..50d1673 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -25,121 +25,30 @@
 int asix_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
 		  u16 size, void *data)
 {
-	void *buf;
-	int err = -ENOMEM;
-
-	netdev_dbg(dev->net, "asix_read_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
-		   cmd, value, index, size);
-
-	buf = kmalloc(size, GFP_KERNEL);
-	if (!buf)
-		goto out;
-
-	err = usb_control_msg(
-		dev->udev,
-		usb_rcvctrlpipe(dev->udev, 0),
-		cmd,
-		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		value,
-		index,
-		buf,
-		size,
-		USB_CTRL_GET_TIMEOUT);
-	if (err == size)
-		memcpy(data, buf, size);
-	else if (err >= 0)
-		err = -EINVAL;
-	kfree(buf);
+	int ret;
+	ret = usbnet_read_cmd(dev, cmd,
+			       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			       value, index, data, size);
 
-out:
-	return err;
+	if (ret != size && ret >= 0)
+		return -EINVAL;
+	return ret;
 }
 
 int asix_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
 		   u16 size, void *data)
 {
-	void *buf = NULL;
-	int err = -ENOMEM;
-
-	netdev_dbg(dev->net, "asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
-		   cmd, value, index, size);
-
-	if (data) {
-		buf = kmemdup(data, size, GFP_KERNEL);
-		if (!buf)
-			goto out;
-	}
-
-	err = usb_control_msg(
-		dev->udev,
-		usb_sndctrlpipe(dev->udev, 0),
-		cmd,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		value,
-		index,
-		buf,
-		size,
-		USB_CTRL_SET_TIMEOUT);
-	kfree(buf);
-
-out:
-	return err;
-}
-
-static void asix_async_cmd_callback(struct urb *urb)
-{
-	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
-	int status = urb->status;
-
-	if (status < 0)
-		printk(KERN_DEBUG "asix_async_cmd_callback() failed with %d",
-			status);
-
-	kfree(req);
-	usb_free_urb(urb);
+	return usbnet_write_cmd(dev, cmd,
+				USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				value, index, data, size);
 }
 
 void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
 			  u16 size, void *data)
 {
-	struct usb_ctrlrequest *req;
-	int status;
-	struct urb *urb;
-
-	netdev_dbg(dev->net, "asix_write_cmd_async() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
-		   cmd, value, index, size);
-
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		netdev_err(dev->net, "Error allocating URB in write_cmd_async!\n");
-		return;
-	}
-
-	req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
-	if (!req) {
-		netdev_err(dev->net, "Failed to allocate memory for control request\n");
-		usb_free_urb(urb);
-		return;
-	}
-
-	req->bRequestType = USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE;
-	req->bRequest = cmd;
-	req->wValue = cpu_to_le16(value);
-	req->wIndex = cpu_to_le16(index);
-	req->wLength = cpu_to_le16(size);
-
-	usb_fill_control_urb(urb, dev->udev,
-			     usb_sndctrlpipe(dev->udev, 0),
-			     (void *)req, data, size,
-			     asix_async_cmd_callback, req);

[PATCH 07/12] usbnet: net1080: apply introduced usb command APIs

[Ming Lei]

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

diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
index c062a3e..93e0716 100644
--- a/drivers/net/usb/net1080.c
+++ b/drivers/net/usb/net1080.c
@@ -109,13 +109,11 @@ struct nc_trailer {
 static int
 nc_vendor_read(struct usbnet *dev, u8 req, u8 regnum, u16 *retval_ptr)
 {
-	int status = usb_control_msg(dev->udev,
-		usb_rcvctrlpipe(dev->udev, 0),
-		req,
-		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		0, regnum,
-		retval_ptr, sizeof *retval_ptr,
-		USB_CTRL_GET_TIMEOUT);
+	int status = usbnet_read_cmd(dev, req,
+				     USB_DIR_IN | USB_TYPE_VENDOR |
+				     USB_RECIP_DEVICE,
+				     0, regnum, retval_ptr,
+				     sizeof *retval_ptr);
 	if (status > 0)
 		status = 0;
 	if (!status)
@@ -133,13 +131,9 @@ nc_register_read(struct usbnet *dev, u8 regnum, u16 *retval_ptr)
 static void
 nc_vendor_write(struct usbnet *dev, u8 req, u8 regnum, u16 value)
 {
-	usb_control_msg(dev->udev,
-		usb_sndctrlpipe(dev->udev, 0),
-		req,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		value, regnum,
-		NULL, 0,			// data is in setup packet
-		USB_CTRL_SET_TIMEOUT);
+	usbnet_write_cmd(dev, req,
+			 USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			 value, regnum, NULL, 0);
 }
 
 static inline void
@@ -288,37 +282,34 @@ static inline void nc_dump_ttl(struct usbnet *dev, u16 ttl)
 static int net1080_reset(struct usbnet *dev)
 {
 	u16		usbctl, status, ttl;
-	u16		*vp = kmalloc(sizeof (u16), GFP_KERNEL);
+	u16		vp;
 	int		retval;
 
-	if (!vp)
-		return -ENOMEM;
-
 	// nc_dump_registers(dev);
 
-	if ((retval = nc_register_read(dev, REG_STATUS, vp)) < 0) {
+	if ((retval = nc_register_read(dev, REG_STATUS, &vp)) < 0) {
 		netdev_dbg(dev->net, "can't read %s-%s status: %d\n",
 			   dev->udev->bus->bus_name, dev->udev->devpath, retval);
 		goto done;
 	}
-	status = *vp;
+	status = vp;
 	nc_dump_status(dev, status);
 
-	if ((retval = nc_register_read(dev, REG_USBCTL, vp)) < 0) {
+	if ((retval = nc_register_read(dev, REG_USBCTL, &vp)) < 0) {
 		netdev_dbg(dev->net, "can't read USBCTL, %d\n", retval);
 		goto done;
 	}
-	usbctl = *vp;
+	usbctl = vp;
 	nc_dump_usbctl(dev, usbctl);
 
 	nc_register_write(dev, REG_USBCTL,
 			USBCTL_FLUSH_THIS | USBCTL_FLUSH_OTHER);
 
-	if ((retval = nc_register_read(dev, REG_TTL, vp)) < 0) {
+	if ((retval = nc_register_read(dev, REG_TTL, &vp)) < 0) {
 		netdev_dbg(dev->net, "can't read TTL, %d\n", retval);
 		goto done;
 	}
-	ttl = *vp;
+	ttl = vp;
 	// nc_dump_ttl(dev, ttl);
 
 	nc_register_write(dev, REG_TTL,
@@ -331,7 +322,6 @@ static int net1080_reset(struct usbnet *dev)
 	retval = 0;
 
 done:
-	kfree(vp);
 	return retval;
 }
 
@@ -339,13 +329,10 @@ static int net1080_check_connect(struct usbnet *dev)
 {
 	int			retval;
 	u16			status;
-	u16			*vp = kmalloc(sizeof (u16), GFP_KERNEL);
+	u16			vp;
 
-	if (!vp)
-		return -ENOMEM;
-	retval = nc_register_read(dev, REG_STATUS, vp);
-	status = *vp;
-	kfree(vp);
+	retval = nc_register_read(dev, REG_STATUS, &vp);
+	status = vp;
 	if (retval != 0) {
 		netdev_dbg(dev->net, "net1080_check_conn read - %d\n", retval);
 		return retval;
@@ -355,59 +342,22 @@ static int net1080_check_connect(struct usbnet *dev)
 	return 0;
 }
 
-static void nc_flush_complete(struct urb *urb)
-{
-	kfree(urb->context);
-	usb_free_urb(urb);
-}
-
 static void nc_ensure_sync(struct usbnet *dev)
 {
-	dev->frame_errors++;
-	if (dev->frame_errors > 5) {
-		struct urb		*urb;
-		struct usb_ctrlrequest	*req;
-		int			status;
-
-		/* Send a flush */
-		urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (!urb)
-			return;
-
-		req = kmalloc(sizeof *req, GFP_ATOMIC);
-		if (!req) {
-			usb_free_urb(urb);
-			return;
-		}
+	if (++dev->frame_errors <= 5)
+		return;
 
-		req->bRequestType = USB_DIR_OUT
-			| USB_TYPE_VENDOR
-			| USB_RECIP_DEVICE;
-		req->bRequest = REQUEST_REGISTER;
-		req->wValue = cpu_to_le16(USBCTL_FLUSH_THIS
-				| USBCTL_FLUSH_OTHER);
-		req->wIndex = cpu_to_le16(REG_USBCTL);
-		req->wLength = cpu_to_le16(0);
-
-		/* queue an async control request, we don't need
-		 * to do anything when it finishes except clean up.
-		 */
-		usb_fill_control_urb(urb, dev->udev,
-			usb_sndctrlpipe(dev->udev, 0),
-			(unsigned char *) req,
-			NULL, 0,
-			nc_flush_complete, req);
-		status = usb_submit_urb(urb, GFP_ATOMIC);
-		if (status) {
-			kfree(req);
-			usb_free_urb(urb);
-			return;
-		}
+	if (usbnet_write_cmd_async(dev, REQUEST_REGISTER,
+					USB_DIR_OUT | USB_TYPE_VENDOR |
+					USB_RECIP_DEVICE,
+					USBCTL_FLUSH_THIS |
+					USBCTL_FLUSH_OTHER,
+					REG_USBCTL, NULL, 0))
+		return;
 
-		netif_dbg(dev, rx_err, dev->net,
-			  "flush net1080; too many framing errors\n");
-		dev->frame_errors = 0;
-	}
+	netif_dbg(dev, rx_err, dev->net,
+		  "flush net1080; too many framing errors\n");
+	dev->frame_errors = 0;
 }
 
 static int net1080_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
-- 
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 08/12] usbnet: plusb: apply introduced usb command APIs

[Ming Lei]

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

diff --git a/drivers/net/usb/plusb.c b/drivers/net/usb/plusb.c
index 4584b9a..0fcc8e6 100644
--- a/drivers/net/usb/plusb.c
+++ b/drivers/net/usb/plusb.c
@@ -71,13 +71,10 @@
 static inline int
 pl_vendor_req(struct usbnet *dev, u8 req, u8 val, u8 index)
 {
-	return usb_control_msg(dev->udev,
-		usb_rcvctrlpipe(dev->udev, 0),
-		req,
-		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		val, index,
-		NULL, 0,
-		USB_CTRL_GET_TIMEOUT);
+	return usbnet_read_cmd(dev, req,
+				USB_DIR_IN | USB_TYPE_VENDOR |
+				USB_RECIP_DEVICE,
+				val, index, NULL, 0);
 }
 
 static inline int
-- 
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 11/12] usbnet: smsc95xx: apply introduced usb command APIs

[Ming Lei]

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

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 7479a57..1730f75 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -65,11 +65,6 @@ struct smsc95xx_priv {
 	spinlock_t mac_cr_lock;
 };
 
-struct usb_context {
-	struct usb_ctrlrequest req;
-	struct usbnet *dev;
-};
-
 static bool turbo_mode = true;
 module_param(turbo_mode, bool, 0644);
 MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
@@ -77,25 +72,20 @@ 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)
 {
-	u32 *buf = kmalloc(4, GFP_KERNEL);
+	u32 buf;
 	int ret;
 
 	BUG_ON(!dev);
 
-	if (!buf)
-		return -ENOMEM;
-
-	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-		USB_VENDOR_REQUEST_READ_REGISTER,
-		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, buf, 4, USB_CTRL_GET_TIMEOUT);
-
+	ret = usbnet_read_cmd(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);
 
-	le32_to_cpus(buf);
-	*data = *buf;
-	kfree(buf);
+	le32_to_cpus(&buf);
+	*data = buf;
 
 	return ret;
 }
@@ -103,27 +93,22 @@ static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
 static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
 					   u32 data)
 {
-	u32 *buf = kmalloc(4, GFP_KERNEL);
+	u32 buf;
 	int ret;
 
 	BUG_ON(!dev);
 
-	if (!buf)
-		return -ENOMEM;
-
-	*buf = data;
-	cpu_to_le32s(buf);
+	buf = data;
+	cpu_to_le32s(&buf);
 
-	ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		USB_VENDOR_REQUEST_WRITE_REGISTER,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, buf, 4, USB_CTRL_SET_TIMEOUT);
 
+	ret = usbnet_write_cmd(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);
 
-	kfree(buf);
-
 	return ret;
 }
 
@@ -132,11 +117,8 @@ static int smsc95xx_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(dev, USB_REQ_SET_FEATURE,
+				USB_RECIP_DEVICE, feature, 0, NULL, 0);
 }
 
 static int smsc95xx_clear_feature(struct usbnet *dev, u32 feature)
@@ -144,11 +126,8 @@ static int smsc95xx_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(dev, USB_REQ_CLEAR_FEATURE,
+				USB_RECIP_DEVICE, feature, 0, NULL, 0);
 }
 
 /* Loop until the read is completed with timeout
@@ -350,60 +329,20 @@ static int smsc95xx_write_eeprom(struct usbnet *dev, u32 offset, u32 length,
 	return 0;
 }
 
-static void smsc95xx_async_cmd_callback(struct urb *urb)
-{
-	struct usb_context *usb_context = urb->context;
-	struct usbnet *dev = usb_context->dev;
-	int status = urb->status;
-
-	check_warn(status, "async callback failed with %d\n", status);
-
-	kfree(usb_context);
-	usb_free_urb(urb);
-}
-
 static int __must_check smsc95xx_write_reg_async(struct usbnet *dev, u16 index,
 						 u32 *data)
 {
-	struct usb_context *usb_context;
-	int status;
-	struct urb *urb;
 	const u16 size = 4;
+	int ret;
 
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		netdev_warn(dev->net, "Error allocating URB\n");
-		return -ENOMEM;
-	}
-
-	usb_context = kmalloc(sizeof(struct usb_context), GFP_ATOMIC);
-	if (usb_context == NULL) {
-		netdev_warn(dev->net, "Error allocating control msg\n");
-		usb_free_urb(urb);
-		return -ENOMEM;
-	}
-
-	usb_context->req.bRequestType =
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE;
-	usb_context->req.bRequest = USB_VENDOR_REQUEST_WRITE_REGISTER;
-	usb_context->req.wValue = 00;
-	usb_context->req.wIndex = cpu_to_le16(index);
-	usb_context->req.wLength = cpu_to_le16(size);
-
-	usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		(void *)&usb_context->req, data, size,
-		smsc95xx_async_cmd_callback,
-		(void *)usb_context);
-
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status < 0) {
-		netdev_warn(dev->net, "Error submitting control msg, sts=%d\n",
-			    status);
-		kfree(usb_context);
-		usb_free_urb(urb);
-	}

Re: [PATCH 00/12] usbnet: usb_control_msg cleanup

[Oliver Neukum]

On Tuesday 02 October 2012 14:51:11 Ming Lei wrote:
> Hi,
> 
> This patch set introduces 3 helpers for handling usb read, write
> and write_async command, and replaces the low level's implemention
> with the generic ones.

First, very good idea. I'll get to the individual issues in the individual patches.

	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

[PATCH 09/12] usbnet: sierra_net: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/sierra_net.c |   45 ++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index c27d277..eb5c7a8 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -311,10 +311,9 @@ static int sierra_net_send_cmd(struct usbnet *dev,
 	struct sierra_net_data *priv = sierra_net_get_private(dev);
 	int  status;
 
-	status = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-			USB_CDC_SEND_ENCAPSULATED_COMMAND,
-			USB_DIR_OUT|USB_TYPE_CLASS|USB_RECIP_INTERFACE,	0,
-			priv->ifnum, cmd, cmdlen, USB_CTRL_SET_TIMEOUT);
+	status = usbnet_write_cmd(dev, USB_CDC_SEND_ENCAPSULATED_COMMAND,
+				  USB_DIR_OUT|USB_TYPE_CLASS|USB_RECIP_INTERFACE,
+				  0, priv->ifnum, cmd, cmdlen);
 
 	if (status != cmdlen && status != -ENODEV)
 		netdev_err(dev->net, "Submit %s failed %d\n", cmd_name, status);
@@ -632,32 +631,22 @@ static int sierra_net_change_mtu(struct net_device *net, int new_mtu)
 static int sierra_net_get_fw_attr(struct usbnet *dev, u16 *datap)
 {
 	int result = 0;
-	u16 *attrdata;
-
-	attrdata = kmalloc(sizeof(*attrdata), GFP_KERNEL);
-	if (!attrdata)
-		return -ENOMEM;
-
-	result = usb_control_msg(
-			dev->udev,
-			usb_rcvctrlpipe(dev->udev, 0),
-			/* _u8 vendor specific request */
-			SWI_USB_REQUEST_GET_FW_ATTR,
-			USB_DIR_IN | USB_TYPE_VENDOR,	/* __u8 request type */
-			0x0000,		/* __u16 value not used */
-			0x0000,		/* __u16 index  not used */
-			attrdata,	/* char *data */
-			sizeof(*attrdata),		/* __u16 size */
-			USB_CTRL_SET_TIMEOUT);	/* int timeout */
-
-	if (result < 0) {
-		kfree(attrdata);
+	u16 attrdata;
+
+	result = usbnet_read_cmd(dev,
+				/* _u8 vendor specific request */
+				SWI_USB_REQUEST_GET_FW_ATTR,
+				USB_DIR_IN | USB_TYPE_VENDOR,	/* __u8 request type */
+				0x0000,		/* __u16 value not used */
+				0x0000,		/* __u16 index  not used */
+				&attrdata,	/* char *data */
+				sizeof(attrdata)	/* __u16 size */
+				);
+
+	if (result < 0)
 		return -EIO;
-	}
-
-	*datap = le16_to_cpu(*attrdata);
 
-	kfree(attrdata);
+	*datap = le16_to_cpu(attrdata);
 	return result;
 }
 
-- 
1.7.9.5

[PATCH 10/12] usbnet: smsc75xx: apply introduced usb command APIs

[Ming Lei]

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/smsc75xx.c |   39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index b77ae76..1baa53a 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -85,26 +85,21 @@ 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)
 {
-	u32 *buf = kmalloc(4, GFP_KERNEL);
+	u32 buf;
 	int ret;
 
 	BUG_ON(!dev);
 
-	if (!buf)
-		return -ENOMEM;
-
-	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-		USB_VENDOR_REQUEST_READ_REGISTER,
-		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, buf, 4, USB_CTRL_GET_TIMEOUT);
-
+	ret = usbnet_read_cmd(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);
 
-	le32_to_cpus(buf);
-	*data = *buf;
-	kfree(buf);
+	le32_to_cpus(&buf);
+	*data = buf;
 
 	return ret;
 }
@@ -112,28 +107,22 @@ static int __must_check smsc75xx_read_reg(struct usbnet *dev, u32 index,
 static int __must_check smsc75xx_write_reg(struct usbnet *dev, u32 index,
 					   u32 data)
 {
-	u32 *buf = kmalloc(4, GFP_KERNEL);
+	u32 buf;
 	int ret;
 
 	BUG_ON(!dev);
 
-	if (!buf)
-		return -ENOMEM;
-
-	*buf = data;
-	cpu_to_le32s(buf);
-
-	ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-		USB_VENDOR_REQUEST_WRITE_REGISTER,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, buf, 4, USB_CTRL_SET_TIMEOUT);
+	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);
 	if (unlikely(ret < 0))
 		netdev_warn(dev->net,
 			"Failed to write reg index 0x%08x: %d", index, ret);
 
-	kfree(buf);
-
 	return ret;
 }
 
-- 
1.7.9.5

[PATCH 12/12] usbnet: make device out of suspend before calling usbnet_read/write_cmd

[Ming Lei]

This patche gets the runtime PM reference count before calling
usb_control_msg, and puts it after completion of the
usb_control_msg, so that the usb control message can always be
sent to one active device.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/usbnet.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3b51554..3f4bc69 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1609,9 +1609,11 @@ int usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 			goto out;
 	}
 
+	usb_autopm_get_interface(dev->intf);
 	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
 			      cmd, reqtype, value, index, buf, size,
 			      USB_CTRL_GET_TIMEOUT);
+	usb_autopm_put_interface(dev->intf);
 	if (err > 0 && err <= size)
 		memcpy(data, buf, err);
 	kfree(buf);
@@ -1636,9 +1638,11 @@ int usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 			goto out;
 	}
 
+	usb_autopm_get_interface(dev->intf);
 	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
 			      cmd, reqtype, value, index, buf, size,
 			      USB_CTRL_SET_TIMEOUT);
+	usb_autopm_put_interface(dev->intf);
 	kfree(buf);
 
 out:
-- 
1.7.9.5

Re: [PATCH 12/12] usbnet: make device out of suspend before calling usbnet_read/write_cmd

[Oliver Neukum]

On Tuesday 02 October 2012 14:51:23 Ming Lei wrote:
> This patche gets the runtime PM reference count before calling
> usb_control_msg, and puts it after completion of the
> usb_control_msg, so that the usb control message can always be
> sent to one active device.

This is awkward to use in suspend()/resume()
Could you make both versions available?

	Regards
		Oliver

Re: [PATCH 12/12] usbnet: make device out of suspend before calling usbnet_read/write_cmd

[Ming Lei]

On Tue, Oct 9, 2012 at 4:50 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> This is awkward to use in suspend()/resume()
> Could you make both versions available?

Good catch, thanks for your review.

As far as I can think of, the mutex_is_locked() trick can solve the problem.

How about the attached patch?

Thanks,
--
Ming Lei

--
>From 5c417db4ba2fd13091067104e5afe4554750be11 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Date: Tue, 2 Oct 2012 13:46:56 +0800
Subject: [PATCH] usbnet: make device out of suspend before calling
 usbnet_read/write_cmd

This patche gets the runtime PM reference count before calling
usb_control_msg, and puts it after completion of the
usb_control_msg, so that the usb control message can always be
sent to one active device.

Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/smsc75xx.c |    5 +++++
 drivers/net/usb/smsc95xx.c |    5 +++++
 drivers/net/usb/usbnet.c   |   16 ++++++++++++++++
 include/linux/usb/usbnet.h |    1 +
 4 files changed, 27 insertions(+)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 1baa53a..3d43c4d 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -1153,6 +1153,7 @@ static int smsc75xx_suspend(struct usb_interface
*intf, pm_message_t message)
 	ret = usbnet_suspend(intf, message);
 	check_warn_return(ret, "usbnet_suspend error");

+	mutex_lock(&dev->pm_mutex);
 	/* if no wol options set, enter lowest power SUSPEND2 mode */
 	if (!(pdata->wolopts & SUPPORTED_WAKE)) {
 		netdev_info(dev->net, "entering SUSPEND2 mode");
@@ -1183,6 +1184,7 @@ static int smsc75xx_suspend(struct usb_interface
*intf, pm_message_t message)

 		ret = smsc75xx_write_reg(dev, PMT_CTL, val);
 		check_warn_return(ret, "Error writing PMT_CTL");
+		mutex_unlock(&dev->pm_mutex);

 		return 0;
 	}
@@ -1254,6 +1256,7 @@ static int smsc75xx_suspend(struct usb_interface
*intf, pm_message_t message)
 	check_warn_return(ret, "Error reading PMT_CTL");

 	smsc75xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
+	mutex_unlock(&dev->pm_mutex);

 	return 0;
 }
@@ -1265,6 +1268,7 @@ static int smsc75xx_resume(struct usb_interface *intf)
 	int ret;
 	u32 val;

+	mutex_lock(&dev->pm_mutex);
 	if (pdata->wolopts & WAKE_MAGIC) {
 		netdev_info(dev->net, "resuming from SUSPEND0");

@@ -1302,6 +1306,7 @@ static int smsc75xx_resume(struct usb_interface *intf)

 	ret = smsc75xx_wait_ready(dev);
 	check_warn_return(ret, "device not ready in smsc75xx_resume");
+	mutex_unlock(&dev->pm_mutex);

 	return usbnet_resume(intf);
 }
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 1730f75..5202e55e 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1015,6 +1015,7 @@ static int smsc95xx_suspend(struct usb_interface
*intf, pm_message_t message)
 	ret = usbnet_suspend(intf, message);
 	check_warn_return(ret, "usbnet_suspend error");

+	mutex_lock(&dev->pm_mutex);
 	/* if no wol options set, enter lowest power SUSPEND2 mode */
 	if (!(pdata->wolopts & SUPPORTED_WAKE)) {
 		netdev_info(dev->net, "entering SUSPEND2 mode");
@@ -1045,6 +1046,7 @@ static int smsc95xx_suspend(struct usb_interface
*intf, pm_message_t message)

 		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 		check_warn_return(ret, "Error writing PM_CTRL");
+		mutex_unlock(&dev->pm_mutex);

 		return 0;
 	}
@@ -1110,6 +1112,7 @@ static int smsc95xx_suspend(struct usb_interface
*intf, pm_message_t message)
 	check_warn_return(ret, "Error reading PM_CTRL");

 	smsc95xx_set_feature(dev, USB_DEVICE_REMOTE_WAKEUP);
+	mutex_unlock(&dev->pm_mutex);

 	return 0;
 }
@@ -1123,6 +1126,7 @@ static int smsc95xx_resume(struct usb_interface *intf)

 	BUG_ON(!dev);

+	mutex_lock(&dev->pm_mutex);
 	if (pdata->wolopts & WAKE_MAGIC) {
 		smsc95xx_clear_feature(dev, USB_DEVICE_REMOTE_WAKEUP);

@@ -1145,6 +1149,7 @@ static int smsc95xx_resume(struct usb_interface *intf)
 		ret = smsc95xx_write_reg(dev, PM_CTRL, val);
 		check_warn_return(ret, "Error writing PM_CTRL");
 	}
+	mutex_unlock(&dev->pm_mutex);

 	return usbnet_resume(intf);
 	check_warn_return(ret, "usbnet_resume error");
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3b51554..9c872a0 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1401,6 +1401,7 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
 	dev->delay.data = (unsigned long) dev;
 	init_timer (&dev->delay);
 	mutex_init (&dev->phy_mutex);
+	mutex_init (&dev->pm_mutex);

 	dev->net = net;
 	strcpy (net->name, "usb%d");
@@ -1516,11 +1517,13 @@ int usbnet_suspend (struct usb_interface
*intf, pm_message_t message)
 	struct usbnet		*dev = usb_get_intfdata(intf);

 	if (!dev->suspend_count++) {
+		mutex_lock(&dev->pm_mutex);
 		spin_lock_irq(&dev->txq.lock);
 		/* don't autosuspend while transmitting */
 		if (dev->txq.qlen && PMSG_IS_AUTO(message)) {
 			dev->suspend_count--;
 			spin_unlock_irq(&dev->txq.lock);
+			mutex_unlock(&dev->pm_mutex);
 			return -EBUSY;
 		} else {
 			set_bit(EVENT_DEV_ASLEEP, &dev->flags);
@@ -1539,6 +1542,7 @@ int usbnet_suspend (struct usb_interface *intf,
pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+		mutex_unlock(&dev->pm_mutex);
 	}
 	return 0;
 }
@@ -1552,6 +1556,7 @@ int usbnet_resume (struct usb_interface *intf)
 	int                     retval;

 	if (!--dev->suspend_count) {
+		mutex_lock(&dev->pm_mutex);
 		/* resume interrupt URBs */
 		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
 			usb_submit_urb(dev->interrupt, GFP_NOIO);
@@ -1587,6 +1592,7 @@ int usbnet_resume (struct usb_interface *intf)
 				netif_tx_wake_all_queues(dev->net);
 			tasklet_schedule (&dev->bh);
 		}
+		mutex_unlock(&dev->pm_mutex);
 	}
 	return 0;
 }
@@ -1598,6 +1604,7 @@ int usbnet_read_cmd(struct usbnet *dev, u8 cmd,
u8 reqtype,
 {
 	void *buf = NULL;
 	int err = -ENOMEM;
+	int autopm = !mutex_is_locked(&dev->pm_mutex);

 	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
 		   " value=0x%04x index=0x%04x size=%d\n",
@@ -1609,9 +1616,13 @@ int usbnet_read_cmd(struct usbnet *dev, u8 cmd,
u8 reqtype,
 			goto out;
 	}

+	if (autopm)
+		usb_autopm_get_interface(dev->intf);
 	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
 			      cmd, reqtype, value, index, buf, size,
 			      USB_CTRL_GET_TIMEOUT);
+	if (autopm)
+		usb_autopm_put_interface(dev->intf);
 	if (err > 0 && err <= size)
 		memcpy(data, buf, err);
 	kfree(buf);
@@ -1625,6 +1636,7 @@ int usbnet_write_cmd(struct usbnet *dev, u8 cmd,
u8 reqtype,
 {
 	void *buf = NULL;
 	int err = -ENOMEM;
+	int autopm = !mutex_is_locked(&dev->pm_mutex);

 	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
 		   " value=0x%04x index=0x%04x size=%d\n",
@@ -1636,9 +1648,13 @@ int usbnet_write_cmd(struct usbnet *dev, u8
cmd, u8 reqtype,
 			goto out;
 	}

+	if (autopm)
+		usb_autopm_get_interface(dev->intf);
 	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
 			      cmd, reqtype, value, index, buf, size,
 			      USB_CTRL_SET_TIMEOUT);
+	if (autopm)
+		usb_autopm_put_interface(dev->intf);
 	kfree(buf);

 out:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 32a57dd..3b56e5a 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -33,6 +33,7 @@ struct usbnet {
 	wait_queue_head_t	*wait;
 	struct mutex		phy_mutex;
 	unsigned char		suspend_count;
+	struct mutex		pm_mutex;

 	/* i/o info: pipes etc */
 	unsigned		in, out;
-- 
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 12/12] usbnet: make device out of suspend before calling usbnet_read/write_cmd

[Oliver Neukum]

On Wednesday 10 October 2012 10:33:17 Ming Lei wrote:
> On Tue, Oct 9, 2012 at 4:50 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > This is awkward to use in suspend()/resume()
> > Could you make both versions available?
> 
> Good catch, thanks for your review.
> 
> As far as I can think of, the mutex_is_locked() trick can solve the problem.

No, you cannot do this. It introduces a race. The check for mutex_is_locked()
can be positive because the device is being suspended and the IO would
be done when it is too late.
Please don't try to be fancy here, just export two versions of each call.

	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 12/12] usbnet: make device out of suspend before calling usbnet_read/write_cmd

[Ming Lei]

On Wed, Oct 10, 2012 at 10:33 AM, Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> Good catch, thanks for your review.
>
> As far as I can think of, the mutex_is_locked() trick can solve the problem.
>
> How about the attached patch?

This one is still not good.

After further thought, it may be better to fix the problem in net
core(dev_ioctl)
because the problem happens on other net devices too.

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