Make UAS work on HS for devices with and without command tagging support

Make UAS work on HS for devices with and without command tagging support

[Sebastian Andrzej Siewior]

Hi Sarah and Matthew,

the two patches were required to get my new fancy uasp gadget work on HS
with dummy_hcd. It is processing only one command at a time however it
caught a few bugs already :)

To deal with canceled commands (where we may have to kill some status urbs)
I've been thinking about a list within uas_dev_info which keeps track of
all status urbs and counts all commands which are queued. Once we start
removing commands which timed out we should make sure that the number of
commands which remain is the same as the number of status urbs which we
have. If we have more status urbs than commands then we kill some status
urbs. If it is the other way around then someone was too trigger happy.

Sebastian

[PATCH 1/2] usb/uas: fix support on HS (device without command tagging)

[Sebastian Andrzej Siewior]

Right now UAS can not handle HS devices. This patch tries to fix this.
On SS and HS we take the command tag increment it by one and assign it
as the stream_id. This number is used to compare the tag number.
This tag number is later used to find the matching command struct. On SS
we could use urb->stream_id but this way the code flow is the same.
Untagged commands are stashed in ->current_cmd. Once such a command is
in process no other commands are allowed.
What I observed so far is that the first command (INQUIRY) checked if
command tagging is supporteed by the device or not. If so, all further
commands are tagged, if not none are. So the assumption here is if
->current_cmd is available then take it else go by tag the number.
Things will break apart if the initiator mixes tagged & untagged
commands.

With this patch things work for me on HS-UASP without command tagging.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/usb/storage/uas.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 680bea9..97f969c 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -232,13 +232,16 @@ static void uas_stat_cmplt(struct urb *urb)
 		return;
 	}
 
-	tag = be16_to_cpup(&iu->tag) - 1;
-	if (sdev->current_cmnd)
-		cmnd = sdev->current_cmnd;
-	else
-		cmnd = scsi_find_tag(sdev, tag);
-	if (!cmnd)
+	cmnd = sdev->current_cmnd;
+	if (!cmnd) {
+		tag = be16_to_cpup(&iu->tag);
+		cmnd = scsi_find_tag(sdev, tag - 1);
+	}
+	if (!cmnd) {
+		dev_err(&urb->dev->dev, "Missing cmd for status URB\n");
+		usb_free_urb(urb);
 		return;
+	}
 
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
@@ -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;
 
 	if (blk_rq_tagged(cmnd->request)) {
@@ -476,10 +479,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		break;
 	}
 
-	if (!devinfo->use_streams) {
+	if (!devinfo->use_streams)
 		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
-		cmdinfo->stream = 0;
-	}
 
 	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
 	if (err) {
-- 
1.7.7.3

[PATCH 2/2] usb/uas: fix support on HS (device with command tagging)

[Sebastian Andrzej Siewior]

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

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

To fix this problem, I removed status URB from the command struct so it
is no longer part of it. We free it once we done with the command.
During an error condition (we never receive a status answer from the
device) the URB is lost. This is no different compared to what we do
now. Once we close the endpoint the status urb should be removed and
cleaned up.

With this patch things work for me on HS-UASP with command tagging and
multiple commands at a time.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/usb/storage/uas.c |   56 +++++++++++++++++++++------------------------
 1 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 97f969c..37a7028 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -101,8 +101,7 @@ struct uas_dev_info {
 };
 
 enum {
-	ALLOC_STATUS_URB	= (1 << 0),
-	SUBMIT_STATUS_URB	= (1 << 1),
+	SUBMIT_STATUS_URB	= (1 << 0),
 	ALLOC_DATA_IN_URB	= (1 << 2),
 	SUBMIT_DATA_IN_URB	= (1 << 3),
 	ALLOC_DATA_OUT_URB	= (1 << 4),
@@ -116,7 +115,6 @@ struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
 	struct urb *cmd_urb;
-	struct urb *status_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
 	struct list_head list;
@@ -208,6 +206,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	int err;
 
+	usb_free_urb(urb);
 	cmdinfo->state = direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
@@ -291,29 +290,35 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	return urb;
 }
 
-static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
-					struct scsi_cmnd *cmnd, u16 stream_id)
+static int uas_submit_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
+		struct scsi_cmnd *cmnd, u16 stream_id)
 {
 	struct usb_device *udev = devinfo->udev;
-	struct urb *urb = usb_alloc_urb(0, gfp);
+	struct urb *urb;
 	struct sense_iu *iu;
+	int ret;
 
+	urb = usb_alloc_urb(0, gfp);
 	if (!urb)
-		goto out;
+		return -ENOMEM;
 
+	ret = -ENOMEM;
 	iu = kzalloc(sizeof(*iu), gfp);
 	if (!iu)
-		goto free;
+		goto out;
 
 	usb_fill_bulk_urb(urb, udev, devinfo->status_pipe, iu, sizeof(*iu),
-						uas_stat_cmplt, cmnd->device);
+			uas_stat_cmplt, cmnd->device);
 	urb->stream_id = stream_id;
 	urb->transfer_flags |= URB_FREE_BUFFER;
- out:
-	return urb;
- free:
+
+	ret = usb_submit_urb(urb, gfp);
+	if (ret)
+		goto out;
+	return 0;
+out:
 	usb_free_urb(urb);
-	return NULL;
+	return ret;
 }
 
 static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
@@ -369,20 +374,13 @@ 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);
-		if (!cmdinfo->status_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
-		cmdinfo->state &= ~ALLOC_STATUS_URB;
-	}
-
 	if (cmdinfo->state & SUBMIT_STATUS_URB) {
-		if (usb_submit_urb(cmdinfo->status_urb, gfp)) {
-			scmd_printk(KERN_INFO, cmnd,
-					"sense urb submission failure\n");
+		int ret;
+
+		ret = uas_submit_sense_urb(devinfo, gfp, cmnd,
+				cmdinfo->stream);
+		if (ret)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
-		}
 		cmdinfo->state &= ~SUBMIT_STATUS_URB;
 	}
 
@@ -464,8 +462,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	cmnd->scsi_done = done;
 
-	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
-			ALLOC_CMD_URB | SUBMIT_CMD_URB;
+	cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
@@ -485,10 +482,9 @@ 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_STATUS_URB) {
-			usb_free_urb(cmdinfo->status_urb);
+		if (cmdinfo->state & SUBMIT_STATUS_URB)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
-		}
+
 		spin_lock(&uas_work_lock);
 		list_add_tail(&cmdinfo->list, &uas_work_list);
 		spin_unlock(&uas_work_lock);
-- 
1.7.7.3

Re: Make UAS work on HS for devices with and without command tagging support

[Sarah Sharp]

On Wed, Dec 14, 2011 at 07:47:50PM +0100, Sebastian Andrzej Siewior wrote:
> Hi Sarah and Matthew,
> 
> the two patches were required to get my new fancy uasp gadget work on HS
> with dummy_hcd. It is processing only one command at a time however it
> caught a few bugs already :)

