[PATCH 0/7] uas: various fixes, add error handling.

[PATCH 0/7] uas: various fixes, add error handling.

[Gerd Hoffmann]

  Hi,

This patch series improves usb attached scsi support.  It reverts two
bogus fixes which just trade one bug for another, then it fixes the
issues properly.

Any use-after-free issues, leading to horrific "BUG kmalloc-192 (Not
tainted): Poison overwritten" messages and sometimes kernel panics are
gone.

The last patch adds task management support and winds it up in the scsi
error handling callbacks.

Could test it only in non-streams mode, so some additional testing with
usb3 gear would be great.  Reviews are very welcome too.

cheers,
  Gerd

Gerd Hoffmann (7):
  Revert "usb/uas: make sure data urb is gone if we receive status
    before that"
  Revert "usb/uas: one only one status URB/host on stream-less
    connection"
  uas: fix sense urb handling
  uas: keep track of command state, finish scsi cmd when really done.
  uas: improve error handling
  uas: track urbs, kill inflight urbs on disconnect.
  uas: task mgmt & error handling

 drivers/usb/storage/uas.c |  422 ++++++++++++++++++++++++++------------------
 include/linux/usb/uas.h   |   40 +++++
 2 files changed, 289 insertions(+), 173 deletions(-)

[PATCH 1/7] Revert "usb/uas: make sure data urb is gone if we receive status before that"

[Gerd Hoffmann]

This reverts commit e4d8318a85779b25b880187b1b1c44e797bd7d4b.

This patch makes uas.c call usb_unlink_urb on data urbs.  The data urbs
get freed in the completion callback.  This is illegal according to the
usb_unlink_urb documentation.

This patch also makes the code expect the data completion callback
being called before the status completion callback.  This isn't
guaranteed to be the case, even though the actual data transfer should
be finished by the time the status is received.

Background:  The ehci irq handler for example only know that there are
finished transfers, it then has go check the QHs & TDs to see which
transfers did actually finish.  It has no way to figure in which order
the transfers did complete.  The xhci driver can call the callbacks in
completion order thanks to the event queue.  This does nicely explain
why the driver is solid on a (usb2) xhci port whereas it goes crazy on
ehci in my testing.

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c |   90 +++++++-------------------------------------
 1 files changed, 15 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8ec8a6e..f98ba40 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -58,9 +58,6 @@ 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 */
@@ -114,7 +111,6 @@ 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);
@@ -132,15 +128,13 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
-	if (!(cmdinfo->state & DATA_COMPLETES_CMD))
-		cmnd->scsi_done(cmnd);
+	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;
@@ -158,8 +152,7 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
-	if (!(cmdinfo->state & DATA_COMPLETES_CMD))
-		cmnd->scsi_done(cmnd);
+	cmnd->scsi_done(cmnd);
 }
 
 static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
@@ -184,7 +177,6 @@ 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;
 
@@ -210,32 +202,12 @@ 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) {
-		       if (devinfo->use_streams) {
-			       cmdinfo->state |= DATA_COMPLETES_CMD;
-			       usb_unlink_urb(cmdinfo->data_in_urb);
-		       } else {
-			       usb_free_urb(cmdinfo->data_in_urb);
-		       }
-		}
-		if (!(cmdinfo->state & COMPLETED_DATA_OUT) &&
-				cmdinfo->data_out_urb) {
-			if (devinfo->use_streams) {
-				cmdinfo->state |= DATA_COMPLETES_CMD;
-				usb_unlink_urb(cmdinfo->data_in_urb);
-			} else {
-				usb_free_urb(cmdinfo->data_out_urb);
-			}
-		}
-
 		if (urb->actual_length < 16)
 			devinfo->uas_sense_old = 1;
 		if (devinfo->uas_sense_old)
@@ -264,59 +236,27 @@ static void uas_stat_cmplt(struct urb *urb)
 		dev_err(&urb->dev->dev, "failed submit status urb\n");
 }
 
-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)
+static void uas_data_cmplt(struct urb *urb)
 {
-	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;
-
+	struct scsi_data_buffer *sdb = urb->context;
 	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, struct scsi_cmnd *cmnd,
-		enum dma_data_direction dir)
+				unsigned int pipe, u16 stream_id,
+				struct scsi_data_buffer *sdb,
+				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;
-	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;
-	}
-	usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length,
-			complete_fn, cmnd);
-	urb->stream_id = stream_id;
+	usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length, uas_data_cmplt,
+									sdb);
+	if (devinfo->use_streams)
+		urb->stream_id = stream_id;
 	urb->num_sgs = udev->bus->sg_tablesize ? sdb->table.nents : 0;
 	urb->sg = sdb->table.sgl;
  out:
