[PATCH] usb/uas: one only one status URB/host on stream-less connection

[PATCH] usb/uas: one only one status URB/host on stream-less connection

[Sebastian Andrzej Siewior]

The status/sense URB is allocated on per-command basis. A read/write
looks the following way on a stream-less connection:

- send cmd tag X, queue status
- receive status, oh it is a read for tag X. queue status & read
- receive read
- receive status, oh I'm done for tag X. Cool call complete and free
  status urb.

This block repeats itself 1:1 for further commands and looks great so
far. Lets take a look now what happens if we do allow multiple commands:

- send cmd tag X, queue statusX (belongs to the command with the X tag)
- send cmd tag Y, queue statusY (belongs to the command with the Y tag)
- receive statusX, oh it is a read for tag X. queue statusX & a read
- receive read
- receive statusY, oh I'm done for tag X. Cool call complete and free statusY.
- receive statusX, oh it is a read for tag Y. queue statusY & before we
  queue the read the the following message can be observed:
  |sd 0:0:0:0: [sda] sense urb submission failure
  followed by a second attempt with the same result.

In order to address this problem there only one status URB for each scsi
host (as suggested by Matthew) in case we don't have stream support.
This URB is requeued until the device removed. Nothing changes on stream
based endpoints.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Okay. That is the third patch of my UAS series. The only issue I'm aware
now is the fact that uas does not work on hcds which do not support sgs.
One bug at a time.....

 drivers/usb/storage/uas.c |   71 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ecf2070..e3b936e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -99,6 +99,8 @@ struct uas_dev_info {
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
 	struct scsi_cmnd *cmnd;
+	/* only used if no stream support is available */
+	struct urb *status_urb;
 };
 
 enum {
@@ -117,6 +119,7 @@ struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
 	struct urb *cmd_urb;
+	/* only used if stream support is available */
 	struct urb *status_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
@@ -180,7 +183,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 
 	cmnd->result = sense_iu->status;
 	cmnd->scsi_done(cmnd);
-	usb_free_urb(urb);
 }
 
 static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
@@ -205,7 +207,6 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 
 	cmnd->result = sense_iu->status;
 	cmnd->scsi_done(cmnd);
-	usb_free_urb(urb);
 }
 
 static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
@@ -214,7 +215,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_STATUS_URB;
+	cmdinfo->state = direction;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -231,10 +232,12 @@ static void uas_stat_cmplt(struct urb *urb)
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 	struct scsi_cmnd *cmnd;
 	u16 tag;
+	int ret;
 
 	if (urb->status) {
 		dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
-		usb_free_urb(urb);
+		if (devinfo->use_streams)
+			usb_free_urb(urb);
 		return;
 	}
 
@@ -244,7 +247,13 @@ static void uas_stat_cmplt(struct urb *urb)
 	else
 		cmnd = scsi_host_find_tag(shost, tag - 1);
 	if (!cmnd) {
-		usb_free_urb(urb);
+		if (devinfo->use_streams) {
+			usb_free_urb(urb);
+			return;
+		}
+		ret = usb_submit_urb(urb, GFP_ATOMIC);
+		if (ret)
+			dev_err(&urb->dev->dev, "failed submit status urb\n");
 		return;
 	}
 
@@ -270,6 +279,15 @@ static void uas_stat_cmplt(struct urb *urb)
 		scmd_printk(KERN_ERR, cmnd,
 			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
 	}
+
+	if (devinfo->use_streams) {
+		usb_free_urb(urb);
+		return;
+	}
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret)
+		dev_err(&urb->dev->dev, "failed submit status urb\n");
 }
 
 static void uas_data_cmplt(struct urb *urb)
@@ -300,7 +318,7 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 }
 
 static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