Thanks for testing!  Are these patches on top of the patches I sent out
a week ago?  Or just on top of Greg's tree?

> To deal with canceled commands (where we may have to kill some status urbs)
> I've been thinking about a list within uas_dev_info which keeps track of
> all status urbs and counts all commands which are queued.  Once we start
> removing commands which timed out we should make sure that the number of
> commands which remain is the same as the number of status urbs which we
> have. If we have more status urbs than commands then we kill some status
> urbs. If it is the other way around then someone was too trigger happy.

That might work, but we'd have to synchronize the list manipulations
across status completions.  That would get complex in the USB 3.0 case,
where we could (potentially, if the xHCI driver had multiple event ring
support and the host had MSI-X support) have two status completion
handlers running on different CPUs.  One of the completion handlers
would have to spin, waiting on the other.  So if we can avoid a list of
outstanding URBs, that would be better.

Matthew was just mentioning to me that it might be good to just have one
status URB per USB 2.0 UAS device, and just keep re-submitting it.  USB
2.0 devices don't have streams, so there can't be multiple status URBs
being processed at the same time, so I think it would work.  The only
difficulty I see is syncing the status completion with an abort of the
same command, but I think that could be done with an RCU reader lock in
the status completion.  There shouldn't be any performance difference
between re-using one status URB or using a dedicated one in the
cmnd_info structure.  Plus, we wouldn't have to allocate status URBs in
completion handlers if we just used one dedicated status URB.

For USB 3.0 UAS, I think the status URB should stay in the cmnd_info
structure, since we know that when a status URB completes for a
particular stream ID, it is directly associated with that command, and
not a different command.  So I'd like to address the sense URB failure
issue you tried to fix in the second patch, but I think we should go
about it in a different way.

Sarah Sharp

Re: Make UAS work on HS for devices with and without command tagging support

[Sebastian Andrzej Siewior]

On 12/14/2011 11:53 PM, Sarah Sharp wrote:
> Thanks for testing!  Are these patches on top of the patches I sent out
> a week ago?  Or just on top of Greg's tree?

They are ontop of Greg's tree. I know that you sent a patch to fix the
same issue I did on my #1 patch. I remember that something did not work
out. It might be the thing for which I need #2. So if you want me to
rebase those two or redo something please let me know.

>> To deal with canceled commands (where we may have to kill some status urbs)
>> I've been thinking about a list within uas_dev_info which keeps track of
>> all status urbs and counts all commands which are queued.  Once we start
>> removing commands which timed out we should make sure that the number of
>> commands which remain is the same as the number of status urbs which we
>> have. If we have more status urbs than commands then we kill some status
>> urbs. If it is the other way around then someone was too trigger happy.
>
> That might work, but we'd have to synchronize the list manipulations
> across status completions.  That would get complex in the USB 3.0 case,
> where we could (potentially, if the xHCI driver had multiple event ring
> support and the host had MSI-X support) have two status completion
> handlers running on different CPUs.  One of the completion handlers
> would have to spin, waiting on the other.  So if we can avoid a list of
> outstanding URBs, that would be better.

USB3.0. I see. On 3.0 we would have one status urb for each command so
it might end up in the command struct.

> Matthew was just mentioning to me that it might be good to just have one
> status URB per USB 2.0 UAS device, and just keep re-submitting it.  USB
> 2.0 devices don't have streams, so there can't be multiple status URBs
> being processed at the same time, so I think it would work.  The only
Oh. That sounds great. I've been thinking how to avoid multiple
allocation/allocation of them.

> difficulty I see is syncing the status completion with an abort of the
> same command, but I think that could be done with an RCU reader lock in
> the status completion.  There shouldn't be any performance difference
> between re-using one status URB or using a dedicated one in the
> cmnd_info structure.  Plus, we wouldn't have to allocate status URBs in
> completion handlers if we just used one dedicated status URB.

yep. Sounds like a plan.

> For USB 3.0 UAS, I think the status URB should stay in the cmnd_info
> structure, since we know that when a status URB completes for a
> particular stream ID, it is directly associated with that command, and
> not a different command.  So I'd like to address the sense URB failure
> issue you tried to fix in the second patch, but I think we should go
> about it in a different way.

Fine by me. You want me to redo #2 with one status urb/uas device or do
you do it yourself?

> Sarah Sharp

Sebastian

Re: [PATCH 1/2] usb/uas: fix support on HS (device without command tagging)

[Sergei Shtylyov]

Hello.

On 14-12-2011 22:47, Sebastian Andrzej Siewior wrote:

> Right now UAS can not handle HS devices. This patch tries to fix this.
> On SS and HS we take the command tag increment it by one and assign it
> as the stream_id. This number is used to compare the tag number.
> This tag number is later used to find the matching command struct. On SS
> we could use urb->stream_id but this way the code flow is the same.
> Untagged commands are stashed in ->current_cmd. Once such a command is
> in process no other commands are allowed.
> What I observed so far is that the first command (INQUIRY) checked if
> command tagging is supporteed by the device or not. If so, all further
> commands are tagged, if not none are. So the assumption here is if
> ->current_cmd is available then take it else go by tag the number.
> Things will break apart if the initiator mixes tagged&  untagged
> commands.

> With this patch things work for me on HS-UASP without command tagging.

> Signed-off-by: Sebastian Andrzej Siewior<bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>   drivers/usb/storage/uas.c |   21 +++++++++++----------
>   1 files changed, 11 insertions(+), 10 deletions(-)

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 680bea9..97f969c 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -232,13 +232,16 @@ static void uas_stat_cmplt(struct urb *urb)
>   		return;
>   	}
>
> -	tag = be16_to_cpup(&iu->tag) - 1;
> -	if (sdev->current_cmnd)
> -		cmnd = sdev->current_cmnd;
> -	else
> -		cmnd = scsi_find_tag(sdev, tag);
> -	if (!cmnd)
> +	cmnd = sdev->current_cmnd;
> +	if (!cmnd) {
> +		tag = be16_to_cpup(&iu->tag);

    Why not just 'be16_to_cpu(iu->tag)'? Ah, that's how it was before...

WBR, Sergei
--
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: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sarah Sharp]

On Thu, Dec 15, 2011 at 09:44:37AM +0100, Sebastian Andrzej Siewior wrote:
> On 12/14/2011 11:53 PM, Sarah Sharp wrote:
> >Thanks for testing!  Are these patches on top of the patches I sent out
> >a week ago?  Or just on top of Greg's tree?
> 
> They are ontop of Greg's tree. I know that you sent a patch to fix the
> same issue I did on my #1 patch. I remember that something did not work
> out. It might be the thing for which I need #2. So if you want me to
> rebase those two or redo something please let me know.

Yes, I need to probably completely revisit the patchset because there
are still race conditions in there.  I was actually planning on
completely ripping out the workqueue, since having a global workqueue
means the UAS pre-reset function can't be guaranteed to sync with the
workqueue that might be used by a different UAS device.  But that means
making sure that we have error paths in the URB submission code in case
submission fails.  Probably the status URB changes for USB 2.0 should
get in place before that.