@@ -418,8 +358,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, cmnd,
-					DMA_FROM_DEVICE);
+					devinfo->data_in_pipe, cmdinfo->stream,
+					scsi_in(cmnd), DMA_FROM_DEVICE);
 		if (!cmdinfo->data_in_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_DATA_IN_URB;
@@ -436,8 +376,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, cmnd,
-					DMA_TO_DEVICE);
+					devinfo->data_out_pipe, cmdinfo->stream,
+					scsi_out(cmnd), DMA_TO_DEVICE);
 		if (!cmdinfo->data_out_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
-- 
1.7.1

--
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 1/7] Revert "usb/uas: make sure data urb is gone if we receive status before that"

[Greg KH]

On Tue, Jun 19, 2012 at 09:54:48AM +0200, Gerd Hoffmann wrote:
> This reverts commit e4d8318a85779b25b880187b1b1c44e797bd7d4b.
> 
> This patch makes uas.c call usb_unlink_urb on data urbs.  The data urbs
> get freed in the completion callback.  This is illegal according to the
> usb_unlink_urb documentation.
> 
> This patch also makes the code expect the data completion callback
> being called before the status completion callback.  This isn't
> guaranteed to be the case, even though the actual data transfer should
> be finished by the time the status is received.
> 
> Background:  The ehci irq handler for example only know that there are
> finished transfers, it then has go check the QHs & TDs to see which
> transfers did actually finish.  It has no way to figure in which order
> the transfers did complete.  The xhci driver can call the callbacks in
> completion order thanks to the event queue.  This does nicely explain
> why the driver is solid on a (usb2) xhci port whereas it goes crazy on
> ehci in my testing.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

Should this revert also go into 3.4-stable so that the device will work
properly on ehci?

thanks,

greg k-h

Re: [PATCH 1/7] Revert "usb/uas: make sure data urb is gone if we receive status before that"

[Gerd Hoffmann]

On 06/21/12 01:56, Greg KH wrote:
> On Tue, Jun 19, 2012 at 09:54:48AM +0200, Gerd Hoffmann wrote:
>> This reverts commit e4d8318a85779b25b880187b1b1c44e797bd7d4b.
>>
> 
> Should this revert also go into 3.4-stable so that the device will work
> properly on ehci?

Yes.

cheers,
  Gerd

--
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 2/7] Revert "usb/uas: one only one status URB/host on stream-less connection"

[Gerd Hoffmann]

This reverts commit ceb3f91fd53c9fbd7b292fc2754ba4efffeeeedb.

IMO the real bug is assigning status urbs to scsi requests.  First there
is no such link in the non-stream case.  Also there isn't nessesarely a
scsi request in the first place, for example when submitting task
management requests.

This patch just papers over the real bug and introduces different status
urb handling in the stream/non-stream case for no good reason.

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c |   70 ++++++--------------------------------------
 1 files changed, 10 insertions(+), 60 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f98ba40..36279a4 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -46,7 +46,6 @@ 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 {
@@ -65,7 +64,6 @@ 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;
@@ -129,6 +127,7 @@ 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)
@@ -153,6 +152,7 @@ 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,
@@ -161,7 +161,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;
+	cmdinfo->state = direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -178,12 +178,10 @@ 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);
-		if (devinfo->use_streams)
-			usb_free_urb(urb);
+		usb_free_urb(urb);
 		return;
 	}
 
@@ -193,13 +191,7 @@ static void uas_stat_cmplt(struct urb *urb)
 	else
 		cmnd = scsi_host_find_tag(shost, tag - 1);
 	if (!cmnd) {
-		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");
+		usb_free_urb(urb);
 		return;
 	}
 
@@ -225,15 +217,6 @@ 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)
@@ -264,7 +247,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_Host *shost, u16 stream_id)
+					struct scsi_cmnd *cmnd, u16 stream_id)
 {
 	struct usb_device *udev = devinfo->udev;
 	struct urb *urb = usb_alloc_urb(0, gfp);
@@ -278,7 +261,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, shost);
+						uas_stat_cmplt, cmnd->device->host);
 	urb->stream_id = stream_id;
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
@@ -340,8 +323,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->device->host, cmdinfo->stream);
+		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;
@@ -450,8 +433,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	}
 
 	if (!devinfo->use_streams) {
-		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB |
-				ALLOC_STATUS_URB | SUBMIT_STATUS_URB);
+		cmdinfo->state &= ~(SUBMIT_DATA_IN_URB | SUBMIT_DATA_OUT_URB);
 		cmdinfo->stream = 0;
 	}
 
@@ -662,29 +644,6 @@ 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,17 +698,10 @@ 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;

Re: [PATCH 2/7] Revert "usb/uas: one only one status URB/host on stream-less connection"

[Greg KH]