-					struct scsi_cmnd *cmnd, u16 stream_id)
+		struct Scsi_Host *shost, u16 stream_id)
 {
 	struct usb_device *udev = devinfo->udev;
 	struct urb *urb = usb_alloc_urb(0, gfp);
@@ -314,7 +332,7 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 		goto free;
 
 	usb_fill_bulk_urb(urb, udev, devinfo->status_pipe, iu, sizeof(*iu),
-						uas_stat_cmplt, cmnd->device->host);
+						uas_stat_cmplt, shost);
 	urb->stream_id = stream_id;
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
@@ -382,8 +400,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 
 	if (cmdinfo->state & ALLOC_STATUS_URB) {
-		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
-							  cmdinfo->stream);
+		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp,
+				cmnd->device->host, cmdinfo->stream);
 		if (!cmdinfo->status_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_STATUS_URB;
@@ -492,7 +510,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	}
 
 	if (!devinfo->use_streams) {
-		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
+		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB |
+				ALLOC_STATUS_URB | SUBMIT_STATUS_URB);
 		cmdinfo->stream = 0;
 	}
 
@@ -691,6 +710,29 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo)
 	}
 }
 
+static int uas_alloc_status_urb(struct uas_dev_info *devinfo,
+		struct Scsi_Host *shost)
+{
+	if (devinfo->use_streams) {
+		devinfo->status_urb = NULL;
+		return 0;
+	}
+
+	devinfo->status_urb = uas_alloc_sense_urb(devinfo, GFP_KERNEL,
+			shost, 0);
+	if (!devinfo->status_urb)
+		goto err_s_urb;
+
+	if (usb_submit_urb(devinfo->status_urb, GFP_KERNEL))
+		goto err_submit_urb;
+
+	return 0;
+err_submit_urb:
+	usb_free_urb(devinfo->status_urb);
+err_s_urb:
+	return -ENOMEM;
+}
+
 static void uas_free_streams(struct uas_dev_info *devinfo)
 {
 	struct usb_device *udev = devinfo->udev;
@@ -745,10 +787,17 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	shost->hostdata[0] = (unsigned long)devinfo;
 
+	result = uas_alloc_status_urb(devinfo, shost);
+	if (result)
+		goto err_alloc_status;
+
 	scsi_scan_host(shost);
 	usb_set_intfdata(intf, shost);
 	return result;
 
+err_alloc_status:
+	scsi_remove_host(shost);
+	shost = NULL;
 deconfig_eps:
 	uas_free_streams(devinfo);
  free:
@@ -776,6 +825,8 @@ static void uas_disconnect(struct usb_interface *intf)
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
 	scsi_remove_host(shost);
+	usb_kill_urb(devinfo->status_urb);
+	usb_free_urb(devinfo->status_urb);
 	uas_free_streams(devinfo);
 	kfree(devinfo);
 }
-- 
1.7.7.3

Re: [PATCH] usb/uas: one only one status URB/host on stream-less connection

[Alan Stern]

On Tue, 20 Dec 2011, Sebastian Andrzej Siewior wrote:

