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

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

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.

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

index 680bea9..97f969c 100644
@@ -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

Hello.

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.

---
 drivers/usb/storage/uas.c |   56 +++++++++++++++++++++------------------------
 1 files changed, 26 insertions(+), 30 deletions(-)

index 97f969c..37a7028 100644
@@ -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

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?

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

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.

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.

Oh. That sounds great. I've been thinking how to avoid multiple allocation/allocation of them.

yep. Sounds like a plan.

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

Sebastian

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.

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yup. Since we don't have a reference, we can't free it.

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.

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

okay will do.

Okay.

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.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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

Sebastian

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

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

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

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

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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

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.

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

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

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.

---

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

index 09f55f9..4a8c062 100644
@@ -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);

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.

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

index 4a8c062..ecf2070 100644
@@ -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

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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

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

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

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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

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?

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'm not so sure it will with every SS device, see my comment below.

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.

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