On Tue, Jun 19, 2012 at 09:54:49AM +0200, Gerd Hoffmann wrote:
> This reverts commit ceb3f91fd53c9fbd7b292fc2754ba4efffeeeedb.
> 
> IMO the real bug is assigning status urbs to scsi requests.  First there
> is no such link in the non-stream case.  Also there isn't nessesarely a
> scsi request in the first place, for example when submitting task
> management requests.
> 
> This patch just papers over the real bug and introduces different status
> urb handling in the stream/non-stream case for no good reason.
> 
> Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/usb/storage/uas.c |   70 ++++++--------------------------------------
>  1 files changed, 10 insertions(+), 60 deletions(-)

Same here, should this also be reverted in 3.4-stable?

thanks,

greg k-h
--
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 2/7] Revert "usb/uas: one only one status URB/host on stream-less connection"

[Gerd Hoffmann]

On 06/21/12 01:56, Greg KH wrote:
> On Tue, Jun 19, 2012 at 09:54:49AM +0200, Gerd Hoffmann wrote:
>> This reverts commit ceb3f91fd53c9fbd7b292fc2754ba4efffeeeedb.
>>
>> IMO the real bug is assigning status urbs to scsi requests.  First there
>> is no such link in the non-stream case.  Also there isn't nessesarely a
>> scsi request in the first place, for example when submitting task
>> management requests.
>>
>> This patch just papers over the real bug and introduces different status
>> urb handling in the stream/non-stream case for no good reason.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  drivers/usb/storage/uas.c |   70 ++++++--------------------------------------
>>  1 files changed, 10 insertions(+), 60 deletions(-)
> 
> Same here, should this also be reverted in 3.4-stable?

Well, even though the reverted patch is clumsy and takes the wrong
direction, it actually does the job and fixes the issue at hand.

When reverting in stable patch #3 of this series (which fixes the issue
in a better way) must be cherry-picked into stable too, otherwise we'll
get a regression.

I'd tend to not revert this one in stable though.

cheers,
  Gerd

Re: [PATCH 2/7] Revert "usb/uas: one only one status URB/host on stream-less connection"

[Greg KH]

On Thu, Jun 21, 2012 at 10:41:21AM +0200, Gerd Hoffmann wrote:
> On 06/21/12 01:56, Greg KH wrote:
> > On Tue, Jun 19, 2012 at 09:54:49AM +0200, Gerd Hoffmann wrote:
> >> This reverts commit ceb3f91fd53c9fbd7b292fc2754ba4efffeeeedb.
> >>
> >> IMO the real bug is assigning status urbs to scsi requests.  First there
> >> is no such link in the non-stream case.  Also there isn't nessesarely a
> >> scsi request in the first place, for example when submitting task
> >> management requests.
> >>
> >> This patch just papers over the real bug and introduces different status
> >> urb handling in the stream/non-stream case for no good reason.
> >>
> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >>  drivers/usb/storage/uas.c |   70 ++++++--------------------------------------
> >>  1 files changed, 10 insertions(+), 60 deletions(-)
> > 
> > Same here, should this also be reverted in 3.4-stable?
> 
> Well, even though the reverted patch is clumsy and takes the wrong
> direction, it actually does the job and fixes the issue at hand.
> 
> When reverting in stable patch #3 of this series (which fixes the issue
> in a better way) must be cherry-picked into stable too, otherwise we'll
> get a regression.
> 
> I'd tend to not revert this one in stable though.

Ok, thanks, I'll not revert it in staging.

greg k-h

[PATCH 3/7] uas: fix sense urb handling

[Gerd Hoffmann]

Stop reusing sense urbs, just allocate a fresh one each time and free it
when done.

Stop storing a sense urb pointer in the scsi request, all you can do
with it is misusing.  For example requeuing the sense urb, then f*ck it
up by picking the wrong one in case tagged requests don't finish in the
same order you've submitted them.  Also note that (not-yet supported)
task management ops don't have a scsi request in the first place.

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c |   49 +++++++++++++++++++++++++-------------------
 1 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 36279a4..b012738 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -49,7 +49,6 @@ struct uas_dev_info {
 };
 
 enum {
-	ALLOC_STATUS_URB	= (1 << 0),
 	SUBMIT_STATUS_URB	= (1 << 1),
 	ALLOC_DATA_IN_URB	= (1 << 2),
 	SUBMIT_DATA_IN_URB	= (1 << 3),
@@ -64,7 +63,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;
@@ -127,7 +125,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)
@@ -152,7 +149,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,
@@ -217,6 +213,7 @@ static void uas_stat_cmplt(struct urb *urb)
 		scmd_printk(KERN_ERR, cmnd,
 			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
 	}
+	usb_free_urb(urb);
 }
 
 static void uas_data_cmplt(struct urb *urb)
@@ -247,7 +244,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);
@@ -261,7 +258,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:
@@ -317,24 +314,35 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
  * daft to me.
  */
 