> The status/sense URB is allocated on per-command basis. A read/write
> looks the following way on a stream-less connection:
> 
> - send cmd tag X, queue status
> - receive status, oh it is a read for tag X. queue status & read
> - receive read
> - receive status, oh I'm done for tag X. Cool call complete and free
>   status urb.
> 
> This block repeats itself 1:1 for further commands and looks great so
> far. Lets take a look now what happens if we do allow multiple commands:
> 
> - send cmd tag X, queue statusX (belongs to the command with the X tag)
> - send cmd tag Y, queue statusY (belongs to the command with the Y tag)
> - receive statusX, oh it is a read for tag X. queue statusX & a read
> - receive read
> - receive statusY, oh I'm done for tag X. Cool call complete and free statusY.
> - receive statusX, oh it is a read for tag Y. queue statusY & before we
>   queue the read the the following message can be observed:
>   |sd 0:0:0:0: [sda] sense urb submission failure
>   followed by a second attempt with the same result.
> 
> In order to address this problem there only one status URB for each scsi
> host (as suggested by Matthew) in case we don't have stream support.
> This URB is requeued until the device removed. Nothing changes on stream
> based endpoints.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
> Okay. That is the third patch of my UAS series. The only issue I'm aware
> now is the fact that uas does not work on hcds which do not support sgs.
> One bug at a time.....
> 
>  drivers/usb/storage/uas.c |   71 ++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index ecf2070..e3b936e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -99,6 +99,8 @@ struct uas_dev_info {
>  	unsigned use_streams:1;
>  	unsigned uas_sense_old:1;
>  	struct scsi_cmnd *cmnd;
> +	/* only used if no stream support is available */
> +	struct urb *status_urb;
>  };
>  
>  enum {
> @@ -117,6 +119,7 @@ struct uas_cmd_info {
>  	unsigned int state;
>  	unsigned int stream;
>  	struct urb *cmd_urb;
> +	/* only used if stream support is available */
>  	struct urb *status_urb;
>  	struct urb *data_in_urb;
>  	struct urb *data_out_urb;

Very minor stylistic point...  It's not clear whether the comments
apply to the items preceding them or the items following them.  You
might want to put each comment on the same line with its respective
item.

Alan Stern

P.S.: Another very minor point, this time grammatical.  (In fact this
is an _extremely_ common mistake -- virtually everybody does it, native
English speakers included.)

The word "only" generally modifies the term following it.  So when you
write "X is only used if Y", the grammatical meaning is: "If Y is true
then X is used but nothing else happens to X -- for example, X isn't
changed or deleted".  (As an illustration of this point, consider the
sentence "The disk is only read, not written, if the read-protect 
flag is set".)

Instead you should write "X is used only if Y", which means what you
want: "If Y isn't true then X isn't used".  Or in this case: "used only 
if stream support is available".

--
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 v2] usb/uas: one only one status URB/host on stream-less connection

[Sebastian Andrzej Siewior]

The status/sense URB is allocated on per-command basis. A read/write
looks the following way on a stream-less connection:

- send cmd tag X, queue status
- receive status, oh it is a read for tag X. queue status & read
- receive read
- receive status, oh I'm done for tag X. Cool call complete and free
  status urb.

This block repeats itself 1:1 for further commands and looks great so
far. Lets take a look now what happens if we do allow multiple commands:

- send cmd tag X, queue statusX (belongs to the command with the X tag)
- send cmd tag Y, queue statusY (belongs to the command with the Y tag)
- receive statusX, oh it is a read for tag X. queue statusX & a read
- receive read
- receive statusY, oh I'm done for tag X. Cool call complete and free statusY.
- receive statusX, oh it is a read for tag Y. queue statusY & before we
  queue the read the the following message can be observed:
  |sd 0:0:0:0: [sda] sense urb submission failure
  followed by a second attempt with the same result.

In order to address this problem there only one status URB for each scsi
host (as suggested by Matthew) in case we don't have stream support.
This URB is requeued until the device removed. Nothing changes on stream
based endpoints.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
* Alan Stern | 2011-12-20 10:49:06 [-0500]:
>Very minor stylistic point...  It's not clear whether the comments
>apply to the items preceding them or the items following them.  You
>might want to put each comment on the same line with its respective
>item.
>
>Alan Stern
>
>P.S.: Another very minor point, this time grammatical.  (In fact this
>is an _extremely_ common mistake -- virtually everybody does it, native
>English speakers included.)
>
>The word "only" generally modifies the term following it.  So when you
>write "X is only used if Y", the grammatical meaning is: "If Y is true
>then X is used but nothing else happens to X -- for example, X isn't
>changed or deleted".  (As an illustration of this point, consider the
>sentence "The disk is only read, not written, if the read-protect 
>flag is set".)
>
>Instead you should write "X is used only if Y", which means what you
>want: "If Y isn't true then X isn't used".  Or in this case: "used only 
>if stream support is available".

I updated the comment thing. Thanks for the grammer hint, I try to keep
this mind.

 drivers/usb/storage/uas.c |   70 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ecf2070..2f11431 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -99,6 +99,7 @@ struct uas_dev_info {
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
 	struct scsi_cmnd *cmnd;
+	struct urb *status_urb; /* used only if stream support is available */
 };
 
 enum {
@@ -117,6 +118,7 @@ struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
 	struct urb *cmd_urb;
+	/* status_urb is used only if stream support isn't available */
 	struct urb *status_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
@@ -180,7 +182,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 
 	cmnd->result = sense_iu->status;
 	cmnd->scsi_done(cmnd);
-	usb_free_urb(urb);
 }
 
 static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
@@ -205,7 +206,6 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 
 	cmnd->result = sense_iu->status;
 	cmnd->scsi_done(cmnd);
-	usb_free_urb(urb);
 }
 
 static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
