Hi,
Thanks for working on this. I've not done much work
(if any) on uas lately) Are you interested in becoming a
co-maintainer for the uas driver, or even taking it over
completely ?
On 19-01-16 11:39, Oliver Neukum wrote:
UAS uses a shared tag space. We reserve tag #1 for management.
That leaves us with an offset of 2.
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/storage/uas.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4298885..6f312f9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -32,6 +32,12 @@
#define MAX_CMNDS 256
+/*
+ * 1 is due to the different base
+ * and 1 is reserved for the tmf
+ */
+#define TAG_OFFSET (1 + 1)
+
struct uas_dev_info {
struct usb_interface *intf;
struct usb_device *udev;
@@ -229,7 +235,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
DATA_OUT_URB_INFLIGHT |
COMMAND_ABORTED))
return -EBUSY;
- devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
+ devinfo->cmnd[cmdinfo->uas_tag - TAG_OFFSET] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd->scsi_done(cmnd);
return 0;
@@ -296,10 +302,10 @@ static void uas_stat_cmplt(struct urb *urb)
goto out;
}
- idx = be16_to_cpup(&iu->tag) - 1;
+ idx = be16_to_cpup(&iu->tag) - TAG_OFFSET;
if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
dev_err(&urb->dev->dev,
- "stat urb: no pending cmd for uas-tag %d\n", idx + 1);
+ "stat urb: no pending cmd for uas-tag %d\n", idx + TAG_OFFSET);
goto out;
}
@@ -661,7 +667,11 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
cmnd->scsi_done = done;
memset(cmdinfo, 0, sizeof(*cmdinfo));
- cmdinfo->uas_tag = idx + 1; /* uas-tag == usb-stream-id, so 1 based */
+ cmdinfo->uas_tag = idx + TAG_OFFSET;
+ /*
+ * uas-tag == usb-stream-id, so 1 based
+ * and 1 reserved for management
+ */
cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
switch (cmnd->sc_data_direction) {
idx comes from:
for (idx = 0; idx < devinfo->qdepth; idx++) {
if (!devinfo->cmnd[idx])
break;
}
devinfo->qdepth == max streams supported on the uas bridge, lets say 32,
idx may now become 31 here, TAG_OFFSET is 2, 31 + 2 = 33, which means
we end up trying to use an out of range bulk stream, not good.
@@ -717,7 +727,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
cmdinfo->state |= COMMAND_ABORTED;
/* Drop all refs to this cmnd, kill data urbs to break their ref */
- devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
+ devinfo->cmnd[cmdinfo->uas_tag - TAG_OFFSET] = NULL;
if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
If you do this you should also decrease the qdepth reported to the scsi
subsys by one:
In uas_slave_configure() :
scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
This should then become - 3, and we should probably document this,
rather then the existing undocumented - 2:
- 1 to make sure we always have a slot for untagged commands
- 1 because of the strream-ids being one based, and I suspect some
firmwares have their array indexing wrong and thus using the lasy
stream leads to an out of bounds array access in the firmware.
This may be just paranoia though, so we may be able to remove this
- 1, but better safe then sorry.
- 1 for the reserved slot
Or maybe reduce devinfo->qdepth by one, that would also fix the problem
I've described above.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