-static int uas_submit_urbs(struct scsi_cmnd *cmnd,
-					struct uas_dev_info *devinfo, gfp_t gfp)
+static int uas_submit_sense_urb(struct Scsi_Host *shost,
+				gfp_t gfp, unsigned int stream)
 {
-	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
+	struct urb *urb;
 
-	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;
+	urb = uas_alloc_sense_urb(devinfo, gfp, shost, stream);
+	if (!urb)
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	if (usb_submit_urb(urb, gfp)) {
+		shost_printk(KERN_INFO, shost,
+			     "sense urb submission failure\n");
+		usb_free_urb(urb);
+		return SCSI_MLQUEUE_DEVICE_BUSY;
 	}
+	return 0;
+}
+
+static int uas_submit_urbs(struct scsi_cmnd *cmnd,
+			   struct uas_dev_info *devinfo, gfp_t gfp)
+{
+	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	int err;
 
 	if (cmdinfo->state & SUBMIT_STATUS_URB) {
-		if (usb_submit_urb(cmdinfo->status_urb, gfp)) {
-			scmd_printk(KERN_INFO, cmnd,
-					"sense urb submission failure\n");
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+		err = uas_submit_sense_urb(cmnd->device->host, gfp,
+					   cmdinfo->stream);
+		if (err) {
+			return err;
 		}
 		cmdinfo->state &= ~SUBMIT_STATUS_URB;
 	}
@@ -417,7 +425,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	cmnd->scsi_done = done;
 
-	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
+	cmdinfo->state = SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
@@ -441,7 +449,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	if (err) {
 		/* If we did nothing, give up now */
 		if (cmdinfo->state & SUBMIT_STATUS_URB) {
-			usb_free_urb(cmdinfo->status_urb);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		spin_lock(&uas_work_lock);
-- 
1.7.1

--
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 7/7] uas: task mgmt & error handling

[Gerd Hoffmann]

Add task management support, wind up in abort and device reset error
handlers.  Cancel all in-flight urbs in bus reset handler.

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c |  160 ++++++++++++++++++++++++++++++++++++--------
 include/linux/usb/uas.h   |   40 +++++++++++
 2 files changed, 171 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 875829a..638cd64 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -43,7 +43,8 @@ struct uas_dev_info {
 	struct usb_device *udev;
 	struct usb_anchor sense_urbs;
 	struct usb_anchor data_urbs;
-	int qdepth;
+	int qdepth, resetting;
+	struct response_ui response;
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
@@ -68,6 +69,7 @@ enum {
 struct uas_cmd_info {
 	unsigned int state;
 	unsigned int stream;
+	unsigned int aborted;
 	struct urb *cmd_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
@@ -222,16 +224,24 @@ static void uas_stat_cmplt(struct urb *urb)
 		return;
 	}
 
+	if (devinfo->resetting) {
+		usb_free_urb(urb);
+		return;
+	}
+
 	tag = be16_to_cpup(&iu->tag) - 1;
 	if (tag == 0)
 		cmnd = devinfo->cmnd;
 	else
 		cmnd = scsi_host_find_tag(shost, tag - 1);
 	if (!cmnd) {
-		usb_free_urb(urb);
-		return;
+		if (iu->iu_id != IU_ID_RESPONSE) {
+			usb_free_urb(urb);
+			return;
+		}
+	} else {
+		cmdinfo = (void *)&cmnd->SCp;
 	}
-	cmdinfo = (void *)&cmnd->SCp;
 
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
@@ -260,6 +270,10 @@ static void uas_stat_cmplt(struct urb *urb)
 	case IU_ID_WRITE_READY:
 		uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
 		break;
+	case IU_ID_RESPONSE:
+		/* store results for uas_eh_task_mgmt() */
+		memcpy(&devinfo->response, iu, sizeof(devinfo->response));
+		break;
 	default:
 		scmd_printk(KERN_ERR, cmnd,
 			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
@@ -287,6 +301,9 @@ static void uas_data_cmplt(struct urb *urb)
 	} else {
 		sdb->resid = sdb->length - urb->actual_length;
 	}
+	if (cmdinfo->aborted) {
+		return;
+	}
 	uas_try_complete(cmnd, __func__);
 }
 
@@ -377,6 +394,51 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	return NULL;
 }
 
+static int uas_submit_task_urb(struct scsi_cmnd *cmnd, gfp_t gfp,
+			       u8 function, u16 stream_id)
+{
+	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+	struct usb_device *udev = devinfo->udev;
+	struct urb *urb = usb_alloc_urb(0, gfp);
+	struct task_mgmt_iu *iu;
+	int err = -ENOMEM;
+
+	if (!urb)
+		goto err;
+
+	iu = kzalloc(sizeof(*iu), gfp);
+	if (!iu)
+		goto err;
+
+	iu->iu_id = IU_ID_TASK_MGMT;
+	iu->tag = cpu_to_be16(stream_id);
+	int_to_scsilun(cmnd->device->lun, &iu->lun);
+
+	iu->function = function;
+	switch (function) {
+	case TMF_ABORT_TASK:
+		if (blk_rq_tagged(cmnd->request))
+			iu->task_tag = cpu_to_be16(cmnd->request->tag + 2);
+		else
+			iu->task_tag = cpu_to_be16(1);
+		break;
+	}
+
+	usb_fill_bulk_urb(urb, udev, devinfo->cmd_pipe, iu, sizeof(*iu),
+			  usb_free_urb, NULL);
+	urb->transfer_flags |= URB_FREE_BUFFER;
+
+	err = usb_submit_urb(urb, gfp);
+	if (err)
+		goto err;
+
+	return 0;
+
+err:
+	usb_free_urb(urb);
+	return err;
+}
+
 /*
  * Why should I request the Status IU before sending the Command IU?  Spec
  * says to, but also says the device may receive them in any order.  Seems
@@ -502,6 +564,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	cmdinfo->state = SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
+	cmdinfo->aborted = 0;
 
 	switch (cmnd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
@@ -537,34 +600,66 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 static DEF_SCSI_QCMD(uas_queuecommand)
 
-static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
+static int uas_eh_task_mgmt(struct scsi_cmnd *cmnd,
+			    const char *fname, u8 function)
 {
-	uas_log_cmd_state(cmnd, __func__);
+	struct Scsi_Host *shost = cmnd->device->host;
+	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
+	u16 tag = 9999; /* FIXME */
 
-/* XXX: Send ABORT TASK Task Management command */
-	return FAILED;
+	memset(&devinfo->response, 0, sizeof(devinfo->response));
+	if (uas_submit_sense_urb(shost, GFP_NOIO, tag)) {
+		shost_printk(KERN_INFO, shost,
+			     "%s: %s: submit sense urb failed\n",
+			     __func__, fname);
+		return FAILED;
+	}
+	if (uas_submit_task_urb(cmnd, GFP_NOIO, function, tag)) {
+		shost_printk(KERN_INFO, shost,
+			     "%s: %s: submit task mgmt urb failed\n",
+			     __func__, fname);
+		return FAILED;
+	}
+	if (0 == usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 3000)) {
+		shost_printk(KERN_INFO, shost,
+			     "%s: %s timed out\n", __func__, fname);
+		return FAILED;
+	}
+	if (be16_to_cpu(devinfo->response.tag) != tag) {
+		shost_printk(KERN_INFO, shost,
+			     "%s: %s failed (wrong tag %d/%d)\n", __func__,
+			     fname, be16_to_cpu(devinfo->response.tag), tag);
+		return FAILED;
+	}
+	if (devinfo->response.response_code != RC_TMF_COMPLETE) {
+		shost_printk(KERN_INFO, shost,
+			     "%s: %s failed (rc 0x%x)\n", __func__,
+			     fname, devinfo->response.response_code);
+		return FAILED;
+	}
+	return SUCCESS;
 }
 
-static int uas_eh_device_reset_handler(struct scsi_cmnd *cmnd)
+static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 {
-	struct scsi_device *sdev = cmnd->device;
-	sdev_printk(KERN_INFO, sdev, "%s tag %d\n", __func__,
-							cmnd->request->tag);
+	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	int ret;
 
-/* XXX: Send LOGICAL UNIT RESET Task Management command */
-	return FAILED;
+	uas_log_cmd_state(cmnd, __func__);
+	cmdinfo->aborted = 1;
+	ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
+	if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+		usb_kill_urb(cmdinfo->data_in_urb);
+	if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+		usb_kill_urb(cmdinfo->data_out_urb);
+	return ret;
 }
 
-static int uas_eh_target_reset_handler(struct scsi_cmnd *cmnd)
+static int uas_eh_device_reset_handler(struct scsi_cmnd *cmnd)
 {
-	struct scsi_device *sdev = cmnd->device;
-	sdev_printk(KERN_INFO, sdev, "%s tag %d\n", __func__,
-							cmnd->request->tag);
-
-/* XXX: Can we reset just the one USB interface?
- * Would calling usb_set_interface() have the right effect?
- */
-	return FAILED;
+	sdev_printk(KERN_INFO, cmnd->device, "%s\n", __func__);
+	return uas_eh_task_mgmt(cmnd, "LOGICAL UNIT RESET",
+				TMF_LOGICAL_UNIT_RESET);
 }
 
 static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
@@ -572,14 +667,21 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	struct scsi_device *sdev = cmnd->device;
 	struct uas_dev_info *devinfo = sdev->hostdata;
 	struct usb_device *udev = devinfo->udev;
+	int err;
 
-	sdev_printk(KERN_INFO, sdev, "%s tag %d\n", __func__,
-							cmnd->request->tag);
+	devinfo->resetting = 1;
+	usb_kill_anchored_urbs(&devinfo->sense_urbs);
+	usb_kill_anchored_urbs(&devinfo->data_urbs);
+	err = usb_reset_device(udev);
+	devinfo->resetting = 0;
 
-	if (usb_reset_device(udev))
-		return SUCCESS;
+	if (err) {
+		shost_printk(KERN_INFO, sdev->host, "%s FAILED\n", __func__);
+		return FAILED;
+	}
 
-	return FAILED;
+	shost_printk(KERN_INFO, sdev->host, "%s success\n", __func__);
+	return SUCCESS;
 }
 
 static int uas_slave_alloc(struct scsi_device *sdev)
