[PATCH 0/5] UAS updates

[PATCH 0/5] UAS updates

[Matthew Wilcox]

Hi Greg,

The following changes since commit 0fcdcfbbc98f70f559e4b36773a69972489a6d8f:

  Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq (2010-12-14 18:50:10 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/willy/uas.git master

Matthew Wilcox (5):
      uas: Fix up the Sense IU
      uas: Use kzalloc instead of kmalloc
      uas: Rename sense pipe and sense urb to status pipe and status urb
      uas: Ensure we only bind to a UAS interface
      uas: Use GFP_NOIO instead of GFP_KERNEL in I/O submission path

 drivers/usb/storage/uas.c |   82 +++++++++++++++++++++++++++++---------------
 1 files changed, 54 insertions(+), 28 deletions(-)

I'll send the patches as replies to this mail.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 1/5] uas: Fix up the Sense IU

[matthew]

From: Matthew Wilcox <matthew@wil.cx>

Add a comment to the Sense IU data structure that it's also used for Read
Ready and Write Ready.  Remove the 'service response' element since it's
gone from the current draft (04).

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/usb/storage/uas.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 339fac3..ca277c2 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -49,14 +49,17 @@ struct command_iu {
 	__u8 cdb[16];	/* XXX: Overflow-checking tools may misunderstand */
 };
 
+/*
+ * Also used for the Read Ready and Write Ready IUs since they have the
+ * same first four bytes
+ */
 struct sense_iu {
 	__u8 iu_id;
 	__u8 rsvd1;
 	__be16 tag;
 	__be16 status_qual;
 	__u8 status;
-	__u8 service_response;
-	__u8 rsvd8[6];
+	__u8 rsvd7[7];
 	__be16 len;
 	__u8 sense[SCSI_SENSE_BUFFERSIZE];
 };
-- 
1.7.2.3

[PATCH 2/5] uas: Use kzalloc instead of kmalloc

[matthew]

From: Matthew Wilcox <matthew@wil.cx>

The IUs are not being fully initialised by the driver (due to the reserved
space).  Since we should be zeroing reserved fields, use kzalloc to do
it for us.

Reported-by: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/usb/storage/uas.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ca277c2..981fdef 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -297,7 +297,7 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	if (!urb)
 		goto out;
 
-	iu = kmalloc(sizeof(*iu), gfp);
+	iu = kzalloc(sizeof(*iu), gfp);
 	if (!iu)
 		goto free;
 
@@ -328,7 +328,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	if (len < 0)
 		len = 0;
 	len = ALIGN(len, 4);
-	iu = kmalloc(sizeof(*iu) + len, gfp);
+	iu = kzalloc(sizeof(*iu) + len, gfp);
 	if (!iu)
 		goto free;
 
-- 
1.7.2.3

[PATCH 3/5] uas: Rename sense pipe and sense urb to status pipe and status urb

[matthew]

From: Matthew Wilcox <matthew@wil.cx>

The spec calls this the status pipe.  While it is used to receive sense IUs,
it is also used to receive other IUs, so this can be confusing.

Reported-by: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/usb/storage/uas.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 981fdef..f82787f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -100,8 +100,8 @@ struct uas_dev_info {
 };
 
 enum {
-	ALLOC_SENSE_URB		= (1 << 0),
-	SUBMIT_SENSE_URB	= (1 << 1),
+	ALLOC_STATUS_URB	= (1 << 0),
+	SUBMIT_STATUS_URB	= (1 << 1),
 	ALLOC_DATA_IN_URB	= (1 << 2),
 	SUBMIT_DATA_IN_URB	= (1 << 3),
 	ALLOC_DATA_OUT_URB	= (1 << 4),
@@ -115,7 +115,7 @@ struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
 	struct urb *cmd_urb;
-	struct urb *sense_urb;
+	struct urb *status_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
 	struct list_head list;
@@ -207,7 +207,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	int err;
 
-	cmdinfo->state = direction | SUBMIT_SENSE_URB;
+	cmdinfo->state = direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -360,21 +360,21 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 
-	if (cmdinfo->state & ALLOC_SENSE_URB) {
-		cmdinfo->sense_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
-							cmdinfo->stream);
-		if (!cmdinfo->sense_urb)
+	if (cmdinfo->state & ALLOC_STATUS_URB) {
+		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
+							  cmdinfo->stream);
+		if (!cmdinfo->status_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
-		cmdinfo->state &= ~ALLOC_SENSE_URB;
+		cmdinfo->state &= ~ALLOC_STATUS_URB;
 	}
 
-	if (cmdinfo->state & SUBMIT_SENSE_URB) {
-		if (usb_submit_urb(cmdinfo->sense_urb, gfp)) {
+	if (cmdinfo->state & SUBMIT_STATUS_URB) {
+		if (usb_submit_urb(cmdinfo->status_urb, gfp)) {
 			scmd_printk(KERN_INFO, cmnd,
 					"sense urb submission failure\n");
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
-		cmdinfo->state &= ~SUBMIT_SENSE_URB;
+		cmdinfo->state &= ~SUBMIT_STATUS_URB;
 	}
 
 	if (cmdinfo->state & ALLOC_DATA_IN_URB) {
@@ -443,7 +443,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
 
-	if (!cmdinfo->sense_urb && sdev->current_cmnd)
+	if (!cmdinfo->status_urb && sdev->current_cmnd)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	if (blk_rq_tagged(cmnd->request)) {
@@ -455,7 +455,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	cmnd->scsi_done = done;
 
-	cmdinfo->state = ALLOC_SENSE_URB | SUBMIT_SENSE_URB |
+	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
@@ -478,8 +478,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
 	if (err) {
 		/* If we did nothing, give up now */
-		if (cmdinfo->state & SUBMIT_SENSE_URB) {
-			usb_free_urb(cmdinfo->sense_urb);
+		if (cmdinfo->state & SUBMIT_STATUS_URB) {
+			usb_free_urb(cmdinfo->status_urb);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		spin_lock(&uas_work_lock);
-- 
1.7.2.3

[PATCH 4/5] uas: Ensure we only bind to a UAS interface

[matthew]

From: Matthew Wilcox <matthew@wil.cx>

While all existing UAS devices use alternate interface 1, this is not
guaranteed, and it has caused confusion with people trying to bind the
uas driver to non-uas devices.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/usb/storage/uas.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f82787f..0ebe7f6 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -581,6 +581,34 @@ static struct usb_device_id uas_usb_ids[] = {
 };
 MODULE_DEVICE_TABLE(usb, uas_usb_ids);
 
+static int uas_is_interface(struct usb_host_interface *intf)
+{
+	return (intf->desc.bInterfaceClass == USB_CLASS_MASS_STORAGE &&
+		intf->desc.bInterfaceSubClass == USB_SC_SCSI &&
+		intf->desc.bInterfaceProtocol == USB_PR_UAS);
+}
+
+static int uas_switch_interface(struct usb_device *udev,
+						struct usb_interface *intf)
+{
+	int i;
+
+	if (uas_is_interface(intf->cur_altsetting))
+		return 0;
+
+	for (i = 0; i < intf->num_altsetting; i++) {
+		struct usb_host_interface *alt = &intf->altsetting[i];
+		if (alt == intf->cur_altsetting)
+			continue;
+		if (uas_is_interface(alt))
+			return usb_set_interface(udev,
+						alt->desc.bInterfaceNumber,
+						alt->desc.bAlternateSetting);
+	}
+
+	return -ENODEV;
+}
+
 static void uas_configure_endpoints(struct uas_dev_info *devinfo)
 {
 	struct usb_host_endpoint *eps[4] = { };
@@ -654,13 +682,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	struct uas_dev_info *devinfo;
 	struct usb_device *udev = interface_to_usbdev(intf);
 
-	if (id->bInterfaceProtocol == 0x50) {
-		int ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
-/* XXX: Shouldn't assume that 1 is the alternative we want */
-		int ret = usb_set_interface(udev, ifnum, 1);
-		if (ret)
-			return -ENODEV;
-	}
+	if (uas_switch_interface(udev, intf))
+		return -ENODEV;
 
 	devinfo = kmalloc(sizeof(struct uas_dev_info), GFP_KERNEL);
 	if (!devinfo)
-- 
1.7.2.3

[PATCH 5/5] uas: Use GFP_NOIO instead of GFP_KERNEL in I/O submission path

[matthew-Ztpu424NOJ8]

From: Matthew Wilcox <matthew-Ztpu424NOJ8@public.gmane.org>

If swap is on a UAS device, we could recurse into the driver by using
GFP_KERNEL.  Using GFP_NOIO ensures we won't.

Reported-by: James Bottomley <James.Bottomley-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Matthew Wilcox <willy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/usb/storage/uas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 0ebe7f6..23f0dd9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -141,7 +141,7 @@ static void uas_do_work(struct work_struct *work)
 		struct scsi_pointer *scp = (void *)cmdinfo;
 		struct scsi_cmnd *cmnd = container_of(scp,
 							struct scsi_cmnd, SCp);
-		uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_KERNEL);
+		uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
 	}
 }
 
-- 
1.7.2.3

--
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 5/5] uas: Use GFP_NOIO instead of GFP_KERNEL in I/O submission path

[matthew]

From: Matthew Wilcox <matthew@wil.cx>

If swap is on a UAS device, we could recurse into the driver by using
GFP_KERNEL.  Using GFP_NOIO ensures we won't.

Reported-by: James Bottomley <James.Bottomley@suse.de>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/usb/storage/uas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 0ebe7f6..23f0dd9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -141,7 +141,7 @@ static void uas_do_work(struct work_struct *work)
 		struct scsi_pointer *scp = (void *)cmdinfo;
 		struct scsi_cmnd *cmnd = container_of(scp,
 							struct scsi_cmnd, SCp);
-		uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_KERNEL);
+		uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
 	}
 }
 
-- 
1.7.2.3