@@ -214,7 +214,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_STATUS_URB;
+	cmdinfo->state = direction;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -231,10 +231,12 @@ static void uas_stat_cmplt(struct urb *urb)
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 	struct scsi_cmnd *cmnd;
 	u16 tag;
+	int ret;
 
 	if (urb->status) {
 		dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
-		usb_free_urb(urb);
+		if (devinfo->use_streams)
+			usb_free_urb(urb);
 		return;
 	}
 
@@ -244,7 +246,13 @@ static void uas_stat_cmplt(struct urb *urb)
 	else
 		cmnd = scsi_host_find_tag(shost, tag - 1);
 	if (!cmnd) {
-		usb_free_urb(urb);
+		if (devinfo->use_streams) {
+			usb_free_urb(urb);
+			return;
+		}
+		ret = usb_submit_urb(urb, GFP_ATOMIC);
+		if (ret)
+			dev_err(&urb->dev->dev, "failed submit status urb\n");
 		return;
 	}
 
@@ -270,6 +278,15 @@ static void uas_stat_cmplt(struct urb *urb)
 		scmd_printk(KERN_ERR, cmnd,
 			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
 	}
+
+	if (devinfo->use_streams) {
+		usb_free_urb(urb);
+		return;
+	}
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret)
+		dev_err(&urb->dev->dev, "failed submit status urb\n");
 }
 
 static void uas_data_cmplt(struct urb *urb)
@@ -300,7 +317,7 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 }
 
 static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
-					struct scsi_cmnd *cmnd, u16 stream_id)
+		struct Scsi_Host *shost, u16 stream_id)
 {
 	struct usb_device *udev = devinfo->udev;
 	struct urb *urb = usb_alloc_urb(0, gfp);
@@ -314,7 +331,7 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 		goto free;
 
 	usb_fill_bulk_urb(urb, udev, devinfo->status_pipe, iu, sizeof(*iu),
-						uas_stat_cmplt, cmnd->device->host);
+						uas_stat_cmplt, shost);
 	urb->stream_id = stream_id;
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
@@ -382,8 +399,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 
 	if (cmdinfo->state & ALLOC_STATUS_URB) {
-		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
-							  cmdinfo->stream);
+		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp,
+				cmnd->device->host, cmdinfo->stream);
 		if (!cmdinfo->status_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_STATUS_URB;
@@ -492,7 +509,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	}
 
 	if (!devinfo->use_streams) {
-		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
+		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB |
+				ALLOC_STATUS_URB | SUBMIT_STATUS_URB);
 		cmdinfo->stream = 0;
 	}
 
@@ -691,6 +709,29 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo)
 	}
 }
 
+static int uas_alloc_status_urb(struct uas_dev_info *devinfo,
+		struct Scsi_Host *shost)
+{
+	if (devinfo->use_streams) {
+		devinfo->status_urb = NULL;
+		return 0;
+	}
+
+	devinfo->status_urb = uas_alloc_sense_urb(devinfo, GFP_KERNEL,
+			shost, 0);
+	if (!devinfo->status_urb)
+		goto err_s_urb;
+
+	if (usb_submit_urb(devinfo->status_urb, GFP_KERNEL))
+		goto err_submit_urb;
+
+	return 0;
+err_submit_urb:
+	usb_free_urb(devinfo->status_urb);
+err_s_urb:
+	return -ENOMEM;
+}
+
 static void uas_free_streams(struct uas_dev_info *devinfo)
 {
 	struct usb_device *udev = devinfo->udev;
@@ -745,10 +786,17 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	shost->hostdata[0] = (unsigned long)devinfo;
 
+	result = uas_alloc_status_urb(devinfo, shost);
+	if (result)
+		goto err_alloc_status;
+
 	scsi_scan_host(shost);
 	usb_set_intfdata(intf, shost);
 	return result;
 
+err_alloc_status:
+	scsi_remove_host(shost);
+	shost = NULL;
 deconfig_eps:
 	uas_free_streams(devinfo);
  free:
@@ -776,6 +824,8 @@ static void uas_disconnect(struct usb_interface *intf)
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
 	scsi_remove_host(shost);
+	usb_kill_urb(devinfo->status_urb);
+	usb_free_urb(devinfo->status_urb);
 	uas_free_streams(devinfo);
 	kfree(devinfo);
 }