To fix the issue with HS devices, I think we might need my patch #2 and
#3.  Then I don't think you'll need this ending bit in your patch #1,
since my patch #2 took care of making sure not to use cmdinfo->stream
for USB 2.0 commands:

@@ -476,10 +479,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
                break;
        }

-       if (!devinfo->use_streams) {
+       if (!devinfo->use_streams)
                cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
-               cmdinfo->stream = 0;
-       }

        err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
        if (err) {

I also think this bit:

@@ -232,13 +232,16 @@ static void uas_stat_cmplt(struct urb *urb)
                return;
        }

-       tag = be16_to_cpup(&iu->tag) - 1;
-       if (sdev->current_cmnd)
-               cmnd = sdev->current_cmnd;
-       else
-               cmnd = scsi_find_tag(sdev, tag);
-       if (!cmnd)
+       cmnd = sdev->current_cmnd;
+       if (!cmnd) {
+               tag = be16_to_cpup(&iu->tag);
+               cmnd = scsi_find_tag(sdev, tag - 1);
+       }
+       if (!cmnd) {
+               dev_err(&urb->dev->dev, "Missing cmd for status URB\n");
+               usb_free_urb(urb);
                return;
+       }

Is equivalent to my patch #3:

@@ -246,8 +246,10 @@ static void uas_stat_cmplt(struct urb *urb)
                cmnd = sdev->current_cmnd;
        else
                cmnd = scsi_find_tag(sdev, tag);
-       if (!cmnd)
+       if (!cmnd) {
+               usb_free_urb(urb);
                return;
+       }

        switch (iu->iu_id) {
        case IU_ID_STATUS:

You're rearranging the code a lot there, but it looks like the only behavioral
change you made was to free the status URB if the command couldn't be found.
Or are you also trying to skip setting tag to a big negative number in the
untagged case?  tag is not used in the rest of the function if
sdev->current_cmnd is non-NULL, so I don't think it matters much.

I'm not sure what you're trying to fix with this bit of code:

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

        if (blk_rq_tagged(cmnd->request)) {

Was that supposed to be part of the second patch?

So I think you could replace your patch #1 with my patches #2 and #3,
and then redo your patch #2.

> >Matthew was just mentioning to me that it might be good to just have one
> >status URB per USB 2.0 UAS device, and just keep re-submitting it.  USB
> >2.0 devices don't have streams, so there can't be multiple status URBs
> >being processed at the same time, so I think it would work.  The only
> Oh. That sounds great. I've been thinking how to avoid multiple
> allocation/allocation of them.
>
> >For USB 3.0 UAS, I think the status URB should stay in the cmnd_info
> >structure, since we know that when a status URB completes for a
> >particular stream ID, it is directly associated with that command, and
> >not a different command.  So I'd like to address the sense URB failure
> >issue you tried to fix in the second patch, but I think we should go
> >about it in a different way.
> 
> Fine by me. You want me to redo #2 with one status urb/uas device or do
> you do it yourself?

If you want to take a stab at redoing your patch #2 to use only one
status URB for USB 2.0 devices, I would appreciate it.  Then I can build
the abort/reset synchronization on top of it.

Can you test the USB 3.0 side as well, or are you just testing USB 2.0
with your device?

Sarah Sharp
--
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: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sebastian Andrzej Siewior]

* Sarah Sharp | 2011-12-15 13:12:01 [-0800]:

>You're rearranging the code a lot there, but it looks like the only behavioral
>change you made was to free the status URB if the command couldn't be found.
Yup. Since we don't have a reference, we can't free it.

>Or are you also trying to skip setting tag to a big negative number in the
>untagged case?  tag is not used in the rest of the function if
>sdev->current_cmnd is non-NULL, so I don't think it matters much.

If it is untagged, the tag number should be always 1 and ->current_cmnd
set. There can be only one untagged command at a time so therefore I
look first for current_cmnd then look at the tag.

>I'm not sure what you're trying to fix with this bit of code:
>
>@@ -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;
>
>        if (blk_rq_tagged(cmnd->request)) {
>
>Was that supposed to be part of the second patch?
Didn't I remove status_urb? Plus there can be only one default command
at a time so looking at current_cmd seems wrong (unless we set
->current_cmnd and drop the reqeust due to -ENOMEM).

>So I think you could replace your patch #1 with my patches #2 and #3,
>and then redo your patch #2.
okay will do.

>> >Matthew was just mentioning to me that it might be good to just have one
>> >status URB per USB 2.0 UAS device, and just keep re-submitting it.  USB
>> >2.0 devices don't have streams, so there can't be multiple status URBs
>> >being processed at the same time, so I think it would work.  The only
>> Oh. That sounds great. I've been thinking how to avoid multiple
>> allocation/allocation of them.
>>
>> >For USB 3.0 UAS, I think the status URB should stay in the cmnd_info
>> >structure, since we know that when a status URB completes for a
>> >particular stream ID, it is directly associated with that command, and
>> >not a different command.  So I'd like to address the sense URB failure
>> >issue you tried to fix in the second patch, but I think we should go
>> >about it in a different way.
>> 
>> Fine by me. You want me to redo #2 with one status urb/uas device or do
>> you do it yourself?
>
>If you want to take a stab at redoing your patch #2 to use only one
>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
>the abort/reset synchronization on top of it.
Okay.

>Can you test the USB 3.0 side as well, or are you just testing USB 2.0
>with your device?

I made usb3.0 and stream support just work the other day but those
patches were made before that. The usb3 test worked without problem but
I don't have any error recovery/handling yet.

>Sarah Sharp

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: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:

>>If you want to take a stab at redoing your patch #2 to use only one
>>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
>>the abort/reset synchronization on top of it.
>Okay.

Just once things started to become easy.... So while I tried to have
only one status urb which I always re-submit (as Matthew/ You suggested)
I run into the problem that I don't have struct scsi_device yet. So I
just created a device with two luns to see if this struct happens always
to be same. Ofcourse it is not. 
So if we re-submit the one status URB over and over again, we have to
get the device right.
The only thing that we have left to get our device in the status
complete callback is the tag number which means we need a reverse lookup
for that. To make it little more fun, those two devices have their own
request queue so we can't use a 1:1 queue slot <-> tag number mapping
because we have to distinguish between tag 0 for lun0 and tag 0 for
lun1.
I admit now that it was a bad idea to test lun support on high speed.
The good news is that it worked on SS :)

| scsi0 : uas
| usbcore: registered new interface driver uas
| scsi 0:0:0:0: Direct-Access     LIO-ORG  RAMDISK-MCP      4.0  PQ: 0 ANSI: 5
| scsi 0:0:0:1: Direct-Access     LIO-ORG  FILEIO           4.0  PQ: 0 ANSI: 5
| sd 0:0:0:0: [sda] 262144 512-byte logical blocks: (134 MB/128 MiB)
| sd 0:0:0:1: [sdb] 61441 512-byte logical blocks: (31.4 MB/30.0 MiB)
| sd 0:0:0:0: [sda] Write Protect is off
| sd 0:0:0:0: [sda] Mode Sense: 2f 00 00 00
| sd 0:0:0:1: [sdb] Write Protect is off
| sd 0:0:0:1: [sdb] Mode Sense: 2f 00 00 00
| sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
| sd 0:0:0:1: [sdb] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
|  sda: unknown partition table
|  sdb: sdb1
| sd 0:0:0:1: [sdb] Attached SCSI disk
| sd 0:0:0:0: [sda] Attached SCSI disk

Going through to my log I saw that I have sometimes two requests with
the same tag on both luns more or less at the same time. So on SS both
requests are enqueued on the same stream with the same tag number. This
works because they are processed in order. This is not wrong but it
would be more efficient if we would enqueue them on two streams.

So. Where do we start? What about this:
Unique tag number => tag <-> device struct => one status urb for HS

>>Sarah Sharp

Sebastian

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Matthew Wilcox]