@@ -604,7 +706,6 @@ static struct scsi_host_template uas_host_template = {
 	.slave_configure = uas_slave_configure,
 	.eh_abort_handler = uas_eh_abort_handler,
 	.eh_device_reset_handler = uas_eh_device_reset_handler,
-	.eh_target_reset_handler = uas_eh_target_reset_handler,
 	.eh_bus_reset_handler = uas_eh_bus_reset_handler,
 	.can_queue = 65536,	/* Is there a limit on the _host_ ? */
 	.this_id = -1,
@@ -766,6 +867,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	devinfo->intf = intf;
 	devinfo->udev = udev;
+	devinfo->resetting = 0;
 	init_usb_anchor(&devinfo->sense_urbs);
 	init_usb_anchor(&devinfo->data_urbs);
 	uas_configure_endpoints(devinfo);
diff --git a/include/linux/usb/uas.h b/include/linux/usb/uas.h
index 9a988e4..5499ab5 100644
--- a/include/linux/usb/uas.h
+++ b/include/linux/usb/uas.h
@@ -20,6 +20,28 @@ enum {
 	IU_ID_WRITE_READY	= 0x07,
 };
 
+enum {
+	TMF_ABORT_TASK          = 0x01,
+	TMF_ABORT_TASK_SET      = 0x02,
+	TMF_CLEAR_TASK_SET      = 0x04,
+	TMF_LOGICAL_UNIT_RESET  = 0x08,
+	TMF_I_T_NEXUS_RESET     = 0x10,
+	TMF_CLEAR_ACA           = 0x40,
+	TMF_QUERY_TASK          = 0x80,
+	TMF_QUERY_TASK_SET      = 0x81,
+	TMF_QUERY_ASYNC_EVENT   = 0x82,
+};
+
+enum {
+	RC_TMF_COMPLETE         = 0x00,
+	RC_INVALID_INFO_UNIT    = 0x02,
+	RC_TMF_NOT_SUPPORTED    = 0x04,
+	RC_TMF_FAILED           = 0x05,
+	RC_TMF_SUCCEEDED        = 0x08,
+	RC_INCORRECT_LUN        = 0x09,
+	RC_OVERLAPPED_TAG       = 0x0a,
+};
+
 struct command_iu {
 	__u8 iu_id;
 	__u8 rsvd1;
@@ -32,6 +54,16 @@ struct command_iu {
 	__u8 cdb[16];	/* XXX: Overflow-checking tools may misunderstand */
 };
 
+struct task_mgmt_iu {
+	__u8 iu_id;
+	__u8 rsvd1;
+	__be16 tag;
+	__u8 function;
+	__u8 rsvd2;
+	__be16 task_tag;
+	struct scsi_lun lun;
+};
+
 /*
  * Also used for the Read Ready and Write Ready IUs since they have the
  * same first four bytes
@@ -47,6 +79,14 @@ struct sense_iu {
 	__u8 sense[SCSI_SENSE_BUFFERSIZE];
 };
 
+struct response_ui {
+	__u8 iu_id;
+	__u8 rsvd1;
+	__be16 tag;
+	__be16 add_response_info;
+	__u8 response_code;
+};
+
 struct usb_pipe_usage_descriptor {
 	__u8  bLength;
 	__u8  bDescriptorType;
-- 
1.7.1

--
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 4/7] uas: keep track of command state, finish scsi cmd when really done.

[Gerd Hoffmann]

Set state bits after submitting data urbs & command urbs, so we know
what is in flight.  Clear data bits when the data urb is finished, clear
command bit when we see the status urb for the command.  Finish the scsi
command after running both status and data completion handlers for the
command.

Add a cmd status logging function for debugging purposes.  Hook it into
the error handler, so we see in the log what status a command is in
which the scsi layer wants cancel.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/usb/storage/uas.c |   86 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b012738..b589b2e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -56,6 +56,10 @@ enum {
 	SUBMIT_DATA_OUT_URB	= (1 << 5),
 	ALLOC_CMD_URB		= (1 << 6),
 	SUBMIT_CMD_URB		= (1 << 7),
+	COMMAND_INFLIGHT        = (1 << 8),
+	DATA_IN_URB_INFLIGHT    = (1 << 9),
+	DATA_OUT_URB_INFLIGHT   = (1 << 10),
+	COMMAND_COMPLETED       = (1 << 11),
 };
 
 /* Overrides scsi_pointer */
@@ -124,7 +128,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
-	cmnd->scsi_done(cmnd);
 }
 
 static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
@@ -148,16 +151,51 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 	}
 
 	cmnd->result = sense_iu->status;