-- 
1.7.7.3

Sebastian

Re: [PATCH v2] usb/uas: one only one status URB/host on stream-less connection

[Sarah Sharp]

Hi Sebastian,

The patch looks good (although I haven't tested it).  However, I think
you should move this chunk from one of your previous patches:

@@ -449,7 +452,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,

        BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));

-       if (!cmdinfo->status_urb && sdev->current_cmnd)
+       if (sdev->current_cmnd)
                return SCSI_MLQUEUE_DEVICE_BUSY;

into this patch, to keep the change to the status URB in one patch.

Also, do you have a git branch up for the UAS work somewhere?  We need
to figure out how to merge your bug fix patches with my bug fix patches
into one patchset.  It's probably easier if you send your current
patchset, or a pointer to your branch.

Sarah Sharp

On Tue, Dec 20, 2011 at 06:16:21PM +0100, Sebastian Andrzej Siewior wrote:
> The status/sense URB is allocated on per-command basis. A read/write
> looks the following way on a stream-less connection:
> 
> - send cmd tag X, queue status
> - receive status, oh it is a read for tag X. queue status & read
> - receive read
> - receive status, oh I'm done for tag X. Cool call complete and free
>   status urb.
> 
> This block repeats itself 1:1 for further commands and looks great so
> far. Lets take a look now what happens if we do allow multiple commands:
> 
> - send cmd tag X, queue statusX (belongs to the command with the X tag)
> - send cmd tag Y, queue statusY (belongs to the command with the Y tag)
> - receive statusX, oh it is a read for tag X. queue statusX & a read
> - receive read
> - receive statusY, oh I'm done for tag X. Cool call complete and free statusY.
> - receive statusX, oh it is a read for tag Y. queue statusY & before we
>   queue the read the the following message can be observed:
>   |sd 0:0:0:0: [sda] sense urb submission failure
>   followed by a second attempt with the same result.
> 
> In order to address this problem there only one status URB for each scsi
> host (as suggested by Matthew) in case we don't have stream support.
> This URB is requeued until the device removed. Nothing changes on stream
> based endpoints.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
> * Alan Stern | 2011-12-20 10:49:06 [-0500]:
> >Very minor stylistic point...  It's not clear whether the comments
> >apply to the items preceding them or the items following them.  You
> >might want to put each comment on the same line with its respective
> >item.
> >
> >Alan Stern
> >
> >P.S.: Another very minor point, this time grammatical.  (In fact this
> >is an _extremely_ common mistake -- virtually everybody does it, native
> >English speakers included.)
> >
> >The word "only" generally modifies the term following it.  So when you
> >write "X is only used if Y", the grammatical meaning is: "If Y is true
> >then X is used but nothing else happens to X -- for example, X isn't
> >changed or deleted".  (As an illustration of this point, consider the
> >sentence "The disk is only read, not written, if the read-protect 
> >flag is set".)
> >
> >Instead you should write "X is used only if Y", which means what you
> >want: "If Y isn't true then X isn't used".  Or in this case: "used only 
> >if stream support is available".
> 
> I updated the comment thing. Thanks for the grammer hint, I try to keep
> this mind.
> 
>  drivers/usb/storage/uas.c |   70 ++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index ecf2070..2f11431 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -99,6 +99,7 @@ struct uas_dev_info {
>  	unsigned use_streams:1;
>  	unsigned uas_sense_old:1;
>  	struct scsi_cmnd *cmnd;
> +	struct urb *status_urb; /* used only if stream support is available */
>  };
>  
>  enum {
> @@ -117,6 +118,7 @@ struct uas_cmd_info {
>  	unsigned int state;
>  	unsigned int stream;
>  	struct urb *cmd_urb;
> +	/* status_urb is used only if stream support isn't available */
>  	struct urb *status_urb;
>  	struct urb *data_in_urb;
>  	struct urb *data_out_urb;
> @@ -180,7 +182,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
>  
>  	cmnd->result = sense_iu->status;
>  	cmnd->scsi_done(cmnd);
> -	usb_free_urb(urb);
>  }
>  
>  static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
> @@ -205,7 +206,6 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
>  
>  	cmnd->result = sense_iu->status;
>  	cmnd->scsi_done(cmnd);
> -	usb_free_urb(urb);
>  }
>  
>  static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
> @@ -214,7 +214,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_STATUS_URB;
> +	cmdinfo->state = direction;
>  	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
>  	if (err) {
>  		spin_lock(&uas_work_lock);
> @@ -231,10 +231,12 @@ static void uas_stat_cmplt(struct urb *urb)
>  	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
>  	struct scsi_cmnd *cmnd;
>  	u16 tag;
> +	int ret;
>  
>  	if (urb->status) {
>  		dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
> -		usb_free_urb(urb);
> +		if (devinfo->use_streams)
> +			usb_free_urb(urb);
>  		return;
>  	}
>  
> @@ -244,7 +246,13 @@ static void uas_stat_cmplt(struct urb *urb)
>  	else
>  		cmnd = scsi_host_find_tag(shost, tag - 1);
>  	if (!cmnd) {
> -		usb_free_urb(urb);
> +		if (devinfo->use_streams) {
> +			usb_free_urb(urb);
> +			return;
> +		}
> +		ret = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (ret)
> +			dev_err(&urb->dev->dev, "failed submit status urb\n");
>  		return;
>  	}
>  
> @@ -270,6 +278,15 @@ static void uas_stat_cmplt(struct urb *urb)
>  		scmd_printk(KERN_ERR, cmnd,
>  			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
>  	}
> +
> +	if (devinfo->use_streams) {
> +		usb_free_urb(urb);
> +		return;
> +	}
> +
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (ret)
> +		dev_err(&urb->dev->dev, "failed submit status urb\n");
>  }
>  
>  static void uas_data_cmplt(struct urb *urb)
> @@ -300,7 +317,7 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
>  }
>  
>  static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
> -					struct scsi_cmnd *cmnd, u16 stream_id)
> +		struct Scsi_Host *shost, u16 stream_id)
>  {
>  	struct usb_device *udev = devinfo->udev;
>  	struct urb *urb = usb_alloc_urb(0, gfp);
> @@ -314,7 +331,7 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
>  		goto free;
>  
>  	usb_fill_bulk_urb(urb, udev, devinfo->status_pipe, iu, sizeof(*iu),
> -						uas_stat_cmplt, cmnd->device->host);
> +						uas_stat_cmplt, shost);
>  	urb->stream_id = stream_id;
>  	urb->transfer_flags |= URB_FREE_BUFFER;
>   out:
> @@ -382,8 +399,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>  	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>  
>  	if (cmdinfo->state & ALLOC_STATUS_URB) {
> -		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd,
> -							  cmdinfo->stream);
> +		cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp,
> +				cmnd->device->host, cmdinfo->stream);
>  		if (!cmdinfo->status_urb)
>  			return SCSI_MLQUEUE_DEVICE_BUSY;
>  		cmdinfo->state &= ~ALLOC_STATUS_URB;
> @@ -492,7 +509,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
>  	}
>  
>  	if (!devinfo->use_streams) {
> -		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
> +		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB |
> +				ALLOC_STATUS_URB | SUBMIT_STATUS_URB);
>  		cmdinfo->stream = 0;
>  	}
>  
> @@ -691,6 +709,29 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo)
>  	}
>  }
>  
> +static int uas_alloc_status_urb(struct uas_dev_info *devinfo,
> +		struct Scsi_Host *shost)
> +{
> +	if (devinfo->use_streams) {
> +		devinfo->status_urb = NULL;
> +		return 0;
> +	}
> +
> +	devinfo->status_urb = uas_alloc_sense_urb(devinfo, GFP_KERNEL,
> +			shost, 0);
> +	if (!devinfo->status_urb)
> +		goto err_s_urb;
> +
> +	if (usb_submit_urb(devinfo->status_urb, GFP_KERNEL))
> +		goto err_submit_urb;
> +
> +	return 0;
> +err_submit_urb:
> +	usb_free_urb(devinfo->status_urb);
> +err_s_urb:
> +	return -ENOMEM;
> +}
> +
>  static void uas_free_streams(struct uas_dev_info *devinfo)
>  {
>  	struct usb_device *udev = devinfo->udev;
> @@ -745,10 +786,17 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  
>  	shost->hostdata[0] = (unsigned long)devinfo;
>  
> +	result = uas_alloc_status_urb(devinfo, shost);
> +	if (result)
> +		goto err_alloc_status;
> +
>  	scsi_scan_host(shost);
>  	usb_set_intfdata(intf, shost);
>  	return result;
>  
> +err_alloc_status:
> +	scsi_remove_host(shost);
> +	shost = NULL;
>  deconfig_eps:
>  	uas_free_streams(devinfo);
>   free:
> @@ -776,6 +824,8 @@ static void uas_disconnect(struct usb_interface *intf)
>  	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
>  
>  	scsi_remove_host(shost);
> +	usb_kill_urb(devinfo->status_urb);
> +	usb_free_urb(devinfo->status_urb);
>  	uas_free_streams(devinfo);
>  	kfree(devinfo);
>  }
> -- 
> 1.7.7.3
> 
> Sebastian
--
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 v2] usb/uas: one only one status URB/host on stream-less connection