On Fri, Dec 16, 2011 at 09:12:36PM +0100, Sebastian Andrzej Siewior wrote:
> * Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:
> 
> >>If you want to take a stab at redoing your patch #2 to use only one
> >>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
> >>the abort/reset synchronization on top of it.
> >Okay.
> 
> Just once things started to become easy.... So while I tried to have
> only one status urb which I always re-submit (as Matthew/ You suggested)
> I run into the problem that I don't have struct scsi_device yet. So I
> just created a device with two luns to see if this struct happens always
> to be same. Ofcourse it is not. 

Can you not send one status URB per LUN (instead of one per command)?

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sebastian Andrzej Siewior]

* Matthew Wilcox | 2011-12-16 15:31:46 [-0500]:

>On Fri, Dec 16, 2011 at 09:12:36PM +0100, Sebastian Andrzej Siewior wrote:
>> * Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:
>> 
>> >>If you want to take a stab at redoing your patch #2 to use only one
>> >>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
>> >>the abort/reset synchronization on top of it.
>> >Okay.
>> 
>> Just once things started to become easy.... So while I tried to have
>> only one status urb which I always re-submit (as Matthew/ You suggested)
>> I run into the problem that I don't have struct scsi_device yet. So I
>> just created a device with two luns to see if this struct happens always
>> to be same. Ofcourse it is not. 
>
>Can you not send one status URB per LUN (instead of one per command)?

The thing is by the time a status URB completes I have only the *TAG*
number from the device which tells to which command it belongs. Sending
one status per LUN does not help because once a status URB with TAG 1
arrived I have no idea to which device/LUN it does belong. ->context
does not help here at all.

Sebastian

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sarah Sharp]

On Fri, Dec 16, 2011 at 09:12:36PM +0100, Sebastian Andrzej Siewior wrote:
> * Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:
> 
> >>If you want to take a stab at redoing your patch #2 to use only one
> >>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
> >>the abort/reset synchronization on top of it.
> >Okay.
> 
> Just once things started to become easy....

Nothing with UAS is coming easy, it seems. :)

> So while I tried to have
> only one status urb which I always re-submit (as Matthew/ You suggested)
> I run into the problem that I don't have struct scsi_device yet.

You mean you don't have the scsi_device in the status completion
handler?  Or where don't you have it?  In the probe function?

> So I
> just created a device with two luns to see if this struct happens always
> to be same. Ofcourse it is not. 
> So if we re-submit the one status URB over and over again, we have to
> get the device right.
> The only thing that we have left to get our device in the status
> complete callback is the tag number which means we need a reverse lookup
> for that. To make it little more fun, those two devices have their own
> request queue so we can't use a 1:1 queue slot <-> tag number mapping
> because we have to distinguish between tag 0 for lun0 and tag 0 for
> lun1.

Does that mean the SCSI core will try to submit multiple untagged
commands for each SCSI device?  If the code is structured such that only
one untagged command can be outstanding at once, is the SCSI core going
to retry the command later if we return SCSI_MLQUEUE_DEVICE_BUSY for the
second untagged command?  If it reacts badly, we might need to allow one
untagged command per lun.

> I admit now that it was a bad idea to test lun support on high speed.
> The good news is that it worked on SS :)

I'm not so sure it will with every SS device, see my comment below.

> | scsi0 : uas
> | usbcore: registered new interface driver uas
> | scsi 0:0:0:0: Direct-Access     LIO-ORG  RAMDISK-MCP      4.0  PQ: 0 ANSI: 5
> | scsi 0:0:0:1: Direct-Access     LIO-ORG  FILEIO           4.0  PQ: 0 ANSI: 5
> | sd 0:0:0:0: [sda] 262144 512-byte logical blocks: (134 MB/128 MiB)
> | sd 0:0:0:1: [sdb] 61441 512-byte logical blocks: (31.4 MB/30.0 MiB)
> | sd 0:0:0:0: [sda] Write Protect is off
> | sd 0:0:0:0: [sda] Mode Sense: 2f 00 00 00
> | sd 0:0:0:1: [sdb] Write Protect is off
> | sd 0:0:0:1: [sdb] Mode Sense: 2f 00 00 00
> | sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
> | sd 0:0:0:1: [sdb] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
> |  sda: unknown partition table
> |  sdb: sdb1
> | sd 0:0:0:1: [sdb] Attached SCSI disk
> | sd 0:0:0:0: [sda] Attached SCSI disk
> 
> Going through to my log I saw that I have sometimes two requests with
> the same tag on both luns more or less at the same time. So on SS both
> requests are enqueued on the same stream with the same tag number. This
> works because they are processed in order. This is not wrong but it
> would be more efficient if we would enqueue them on two streams.

I'm a bit concerned about the two status URBs queued to the stream
rings.  Say the SCSI core submits two commands, one for LUN 0 and one
for LUN 1, both on stream X.  The URB for LUN 0 is queued first, then
the URB for LUN 1.  The device recieves an ERDY on the command pipe,
retrieves the command for LUN 0, and then receives the ERDY on the
command pipe and retrieves the command for LUN 1.  If the device decides
to reorder those commands, it could return the data or status URB for
LUN 1 first, which may not work, because the URBs for LUN 0 are the
first URBs on the stream endpoint ring.

There's actually some wording in the UAS spec (section 4.2) that forbids
the same tag to be used across multiple LUNs:

"If a UAS target device performs tag checking and a UAS target port
calls SCSI Command Received () with a tag already in use by another
command (i.e., an overlapped command) in any logical unit, then the task
router and task manager(s) shall:
a) abort all task management functions received on that I_T nexus; and
b) respond to the overlapped command as defined in SAM-4."

So the UAS driver is going to have to make sure unique tags are used
across each LUN.  That means we can't just add one to the request tag in
the SCSI command, we need a tag map to keep track of unused tags.  We'll
also need to adjust the SCSI command queue length for each scsi_device
to make sure one LUN isn't starving another LUN by taking up all the
available streams the device or host can handle.  Possibly we could just
divide the stream ID address space evenly between all LUNs.