+}
+
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
+{
+	struct uas_cmd_info *ci = (void *)&cmnd->SCp;
+
+	scmd_printk(KERN_INFO, cmnd, "%s %p tag %d, inflight:"
+		    "%s%s%s%s%s%s%s%s%s%s%s\n",
+		    caller, cmnd, cmnd->request->tag,
+		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
+		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
+		    (ci->state & SUBMIT_DATA_IN_URB)    ? " s-in"  : "",
+		    (ci->state & ALLOC_DATA_OUT_URB)    ? " a-out" : "",
+		    (ci->state & SUBMIT_DATA_OUT_URB)   ? " s-out" : "",
+		    (ci->state & ALLOC_CMD_URB)         ? " a-cmd" : "",
+		    (ci->state & SUBMIT_CMD_URB)        ? " s-cmd" : "",
+		    (ci->state & COMMAND_INFLIGHT)      ? " CMD"   : "",
+		    (ci->state & DATA_IN_URB_INFLIGHT)  ? " IN"    : "",
+		    (ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT"   : "",
+		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "");
+}
+
+static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
+{
+	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+
+	if (cmdinfo->state & (COMMAND_INFLIGHT |
+			      DATA_IN_URB_INFLIGHT |
+			      DATA_OUT_URB_INFLIGHT))
+		return -EBUSY;
+	BUG_ON(cmdinfo->state & COMMAND_COMPLETED);
+	cmdinfo->state |= COMMAND_COMPLETED;
+	usb_free_urb(cmdinfo->data_in_urb);
+	usb_free_urb(cmdinfo->data_out_urb);
 	cmnd->scsi_done(cmnd);
+	return 0;
 }
 
 static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
-							unsigned direction)
+			  unsigned direction)
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	int err;
 