[Sebastian Andrzej Siewior]

* Sarah Sharp | 2011-12-21 11:25:43 [-0800]:

>Hi Sebastian,
Hi Sarah,

>The patch looks good (although I haven't tested it).  However, I think
>you should move this chunk from one of your previous patches:
>
>@@ -449,7 +452,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
>
>        BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
>
>-       if (!cmdinfo->status_urb && sdev->current_cmnd)
>+       if (sdev->current_cmnd)
>                return SCSI_MLQUEUE_DEVICE_BUSY;
>
>into this patch, to keep the change to the status URB in one patch.

This isn't completely the chunk I have, it is:

-       if (!cmdinfo->status_urb && sdev->current_cmnd)
+       if (devinfo->cmnd)
                return SCSI_MLQUEUE_DEVICE_BUSY;

So I replace sdev with devinfo. Not sure why status_urb is considered
here at all since it has to be NULL because it is a new command. We
should look at "default command pointer" to check if we have a untagged
command pending.

>Also, do you have a git branch up for the UAS work somewhere?  We need
>to figure out how to merge your bug fix patches with my bug fix patches
>into one patchset.  It's probably easier if you send your current
>patchset, or a pointer to your branch.

I did as you asked and applied your patches 1-3 and added my stuff on
top. You can pull from:

git pull git://linutronix.de/users/bigeasy/linux for_sarah

The following changes since commit 5611cc4572e889b62a7b4c72a413536bf6a9c416:

  Linux 3.2-rc4 (2011-12-01 14:56:01 -0800)

are available in the git repository at:
  git://linutronix.de/users/bigeasy/linux for_sarah

Sarah Sharp (3):
      UAS: Re-add workqueue items if submission fails.
      UAS: Use unique tags on non-streams devices.
      UAS: Free status URB when we can't find the SCSI tag.

Sebastian Andrzej Siewior (3):
      usb/uas: use unique tags for all LUNs
      usb/uas: use scsi_host_find_tag() to find command from a tag
      usb/uas: one only one status URB/host on stream-less connection

 drivers/usb/storage/uas.c |  157 +++++++++++++++++++++++++++++++++------------
 1 files changed, 117 insertions(+), 40 deletions(-)

which is -rc4 based which is almost what I had for testing (a bunch of
target patches are missing). I also prepared a gpg signed tag for you so
if you have a recent version git you should be able to pull it via
   git pull git://linutronix.de/users/bigeasy/linux uas_for_sarah

:)

