On Fri, Jan 20, 2012 at 08:57:20PM +0100, Sebastian Andrzej Siewior wrote:
> Just run into the following:
> - new disk arrived in the system
> - udev couldn't wait to get its hands on to to run ata_id /dev/sda
> - this sent the cdb 0xa1 to the device.
> - my UAS-gadget recevied the cdb and had no idea what to do with it. It
> decided to send a status URB back with sense set to invalid opcode.
Oh, right, UAS devices aren't supposed to stall transfers. Was skipping
to the status URB the correct behavior?
> - the host side received it status and completed the scsi command.
> - the host sent another scsi with 4kib data buffer
> - Now I was confused why the data transfer is only 512 bytes instead of
> 4kib since the host is always allocating the complete transfer in one
> go.
> - Finally the system crashed while walking through the sg list.
>
> This patch adds three new flags in order to distinguish between DATA
> URB completed and outstanding. If we receive status before data, we
> cancel data and let data complete the command.
> This solves the problem for IN and OUT transfers but does not work for
> BIDI.
I'm not sure the current patch will work with USB 2.0 UAS devices. The
device receives the setup URB. Later it responds with an error on the
status pipe about the legal request. But the data URBs would have never
been submitted because we never got the read/write ready response. I
think that means the call to usb_unlink_urb() will just fail without
calling the data URBs' completion functions. Then the SCSI command will
never get completed for USB 2.0 UAS devices with the type of error you
described. The previous code would have worked just fine though. :)
Why not have an atomic count in the uas_cmd_info of the number of URBs
that need to be completed? The status completion function can decrement
and test the count, and if it's non-zero, it should call
usb_unlink_urb() for the data URBs. If it's zero, we know the data URBs
(including both data URBs in the bidirectional sense) completed and we
can free all the URBs and complete the SCSI command.
Actually it sounds sort of like we just need real cancellation code with
USB anchors. Then we'd just mark the uas_cmd_info as being "dead"
because we got the status URB with the legal request, and ask some sort
of cancellation workqueue to go about killing and waiting for the
anchored data URBs to complete. But the atomic count might do until we
can get some real cancellation code in there.
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/storage/uas.c | 89 +++++++++++++++++++++++++++++++++++----------
> 1 files changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index f98ba40..17e4a96 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -58,6 +58,9 @@ enum {
> SUBMIT_DATA_OUT_URB = (1 << 5),
> ALLOC_CMD_URB = (1 << 6),
> SUBMIT_CMD_URB = (1 << 7),
> + COMPLETED_DATA_IN = (1 << 8),
> + COMPLETED_DATA_OUT = (1 << 9),
> + DATA_COMPLETES_CMD = (1 << 10),
> };
>
> /* Overrides scsi_pointer */
> @@ -111,6 +114,7 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
> {
> struct sense_iu *sense_iu = urb->transfer_buffer;
> struct scsi_device *sdev = cmnd->device;
> + struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>
> if (urb->actual_length > 16) {
> unsigned len = be16_to_cpup(&sense_iu->len);
> @@ -128,13 +132,15 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
> }
>
> cmnd->result = sense_iu->status;
> - cmnd->scsi_done(cmnd);
> + if (!(cmdinfo->state & DATA_COMPLETES_CMD))
> + cmnd->scsi_done(cmnd);
> }
>
> static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
> {
> struct sense_iu_old *sense_iu = urb->transfer_buffer;
> struct scsi_device *sdev = cmnd->device;
> + struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>
> if (urb->actual_length > 8) {
> unsigned len = be16_to_cpup(&sense_iu->len) - 2;
> @@ -152,7 +158,8 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
> }
>
> cmnd->result = sense_iu->status;
> - cmnd->scsi_done(cmnd);
> + if (!(cmdinfo->state & DATA_COMPLETES_CMD))
> + cmnd->scsi_done(cmnd);
> }
>
> static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
> @@ -177,6 +184,7 @@ static void uas_stat_cmplt(struct urb *urb)
> struct Scsi_Host *shost = urb->context;
> struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
> struct scsi_cmnd *cmnd;
> + struct uas_cmd_info *cmdinfo;
> u16 tag;
> int ret;
>
> @@ -202,12 +210,26 @@ static void uas_stat_cmplt(struct urb *urb)
> dev_err(&urb->dev->dev, "failed submit status urb\n");
> return;
> }
> + cmdinfo = (void *)&cmnd->SCp;
>
> switch (iu->iu_id) {
> case IU_ID_STATUS:
> if (devinfo->cmnd == cmnd)
> devinfo->cmnd = NULL;
>
> + if (!(cmdinfo->state & COMPLETED_DATA_IN) &&
> + cmdinfo->data_in_urb) {
> +
> + cmdinfo->state |= DATA_COMPLETES_CMD;
> + usb_unlink_urb(cmdinfo->data_in_urb);
> + }
> + if (!(cmdinfo->state & COMPLETED_DATA_OUT) &&
> + cmdinfo->data_out_urb) {
> +
> + cmdinfo->state |= DATA_COMPLETES_CMD;
> + usb_unlink_urb(cmdinfo->data_in_urb);
> + }
> +
> if (urb->actual_length < 16)
> devinfo->uas_sense_old = 1;
> if (devinfo->uas_sense_old)
> @@ -236,27 +258,59 @@ static void uas_stat_cmplt(struct urb *urb)
> dev_err(&urb->dev->dev, "failed submit status urb\n");
> }
>
> -static void uas_data_cmplt(struct urb *urb)
> +static void uas_data_out_cmplt(struct urb *urb)
> +{
> + struct scsi_cmnd *cmnd = urb->context;
> + struct scsi_data_buffer *sdb = scsi_out(cmnd);
> + struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> +
> + cmdinfo->state |= COMPLETED_DATA_OUT;
> +
> + sdb->resid = sdb->length - urb->actual_length;
> + usb_free_urb(urb);
> +
> + if (cmdinfo->state & DATA_COMPLETES_CMD)
> + cmnd->scsi_done(cmnd);
> +}
> +
> +static void uas_data_in_cmplt(struct urb *urb)
> {
> - struct scsi_data_buffer *sdb = urb->context;
> + struct scsi_cmnd *cmnd = urb->context;
> + struct scsi_data_buffer *sdb = scsi_in(cmnd);
> + struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> +
> + cmdinfo->state |= COMPLETED_DATA_IN;
> +
> sdb->resid = sdb->length - urb->actual_length;
> usb_free_urb(urb);
> +
> + if (cmdinfo->state & DATA_COMPLETES_CMD)
> + cmnd->scsi_done(cmnd);
> }
>
> static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
> - unsigned int pipe, u16 stream_id,
> - struct scsi_data_buffer *sdb,
> - enum dma_data_direction dir)
> + unsigned int pipe, struct scsi_cmnd *cmnd,
> + enum dma_data_direction dir)
> {
> + struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> struct usb_device *udev = devinfo->udev;
> struct urb *urb = usb_alloc_urb(0, gfp);
> + struct scsi_data_buffer *sdb;
> + usb_complete_t complete_fn;
> + u16 stream_id = cmdinfo->stream;
>
> if (!urb)
> goto out;
> - usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length, uas_data_cmplt,
> - sdb);
> - if (devinfo->use_streams)
> - urb->stream_id = stream_id;
> + if (dir == DMA_FROM_DEVICE) {
> + sdb = scsi_in(cmnd);
> + complete_fn = uas_data_in_cmplt;
> + } else {
> + sdb = scsi_out(cmnd);
> + complete_fn = uas_data_out_cmplt;
> + }
You can use usb_pipein() and usb_pipeout() to check the direction of the
endpoint, right? That would cut down on the number of lines you're
changing here, which will make it easier to read the bug fix.
> + usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length,
> + complete_fn, cmnd);
> + urb->stream_id = stream_id;
> urb->num_sgs = udev->bus->sg_tablesize ? sdb->table.nents : 0;
> urb->sg = sdb->table.sgl;
> out:
> @@ -309,10 +363,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
> goto free;
>
> iu->iu_id = IU_ID_COMMAND;
> - if (blk_rq_tagged(cmnd->request))
> - iu->tag = cpu_to_be16(cmnd->request->tag + 2);
> - else
> - iu->tag = cpu_to_be16(1);
> + iu->tag = cpu_to_be16(stream_id);
> iu->prio_attr = UAS_SIMPLE_TAG;
> iu->len = len;
> int_to_scsilun(sdev->lun, &iu->lun);
> @@ -358,8 +409,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>
> if (cmdinfo->state & ALLOC_DATA_IN_URB) {
> cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, gfp,
> - devinfo->data_in_pipe, cmdinfo->stream,
> - scsi_in(cmnd), DMA_FROM_DEVICE);
> + devinfo->data_in_pipe, cmnd,
> + DMA_FROM_DEVICE);
> if (!cmdinfo->data_in_urb)
> return SCSI_MLQUEUE_DEVICE_BUSY;
> cmdinfo->state &= ~ALLOC_DATA_IN_URB;
> @@ -376,8 +427,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>
> if (cmdinfo->state & ALLOC_DATA_OUT_URB) {
> cmdinfo->data_out_urb = uas_alloc_data_urb(devinfo, gfp,
> - devinfo->data_out_pipe, cmdinfo->stream,
> - scsi_out(cmnd), DMA_TO_DEVICE);
> + devinfo->data_out_pipe, cmnd,
> + DMA_TO_DEVICE);
> if (!cmdinfo->data_out_urb)
> return SCSI_MLQUEUE_DEVICE_BUSY;
> cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
> --
> 1.7.8.3
Sarah Sharp
--
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
