From ea9da1c79eb9a28176550d0b8ba9166e6e5f42b8 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Fri, 2 Dec 2011 11:55:44 -0800 Subject: UAS: Re-add workqueue items if submission fails. If the original submission (or allocation) of the URBs for a SCSI command fails, the UAS driver sticks the command structure in a workqueue and schedules uas_do_work() to run. That function removes the entire queue before walking across it and attempting to resubmit. Unfortunately, if the second submission fails, we will leak memory (because an allocated URB was not submitted) and possibly leave the SCSI command partially enqueued on some of the stream rings. Fix this by checking whether the second submission failed and re-queueing the command to the UAS workqueue and scheduling it. Signed-off-by: Sarah Sharp Cc: Matthew Wilcox Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 1d10d5b8204c..4bbaf6e150e4 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -125,29 +125,38 @@ struct uas_cmd_info { /* I hate forward declarations, but I actually have a loop */ static int uas_submit_urbs(struct scsi_cmnd *cmnd, struct uas_dev_info *devinfo, gfp_t gfp); +static void uas_do_work(struct work_struct *work); +static DECLARE_WORK(uas_work, uas_do_work); static DEFINE_SPINLOCK(uas_work_lock); static LIST_HEAD(uas_work_list); static void uas_do_work(struct work_struct *work) { struct uas_cmd_info *cmdinfo; + struct uas_cmd_info *temp; struct list_head list; + int err; spin_lock_irq(&uas_work_lock); list_replace_init(&uas_work_list, &list); spin_unlock_irq(&uas_work_lock); - list_for_each_entry(cmdinfo, &list, list) { + list_for_each_entry_safe(cmdinfo, temp, &list, list) { struct scsi_pointer *scp = (void *)cmdinfo; struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp); - uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO); + err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO); + if (err) { + list_del(&cmdinfo->list); + spin_lock_irq(&uas_work_lock); + list_add_tail(&cmdinfo->list, &uas_work_list); + spin_unlock_irq(&uas_work_lock); + schedule_work(&uas_work); + } } } -static DECLARE_WORK(uas_work, uas_do_work); - static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd) { struct sense_iu *sense_iu = urb->transfer_buffer; -- cgit v1.2.3 From 9eb445410db99e5f5f660e97a2165a0567bd909e Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Fri, 2 Dec 2011 11:55:46 -0800 Subject: UAS: Use unique tags on non-streams devices. UAS can work with either USB 3.0 devices that support bulk streams, or USB 2.0 devices that do not support bulk streams. When we're working with a non-streams device, we need to be able to uniquely identify a SCSI command with a tag in the IU. Devices will barf and abort all queued commands if they find a duplicate tag. uas_queuecommand_lck() sets cmdinfo->stream to zero if the device doesn't support streams, which is later passed into uas_alloc_cmd_urb() as the variable stream. This means the UAS driver was setting the tag in all commands to zero for non-stream devices. So the UAS driver won't currently work with USB 2.0 devices. Use the SCSI command tag instead of the stream ID for the command IU tag. We have to add one to the SCSI command tag because SCSI tags are zero-based, but stream IDs are one-based, and the command tag must match the stream ID that we're queueing the data IUs for. Untagged SCSI commands use stream ID 1. Signed-off-by: Sarah Sharp Cc: Matthew Wilcox Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 4bbaf6e150e4..28d9b1909389 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -343,7 +343,10 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp, goto free; iu->iu_id = IU_ID_COMMAND; - iu->tag = cpu_to_be16(stream_id); + if (blk_rq_tagged(cmnd->request)) + iu->tag = cpu_to_be16(cmnd->request->tag + 1); + else + iu->tag = cpu_to_be16(1); iu->prio_attr = UAS_SIMPLE_TAG; iu->len = len; int_to_scsilun(sdev->lun, &iu->lun); -- cgit v1.2.3 From 96c1eb9873caffc507a1951c36b43fdcf3ddeff3 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Fri, 2 Dec 2011 11:55:48 -0800 Subject: UAS: Free status URB when we can't find the SCSI tag. In the UAS status URB completion handler, we need to free the URB, no matter what happens. Fix a bug where we would leak the URB (and its buffer) if we couldn't find a SCSI command that is associated with this status phase. Signed-off-by: Sarah Sharp Cc: Matthew Wilcox Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 28d9b1909389..9dd4aaee85cc 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -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: -- cgit v1.2.3 From dae51546b6564b06cbae4191d4f2dee7136be3c1 Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Mon, 19 Dec 2011 17:06:08 +0100 Subject: usb/uas: use unique tags for all LUNs 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. Tag 1 is used for devices which do not adverise command queueing. This patch initilizes the queue before adding the scsi host like the other two user in tree. Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 9dd4aaee85cc..6974f4bed2fd 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -684,6 +684,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 @@ -713,18 +724,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) @@ -746,18 +765,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); - + uas_free_streams(devinfo); kfree(devinfo); } -- cgit v1.2.3 From 22188f4a933c6e86ac67f52028895c795896492e Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Mon, 19 Dec 2011 20:22:39 +0100 Subject: usb/uas: use scsi_host_find_tag() to find command from a tag 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 are required: - don't use sdev->current_cmnd anymore Since we can have devices which don't support command queueing we must ensure that we can tell the two commands apart. Even if a device supports comand queuing we send the INQUIRY command "untagged" for LUN1 while we can send a tagged command to LUN0 at the same time. devinfo->cmnd is used for stashing the one "untagged" command. - tag number is altered. If stream support is used then the tag number must match the stream number. Therefore we can't use tag 0 and must start at tag 1. In case we have untagged commands (at least the first command) we must be able to distinguish between command tag 0 (which becomes 1) and untagged command (which becomes curently also 1). The following tag numbers are used: 0: never 1: for untagged commands (devinfo->cmnd) 2+: tagged commands. Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 6974f4bed2fd..e2386e8c7678 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); iu->prio_attr = UAS_SIMPLE_TAG; @@ -458,13 +458,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; } @@ -565,7 +565,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; } @@ -633,6 +633,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; @@ -728,7 +729,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; -- cgit v1.2.3 From ceb3f91fd53c9fbd7b292fc2754ba4efffeeeedb Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Tue, 20 Dec 2011 14:50:26 +0100 Subject: usb/uas: one only one status URB/host on stream-less connection The status/sense URB is allocated on per-command basis. A read/write looks the following way on a stream-less connection: - send cmd tag X, queue status - receive status, oh it is a read for tag X. queue status & read - receive read - receive status, oh I'm done for tag X. Cool call complete and free status urb. This block repeats itself 1:1 for further commands and looks great so far. Lets take a look now what happens if we do allow multiple commands: - send cmd tag X, queue statusX (belongs to the command with the X tag) - send cmd tag Y, queue statusY (belongs to the command with the Y tag) - receive statusX, oh it is a read for tag X. queue statusX & a read - receive read - receive statusY, oh I'm done for tag X. Cool call complete and free statusY. - receive statusX, oh it is a read for tag Y. queue statusY & before we queue the read the the following message can be observed: |sd 0:0:0:0: [sda] sense urb submission failure followed by a second attempt with the same result. In order to address this problem we will use only one status URB for each scsi host in case we don't have stream support (as suggested by Matthew). This URB is requeued until the device removed. Nothing changes on stream based endpoints. Signed-off-by: Sebastian Andrzej Siewior --- drivers/usb/storage/uas.c | 70 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 10 deletions(-) (limited to 'drivers/usb') diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index e2386e8c7678..036e96900956 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -99,6 +99,7 @@ struct uas_dev_info { unsigned use_streams:1; unsigned uas_sense_old:1; struct scsi_cmnd *cmnd; + struct urb *status_urb; /* used only if stream support is available */ }; enum { @@ -117,6 +118,7 @@ struct uas_cmd_info { unsigned int state; unsigned int stream; struct urb *cmd_urb; + /* status_urb is used only if stream support isn't available */ struct urb *status_urb; struct urb *data_in_urb; struct urb *data_out_urb; @@ -180,7 +182,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd) cmnd->result = sense_iu->status; cmnd->scsi_done(cmnd); - usb_free_urb(urb); } static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd) @@ -205,7 +206,6 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd) cmnd->result = sense_iu->status; cmnd->scsi_done(cmnd); - usb_free_urb(urb); } static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd, @@ -214,7 +214,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd, struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp; int err; - cmdinfo->state = direction | SUBMIT_STATUS_URB; + cmdinfo->state = direction; err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC); if (err) { spin_lock(&uas_work_lock); @@ -231,10 +231,12 @@ static void uas_stat_cmplt(struct urb *urb) struct uas_dev_info *devinfo = (void *)shost->hostdata[0]; struct scsi_cmnd *cmnd; u16 tag; + int ret; if (urb->status) { dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status); - usb_free_urb(urb); + if (devinfo->use_streams) + usb_free_urb(urb); return; } @@ -244,7 +246,13 @@ static void uas_stat_cmplt(struct urb *urb) else cmnd = scsi_host_find_tag(shost, tag - 1); if (!cmnd) { - usb_free_urb(urb); + if (devinfo->use_streams) { + usb_free_urb(urb); + return; + } + ret = usb_submit_urb(urb, GFP_ATOMIC); + if (ret) + dev_err(&urb->dev->dev, "failed submit status urb\n"); return; } @@ -270,6 +278,15 @@ static void uas_stat_cmplt(struct urb *urb) scmd_printk(KERN_ERR, cmnd, "Bogus IU (%d) received on status pipe\n", iu->iu_id); } + + if (devinfo->use_streams) { + usb_free_urb(urb); + return; + } + + ret = usb_submit_urb(urb, GFP_ATOMIC); + if (ret) + dev_err(&urb->dev->dev, "failed submit status urb\n"); } static void uas_data_cmplt(struct urb *urb) @@ -300,7 +317,7 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp, } static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp, - struct scsi_cmnd *cmnd, u16 stream_id) + struct Scsi_Host *shost, u16 stream_id) { struct usb_device *udev = devinfo->udev; struct urb *urb = usb_alloc_urb(0, gfp); @@ -314,7 +331,7 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp, goto free; usb_fill_bulk_urb(urb, udev, devinfo->status_pipe, iu, sizeof(*iu), - uas_stat_cmplt, cmnd->device->host); + uas_stat_cmplt, shost); urb->stream_id = stream_id; urb->transfer_flags |= URB_FREE_BUFFER; out: @@ -376,8 +393,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp; if (cmdinfo->state & ALLOC_STATUS_URB) { - cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, cmnd, - cmdinfo->stream); + cmdinfo->status_urb = uas_alloc_sense_urb(devinfo, gfp, + cmnd->device->host, cmdinfo->stream); if (!cmdinfo->status_urb) return SCSI_MLQUEUE_DEVICE_BUSY; cmdinfo->state &= ~ALLOC_STATUS_URB; @@ -486,7 +503,8 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd, } if (!devinfo->use_streams) { - cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB); + cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB | + ALLOC_STATUS_URB | SUBMIT_STATUS_URB); cmdinfo->stream = 0; } @@ -685,6 +703,29 @@ static void uas_configure_endpoints(struct uas_dev_info *devinfo) } } +static int uas_alloc_status_urb(struct uas_dev_info *devinfo, + struct Scsi_Host *shost) +{ + if (devinfo->use_streams) { + devinfo->status_urb = NULL; + return 0; + } + + devinfo->status_urb = uas_alloc_sense_urb(devinfo, GFP_KERNEL, + shost, 0); + if (!devinfo->status_urb) + goto err_s_urb; + + if (usb_submit_urb(devinfo->status_urb, GFP_KERNEL)) + goto err_submit_urb; + + return 0; +err_submit_urb: + usb_free_urb(devinfo->status_urb); +err_s_urb: + return -ENOMEM; +} + static void uas_free_streams(struct uas_dev_info *devinfo) { struct usb_device *udev = devinfo->udev; @@ -739,10 +780,17 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) shost->hostdata[0] = (unsigned long)devinfo; + result = uas_alloc_status_urb(devinfo, shost); + if (result) + goto err_alloc_status; + scsi_scan_host(shost); usb_set_intfdata(intf, shost); return result; +err_alloc_status: + scsi_remove_host(shost); + shost = NULL; deconfig_eps: uas_free_streams(devinfo); free: @@ -770,6 +818,8 @@ static void uas_disconnect(struct usb_interface *intf) struct uas_dev_info *devinfo = (void *)shost->hostdata[0]; scsi_remove_host(shost); + usb_kill_urb(devinfo->status_urb); + usb_free_urb(devinfo->status_urb); uas_free_streams(devinfo); kfree(devinfo); } -- cgit v1.2.3