-	cmdinfo->state = direction | SUBMIT_STATUS_URB;
+	cmdinfo->state |= direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
 	if (err) {
 		spin_lock(&uas_work_lock);
@@ -173,6 +211,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;
 
 	if (urb->status) {
@@ -190,6 +229,7 @@ static void uas_stat_cmplt(struct urb *urb)
 		usb_free_urb(urb);
 		return;
 	}
+	cmdinfo = (void *)&cmnd->SCp;
 
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
@@ -202,6 +242,8 @@ static void uas_stat_cmplt(struct urb *urb)
 			uas_sense_old(urb, cmnd);
 		else
 			uas_sense(urb, cmnd);
+		cmdinfo->state &= ~COMMAND_INFLIGHT;
+		uas_try_complete(cmnd, __func__);
 		break;
 	case IU_ID_READ_READY:
 		uas_xfer_data(urb, cmnd, SUBMIT_DATA_IN_URB);
@@ -218,23 +260,36 @@ static void uas_stat_cmplt(struct urb *urb)
 
 static void uas_data_cmplt(struct urb *urb)
 {
-	struct scsi_data_buffer *sdb = urb->context;
+	struct scsi_cmnd *cmnd = urb->context;
+	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	struct scsi_data_buffer *sdb = NULL;
+
+	if (cmdinfo->data_in_urb == urb) {
+		sdb = scsi_in(cmnd);
+		cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
+	} else if (cmdinfo->data_out_urb == urb) {
+		sdb = scsi_out(cmnd);
+		cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
+	}
+	BUG_ON(sdb == NULL);
 	sdb->resid = sdb->length - urb->actual_length;
-	usb_free_urb(urb);
+	uas_try_complete(cmnd, __func__);
 }
 
 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, u16 stream_id,
+				      struct scsi_cmnd *cmnd,
+				      enum dma_data_direction dir)
 {
 	struct usb_device *udev = devinfo->udev;
 	struct urb *urb = usb_alloc_urb(0, gfp);
+	struct scsi_data_buffer *sdb = (dir == DMA_FROM_DEVICE)
+		? scsi_in(cmnd) : scsi_out(cmnd);
 
 	if (!urb)
 		goto out;
-	usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length, uas_data_cmplt,
-									sdb);
+	usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb->length,
+			  uas_data_cmplt, cmnd);
 	if (devinfo->use_streams)
 		urb->stream_id = stream_id;
 	urb->num_sgs = udev->bus->sg_tablesize ? sdb->table.nents : 0;