> So. Where do we start? What about this:
> Unique tag number => tag <-> device struct => one status urb for HS

Yes, I think we need unique tags for each scsi_device, and one status
URB per scsi_device for high speed devices.  Possibly the
uas_queuecmd_lck just needs to allocate a status URB for each device if
one isn't already allocated.  Then you could store the scsi_device
pointer from the scsi_cmd in the status URB.  The status URBs pointers
could be stored in an array in the devinfo structure (although it's hard
to figure out how big of an array until we know how many LUNs the device
has).

Sarah Sharp

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Sarah Sharp]

On Fri, Dec 16, 2011 at 09:42:41PM +0100, Sebastian Andrzej Siewior wrote:
> * Matthew Wilcox | 2011-12-16 15:31:46 [-0500]:
> 
> >On Fri, Dec 16, 2011 at 09:12:36PM +0100, Sebastian Andrzej Siewior wrote:
> >> * Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:
> >> 
> >> >>If you want to take a stab at redoing your patch #2 to use only one
> >> >>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
> >> >>the abort/reset synchronization on top of it.
> >> >Okay.
> >> 
> >> Just once things started to become easy.... So while I tried to have
> >> only one status urb which I always re-submit (as Matthew/ You suggested)
> >> I run into the problem that I don't have struct scsi_device yet. So I
> >> just created a device with two luns to see if this struct happens always
> >> to be same. Ofcourse it is not. 
> >
> >Can you not send one status URB per LUN (instead of one per command)?
> 
> The thing is by the time a status URB completes I have only the *TAG*
> number from the device which tells to which command it belongs. Sending
> one status per LUN does not help because once a status URB with TAG 1
> arrived I have no idea to which device/LUN it does belong. ->context
> does not help here at all.

Oh, right, now I understand.  With USB 2.0, we might have one status URB
per LUN, but they're being queued to the same endpoint ring.  Since the
device can re-order the commands any way it likes, we can't rely on the
scsi_device that's stored on the urb->context to be correct.

Ok, I think we just need to divide the tag address space equally between
devices.  The devinfo can keep track of the pointers to the scsi_devices
for each LUN, and what tag range each LUN has.  So if you have 255 tags
available, and two LUNs, you can give one of them tag 1 to 122, and the
other tags 123 to 254.

Sarah Sharp

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Alan Stern]

On Fri, 16 Dec 2011, Sarah Sharp wrote:

> Ok, I think we just need to divide the tag address space equally between
> devices.  The devinfo can keep track of the pointers to the scsi_devices
> for each LUN, and what tag range each LUN has.  So if you have 255 tags
> available, and two LUNs, you can give one of them tag 1 to 122, and the
> other tags 123 to 254.

The block layer already does a lot of this work for you.  Look through
block/blk-tags.c and you'll see what I mean: You ought to be able to
assign the same blk_queue_tag to all the request queues for all the
LUNs in your device.  And you can call blk_queue_find_tag() to do the 
reverse mapping.

Alan Stern

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[James Bottomley]

On Fri, 2011-12-16 at 13:36 -0800, Sarah Sharp wrote:
> On Fri, Dec 16, 2011 at 09:42:41PM +0100, Sebastian Andrzej Siewior wrote:
> > * Matthew Wilcox | 2011-12-16 15:31:46 [-0500]:
> > 
> > >On Fri, Dec 16, 2011 at 09:12:36PM +0100, Sebastian Andrzej Siewior wrote:
> > >> * Sebastian Andrzej Siewior | 2011-12-16 15:47:24 [+0100]:
> > >> 
> > >> >>If you want to take a stab at redoing your patch #2 to use only one
> > >> >>status URB for USB 2.0 devices, I would appreciate it.  Then I can build
> > >> >>the abort/reset synchronization on top of it.
> > >> >Okay.
> > >> 
> > >> Just once things started to become easy.... So while I tried to have
> > >> only one status urb which I always re-submit (as Matthew/ You suggested)
> > >> I run into the problem that I don't have struct scsi_device yet. So I
> > >> just created a device with two luns to see if this struct happens always
> > >> to be same. Ofcourse it is not. 
> > >
> > >Can you not send one status URB per LUN (instead of one per command)?
> > 
> > The thing is by the time a status URB completes I have only the *TAG*
> > number from the device which tells to which command it belongs. Sending
> > one status per LUN does not help because once a status URB with TAG 1
> > arrived I have no idea to which device/LUN it does belong. ->context
> > does not help here at all.
> 
> Oh, right, now I understand.  With USB 2.0, we might have one status URB
> per LUN, but they're being queued to the same endpoint ring.  Since the
> device can re-order the commands any way it likes, we can't rely on the
> scsi_device that's stored on the urb->context to be correct.
> 
> Ok, I think we just need to divide the tag address space equally between
> devices.  The devinfo can keep track of the pointers to the scsi_devices
> for each LUN, and what tag range each LUN has.  So if you have 255 tags
> available, and two LUNs, you can give one of them tag 1 to 122, and the
> other tags 123 to 254.

Well, no, what you want to do is use a shared tag map at the block
level.  That will manage a joint tag space for N queues without your
having to partition it arbitrarily.  see scsi_host_find_tag() and
scsi_init_shared_tag_map().

James

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Matthew Wilcox]

On Fri, Dec 16, 2011 at 10:47:39PM +0100, James Bottomley wrote:
> Well, no, what you want to do is use a shared tag map at the block
> level.  That will manage a joint tag space for N queues without your
> having to partition it arbitrarily.  see scsi_host_find_tag() and
> scsi_init_shared_tag_map().

By the way, the same verbiage about tags being shared across LUNs is also
present in the SAS spec (and various other specs last time I looked,
which wasn't recently).  We should probably make this the default in
the SCSI core.

I suspect it hasn't bitten us because there are so few multi-LUN devices
out there, and it's only a "May" check, not "Must" check.  But Linux is
technically in violation of the SAS spec by sending two commands to the
same target with the same tag.
--
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] usb/uas: use unique tags for all LUNs

[Sebastian Andrzej Siewior]

I observed that on a device with multiple LUNs UAS was re-using the same
tag number for requests which were issued at the same time to both LUNs.
This patch uses scsi_init_shared_tag_map() to use unique tags for all
LUNs. With this patch I haven't seen the same tag number during the init
sequence anymore. Devices which do not adverise command queueing I saw
only tag 1.
This patch initilizes the queue before adding the host like the other
two user in tree.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
* James Bottomley | 2011-12-16 22:47:39 [+0100]:

>Well, no, what you want to do is use a shared tag map at the block
>level.  That will manage a joint tag space for N queues without your
>having to partition it arbitrarily.  see scsi_host_find_tag() and
>scsi_init_shared_tag_map().