>Sarah Sharp

Sebastian

Re: [PATCH v2] usb/uas: one only one status URB/host on stream-less connection

[Sarah Sharp]

On Thu, Dec 22, 2011 at 03:15:25PM +0100, Sebastian Andrzej Siewior wrote:
> * Sarah Sharp | 2011-12-21 11:25:43 [-0800]:
> 
> >Hi Sebastian,
> Hi Sarah,
> 
> >The patch looks good (although I haven't tested it).  However, I think
> >you should move this chunk from one of your previous patches:
> >
> >@@ -449,7 +452,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
> >
> >        BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
> >
> >-       if (!cmdinfo->status_urb && sdev->current_cmnd)
> >+       if (sdev->current_cmnd)
> >                return SCSI_MLQUEUE_DEVICE_BUSY;
> >
> >into this patch, to keep the change to the status URB in one patch.
> 
> This isn't completely the chunk I have, it is:
> 
> -       if (!cmdinfo->status_urb && sdev->current_cmnd)
> +       if (devinfo->cmnd)
>                 return SCSI_MLQUEUE_DEVICE_BUSY;
> 
> So I replace sdev with devinfo. Not sure why status_urb is considered
> here at all since it has to be NULL because it is a new command. We
> should look at "default command pointer" to check if we have a untagged
> command pending.

Ah, ok, makes sense.

> >Also, do you have a git branch up for the UAS work somewhere?  We need
> >to figure out how to merge your bug fix patches with my bug fix patches
> >into one patchset.  It's probably easier if you send your current
> >patchset, or a pointer to your branch.
> 
> I did as you asked and applied your patches 1-3 and added my stuff on
> top. You can pull from:
> 
> git pull git://linutronix.de/users/bigeasy/linux for_sarah
> 
> The following changes since commit 5611cc4572e889b62a7b4c72a413536bf6a9c416:
> 
>   Linux 3.2-rc4 (2011-12-01 14:56:01 -0800)
> 
> are available in the git repository at:
>   git://linutronix.de/users/bigeasy/linux for_sarah
> 
> Sarah Sharp (3):
>       UAS: Re-add workqueue items if submission fails.
>       UAS: Use unique tags on non-streams devices.
>       UAS: Free status URB when we can't find the SCSI tag.
> 
> Sebastian Andrzej Siewior (3):
>       usb/uas: use unique tags for all LUNs
>       usb/uas: use scsi_host_find_tag() to find command from a tag
>       usb/uas: one only one status URB/host on stream-less connection
> 
>  drivers/usb/storage/uas.c |  157 +++++++++++++++++++++++++++++++++------------
>  1 files changed, 117 insertions(+), 40 deletions(-)
> 
> which is -rc4 based which is almost what I had for testing (a bunch of
> target patches are missing). I also prepared a gpg signed tag for you so
> if you have a recent version git you should be able to pull it via
>    git pull git://linutronix.de/users/bigeasy/linux uas_for_sarah
> 
> :)

Great!  I'll pull from that and start building on top of it.  Honestly
though, I won't have time for this until after the holidays.  I can ask
Greg to pull these as-is for 3.3 if you're really antsy to get them out.

Sarah Sharp

Re: [PATCH v2] usb/uas: one only one status URB/host on stream-less connection

[Sebastian Andrzej Siewior]

* Sarah Sharp | 2011-12-22 17:07:20 [-0800]:

>
>Great!  I'll pull from that and start building on top of it.  Honestly
>though, I won't have time for this until after the holidays.  I can ask
>Greg to pull these as-is for 3.3 if you're really antsy to get them out.

It is up to you. I have fixes which will make upstream and this is good
enough for me. I had a USB3.0 HD the other day but it did not support
UAS it as talking the old protocol. So I guess most users won't be bit
by this right now.

>Sarah Sharp

Sebastian