@@ -350,7 +405,7 @@ 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);
+					cmnd, DMA_FROM_DEVICE);
 		if (!cmdinfo->data_in_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_DATA_IN_URB;
@@ -363,12 +418,13 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
+		cmdinfo->state |= DATA_IN_URB_INFLIGHT;
 	}
 
 	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);
+					cmnd, DMA_TO_DEVICE);
 		if (!cmdinfo->data_out_urb)
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
@@ -381,6 +437,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
+		cmdinfo->state |= DATA_OUT_URB_INFLIGHT;
 	}
 
 	if (cmdinfo->state & ALLOC_CMD_URB) {
@@ -398,6 +455,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->state &= ~SUBMIT_CMD_URB;
+		cmdinfo->state |= COMMAND_INFLIGHT;
 	}
 
 	return 0;
@@ -464,9 +522,7 @@ static DEF_SCSI_QCMD(uas_queuecommand)
 
 static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 {
-	struct scsi_device *sdev = cmnd->device;
-	sdev_printk(KERN_INFO, sdev, "%s tag %d\n", __func__,
-							cmnd->request->tag);
+	uas_log_cmd_state(cmnd, __func__);
 
 /* XXX: Send ABORT TASK Task Management command */
 	return FAILED;
-- 
1.7.1

[PATCH 5/7] uas: improve error handling

[Gerd Hoffmann]

(1) Handle data pipe errors: When the data urb failed we
    didn't transfer anything, update scsi_cmnd accordingly.
(2) Cancel data transfers when we got back an error on the
    status pipe.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/usb/storage/uas.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b589b2e..d8b7bc6 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -242,6 +242,13 @@ static void uas_stat_cmplt(struct urb *urb)
 			uas_sense_old(urb, cmnd);
 		else
 			uas_sense(urb, cmnd);
+		if (cmnd->result != 0) {
+			/* cancel data transfers on error */
+			if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+				usb_unlink_urb(cmdinfo->data_in_urb);
+			if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+				usb_unlink_urb(cmdinfo->data_out_urb);
+		}
 		cmdinfo->state &= ~COMMAND_INFLIGHT;
 		uas_try_complete(cmnd, __func__);
 		break;
@@ -272,7 +279,12 @@ static void uas_data_cmplt(struct urb *urb)
 		cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
 	}
 	BUG_ON(sdb == NULL);
-	sdb->resid = sdb->length - urb->actual_length;
+	if (urb->status) {
+		/* error: no data transfered */
+		sdb->resid = sdb->length;
+	} else {
+		sdb->resid = sdb->length - urb->actual_length;
+	}
 	uas_try_complete(cmnd, __func__);
 }
 
-- 
1.7.1

[PATCH 6/7] uas: track urbs, kill inflight urbs on disconnect.

[Gerd Hoffmann]

Use separate anchors for data and sense urbs, which
I think will be useful when implementing error handling.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/usb/storage/uas.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d8b7bc6..875829a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -41,6 +41,8 @@ struct sense_iu_old {
 struct uas_dev_info {
 	struct usb_interface *intf;
 	struct usb_device *udev;
+	struct usb_anchor sense_urbs;
+	struct usb_anchor data_urbs;
 	int qdepth;
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
 	unsigned use_streams:1;
@@ -396,6 +398,7 @@ static int uas_submit_sense_urb(struct Scsi_Host *shost,
 		usb_free_urb(urb);
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 	}
+	usb_anchor_urb(urb, &devinfo->sense_urbs);
 	return 0;
 }
 
@@ -431,6 +434,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
 		cmdinfo->state |= DATA_IN_URB_INFLIGHT;
+		usb_anchor_urb(cmdinfo->data_in_urb, &devinfo->data_urbs);
 	}
 
 	if (cmdinfo->state & ALLOC_DATA_OUT_URB) {
@@ -450,6 +454,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
 		cmdinfo->state |= DATA_OUT_URB_INFLIGHT;
+		usb_anchor_urb(cmdinfo->data_out_urb, &devinfo->data_urbs);
 	}
 
 	if (cmdinfo->state & ALLOC_CMD_URB) {
@@ -761,6 +766,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	devinfo->intf = intf;
 	devinfo->udev = udev;
+	init_usb_anchor(&devinfo->sense_urbs);
+	init_usb_anchor(&devinfo->data_urbs);
 	uas_configure_endpoints(devinfo);
 
 	result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 2);
@@ -804,6 +811,8 @@ static void uas_disconnect(struct usb_interface *intf)
 	struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
 	scsi_remove_host(shost);
+	usb_kill_anchored_urbs(&devinfo->sense_urbs);
+	usb_kill_anchored_urbs(&devinfo->data_urbs);
 	uas_free_streams(devinfo);
 	kfree(devinfo);
 }
-- 
1.7.1