James, thank you for the two hints. This patch implements one of them.

 drivers/usb/storage/uas.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 09f55f9..4a8c062 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -690,6 +690,17 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo)
 	}
 }
 
+static void uas_free_streams(struct uas_dev_info *devinfo)
+{
+	struct usb_device *udev = devinfo->udev;
+	struct usb_host_endpoint *eps[3];
+
+	eps[0] = usb_pipe_endpoint(udev, devinfo->status_pipe);
+	eps[1] = usb_pipe_endpoint(udev, devinfo->data_in_pipe);
+	eps[2] = usb_pipe_endpoint(udev, devinfo->data_out_pipe);
+	usb_free_streams(devinfo->intf, eps, 3, GFP_KERNEL);
+}
+
 /*
  * XXX: What I'd like to do here is register a SCSI host for each USB host in
  * the system.  Follow usb-storage's design of registering a SCSI host for
@@ -719,18 +730,26 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	shost->max_id = 1;
 	shost->sg_tablesize = udev->bus->sg_tablesize;
 
-	result = scsi_add_host(shost, &intf->dev);
-	if (result)
-		goto free;
-	shost->hostdata[0] = (unsigned long)devinfo;
-
 	devinfo->intf = intf;
 	devinfo->udev = udev;
 	uas_configure_endpoints(devinfo);
 
+	result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 1);
+	if (result)
+		goto free;
+
+	result = scsi_add_host(shost, &intf->dev);
+	if (result)
+		goto deconfig_eps;
+
+	shost->hostdata[0] = (unsigned long)devinfo;
+
 	scsi_scan_host(shost);
 	usb_set_intfdata(intf, shost);
 	return result;
+
+deconfig_eps:
+	uas_free_streams(devinfo);
  free:
 	kfree(devinfo);
 	if (shost)
@@ -752,18 +771,11 @@ static int uas_post_reset(struct usb_interface *intf)
 
 static void uas_disconnect(struct usb_interface *intf)
 {
-	struct usb_device *udev = interface_to_usbdev(intf);
-	struct usb_host_endpoint *eps[3];
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
 	scsi_remove_host(shost);
-
-	eps[0] = usb_pipe_endpoint(udev, devinfo->status_pipe);
-	eps[1] = usb_pipe_endpoint(udev, devinfo->data_in_pipe);
-	eps[2] = usb_pipe_endpoint(udev, devinfo->data_out_pipe);
-	usb_free_streams(intf, eps, 3, GFP_KERNEL);

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[James Bottomley]

On Mon, 2011-12-19 at 11:12 -0500, Matthew Wilcox wrote:
> On Fri, Dec 16, 2011 at 10:47:39PM +0100, James Bottomley wrote:
> > Well, no, what you want to do is use a shared tag map at the block
> > level.  That will manage a joint tag space for N queues without your
> > having to partition it arbitrarily.  see scsi_host_find_tag() and
> > scsi_init_shared_tag_map().
> 
> By the way, the same verbiage about tags being shared across LUNs is also
> present in the SAS spec (and various other specs last time I looked,
> which wasn't recently).

I don't think so: The SAS spec still (as of 2.1) has the nexus being
I_T_L_Q.  That means a unique combination of initiator port, target
port, lun and tag identify a nexus.  To have the nexus be blind to the
Lun (i.e. tags shared across LUNs) it would have to be identified by
I_T_Q, which I haven't actually seen in any standard.

I'm not even sure a transport would be allowed to override this; SAM is
pretty clear (s 4.12 The Nexus Object) that a nexus is either I_T, I_T_L
or I_T_L_Q.  I_T_Q isn't listed as being allowable for a nexus.

James


>   We should probably make this the default in
> the SCSI core.
> 
> I suspect it hasn't bitten us because there are so few multi-LUN devices
> out there, and it's only a "May" check, not "Must" check.  But Linux is
> technically in violation of the SAS spec by sending two commands to the
> same target with the same tag.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Matthew Wilcox]

On Mon, Dec 19, 2011 at 11:14:10AM -0600, James Bottomley wrote:
> On Mon, 2011-12-19 at 11:12 -0500, Matthew Wilcox wrote:
> > By the way, the same verbiage about tags being shared across LUNs is also
> > present in the SAS spec (and various other specs last time I looked,
> > which wasn't recently).
> 
> I don't think so: The SAS spec still (as of 2.1) has the nexus being
> I_T_L_Q.  That means a unique combination of initiator port, target
> port, lun and tag identify a nexus.  To have the nexus be blind to the
> Lun (i.e. tags shared across LUNs) it would have to be identified by
> I_T_Q, which I haven't actually seen in any standard.

sas2r14:

10.2.3 Device server error handling
If the SCSI target device performs tag checking and an SSP target port calls SCSI Command Received () with
a tag already in use by another SCSI command (i.e., an overlapped command) in any logical unit, the task
router and device server(s) shall abort all task management functions received on that I_T nexus and shall
respond to the overlapped command as defined in SAM-4.

> I'm not even sure a transport would be allowed to override this; SAM is
> pretty clear (s 4.12 The Nexus Object) that a nexus is either I_T, I_T_L
> or I_T_L_Q.  I_T_Q isn't listed as being allowable for a nexus.

sam4r13f:

4.7.2 Command identifier

A command identifier (i.e., the Q in an I_T_L_Q nexus) is assigned by a
SCSI initiator device to uniquely identify one command in the context of a
particular I_T_L nexus, allowing more than one command to be outstanding
for that I_T_L nexus at the same time. Each SCSI transport protocol
defines the size of the command identifier, up to a maximum of 64 bytes,
to be used by SCSI ports that support that SCSI transport protocol.

SCSI transport protocols may define additional restrictions on command
identifier assignments (e.g., requiring command identifiers to be unique
per I_T nexus or per I_T_L nexus, or sharing command identifier values
with other uses such as task management functions).

[PATCH] usb/uas: use scsi_host_find_tag() to find command from a tag

[Sebastian Andrzej Siewior]

In "usb/uas: use unique tags for all LUNs" we make sure to create unique
tags across all LUNs. This patch uses scsi_host_find_tag() to obtain the
correct command which is associated with the tag.
The following changes were required:
- don't use sdev->current_cmnd anymore
  Since be can have devices which don't support command tagging we must
  ensure that we can tell the two commands apart. devinfo->cmnd is used
  for this.
- tag number are altered. On SuperSpeed the tag number must match the
  stream number. Therefore we can't use tag and must start at tag 1.
  In case we have untagged commands (atleast the first command and if
  the device does not support tagging) we must be able to distinguish
  between command tag 0 (which becomes 1) and untagged command.
  the following tag numbers are used:
  0: never
  1: for untagged commands (devinfo->cmnd)
  2+: tagged commands.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Sarah this is the second piece. Now it shouldn't be a problem anymore to
get one status URB in case streams are not used.

 drivers/usb/storage/uas.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4a8c062..ecf2070 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -98,6 +98,7 @@ struct uas_dev_info {
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
+	struct scsi_cmnd *cmnd;
 };
 
 enum {
@@ -178,8 +179,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
-	if (sdev->current_cmnd)
-		sdev->current_cmnd = NULL;
 	cmnd->scsi_done(cmnd);
 	usb_free_urb(urb);
 }
@@ -205,8 +204,6 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
-	if (sdev->current_cmnd)
-		sdev->current_cmnd = NULL;
 	cmnd->scsi_done(cmnd);
 	usb_free_urb(urb);
 }
@@ -230,8 +227,8 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
 static void uas_stat_cmplt(struct urb *urb)
 {
 	struct iu *iu = urb->transfer_buffer;
-	struct scsi_device *sdev = urb->context;
-	struct uas_dev_info *devinfo = sdev->hostdata;
+	struct Scsi_Host *shost = urb->context;
+	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 	struct scsi_cmnd *cmnd;
 	u16 tag;
 
@@ -242,10 +239,10 @@ static void uas_stat_cmplt(struct urb *urb)
 	}
 
 	tag = be16_to_cpup(&iu->tag) - 1;
-	if (sdev->current_cmnd)
-		cmnd = sdev->current_cmnd;
+	if (tag == 0)
+		cmnd = devinfo->cmnd;
 	else
-		cmnd = scsi_find_tag(sdev, tag);
+		cmnd = scsi_host_find_tag(shost, tag - 1);
 	if (!cmnd) {
 		usb_free_urb(urb);
 		return;
@@ -253,6 +250,9 @@ static void uas_stat_cmplt(struct urb *urb)
 
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
+		if (devinfo->cmnd == cmnd)
+			devinfo->cmnd = NULL;
+
 		if (urb->actual_length < 16)
 			devinfo->uas_sense_old = 1;
 		if (devinfo->uas_sense_old)
@@ -314,7 +314,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);
+						uas_stat_cmplt, cmnd->device->host);
 	urb->stream_id = stream_id;
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
@@ -346,7 +346,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 
 	iu->iu_id = IU_ID_COMMAND;
 	if (blk_rq_tagged(cmnd->request))
-		iu->tag = cpu_to_be16(cmnd->request->tag + 1);
+		iu->tag = cpu_to_be16(cmnd->request->tag + 2);
 	else
 		iu->tag = cpu_to_be16(1);
 
@@ -464,13 +464,13 @@ 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 (devinfo->cmnd)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	if (blk_rq_tagged(cmnd->request)) {
-		cmdinfo->stream = cmnd->request->tag + 1;
+		cmdinfo->stream = cmnd->request->tag + 2;
 	} else {
-		sdev->current_cmnd = cmnd;
+		devinfo->cmnd = cmnd;
 		cmdinfo->stream = 1;
 	}
 
@@ -571,7 +571,7 @@ static int uas_slave_configure(struct scsi_device *sdev)
 {
 	struct uas_dev_info *devinfo = sdev->hostdata;
 	scsi_set_tag_type(sdev, MSG_ORDERED_TAG);
-	scsi_activate_tcq(sdev, devinfo->qdepth - 1);
+	scsi_activate_tcq(sdev, devinfo->qdepth - 2);
 	return 0;
 }
 
@@ -639,6 +639,7 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo)
 	unsigned i, n_endpoints = intf->cur_altsetting->desc.bNumEndpoints;
 
 	devinfo->uas_sense_old = 0;
+	devinfo->cmnd = NULL;
 
 	for (i = 0; i < n_endpoints; i++) {
 		unsigned char *extra = endpoint[i].extra;
@@ -734,7 +735,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	devinfo->udev = udev;
 	uas_configure_endpoints(devinfo);
 
-	result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 1);
+	result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 2);
 	if (result)
 		goto free;
 
-- 
1.7.7.3

Re: [PATCH] usb/uas: use scsi_host_find_tag() to find command from a tag

[Matthew Wilcox]

On Mon, Dec 19, 2011 at 08:39:55PM +0100, Sebastian Andrzej Siewior wrote:
> In "usb/uas: use unique tags for all LUNs" we make sure to create unique
> tags across all LUNs. This patch uses scsi_host_find_tag() to obtain the
> correct command which is associated with the tag.
> The following changes were required:
> - don't use sdev->current_cmnd anymore
>   Since be can have devices which don't support command tagging we must
>   ensure that we can tell the two commands apart. devinfo->cmnd is used
>   for this.

I don't understand.  There's one devinfo per sdev.  How does moving the
untagged command anchor from sdev to devinfo change anything?

It's my understanding that the SCSI core will only send either a single
untagged command, or tagged commands.  ie any outstanding tagged command
will cause an untagged command to be deferred, and an outstanding untagged
command will prevent any other command from being sent to the driver.
--
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] usb/uas: use scsi_host_find_tag() to find command from a tag

[Sarah Sharp]

On Mon, Dec 19, 2011 at 02:50:50PM -0500, Matthew Wilcox wrote:
> On Mon, Dec 19, 2011 at 08:39:55PM +0100, Sebastian Andrzej Siewior wrote:
> > In "usb/uas: use unique tags for all LUNs" we make sure to create unique
> > tags across all LUNs. This patch uses scsi_host_find_tag() to obtain the
> > correct command which is associated with the tag.
> > The following changes were required:
> > - don't use sdev->current_cmnd anymore
> >   Since be can have devices which don't support command tagging we must
> >   ensure that we can tell the two commands apart. devinfo->cmnd is used
> >   for this.
> 
> I don't understand.  There's one devinfo per sdev.  How does moving the
> untagged command anchor from sdev to devinfo change anything?

I thought there was only one devinfo per USB device.  uas_probe() is
only called once per USB device, and that's where devinfo is allocated and
the scsi_host is created.

> It's my understanding that the SCSI core will only send either a single
> untagged command, or tagged commands.  ie any outstanding tagged command
> will cause an untagged command to be deferred, and an outstanding untagged
> command will prevent any other command from being sent to the driver.

Across all scsi_devices on a scsi_host?

In the status command completion for USB 2.0 devices, we can't know
which scsi_device we're completing the command for.  Is it an untagged
command completion for LUN 1 or LUN 2?  We can't tell, because the
device could reorder the commands across LUNs.  So for untagged USB 2.0
devices, we need to be sure there is only one untagged command pending
per scsi_host, and pull the command out of devinfo.

Sarah Sharp

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[James Bottomley]

On Mon, 2011-12-19 at 13:36 -0500, Matthew Wilcox wrote:
> On Mon, Dec 19, 2011 at 11:14:10AM -0600, James Bottomley wrote:
> > On Mon, 2011-12-19 at 11:12 -0500, Matthew Wilcox wrote:
> > > By the way, the same verbiage about tags being shared across LUNs is also
> > > present in the SAS spec (and various other specs last time I looked,
> > > which wasn't recently).
> > 
> > I don't think so: The SAS spec still (as of 2.1) has the nexus being
> > I_T_L_Q.  That means a unique combination of initiator port, target
> > port, lun and tag identify a nexus.  To have the nexus be blind to the
> > Lun (i.e. tags shared across LUNs) it would have to be identified by
> > I_T_Q, which I haven't actually seen in any standard.
> 
> sas2r14:
> 
> 10.2.3 Device server error handling
> If the SCSI target device performs tag checking and an SSP target port calls SCSI Command Received () with
> a tag already in use by another SCSI command (i.e., an overlapped command) in any logical unit, the task
> router and device server(s) shall abort all task management functions received on that I_T nexus and shall
> respond to the overlapped command as defined in SAM-4.
> 
> > I'm not even sure a transport would be allowed to override this; SAM is
> > pretty clear (s 4.12 The Nexus Object) that a nexus is either I_T, I_T_L
> > or I_T_L_Q.  I_T_Q isn't listed as being allowable for a nexus.
> 
> sam4r13f:
> 
> 4.7.2 Command identifier
> 
> A command identifier (i.e., the Q in an I_T_L_Q nexus) is assigned by a
> SCSI initiator device to uniquely identify one command in the context of a
> particular I_T_L nexus, allowing more than one command to be outstanding
> for that I_T_L nexus at the same time. Each SCSI transport protocol
> defines the size of the command identifier, up to a maximum of 64 bytes,
> to be used by SCSI ports that support that SCSI transport protocol.
> 
> SCSI transport protocols may define additional restrictions on command
> identifier assignments (e.g., requiring command identifiers to be unique
> per I_T nexus or per I_T_L nexus, or sharing command identifier values
> with other uses such as task management functions).

Hm, OK, that was actually something added to SAM-4.  It's not present in
SAM-3.  However, it does look like SAS always tried to get away with
this, notably by deciding in SAS-1 (which was SAM-3 based) that the
Frame tag wasn't the same thing as a SAM-3 Q.

James

Re: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[Matthew Wilcox]

On Mon, Dec 19, 2011 at 02:27:38PM -0600, James Bottomley wrote:
> Hm, OK, that was actually something added to SAM-4.  It's not present in
> SAM-3.  However, it does look like SAS always tried to get away with
> this, notably by deciding in SAS-1 (which was SAM-3 based) that the
> Frame tag wasn't the same thing as a SAM-3 Q.

So ... T10 have put us in a bit of an awkward position here; by solving
a problem that didn't need to be solved, they've created a new problem.
We're sending a packet stream that's permitted by SAS-1 standards and not
by SAS-2 standards (but probably isn't going to be checked by any device).
To be technically correct, we've got to use shared tag maps (with the
extra locking overhead) for any device that claims to be SAS-2 compliant.
That's ... complex.

So are we just going to pretend that T10 never made this change?  :-)

Re: [PATCH] usb/uas: use scsi_host_find_tag() to find command from a tag

[Matthew Wilcox]

On Mon, Dec 19, 2011 at 12:12:26PM -0800, Sarah Sharp wrote:
> On Mon, Dec 19, 2011 at 02:50:50PM -0500, Matthew Wilcox wrote:
> > On Mon, Dec 19, 2011 at 08:39:55PM +0100, Sebastian Andrzej Siewior wrote:
> > > In "usb/uas: use unique tags for all LUNs" we make sure to create unique
> > > tags across all LUNs. This patch uses scsi_host_find_tag() to obtain the
> > > correct command which is associated with the tag.
> > > The following changes were required:
> > > - don't use sdev->current_cmnd anymore
> > >   Since be can have devices which don't support command tagging we must
> > >   ensure that we can tell the two commands apart. devinfo->cmnd is used
> > >   for this.
> > 
> > I don't understand.  There's one devinfo per sdev.  How does moving the
> > untagged command anchor from sdev to devinfo change anything?
> 
> I thought there was only one devinfo per USB device.  uas_probe() is
> only called once per USB device, and that's where devinfo is allocated and
> the scsi_host is created.

Oh, duh, there's one devinfo per Scsi_Host.  Sorry, my mistake.

> > It's my understanding that the SCSI core will only send either a single
> > untagged command, or tagged commands.  ie any outstanding tagged command
> > will cause an untagged command to be deferred, and an outstanding untagged
> > command will prevent any other command from being sent to the driver.
> 
> Across all scsi_devices on a scsi_host?

Per scsi_dev ... which of course is insufficient ...

> In the status command completion for USB 2.0 devices, we can't know
> which scsi_device we're completing the command for.  Is it an untagged
> command completion for LUN 1 or LUN 2?  We can't tell, because the
> device could reorder the commands across LUNs.  So for untagged USB 2.0
> devices, we need to be sure there is only one untagged command pending
> per scsi_host, and pull the command out of devinfo.

Yes, makes sense now :-)  So we can have multiple tagged commands to LUN 2
and a single untagged command to LUN 1, which is just fine.
--
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: [usb-storage] Re: Make UAS work on HS for devices with and without command tagging support

[James Bottomley]

On Mon, 2011-12-19 at 15:57 -0500, Matthew Wilcox wrote:
> On Mon, Dec 19, 2011 at 02:27:38PM -0600, James Bottomley wrote:
> > Hm, OK, that was actually something added to SAM-4.  It's not present in
> > SAM-3.  However, it does look like SAS always tried to get away with
> > this, notably by deciding in SAS-1 (which was SAM-3 based) that the
> > Frame tag wasn't the same thing as a SAM-3 Q.
> 
> So ... T10 have put us in a bit of an awkward position here; by solving
> a problem that didn't need to be solved, they've created a new problem.
> We're sending a packet stream that's permitted by SAS-1 standards and not
> by SAS-2 standards (but probably isn't going to be checked by any device).
> To be technically correct, we've got to use shared tag maps (with the
> extra locking overhead) for any device that claims to be SAS-2 compliant.
> That's ... complex.
> 
> So are we just going to pretend that T10 never made this change?  :-)

No it's worse than that ... SAS-1 also had this language:

        9.2.1

        [...]

        For COMMAND frames and TASK frames, the SSP initiator port shall
        set the TAG field to a value that is unique for the I_T nexus
        established by the connection (see 7.12). An SSP initiator port
        shall not reuse the same tag when transmitting COMMAND frames or
        TASK frames to different LUNs in the same SSP target port. An
        SSP initiator port may reuse a tag when transmitting frames to
        different SSP target ports. The TAG field in a COMMAND frame
        contains the task tag defined in SAM-3. The TAG field in a TASK
        frame does not correspond to a SAM-3 task tag, but corresponds
        to an SAM-3 association (see 10.2.1). The tag space used in the
        TAG fields is shared across COMMAND frames and TASK frames
        (e.g., if a tag is used for a COMMAND frame, it is not also used
        for a concurrent TASK frame).

Note the weasel language about the tag not being the SAM-3 tag but
containing it.  I've no idea what a SAM-3 "association" is because a
word search on the SAM-3 spec doesn't find any such thing.  It looks
like SAS always intended this and SAM-4 was modified to allow it.

James