[PATCH 00/21] uas: rewrite error handling for robustness + misc cleanups

[PATCH 00/21] uas: rewrite error handling for robustness + misc cleanups

[Hans de Goede]

Hi Greg, et al,

Since we've been receiving multiple bug reports with crashes / oopses related
to uas error handling, I've spend the last 7 days rewriting the error handling
code. This new code has been extensively tested, doing externally triggered
usb-device-resets and scsi bus resets while having multiple io streams
active. It has all seen some serious shakedown in my attempts to get the
ASM1051 to work, which would throw all kind of fun errors.

Without this series (and without the blacklist) connecting an ASM1051 device
causes a lockup 30 seconds after plug-in (so when the first scsi timeout
fires), with this series it goes into error handling (*) for ages before
finding the disk (and then some more with some disks), but it actually
works somewhat, and the system does not crash (nor oopses).

Greg, baring review turning up any issues, can you please queue this up for
3.18 ?

Thanks & Regards,

Hans


*) Because the ASM1051 chokes on report opcodes

[PATCH 04/21] uas: Add uas_get_tag() helper function

[Hans de Goede]

Factor out the mapping of scsi-tags -> uas-tags/stream-ids to a helper function
so that there is a single place where this "magic" happens.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4f745a3..05c1c81 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -257,13 +257,29 @@ static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
 	cmnd->result = sense_iu->status;
 }
 
+/*
+ * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
+ * which go from 1 - nr_streams. And we use 1 for untagged commands.
+ */
+static int uas_get_tag(struct scsi_cmnd *cmnd)
+{
+	int tag;
+
+	if (blk_rq_tagged(cmnd->request))
+		tag = cmnd->request->tag + 2;
+	else
+		tag = 1;
+
+	return tag;
+}
+
 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%s%s%s\n",
-		    caller, cmnd, cmnd->request->tag,
+		    caller, cmnd, uas_get_tag(cmnd),
 		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
 		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
 		    (ci->state & SUBMIT_DATA_IN_URB)    ? " s-in"  : "",
@@ -514,10 +530,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(uas_get_tag(cmnd));
 	iu->prio_attr = UAS_SIMPLE_TAG;
 	iu->len = len;
 	int_to_scsilun(sdev->lun, &iu->lun);
@@ -679,17 +692,13 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	memset(cmdinfo, 0, sizeof(*cmdinfo));
 
-	if (blk_rq_tagged(cmnd->request)) {
-		cmdinfo->stream = cmnd->request->tag + 2;
-	} else {
+	if (!blk_rq_tagged(cmnd->request))
 		devinfo->cmnd = cmnd;
-		cmdinfo->stream = 1;
-	}
 
 	cmnd->scsi_done = done;
 
-	cmdinfo->state = SUBMIT_STATUS_URB |
-			ALLOC_CMD_URB | SUBMIT_CMD_URB;
+	cmdinfo->stream = uas_get_tag(cmnd);
+	cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
-- 
2.1.0

[PATCH 05/21] uas: Do not use scsi_host_find_tag

[Hans de Goede]

Using scsi_host_find_tag with tags returned by the device is unsafe for
multiple reasons:

1) It returns tags->rqs[tag], which may be non NULL even when the cmnd is
   not owned by us
2) It returns tags->rqs[tag], without holding any locks protecting it
3) It returns tags->rqs[tag], without doing any boundary checking

Instead keep our own list which maps tags -> inflight cmnds.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 05c1c81..b2423d3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -29,6 +29,8 @@
 
 #include "uas-detect.h"
 
+#define MAX_CMNDS 256
+
 /*
  * The r00-r01c specs define this version of the SENSE IU data structure.
  * It's still in use by several different firmware releases.
@@ -54,7 +56,7 @@ struct uas_dev_info {
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
 	unsigned shutdown:1;
-	struct scsi_cmnd *cmnd;
+	struct scsi_cmnd *cmnd[MAX_CMNDS];
 	spinlock_t lock;
 	struct work_struct work;
 	struct list_head inflight_list;
@@ -314,6 +316,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	if (cmdinfo->state & COMMAND_ABORTED)
 		scmd_printk(KERN_INFO, cmnd, "abort completed\n");
 	list_del(&cmdinfo->list);
+	devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
 	cmnd->scsi_done(cmnd);
 	return 0;
 }
@@ -339,7 +342,7 @@ static void uas_stat_cmplt(struct urb *urb)
 	struct scsi_cmnd *cmnd;
 	struct uas_cmd_info *cmdinfo;
 	unsigned long flags;
-	u16 tag;
+	unsigned int idx;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
 
@@ -357,21 +360,17 @@ static void uas_stat_cmplt(struct urb *urb)
 		goto out;
 	}
 
-	tag = be16_to_cpup(&iu->tag) - 1;
-	if (tag == 0)
-		cmnd = devinfo->cmnd;
-	else
-		cmnd = scsi_host_find_tag(shost, tag - 1);
-
-	if (!cmnd)
+	idx = be16_to_cpup(&iu->tag) - 1;
+	if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
+		dev_err(&urb->dev->dev,
+			"stat urb: no pending cmd for tag %d\n", idx + 1);
 		goto out;
+	}
 
+	cmnd = devinfo->cmnd[idx];
 	cmdinfo = (void *)&cmnd->SCp;
 	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)
@@ -672,6 +671,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	struct uas_dev_info *devinfo = sdev->hostdata;
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	unsigned long flags;
+	unsigned int stream;
 	int err;
 
 	BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
@@ -685,19 +685,16 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		return 0;
 	}
 
-	if (devinfo->cmnd) {
+	stream = uas_get_tag(cmnd);
+	if (devinfo->cmnd[stream - 1]) {
 		spin_unlock_irqrestore(&devinfo->lock, flags);
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 	}
 
-	memset(cmdinfo, 0, sizeof(*cmdinfo));
-
-	if (!blk_rq_tagged(cmnd->request))
-		devinfo->cmnd = cmnd;
-
 	cmnd->scsi_done = done;
 
-	cmdinfo->stream = uas_get_tag(cmnd);
+	memset(cmdinfo, 0, sizeof(*cmdinfo));
+	cmdinfo->stream = stream;
 	cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
 	switch (cmnd->sc_data_direction) {
@@ -727,6 +724,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		uas_add_work(cmdinfo);
 	}
 
+	devinfo->cmnd[stream - 1] = cmnd;
 	list_add_tail(&cmdinfo->list, &devinfo->inflight_list);
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 	return 0;
@@ -862,7 +860,6 @@ static int uas_configure_endpoints(struct uas_dev_info *devinfo)
 	int r;
 
 	devinfo->uas_sense_old = 0;
-	devinfo->cmnd = NULL;
 
 	r = uas_find_endpoints(devinfo->intf->cur_altsetting, eps);
 	if (r)
@@ -882,7 +879,7 @@ static int uas_configure_endpoints(struct uas_dev_info *devinfo)
 		devinfo->use_streams = 0;
 	} else {
 		devinfo->qdepth = usb_alloc_streams(devinfo->intf, eps + 1,
-						    3, 256, GFP_NOIO);
+						    3, MAX_CMNDS, GFP_NOIO);
 		if (devinfo->qdepth < 0)
 			return devinfo->qdepth;
 		devinfo->use_streams = 1;
-- 
2.1.0

[PATCH 06/21] uas: Check against unexpected completions

[Hans de Goede]

The status urb should not complete before the command has been submitted, nor
should we get a second status urb for the same tag after a IU_ID_STATUS.

Data urbs should not complete before the command has been submitted, but may
complete after the IU_ID_STATUS.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b2423d3..1ef59b0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -369,6 +369,12 @@ static void uas_stat_cmplt(struct urb *urb)
 
 	cmnd = devinfo->cmnd[idx];
 	cmdinfo = (void *)&cmnd->SCp;
+
+	if (!(cmdinfo->state & COMMAND_INFLIGHT)) {
+		scmd_printk(KERN_ERR, cmnd, "unexpected status cmplt\n");
+		goto out;
+	}
+
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
 		if (urb->actual_length < 16)
@@ -434,6 +440,12 @@ static void uas_data_cmplt(struct urb *urb)
 	if (devinfo->resetting)
 		goto out;
 
+	/* Data urbs should not complete before the cmd urb is submitted */
+	if (cmdinfo->state & SUBMIT_CMD_URB) {
+		scmd_printk(KERN_ERR, cmnd, "unexpected data cmplt\n");
+		goto out;
+	}
+
 	if (urb->status) {
 		if (urb->status != -ECONNRESET) {
 			uas_log_cmd_state(cmnd, __func__);
-- 
2.1.0

[PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

From: Sanjeev Sharma <Sanjeev_Sharma-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>

On some architecture spin_is_locked() always return false in
uniprocessor configuration and therefore it would be advise to replace
with lockdep_assert_held().

Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..05b2d8e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
 	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
 
 	uas_log_cmd_state(cmnd, caller);
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	lockdep_assert_held(&devinfo->lock);
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
 	cmdinfo->state |= COMMAND_ABORTED;
 	cmdinfo->state &= ~IS_IN_WORK_LIST;
@@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
 	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
 	struct uas_dev_info *devinfo = cmnd->device->hostdata;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	lockdep_assert_held(&devinfo->lock);
 	cmdinfo->state |= IS_IN_WORK_LIST;
 	schedule_work(&devinfo->work);
 }
@@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	lockdep_assert_held(&devinfo->lock);
 	if (cmdinfo->state & (COMMAND_INFLIGHT |
 			      DATA_IN_URB_INFLIGHT |
 			      DATA_OUT_URB_INFLIGHT |
@@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	struct urb *urb;
 	int err;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	lockdep_assert_held(&devinfo->lock);
 	if (cmdinfo->state & SUBMIT_STATUS_URB) {
 		urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
 		if (!urb)
-- 
2.1.0

--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

Hi,

Note this series is NOT intended for stable, but I accidentally
had "cc = stable@vger.kernel.org" in my .git/config when sending
this series, please ignore for stable.

NACK for stable.

Sorry & Regards,

Hans


On 09/10/2014 01:46 PM, Hans de Goede wrote:
> From: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> 
> On some architecture spin_is_locked() always return false in
> uniprocessor configuration and therefore it would be advise to replace
> with lockdep_assert_held().
> 
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/storage/uas.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..05b2d8e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  
>  	uas_log_cmd_state(cmnd, caller);
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
>  	cmdinfo->state |= COMMAND_ABORTED;
>  	cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  	struct uas_dev_info *devinfo = cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	cmdinfo->state |= IS_IN_WORK_LIST;
>  	schedule_work(&devinfo->work);
>  }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
>  	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>  	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	if (cmdinfo->state & (COMMAND_INFLIGHT |
>  			      DATA_IN_URB_INFLIGHT |
>  			      DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>  	struct urb *urb;
>  	int err;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	if (cmdinfo->state & SUBMIT_STATUS_URB) {
>  		urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
>  		if (!urb)
> 

Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Oliver Neukum]

On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> Hi,
> 
> Note this series is NOT intended for stable, but I accidentally
> had "cc = stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" in my .git/config when sending
> this series, please ignore for stable.
> 
> NACK for stable.

If this is not for stable, what do you intend to do about
the problems in stable? For example patch#01 of this series
looks like clear stable material to me.

	Regards
		Oliver


--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Sharma, Sanjeev]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1062 bytes --]

-----Original Message-----
From: Oliver Neukum [mailto:oneukum@suse.de] 
Sent: Wednesday, September 10, 2014 5:26 PM
To: Hans de Goede
Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-scsi@vger.kernel.org; stable@vger.kernel.org; Sharma, Sanjeev
Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> Hi,
> 
> Note this series is NOT intended for stable, but I accidentally had 
> "cc = stable@vger.kernel.org" in my .git/config when sending this 
> series, please ignore for stable.
> 
> NACK for stable.

If this is not for stable, what do you intend to do about the problems in stable? For example patch#01 of this series looks like clear stable material to me.

	Regards
		Oliver

This is not clear to me also. Please Clarify "Note this series is NOT intended for stable"  means ?

Regards
Sanjeev Sharma

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

Hi,

On 09/10/2014 01:56 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
>> Hi,
>>
>> Note this series is NOT intended for stable, but I accidentally
>> had "cc = stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" in my .git/config when sending
>> this series, please ignore for stable.
>>
>> NACK for stable.
> 
> If this is not for stable, what do you intend to do about
> the problems in stable? For example patch#01 of this series
> looks like clear stable material to me.

The plan for stable is mostly, as lame as that is, to make sure
we get all the right quirks in place so that error handling
does not get triggered, for now.

I agree that once this set has seen wider testing, we should
reconsider, and probably add it, to stable. But at this point
in time I'm worried that it may cause regressions, and as such
it is not stable material atm IHMO.

Regards,

Hans
--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Oliver Neukum]

On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/10/2014 01:56 PM, Oliver Neukum wrote:
> > On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> Note this series is NOT intended for stable, but I accidentally
> >> had "cc = stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" in my .git/config when sending
> >> this series, please ignore for stable.
> >>
> >> NACK for stable.
> > 
> > If this is not for stable, what do you intend to do about
> > the problems in stable? For example patch#01 of this series
> > looks like clear stable material to me.
> 
> The plan for stable is mostly, as lame as that is, to make sure
> we get all the right quirks in place so that error handling
> does not get triggered, for now.

How? A medium can be defect. Short of entirely disabling it,
error handling will be triggered.

> I agree that once this set has seen wider testing, we should
> reconsider, and probably add it, to stable. But at this point
> in time I'm worried that it may cause regressions, and as such
> it is not stable material atm IHMO.

Well, we would exchange something known to work imperfectly
for something feared to work imperfectly.

	Regards
		Oliver


--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

Hi,

On 09/10/2014 02:54 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 09/10/2014 01:56 PM, Oliver Neukum wrote:
>>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> Note this series is NOT intended for stable, but I accidentally
>>>> had "cc = stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" in my .git/config when sending
>>>> this series, please ignore for stable.
>>>>
>>>> NACK for stable.
>>>
>>> If this is not for stable, what do you intend to do about
>>> the problems in stable? For example patch#01 of this series
>>> looks like clear stable material to me.
>>
>> The plan for stable is mostly, as lame as that is, to make sure
>> we get all the right quirks in place so that error handling
>> does not get triggered, for now.
> 
> How? A medium can be defect. Short of entirely disabling it,
> error handling will be triggered.

I agree that this is a concern, but defective disks are not the
norm. All the bugs I've received sofar seem to be about incompatibilities
between the Linux uas/scsi stack and the device, not defective mediums.

>> I agree that once this set has seen wider testing, we should
>> reconsider, and probably add it, to stable. But at this point
>> in time I'm worried that it may cause regressions, and as such
>> it is not stable material atm IHMO.
> 
> Well, we would exchange something known to work imperfectly
> for something feared to work imperfectly.

True. Note as said I'm not against this going into stable, I just don't
want to rush it into stable. So first lets get it reviewed and into
3.18 (and see how it works for the users who have been having troubles
sofar, see my request for testing), and then see from there.

I assume that you agree that this is (way) too late for 3.17?

Regards,

Hans
--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 03:15:41PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/10/2014 02:54 PM, Oliver Neukum wrote:
> > On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 09/10/2014 01:56 PM, Oliver Neukum wrote:
> >>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> Note this series is NOT intended for stable, but I accidentally
> >>>> had "cc = stable@vger.kernel.org" in my .git/config when sending
> >>>> this series, please ignore for stable.
> >>>>
> >>>> NACK for stable.
> >>>
> >>> If this is not for stable, what do you intend to do about
> >>> the problems in stable? For example patch#01 of this series
> >>> looks like clear stable material to me.
> >>
> >> The plan for stable is mostly, as lame as that is, to make sure
> >> we get all the right quirks in place so that error handling
> >> does not get triggered, for now.
> > 
> > How? A medium can be defect. Short of entirely disabling it,
> > error handling will be triggered.
> 
> I agree that this is a concern, but defective disks are not the
> norm. All the bugs I've received sofar seem to be about incompatibilities
> between the Linux uas/scsi stack and the device, not defective mediums.
> 
> >> I agree that once this set has seen wider testing, we should
> >> reconsider, and probably add it, to stable. But at this point
> >> in time I'm worried that it may cause regressions, and as such
> >> it is not stable material atm IHMO.
> > 
> > Well, we would exchange something known to work imperfectly
> > for something feared to work imperfectly.
> 
> True. Note as said I'm not against this going into stable, I just don't
> want to rush it into stable. So first lets get it reviewed and into
> 3.18 (and see how it works for the users who have been having troubles
> sofar, see my request for testing), and then see from there.
> 
> I assume that you agree that this is (way) too late for 3.17?

Yes it is.

And I agree, let's test this out first, and if it solves problems,
_then_ we can backport it to stable as needed.

thanks for the patches, I'll queue them up for 3.18.

greg k-h

RE: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Sharma, Sanjeev]

-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
Sent: Wednesday, September 10, 2014 7:21 PM
To: Hans de Goede
Cc: Oliver Neukum; linux-usb@vger.kernel.org; linux-scsi@vger.kernel.org; stable@vger.kernel.org; Sharma, Sanjeev
Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

On Wed, Sep 10, 2014 at 03:15:41PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/10/2014 02:54 PM, Oliver Neukum wrote:
> > On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 09/10/2014 01:56 PM, Oliver Neukum wrote:
> >>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> Note this series is NOT intended for stable, but I accidentally 
> >>>> had "cc = stable@vger.kernel.org" in my .git/config when sending 
> >>>> this series, please ignore for stable.
> >>>>
> >>>> NACK for stable.
> >>>
> >>> If this is not for stable, what do you intend to do about the 
> >>> problems in stable? For example patch#01 of this series looks like 
> >>> clear stable material to me.
> >>
> >> The plan for stable is mostly, as lame as that is, to make sure we 
> >> get all the right quirks in place so that error handling does not 
> >> get triggered, for now.
> > 
> > How? A medium can be defect. Short of entirely disabling it, error 
> > handling will be triggered.
> 
> I agree that this is a concern, but defective disks are not the norm. 
> All the bugs I've received sofar seem to be about incompatibilities 
> between the Linux uas/scsi stack and the device, not defective mediums.
> 
> >> I agree that once this set has seen wider testing, we should 
> >> reconsider, and probably add it, to stable. But at this point in 
> >> time I'm worried that it may cause regressions, and as such it is 
> >> not stable material atm IHMO.
> > 
> > Well, we would exchange something known to work imperfectly for 
> > something feared to work imperfectly.
> 
> True. Note as said I'm not against this going into stable, I just 
> don't want to rush it into stable. So first lets get it reviewed and 
> into
> 3.18 (and see how it works for the users who have been having troubles 
> sofar, see my request for testing), and then see from there.
> 
> I assume that you agree that this is (way) too late for 3.17?

Yes it is.

And I agree, let's test this out first, and if it solves problems, _then_ we can backport it to stable as needed.

thanks for the patches, I'll queue them up for 3.18.

Thanks all 

There are some more areas in Kernel which need replacement and I will look into those area too.

Regards
Sanjeev Sharma

Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Peter Hurley]

[ +cc Peter Zijlstra, Ingo Molnar ]

On 09/10/2014 07:46 AM, Hans de Goede wrote:
> From: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> 
> On some architecture spin_is_locked() always return false in
> uniprocessor configuration and therefore it would be advise to replace
> with lockdep_assert_held().
> 
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/storage/uas.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..05b2d8e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  
>  	uas_log_cmd_state(cmnd, caller);
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);

This change isn't equivalent.

lockdep_assert_held() will continue to emit warnings; ie., there is no
"once" functionality. Same for the other changes below.

Regards,
Peter Hurley

>  	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
>  	cmdinfo->state |= COMMAND_ABORTED;
>  	cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  	struct uas_dev_info *devinfo = cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	cmdinfo->state |= IS_IN_WORK_LIST;
>  	schedule_work(&devinfo->work);
>  }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
>  	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>  	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	if (cmdinfo->state & (COMMAND_INFLIGHT |
>  			      DATA_IN_URB_INFLIGHT |
>  			      DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>  	struct urb *urb;
>  	int err;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	lockdep_assert_held(&devinfo->lock);
>  	if (cmdinfo->state & SUBMIT_STATUS_URB) {
>  		urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
>  		if (!urb)
> 

Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

Hi,

On 09/10/2014 04:38 PM, Peter Hurley wrote:
> [ +cc Peter Zijlstra, Ingo Molnar ]
> 
> On 09/10/2014 07:46 AM, Hans de Goede wrote:
>> From: Sanjeev Sharma <Sanjeev_Sharma-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>>
>> On some architecture spin_is_locked() always return false in
>> uniprocessor configuration and therefore it would be advise to replace
>> with lockdep_assert_held().
>>
>> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/usb/storage/uas.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 3f42785..05b2d8e 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>>  
>>  	uas_log_cmd_state(cmnd, caller);
>> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
>> +	lockdep_assert_held(&devinfo->lock);
> 
> This change isn't equivalent.
> 
> lockdep_assert_held() will continue to emit warnings; ie., there is no
> "once" functionality. Same for the other changes below.

Given that these should really never ever happen, that is not really a problem
IMHO. The idea ws to replace the wrong use of spin_is_locked with some
other sanity check, preferably a light weight one, the once behavior is not
that important.

Regards,

Hans
--
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 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Sharma, Sanjeev]

-----Original Message-----
From: Hans de Goede [mailto:hdegoede@redhat.com] 
Sent: Wednesday, September 10, 2014 8:33 PM
To: Peter Hurley; Greg Kroah-Hartman
Cc: linux-usb@vger.kernel.org; linux-scsi@vger.kernel.org; stable@vger.kernel.org; Sharma, Sanjeev; Peter Zijlstra; Ingo Molnar
Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

Hi,

On 09/10/2014 04:38 PM, Peter Hurley wrote:
> [ +cc Peter Zijlstra, Ingo Molnar ]
> 
> On 09/10/2014 07:46 AM, Hans de Goede wrote:
>> From: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
>>
>> On some architecture spin_is_locked() always return false in 
>> uniprocessor configuration and therefore it would be advise to 
>> replace with lockdep_assert_held().
>>
>> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/storage/uas.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c 
>> index 3f42785..05b2d8e 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>>  
>>  	uas_log_cmd_state(cmnd, caller);
>> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
>> +	lockdep_assert_held(&devinfo->lock);
> 
> This change isn't equivalent.
> 
> lockdep_assert_held() will continue to emit warnings; ie., there is no 
> "once" functionality. Same for the other changes below.

>Given that these should really never ever happen, that is not really a problem IMHO. The idea ws to replace the wrong use of spin_is_locked with some other sanity check, preferably a light weight one, the once behavior is not that important.

I am totally agree with the hans and that was my intention for UP system.

Regards
Sanjeev Sharma

[PATCH 02/21] uas: Remove task-management / abort error handling code

[Hans de Goede]

There are various bug reports about oopses / hangs with the uas driver,
which all point to the abort-command and logical-unit-reset (task-management)
error handling paths.

Getting these right is very hard, there are quite a few corner cases, and
testing is almost impossible since under normal operation these code paths
are not used at all.

Another problem is that there are also some cases where it simply is not clear
what to do at all. E.g. over usb-2 multiple outstanding commands share the same
endpoint. What if a command gets aborted while its sense urb is half way
through completing (so some data has been transfered but not all). Since the
urb is not yet complete we don't know if the sense urb is actually for this
command, or for one of the other oustanding commands. If it is for one of the
other commands and we cancel it, then we end up in an undefined state. But if
it is actually for the command we're aborting, and the abort succeeds, then it
may never complete...

This exact same problem applies to logical unit resets too, if there are
multiple luns, then commands outstanding on both luns share the sense
endpoint. If there is only a single lun, then doing a logical unit reset is
little better then doing a full usb device reset.

So summarizing because:
1) abort / lun-reset is very tricky to get right
2) Not being able to test the tricky code, which means it will have bugs
3) This being a code path which under normal operation will never happen,
   so being slow / sub-optimal here is not really an issue
4) Under error conditions we will still be able to recover through usb
   device resets.
5) This may be a bit slower in some cases, but this is actually faster in
   cases where the bridge ship has locked up, which seems to be the most
   common error case sofar.

This commit removes the abort / lun-reset error handling paths, and also the
taks-mgmt code since those are the only 2 task-mgmt users. Leaving only the
(tested and testable) usb-device-reset error handling path in place.

Note I realize that this is somewhat of a big hammer, but currently people
are seeing very hard to debug oopses with uas. First let focus on making uas
work reliable, then we can later look into adding more fine grained error
handling.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 177 +---------------------------------------------
 1 file changed, 1 insertion(+), 176 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 05b2d8e..307658d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -2,7 +2,7 @@
  * USB Attached SCSI
  * Note that this is not the same as the USB Mass Storage driver
  *
- * Copyright Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> for Red Hat, Inc. 2013
+ * Copyright Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> for Red Hat, Inc. 2013 - 2014
  * Copyright Matthew Wilcox for Intel Corp, 2010
  * Copyright Sarah Sharp for Intel Corp, 2010
  *
@@ -50,11 +50,9 @@ struct uas_dev_info {
 	struct usb_anchor sense_urbs;
 	struct usb_anchor data_urbs;
 	int qdepth, resetting;
-	struct response_iu response;
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
 	unsigned use_streams:1;
 	unsigned uas_sense_old:1;
-	unsigned running_task:1;
 	unsigned shutdown:1;
 	struct scsi_cmnd *cmnd;
 	spinlock_t lock;
@@ -205,7 +203,6 @@ static void uas_zap_dead(struct uas_dev_info *devinfo)
 				    DATA_OUT_URB_INFLIGHT);
 		uas_try_complete(cmnd, __func__);
 	}
-	devinfo->running_task = 0;
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
@@ -348,13 +345,6 @@ static void uas_stat_cmplt(struct urb *urb)
 		cmnd = scsi_host_find_tag(shost, tag - 1);
 
 	if (!cmnd) {
-		if (iu->iu_id == IU_ID_RESPONSE) {
-			if (!devinfo->running_task)
-				dev_warn(&urb->dev->dev,
-				    "stat urb: recv unexpected response iu\n");
-			/* store results for uas_eh_task_mgmt() */
-			memcpy(&devinfo->response, iu, sizeof(devinfo->response));
-		}
 		usb_free_urb(urb);
 		spin_unlock_irqrestore(&devinfo->lock, flags);
 		return;
@@ -534,56 +524,6 @@ 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),
-			  uas_cmd_cmplt, cmnd);
-	urb->transfer_flags |= URB_FREE_BUFFER;
-
-	usb_anchor_urb(urb, &devinfo->cmd_urbs);
-	err = usb_submit_urb(urb, gfp);
-	if (err) {
-		usb_unanchor_urb(urb);
-		uas_log_cmd_state(cmnd, __func__);
-		scmd_printk(KERN_ERR, cmnd, "task submission err %d\n", 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
@@ -776,118 +716,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 static DEF_SCSI_QCMD(uas_queuecommand)
 
-static int uas_eh_task_mgmt(struct scsi_cmnd *cmnd,
-			    const char *fname, u8 function)
-{
-	struct Scsi_Host *shost = cmnd->device->host;
-	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
-	u16 tag = devinfo->qdepth;
-	unsigned long flags;
-	struct urb *sense_urb;
-	int result = SUCCESS;
-
-	spin_lock_irqsave(&devinfo->lock, flags);
-
-	if (devinfo->resetting) {
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		return FAILED;
-	}
-
-	if (devinfo->running_task) {
-		shost_printk(KERN_INFO, shost,
-			     "%s: %s: error already running a task\n",
-			     __func__, fname);
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		return FAILED;
-	}
-
-	devinfo->running_task = 1;
-	memset(&devinfo->response, 0, sizeof(devinfo->response));
-	sense_urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC,
-					 devinfo->use_streams ? tag : 0);
-	if (!sense_urb) {
-		shost_printk(KERN_INFO, shost,
-			     "%s: %s: submit sense urb failed\n",
-			     __func__, fname);
-		devinfo->running_task = 0;
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		return FAILED;
-	}
-	if (uas_submit_task_urb(cmnd, GFP_ATOMIC, function, tag)) {
-		shost_printk(KERN_INFO, shost,
-			     "%s: %s: submit task mgmt urb failed\n",
-			     __func__, fname);
-		devinfo->running_task = 0;
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		usb_kill_urb(sense_urb);
-		return FAILED;
-	}
-	spin_unlock_irqrestore(&devinfo->lock, flags);
-
-	if (usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 3000) == 0) {
-		/*
-		 * Note we deliberately do not clear running_task here. If we
-		 * allow new tasks to be submitted, there is no way to figure
-		 * out if a received response_iu is for the failed task or for
-		 * the new one. A bus-reset will eventually clear running_task.
-		 */
-		shost_printk(KERN_INFO, shost,
-			     "%s: %s timed out\n", __func__, fname);
-		return FAILED;
-	}
-
-	spin_lock_irqsave(&devinfo->lock, flags);
-	devinfo->running_task = 0;
-	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);
-		result = FAILED;
-	} else 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);
-		result = FAILED;
-	}
-	spin_unlock_irqrestore(&devinfo->lock, flags);
-
-	return result;
-}
-
-static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
-{
-	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
-	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&devinfo->lock, flags);
-
-	if (devinfo->resetting) {
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		return FAILED;
-	}
-
-	uas_mark_cmd_dead(devinfo, cmdinfo, DID_ABORT, __func__);
-	if (cmdinfo->state & COMMAND_INFLIGHT) {
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
-	} else {
-		uas_unlink_data_urbs(devinfo, cmdinfo, &flags);
-		uas_try_complete(cmnd, __func__);
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		ret = SUCCESS;
-	}
-	return ret;
-}
-
-static int uas_eh_device_reset_handler(struct scsi_cmnd *cmnd)
-{
-	sdev_printk(KERN_INFO, cmnd->device, "%s\n", __func__);
-	return uas_eh_task_mgmt(cmnd, "LOGICAL UNIT RESET",
-				TMF_LOGICAL_UNIT_RESET);
-}

Re: [PATCH 02/21] uas: Remove task-management / abort error handling code

[Oliver Neukum]

On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:

> This commit removes the abort / lun-reset error handling paths, and also the
> taks-mgmt code since those are the only 2 task-mgmt users. Leaving only the
> (tested and testable) usb-device-reset error handling path in place.
> 
> Note I realize that this is somewhat of a big hammer, but currently people
> are seeing very hard to debug oopses with uas. First let focus on making uas
> work reliable, then we can later look into adding more fine grained error
> handling.

This hurts. But I suppose it is the conservative choice.

	Regards
		Oliver

[PATCH 03/21] uas: Fix resetting flag handling

[Hans de Goede]

- Make sure we always hold the lock when setting / checking resetting
- Check resetting before checking urb->status
- Add missing check for resetting to uas_data_cmplt
- Add missing check for resetting to uas_do_work

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 49 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 307658d..4f745a3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -127,6 +127,10 @@ static void uas_do_work(struct work_struct *work)
 	int err;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
+
+	if (devinfo->resetting)
+		goto out;
+
 	list_for_each_entry(cmdinfo, &devinfo->inflight_list, list) {
 		struct scsi_pointer *scp = (void *)cmdinfo;
 		struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
@@ -141,6 +145,7 @@ static void uas_do_work(struct work_struct *work)
 		else
 			schedule_work(&devinfo->work);
 	}
+out:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
@@ -320,6 +325,11 @@ static void uas_stat_cmplt(struct urb *urb)
 	unsigned long flags;
 	u16 tag;
 
+	spin_lock_irqsave(&devinfo->lock, flags);
+
+	if (devinfo->resetting)
+		goto out;
+
 	if (urb->status) {
 		if (urb->status == -ENOENT) {
 			dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
@@ -328,27 +338,17 @@ static void uas_stat_cmplt(struct urb *urb)
 			dev_err(&urb->dev->dev, "stat urb: status %d\n",
 				urb->status);
 		}
-		usb_free_urb(urb);
-		return;
-	}
-
-	if (devinfo->resetting) {
-		usb_free_urb(urb);
-		return;
+		goto out;
 	}
 
-	spin_lock_irqsave(&devinfo->lock, flags);
 	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);
-		spin_unlock_irqrestore(&devinfo->lock, flags);
-		return;
-	}
+	if (!cmnd)
+		goto out;
 
 	cmdinfo = (void *)&cmnd->SCp;
 	switch (iu->iu_id) {
@@ -389,6 +389,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);
 	}
+out:
 	usb_free_urb(urb);
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
@@ -402,6 +403,7 @@ static void uas_data_cmplt(struct urb *urb)
 	unsigned long flags;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
+
 	if (cmdinfo->data_in_urb == urb) {
 		sdb = scsi_in(cmnd);
 		cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
@@ -411,7 +413,13 @@ static void uas_data_cmplt(struct urb *urb)
 	}
 	if (sdb == NULL) {
 		WARN_ON_ONCE(1);
-	} else if (urb->status) {
+		goto out;
+	}
+
+	if (devinfo->resetting)
+		goto out;
+
+	if (urb->status) {
 		if (urb->status != -ECONNRESET) {
 			uas_log_cmd_state(cmnd, __func__);
 			scmd_printk(KERN_ERR, cmnd,
@@ -424,6 +432,7 @@ static void uas_data_cmplt(struct urb *urb)
 		sdb->resid = sdb->length - urb->actual_length;
 	}
 	uas_try_complete(cmnd, __func__);
+out:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
@@ -721,6 +730,7 @@ 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;
+	unsigned long flags;
 	int err;
 
 	err = usb_lock_device_for_reset(udev, devinfo->intf);
@@ -731,14 +741,21 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	}
 
 	shost_printk(KERN_INFO, sdev->host, "%s start\n", __func__);
+
+	spin_lock_irqsave(&devinfo->lock, flags);
 	devinfo->resetting = 1;
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
 	uas_abort_inflight(devinfo, DID_RESET, __func__);
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
 	uas_zap_dead(devinfo);
 	err = usb_reset_device(udev);
+
+	spin_lock_irqsave(&devinfo->lock, flags);
 	devinfo->resetting = 0;
+	spin_unlock_irqrestore(&devinfo->lock, flags);
 
 	usb_unlock_device(udev);
 
@@ -1032,8 +1049,12 @@ static void uas_disconnect(struct usb_interface *intf)
 {
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
 	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
+	unsigned long flags;
 
+	spin_lock_irqsave(&devinfo->lock, flags);
 	devinfo->resetting = 1;
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
 	cancel_work_sync(&devinfo->work);
 	uas_abort_inflight(devinfo, DID_NO_CONNECT, __func__);
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
-- 
2.1.0

--
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 03/21] uas: Fix resetting flag handling

[Oliver Neukum]

On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
> - Make sure we always hold the lock when setting / checking resetting
> - Check resetting before checking urb->status
> - Add missing check for resetting to uas_data_cmplt
> - Add missing check for resetting to uas_do_work

Why is the checking for stat and data inconsistent?

	Regards
		Oliver

Re: [PATCH 03/21] uas: Fix resetting flag handling

[Hans de Goede]

Hi,

First of all, thank you for reviewing this.

On 09/10/2014 03:40 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
>> - Make sure we always hold the lock when setting / checking resetting
>> - Check resetting before checking urb->status
>> - Add missing check for resetting to uas_data_cmplt
>> - Add missing check for resetting to uas_do_work
> 
> Why is the checking for stat and data inconsistent?

With stat urbs there is no direct reference from the urb
to the scsi cmnd, since when using usb-2 the sense urbs
may arrive in a different order then the cmnds where issued.
instead for sense urbs we look up the cmnd by tag, likewise
there is no reference from the cmnd back to the sense-urb.

For data urbs however the urb->context pointer points directly
to the cmnd struct, and the cmnd struct has a pointer back in
the form of devinfo->data_[in|out]_urb. When the urb completes
we want to NULL the devinfo->data_[in|out]_urb pointers even
when resetting. So that we don't try to deref a pointer to
a free-ed urb later on (this does not become an issue until
after the "uas: Free data urbs on completion" commit).

Regards,

Hans

[PATCH 07/21] uas: Simplify unlink of data urbs on error

[Hans de Goede]

There is no need for all the trickery with dropping the lock, we can
simply reference the urbs while we hold the lock to ensure the urbs don't
disappear beneath us, and do the actual unlink (+ unreference) after we've
dropped the lock.

This also fixes a race where we may loose of cmnd ownership to the scsi
midlayer without holding the lock due to the midlayer re-claiming ownership
through an abort (which will be handled by a future patch in this series).

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 48 ++++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1ef59b0..b91b5e9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -76,8 +76,7 @@ enum {
 	DATA_OUT_URB_INFLIGHT   = (1 << 10),
 	COMMAND_COMPLETED       = (1 << 11),
 	COMMAND_ABORTED         = (1 << 12),
-	UNLINK_DATA_URBS        = (1 << 13),
-	IS_IN_WORK_LIST         = (1 << 14),
+	IS_IN_WORK_LIST         = (1 << 13),
 };
 
 /* Overrides scsi_pointer */
@@ -98,28 +97,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 static void uas_free_streams(struct uas_dev_info *devinfo);
 static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
 
-/* Must be called with devinfo->lock held, will temporary unlock the lock */
-static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
-				 struct uas_cmd_info *cmdinfo,
-				 unsigned long *lock_flags)
-{
-	/*
-	 * The UNLINK_DATA_URBS flag makes sure uas_try_complete
-	 * (called by urb completion) doesn't release cmdinfo
-	 * underneath us.
-	 */
-	cmdinfo->state |= UNLINK_DATA_URBS;
-	spin_unlock_irqrestore(&devinfo->lock, *lock_flags);
-
-	if (cmdinfo->data_in_urb)
-		usb_unlink_urb(cmdinfo->data_in_urb);
-	if (cmdinfo->data_out_urb)
-		usb_unlink_urb(cmdinfo->data_out_urb);
-
-	spin_lock_irqsave(&devinfo->lock, *lock_flags);
-	cmdinfo->state &= ~UNLINK_DATA_URBS;
-}
-
 static void uas_do_work(struct work_struct *work)
 {
 	struct uas_dev_info *devinfo =
@@ -279,8 +256,8 @@ 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%s%s%s\n",
+	scmd_printk(KERN_INFO, cmnd,
+		    "%s %p tag %d, inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
 		    caller, cmnd, uas_get_tag(cmnd),
 		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
 		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
@@ -294,7 +271,6 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
 		    (ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT"   : "",
 		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "",
 		    (ci->state & COMMAND_ABORTED)       ? " abort" : "",
-		    (ci->state & UNLINK_DATA_URBS)      ? " unlink": "",
 		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
 }
 
@@ -306,8 +282,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	lockdep_assert_held(&devinfo->lock);
 	if (cmdinfo->state & (COMMAND_INFLIGHT |
 			      DATA_IN_URB_INFLIGHT |
-			      DATA_OUT_URB_INFLIGHT |
-			      UNLINK_DATA_URBS))
+			      DATA_OUT_URB_INFLIGHT))
 		return -EBUSY;
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
 	cmdinfo->state |= COMMAND_COMPLETED;
@@ -339,6 +314,8 @@ static void uas_stat_cmplt(struct urb *urb)
 	struct iu *iu = urb->transfer_buffer;
 	struct Scsi_Host *shost = urb->context;
 	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
+	struct urb *data_in_urb = NULL;
+	struct urb *data_out_urb = NULL;
 	struct scsi_cmnd *cmnd;
 	struct uas_cmd_info *cmdinfo;
 	unsigned long flags;
@@ -385,7 +362,8 @@ static void uas_stat_cmplt(struct urb *urb)
 			uas_sense(urb, cmnd);
 		if (cmnd->result != 0) {
 			/* cancel data transfers on error */
-			uas_unlink_data_urbs(devinfo, cmdinfo, &flags);
+			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
 		}
 		cmdinfo->state &= ~COMMAND_INFLIGHT;
 		uas_try_complete(cmnd, __func__);
@@ -413,6 +391,16 @@ static void uas_stat_cmplt(struct urb *urb)
 out:
 	usb_free_urb(urb);
 	spin_unlock_irqrestore(&devinfo->lock, flags);
+
+	/* Unlinking of data urbs must be done without holding the lock */
+	if (data_in_urb) {
+		usb_unlink_urb(data_in_urb);
+		usb_put_urb(data_in_urb);
+	}
+	if (data_out_urb) {
+		usb_unlink_urb(data_out_urb);
+		usb_put_urb(data_out_urb);
+	}
 }
 
 static void uas_data_cmplt(struct urb *urb)
-- 
2.1.0

--
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 08/21] uas: Free data urbs on completion

[Hans de Goede]

Now that we no longer drop our lock to unlink the data urbs, we can simply
free them on completion, making their handling consistent with the other urbs.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b91b5e9..4c7d337 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -286,8 +286,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 		return -EBUSY;
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
 	cmdinfo->state |= COMMAND_COMPLETED;
-	usb_free_urb(cmdinfo->data_in_urb);
-	usb_free_urb(cmdinfo->data_out_urb);
 	if (cmdinfo->state & COMMAND_ABORTED)
 		scmd_printk(KERN_INFO, cmnd, "abort completed\n");
 	list_del(&cmdinfo->list);
@@ -416,9 +414,11 @@ static void uas_data_cmplt(struct urb *urb)
 	if (cmdinfo->data_in_urb == urb) {
 		sdb = scsi_in(cmnd);
 		cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
+		cmdinfo->data_in_urb = NULL;
 	} else if (cmdinfo->data_out_urb == urb) {
 		sdb = scsi_out(cmnd);
 		cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
+		cmdinfo->data_out_urb = NULL;
 	}
 	if (sdb == NULL) {
 		WARN_ON_ONCE(1);
@@ -448,6 +448,7 @@ static void uas_data_cmplt(struct urb *urb)
 	}
 	uas_try_complete(cmnd, __func__);
 out:
+	usb_free_urb(urb);
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
-- 
2.1.0

--
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 10/21] uas: zap_pending: data urbs should have completed at this time

[Hans de Goede]

The data urbs are all killed before calling zap_pending, and their completion
handler should have cleared their inflight flag.

Do not 0 the data inflight flags, and add a check for try_complete succeeding,
as it should always succeed when called from zap_pending.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index bba5498..ec6547d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -150,12 +150,10 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 		struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
 						      SCp);
 		uas_log_cmd_state(cmnd, __func__);
-		/* all urbs are killed, clear inflight bits */
-		cmdinfo->state &= ~(COMMAND_INFLIGHT |
-				    DATA_IN_URB_INFLIGHT |
-				    DATA_OUT_URB_INFLIGHT);
+		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
+		cmdinfo->state &= ~COMMAND_INFLIGHT;
 		cmnd->result = result << 16;
-		uas_try_complete(cmnd, __func__);
+		WARN_ON(uas_try_complete(cmnd, __func__) != 0);
 	}
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
-- 
2.1.0

--
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 10/21] uas: zap_pending: data urbs should have completed at this time

[Oliver Neukum]

On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
>                 uas_log_cmd_state(cmnd, __func__);
> -               /* all urbs are killed, clear inflight bits */
> -               cmdinfo->state &= ~(COMMAND_INFLIGHT |
> -                                   DATA_IN_URB_INFLIGHT |
> -                                   DATA_OUT_URB_INFLIGHT);
> +               /* Sense urbs were killed, clear COMMAND_INFLIGHT
> manually */
> +               cmdinfo->state &= ~COMMAND_INFLIGHT;
>                 cmnd->result = result << 16;
> -               uas_try_complete(cmnd, __func__);
> +               WARN_ON(uas_try_complete(cmnd, __func__) != 0);

That looks like very bad style. WARN_ON() shouldn't
have side effects.

	Regards
		Oliver

Re: [PATCH 10/21] uas: zap_pending: data urbs should have completed at this time

[Hans de Goede]

Hi,

On 09/10/2014 04:10 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
>>                 uas_log_cmd_state(cmnd, __func__);
>> -               /* all urbs are killed, clear inflight bits */
>> -               cmdinfo->state &= ~(COMMAND_INFLIGHT |
>> -                                   DATA_IN_URB_INFLIGHT |
>> -                                   DATA_OUT_URB_INFLIGHT);
>> +               /* Sense urbs were killed, clear COMMAND_INFLIGHT
>> manually */
>> +               cmdinfo->state &= ~COMMAND_INFLIGHT;
>>                 cmnd->result = result << 16;
>> -               uas_try_complete(cmnd, __func__);
>> +               WARN_ON(uas_try_complete(cmnd, __func__) != 0);
> 
> That looks like very bad style. WARN_ON() shouldn't
> have side effects.

A valid point, I'll respin this patch to store the result and check it
separately.

Regards,

Hans

[PATCH 14/21] uas: Fix memleak of non-submitted urbs

[Hans de Goede]

Not all urbs we've allocated are necessarily also submitted, non-submitted
urbs will not be free-ed by their completion handler. So we need to free
them manually.

There are 2 scenarios where this can happen:

1) We have failed to submit some urbs at abort / disconnect
2) When running over usb-2 we may have never tried to submit the data urbs
   when completing the scsi cmnd, because we never got a READ/WRITE_READY iu

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 75ce40c..6ee61bd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -246,6 +246,25 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
 		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
 }
 
+static void uas_free_unsubmitted_urbs(struct scsi_cmnd *cmnd)
+{
+	struct uas_cmd_info *cmdinfo;
+
+	if (!cmnd)
+		return;
+
+	cmdinfo = (void *)&cmnd->SCp;
+
+	if (cmdinfo->state & SUBMIT_CMD_URB)
+		usb_free_urb(cmdinfo->cmd_urb);
+
+	/* data urbs may have never gotten their submit flag set */
+	if (!(cmdinfo->state & DATA_IN_URB_INFLIGHT))
+		usb_free_urb(cmdinfo->data_in_urb);
+	if (!(cmdinfo->state & DATA_OUT_URB_INFLIGHT))
+		usb_free_urb(cmdinfo->data_out_urb);
+}
+
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 {
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
@@ -260,6 +279,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
 	cmdinfo->state |= COMMAND_COMPLETED;
 	devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
+	uas_free_unsubmitted_urbs(cmnd);
 	cmnd->scsi_done(cmnd);
 	return 0;
 }
@@ -726,6 +746,8 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 	if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
 		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
 
+	uas_free_unsubmitted_urbs(cmnd);
+
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 
 	if (data_in_urb) {
-- 
2.1.0

--
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 20/21] uas: Remove support for old sense ui as used in pre-production hardware

[Hans de Goede]

I've access to a number of different uas devices now, and none of them use
old style sense urbs. The only case where these code-paths trigger is with
the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
status iu-s when it does not have any sense data, but uses new style
sense iu-s regardless, as can be seen for scsi cmnds where there is sense
data.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 47 +----------------------------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index e8d2bf1..23c0b97 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -31,20 +31,6 @@
 
 #define MAX_CMNDS 256
 
-/*
- * The r00-r01c specs define this version of the SENSE IU data structure.
- * It's still in use by several different firmware releases.
- */
-struct sense_iu_old {
-	__u8 iu_id;
-	__u8 rsvd1;
-	__be16 tag;
-	__be16 len;
-	__u8 status;
-	__u8 service_response;
-	__u8 sense[SCSI_SENSE_BUFFERSIZE];
-};
-
 struct uas_dev_info {
 	struct usb_interface *intf;
 	struct usb_device *udev;
@@ -54,7 +40,6 @@ struct uas_dev_info {
 	int qdepth, resetting;
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
 	unsigned use_streams:1;
-	unsigned uas_sense_old:1;
 	unsigned shutdown:1;
 	struct scsi_cmnd *cmnd[MAX_CMNDS];
 	spinlock_t lock;
@@ -184,29 +169,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 	cmnd->result = sense_iu->status;
 }
 
-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;
-
-	if (urb->actual_length > 8) {
-		unsigned len = be16_to_cpup(&sense_iu->len) - 2;
-		if (len + 8 != urb->actual_length) {
-			int newlen = min(len + 8, urb->actual_length) - 8;
-			if (newlen < 0)
-				newlen = 0;
-			sdev_printk(KERN_INFO, sdev, "%s: urb length %d "
-				"disagrees with IU sense data length %d, "
-				"using %d bytes of sense data\n", __func__,
-					urb->actual_length, len, newlen);
-			len = newlen;
-		}
-		memcpy(cmnd->sense_buffer, sense_iu->sense, len);
-	}
-
-	cmnd->result = sense_iu->status;
-}
-
 /*
  * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
  * which go from 1 - nr_streams. And we use 1 for untagged commands.
@@ -336,12 +298,7 @@ static void uas_stat_cmplt(struct urb *urb)
 
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
-		if (urb->actual_length < 16)
-			devinfo->uas_sense_old = 1;
-		if (devinfo->uas_sense_old)
-			uas_sense_old(urb, cmnd);
-		else
-			uas_sense(urb, cmnd);
+		uas_sense(urb, cmnd);
 		if (cmnd->result != 0) {
 			/* cancel data transfers on error */
 			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
@@ -888,8 +845,6 @@ static int uas_configure_endpoints(struct uas_dev_info *devinfo)
 	struct usb_device *udev = devinfo->udev;
 	int r;
 
-	devinfo->uas_sense_old = 0;

Re: [PATCH 20/21] uas: Remove support for old sense ui as used in pre-production hardware

[Oliver Neukum]

On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
> I've access to a number of different uas devices now, and none of them use
> old style sense urbs. The only case where these code-paths trigger is with
> the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
> status iu-s when it does not have any sense data, but uses new style
> sense iu-s regardless, as can be seen for scsi cmnds where there is sense
> data.

This looks like a bad idea over all. The version is in the spec.
And you might see true USB<->SCSI bridges passing through the
sense data.

	Regards
		Oliver

Re: [PATCH 20/21] uas: Remove support for old sense ui as used in pre-production hardware

[Hans de Goede]

Hi,

On 09/10/2014 04:06 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
>> I've access to a number of different uas devices now, and none of them use
>> old style sense urbs. The only case where these code-paths trigger is with
>> the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
>> status iu-s when it does not have any sense data, but uses new style
>> sense iu-s regardless, as can be seen for scsi cmnds where there is sense
>> data.
> 
> This looks like a bad idea over all.

Removing this actually makes the ASM1051 bridge, which is the only one
ever sending 8 byte sense iu-s work better, as when it actually has
sense data it sends > 16 byte sense iu-s with the sense data length
at the location defined in the new 16 byte sense iu struct, and with the
old code we then ignore the length field (as we use the wrong field)
and instead use urb->actual_length - 8 bytes of the data as sense data,
while in reality there only is urb->actual_length - 16.

So either we drop support for the old style sense iu, at which point
sense iu-s just works with the ASM1051, or we need to add some extra
magic.

> The version is in the spec.

Not sure what you're trying to say here, the old sense-iu was only
ever present in draft versions. The official uas-r04.pdf and the
uas2r00.pdf draft both only define the new style 16 byte sense iu.

> And you might see true USB<->SCSI bridges passing through the
> sense data.

Right, and we support that, this just drops support for the old style
8 byte sense-iu which was only present in the r00 draft of the spec.

Regards,

Hans

[PATCH 09/21] uas: Simplify reset / disconnect handling

[Hans de Goede]

Drop the whole dance with first moving cmnds to a dead-list. The resetting
flag ensures that no new cmds / urbs will be submitted, and that any urb
completions are short-circuited without trying to complete the scsi cmnd.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 43 ++++++-------------------------------------
 1 file changed, 6 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4c7d337..bba5498 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -60,7 +60,6 @@ struct uas_dev_info {
 	spinlock_t lock;
 	struct work_struct work;
 	struct list_head inflight_list;
-	struct list_head dead_list;
 };
 
 enum {
@@ -128,35 +127,6 @@ out:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
-static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
-			      struct uas_cmd_info *cmdinfo,
-			      int result, const char *caller)
-{
-	struct scsi_pointer *scp = (void *)cmdinfo;
-	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
-
-	uas_log_cmd_state(cmnd, caller);
-	lockdep_assert_held(&devinfo->lock);
-	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
-	cmdinfo->state |= COMMAND_ABORTED;
-	cmdinfo->state &= ~IS_IN_WORK_LIST;
-	cmnd->result = result << 16;
-	list_move_tail(&cmdinfo->list, &devinfo->dead_list);
-}
-
-static void uas_abort_inflight(struct uas_dev_info *devinfo, int result,
-			       const char *caller)
-{
-	struct uas_cmd_info *cmdinfo;
-	struct uas_cmd_info *temp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&devinfo->lock, flags);
-	list_for_each_entry_safe(cmdinfo, temp, &devinfo->inflight_list, list)
-		uas_mark_cmd_dead(devinfo, cmdinfo, result, caller);
-	spin_unlock_irqrestore(&devinfo->lock, flags);
-}
-
 static void uas_add_work(struct uas_cmd_info *cmdinfo)
 {
 	struct scsi_pointer *scp = (void *)cmdinfo;
@@ -168,7 +138,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
 	schedule_work(&devinfo->work);
 }
 
-static void uas_zap_dead(struct uas_dev_info *devinfo)
+static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 {
 	struct uas_cmd_info *cmdinfo;
 	struct uas_cmd_info *temp;
@@ -180,11 +150,11 @@ static void uas_zap_dead(struct uas_dev_info *devinfo)
 		struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
 						      SCp);
 		uas_log_cmd_state(cmnd, __func__);
-		WARN_ON_ONCE(!(cmdinfo->state & COMMAND_ABORTED));
 		/* all urbs are killed, clear inflight bits */
 		cmdinfo->state &= ~(COMMAND_INFLIGHT |
 				    DATA_IN_URB_INFLIGHT |
 				    DATA_OUT_URB_INFLIGHT);
+		cmnd->result = result << 16;
 		uas_try_complete(cmnd, __func__);
 	}
 	spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -754,11 +724,11 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	devinfo->resetting = 1;
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 
-	uas_abort_inflight(devinfo, DID_RESET, __func__);
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
-	uas_zap_dead(devinfo);
+	uas_zap_pending(devinfo, DID_RESET);
+
 	err = usb_reset_device(udev);
 
 	spin_lock_irqsave(&devinfo->lock, flags);
@@ -935,7 +905,6 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	spin_lock_init(&devinfo->lock);
 	INIT_WORK(&devinfo->work, uas_do_work);
 	INIT_LIST_HEAD(&devinfo->inflight_list);
-	INIT_LIST_HEAD(&devinfo->dead_list);
 
 	result = uas_configure_endpoints(devinfo);
 	if (result)
@@ -1063,11 +1032,11 @@ static void uas_disconnect(struct usb_interface *intf)
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 
 	cancel_work_sync(&devinfo->work);
-	uas_abort_inflight(devinfo, DID_NO_CONNECT, __func__);
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
-	uas_zap_dead(devinfo);
+	uas_zap_pending(devinfo, DID_NO_CONNECT);
+
 	scsi_remove_host(shost);
 	uas_free_streams(devinfo);
 	scsi_host_put(shost);
-- 
2.1.0

[PATCH 11/21] uas: Drop inflight list

[Hans de Goede]

We've the same info doubled in both the inflight list and the cmnd array,
drop the list.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ec6547d..dca09de 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -59,7 +59,6 @@ struct uas_dev_info {
 	struct scsi_cmnd *cmnd[MAX_CMNDS];
 	spinlock_t lock;
 	struct work_struct work;
-	struct list_head inflight_list;
 };
 
 enum {
@@ -85,7 +84,6 @@ struct uas_cmd_info {
 	struct urb *cmd_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
-	struct list_head list;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -101,18 +99,21 @@ static void uas_do_work(struct work_struct *work)
 	struct uas_dev_info *devinfo =
 		container_of(work, struct uas_dev_info, work);
 	struct uas_cmd_info *cmdinfo;
+	struct scsi_cmnd *cmnd;
 	unsigned long flags;
-	int err;
+	int i, err;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
 
 	if (devinfo->resetting)
 		goto out;
 
-	list_for_each_entry(cmdinfo, &devinfo->inflight_list, list) {
-		struct scsi_pointer *scp = (void *)cmdinfo;
-		struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
-						      SCp);
+	for (i = 0; i < devinfo->qdepth; i++) {
+		if (!devinfo->cmnd[i])
+			continue;
+
+		cmnd = devinfo->cmnd[i];
+		cmdinfo = (void *)&cmnd->SCp;
 
 		if (!(cmdinfo->state & IS_IN_WORK_LIST))
 			continue;
@@ -141,14 +142,17 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
 static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 {
 	struct uas_cmd_info *cmdinfo;
-	struct uas_cmd_info *temp;
+	struct scsi_cmnd *cmnd;
 	unsigned long flags;
+	int i;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
-	list_for_each_entry_safe(cmdinfo, temp, &devinfo->dead_list, list) {
-		struct scsi_pointer *scp = (void *)cmdinfo;
-		struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
-						      SCp);
+	for (i = 0; i < devinfo->qdepth; i++) {
+		if (!devinfo->cmnd[i])
+			continue;
+
+		cmnd = devinfo->cmnd[i];
+		cmdinfo = (void *)&cmnd->SCp;
 		uas_log_cmd_state(cmnd, __func__);
 		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
 		cmdinfo->state &= ~COMMAND_INFLIGHT;
@@ -256,7 +260,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	cmdinfo->state |= COMMAND_COMPLETED;
 	if (cmdinfo->state & COMMAND_ABORTED)
 		scmd_printk(KERN_INFO, cmnd, "abort completed\n");
-	list_del(&cmdinfo->list);
 	devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
 	cmnd->scsi_done(cmnd);
 	return 0;
@@ -694,7 +697,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 	}
 
 	devinfo->cmnd[stream - 1] = cmnd;
-	list_add_tail(&cmdinfo->list, &devinfo->inflight_list);
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 	return 0;
 }
@@ -902,7 +904,6 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	init_usb_anchor(&devinfo->data_urbs);
 	spin_lock_init(&devinfo->lock);
 	INIT_WORK(&devinfo->work, uas_do_work);
-	INIT_LIST_HEAD(&devinfo->inflight_list);
 
 	result = uas_configure_endpoints(devinfo);
 	if (result)
-- 
2.1.0

[PATCH 12/21] uas: Remove cmnd reference from the cmd urb

[Hans de Goede]

It is not strictly necessary for the cmd urb to have a reference to the
cmnd, and without this reference it becomes easier to drop all references to
a cmnd on an abort.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index dca09de..1fdcfad 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -425,12 +425,9 @@ out:
 
 static void uas_cmd_cmplt(struct urb *urb)
 {
-	struct scsi_cmnd *cmnd = urb->context;
+	if (urb->status)
+		dev_err(&urb->dev->dev, "cmd cmplt err %d\n", urb->status);
 
-	if (urb->status) {
-		uas_log_cmd_state(cmnd, __func__);
-		scmd_printk(KERN_ERR, cmnd, "cmd cmplt err %d\n", urb->status);
-	}
 	usb_free_urb(urb);
 }
 
@@ -508,7 +505,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 	memcpy(iu->cdb, cmnd->cmnd, cmnd->cmd_len);
 
 	usb_fill_bulk_urb(urb, udev, devinfo->cmd_pipe, iu, sizeof(*iu) + len,
-							uas_cmd_cmplt, cmnd);
+							uas_cmd_cmplt, NULL);
 	urb->transfer_flags |= URB_FREE_BUFFER;
  out:
 	return urb;
-- 
2.1.0

[PATCH 13/21] uas: Drop all references to a scsi_cmnd once it has been aborted

[Hans de Goede]

Do not keep references around to a cmnd which is under error handling.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1fdcfad..75ce40c 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -254,12 +254,11 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	lockdep_assert_held(&devinfo->lock);
 	if (cmdinfo->state & (COMMAND_INFLIGHT |
 			      DATA_IN_URB_INFLIGHT |
-			      DATA_OUT_URB_INFLIGHT))
+			      DATA_OUT_URB_INFLIGHT |
+			      COMMAND_ABORTED))
 		return -EBUSY;
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
 	cmdinfo->state |= COMMAND_COMPLETED;
-	if (cmdinfo->state & COMMAND_ABORTED)
-		scmd_printk(KERN_INFO, cmnd, "abort completed\n");
 	devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
 	cmnd->scsi_done(cmnd);
 	return 0;
@@ -700,6 +699,47 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 static DEF_SCSI_QCMD(uas_queuecommand)
 
+/*
+ * For now we do not support actually sending an abort to the device, so
+ * this eh always fails. Still we must define it to make sure that we've
+ * dropped all references to the cmnd in question once this function exits.
+ */
+static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
+{
+	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+	struct urb *data_in_urb = NULL;
+	struct urb *data_out_urb = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&devinfo->lock, flags);
+
+	uas_log_cmd_state(cmnd, __func__);
+
+	/* Ensure that try_complete does not call scsi_done */
+	cmdinfo->state |= COMMAND_ABORTED;
+
+	/* Drop all refs to this cmnd, kill data urbs to break their ref */
+	devinfo->cmnd[uas_get_tag(cmnd) - 1] = 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)
+		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
+	if (data_in_urb) {
+		usb_kill_urb(data_in_urb);
+		usb_put_urb(data_in_urb);
+	}
+	if (data_out_urb) {
+		usb_kill_urb(data_out_urb);
+		usb_put_urb(data_out_urb);
+	}
+
+	return FAILED;
+}
+
 static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 {
 	struct scsi_device *sdev = cmnd->device;
@@ -781,6 +821,7 @@ static struct scsi_host_template uas_host_template = {
 	.queuecommand = uas_queuecommand,
 	.slave_alloc = uas_slave_alloc,
 	.slave_configure = uas_slave_configure,
+	.eh_abort_handler = uas_eh_abort_handler,
 	.eh_bus_reset_handler = uas_eh_bus_reset_handler,
 	.can_queue = 65536,	/* Is there a limit on the _host_ ? */
 	.this_id = -1,
-- 
2.1.0

[PATCH 15/21] uas: pre_reset and suspend: Fix a few races

[Hans de Goede]

The purpose of uas_pre_reset is to:

1) Stop any new commands from being submitted while an externally triggered
   usb-device-reset is running
2) Wait for any pending commands to finish before allowing the usb-device-reset
   to continue

The purpose of uas_suspend is to:
2) Wait for any pending commands to finish before suspending

This commit fixes races in both paths:

1) For 1) we use scsi_block_requests, but the scsi midlayer calls queuecommand
   without holding any locks, so a queuecommand may already past the midlayer
   scsi_block_requests checks when we call it, add a check to uas_queuecommand
   to fix this

2) For 2) we were waiting for all sense-urbs to complete, there are 2 problems
   with this approach:
a) data-urbs may complete after the sense urb, so we need to check for those
   too
b) if a sense-urb completes with a iu id of READ/WRITE_READY a command is not
   yet done. We submit a new sense-urb immediately in this case, but that
   submit may fail (in which case it will get retried by uas_do_work), if this
   happens the sense_urbs anchor may become empty while the cmnd is not yet
   done

Also unblock requests on timeout, to avoid things getting stuck in that case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 61 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6ee61bd..aaca65b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -664,6 +664,10 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
 	BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
 
+	/* Re-check scsi_block_requests now that we've the host-lock */
+	if (cmnd->device->host->host_self_blocked)
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+
 	spin_lock_irqsave(&devinfo->lock, flags);
 
 	if (devinfo->resetting) {
@@ -991,6 +995,54 @@ set_alt0:
 	return result;
 }
 
+static int uas_cmnd_list_empty(struct uas_dev_info *devinfo)
+{
+	unsigned long flags;
+	int i, r = 1;
+
+	spin_lock_irqsave(&devinfo->lock, flags);
+
+	for (i = 0; i < devinfo->qdepth; i++) {
+		if (devinfo->cmnd[i]) {
+			r = 0; /* Not empty */
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
+	return r;
+}
+
+/*
+ * Wait for any pending cmnds to complete, on usb-2 sense_urbs may temporarily
+ * get empty while there still is more work to do due to sense-urbs completing
+ * with a READ/WRITE_READY iu code, so keep waiting until the list gets empty.
+ */
+static int uas_wait_for_pending_cmnds(struct uas_dev_info *devinfo)
+{
+	unsigned long start_time;
+	int r;
+
+	start_time = jiffies;
+	do {
+		flush_work(&devinfo->work);
+
+		r = usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000);
+		if (r == 0)
+			return -ETIME;
+
+		r = usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500);
+		if (r == 0)
+			return -ETIME;
+
+		if (time_after(jiffies, start_time + 5 * HZ))
+			return -ETIME;
+	} while (!uas_cmnd_list_empty(devinfo));
+
+	return 0;
+}
+
 static int uas_pre_reset(struct usb_interface *intf)
 {
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
@@ -1005,10 +1057,9 @@ static int uas_pre_reset(struct usb_interface *intf)
 	scsi_block_requests(shost);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	/* Wait for any pending requests to complete */
-	flush_work(&devinfo->work);
-	if (usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000) == 0) {
+	if (uas_wait_for_pending_cmnds(devinfo) != 0) {
 		shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
+		scsi_unblock_requests(shost);
 		return 1;
 	}
 
@@ -1046,9 +1097,7 @@ static int uas_suspend(struct usb_interface *intf, pm_message_t message)
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
 	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
 
-	/* Wait for any pending requests to complete */
-	flush_work(&devinfo->work);
-	if (usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000) == 0) {
+	if (uas_wait_for_pending_cmnds(devinfo) != 0) {
 		shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
 		return -ETIME;
 	}
-- 
2.1.0

[PATCH 16/21] uas: Use streams on upcoming 10Gbps / 3.1 USB

[Hans de Goede]

Limit the no-streams case to speeds less then USB_SPEED_SUPER.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index aaca65b..685eb37 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -909,7 +909,7 @@ static int uas_configure_endpoints(struct uas_dev_info *devinfo)
 	devinfo->data_out_pipe = usb_sndbulkpipe(udev,
 					    usb_endpoint_num(&eps[3]->desc));
 
-	if (udev->speed != USB_SPEED_SUPER) {
+	if (udev->speed < USB_SPEED_SUPER) {
 		devinfo->qdepth = 32;
 		devinfo->use_streams = 0;
 	} else {
-- 
2.1.0

[PATCH 17/21] uas: Do not log urb status error on cancellation

[Hans de Goede]

Check for both type of cancellation codes for sense and data urbs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 685eb37..46b8788 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -315,10 +315,7 @@ static void uas_stat_cmplt(struct urb *urb)
 		goto out;
 
 	if (urb->status) {
-		if (urb->status == -ENOENT) {
-			dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
-				urb->stream_id);
-		} else {
+		if (urb->status != -ENOENT && urb->status != -ECONNRESET) {
 			dev_err(&urb->dev->dev, "stat urb: status %d\n",
 				urb->status);
 		}
@@ -425,7 +422,7 @@ static void uas_data_cmplt(struct urb *urb)
 	}
 
 	if (urb->status) {
-		if (urb->status != -ECONNRESET) {
+		if (urb->status != -ENOENT && urb->status != -ECONNRESET) {
 			uas_log_cmd_state(cmnd, __func__);
 			scmd_printk(KERN_ERR, cmnd,
 				"data cmplt err %d stream %d\n",
-- 
2.1.0

Re: [PATCH 17/21] uas: Do not log urb status error on cancellation

[Oliver Neukum]

On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
> Check for both type of cancellation codes for sense and data urbs.

Then you should also check for -ESHUTDOWN (unplug of HC)

	Regards
		Oliver

Re: [PATCH 17/21] uas: Do not log urb status error on cancellation

[Hans de Goede]

Hi,

On 09/10/2014 04:00 PM, Oliver Neukum wrote:
> On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote:
>> Check for both type of cancellation codes for sense and data urbs.
> 
> Then you should also check for -ESHUTDOWN (unplug of HC)

The intent here is to stop log pollution when cancelling because
of an abort / bus-reset. When people unplug a device with io pending
they deserve all the log pollution they get IMHO. Also most of the
time things seem to end with -EPROTO then.

Regards,

Hans

[PATCH 18/21] uas: Use scsi_print_command

[Hans de Goede]

Use scsi_print_command to print commands during errors, rather then printing
the rather meaningless pointer to the command.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 46b8788..220f4c7 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -229,8 +229,8 @@ 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%s%s\n",
-		    caller, cmnd, uas_get_tag(cmnd),
+		    "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ",
+		    caller, uas_get_tag(cmnd),
 		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
 		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
 		    (ci->state & SUBMIT_DATA_IN_URB)    ? " s-in"  : "",
@@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
 		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "",
 		    (ci->state & COMMAND_ABORTED)       ? " abort" : "",
 		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
+	scsi_print_command(cmnd);
 }
 
 static void uas_free_unsubmitted_urbs(struct scsi_cmnd *cmnd)
-- 
2.1.0

RE: [PATCH 18/21] uas: Use scsi_print_command

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Hans de Goede
> Sent: Wednesday, 10 September, 2014 6:47 AM
> To: Greg Kroah-Hartman
> Cc: linux-usb@vger.kernel.org; linux-scsi@vger.kernel.org;
> stable@vger.kernel.org; Hans de Goede
> Subject: [PATCH 18/21] uas: Use scsi_print_command
> 
> Use scsi_print_command to print commands during errors, rather then
> printing
> the rather meaningless pointer to the command.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/storage/uas.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 46b8788..220f4c7 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -229,8 +229,8 @@ 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%s%s\n",
> -		    caller, cmnd, uas_get_tag(cmnd),
> +		    "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ",
> +		    caller, uas_get_tag(cmnd),
>  		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
>  		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
>  		    (ci->state & SUBMIT_DATA_IN_URB)    ? " s-in"  : "",
> @@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd
> *cmnd, const char *caller)
>  		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "",
>  		    (ci->state & COMMAND_ABORTED)       ? " abort" : "",
>  		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
> +	scsi_print_command(cmnd);
>  }

The SCSI midlayer already does a scsi_print_command, under
the MLCOMPLETE logging level.  Printing the command in
the LLD is redundant; Printing the scmd pointer was not,
as it matched the prints in the SCSI midlayer, letting
you find all the messages for a command.

In Hannes' logging patch series, Christoph Hellwig suggested
printing the block layer tag instead of the scmd pointer.
If UAS uses that tag directly, then the uas_get_tag() print
already does that.  If they're not identical (looks like 
they differ by 2), then you might want to print both.

More general comment: 
scsi-mq stress testing has shown that calling printk
on each command with an error is problematic.  If the
system runs into hundreds or thousands of errors, the
printks take so long they induce more timeouts and errors.  

As discussed in the threads by Yoshihiro YUNOMAE and Hannes
Reinecke, you might want to:
* tuck the scmd_printk inside SCSI_LOG_LLCOMPLETE so
the user can control the verbosity
* use a _ratelimited version of each print
* plan to add and eventually switch to tracepoints, so
logging doesn't have performance impacts

---
Rob Elliott    HP Server Storage

Re: [PATCH 18/21] uas: Use scsi_print_command

[Hans de Goede]

Hi Robert,

On 09/10/2014 06:08 PM, Elliott, Robert (Server Storage) wrote:
> 
> 
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Hans de Goede
>> Sent: Wednesday, 10 September, 2014 6:47 AM
>> To: Greg Kroah-Hartman
>> Cc: linux-usb@vger.kernel.org; linux-scsi@vger.kernel.org;
>> stable@vger.kernel.org; Hans de Goede
>> Subject: [PATCH 18/21] uas: Use scsi_print_command
>>
>> Use scsi_print_command to print commands during errors, rather then
>> printing
>> the rather meaningless pointer to the command.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/storage/uas.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 46b8788..220f4c7 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -229,8 +229,8 @@ 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%s%s\n",
>> -		    caller, cmnd, uas_get_tag(cmnd),
>> +		    "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ",
>> +		    caller, uas_get_tag(cmnd),
>>  		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
>>  		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
>>  		    (ci->state & SUBMIT_DATA_IN_URB)    ? " s-in"  : "",
>> @@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd
>> *cmnd, const char *caller)
>>  		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "",
>>  		    (ci->state & COMMAND_ABORTED)       ? " abort" : "",
>>  		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
>> +	scsi_print_command(cmnd);
>>  }
> 
> The SCSI midlayer already does a scsi_print_command, under
> the MLCOMPLETE logging level.  Printing the command in
> the LLD is redundant; Printing the scmd pointer was not,
> as it matched the prints in the SCSI midlayer, letting
> you find all the messages for a command.
> 
> In Hannes' logging patch series, Christoph Hellwig suggested
> printing the block layer tag instead of the scmd pointer.
> If UAS uses that tag directly, then the uas_get_tag() print
> already does that.  If they're not identical (looks like 
> they differ by 2), then you might want to print both.
> 
> More general comment: 
> scsi-mq stress testing has shown that calling printk
> on each command with an error is problematic.  If the
> system runs into hundreds or thousands of errors, the
> printks take so long they induce more timeouts and errors.  
> 
> As discussed in the threads by Yoshihiro YUNOMAE and Hannes
> Reinecke, you might want to:
> * tuck the scmd_printk inside SCSI_LOG_LLCOMPLETE so
> the user can control the verbosity
> * use a _ratelimited version of each print
> * plan to add and eventually switch to tracepoints, so
> logging doesn't have performance impacts

Thanks for your feedback, indeed we may need to cut down on the
logging in the future.

ATM our focus is mostly on getting the UAS driver work reliable
with all the (cheap) uas hardware out there. Most errors we
are getting are not disk errors, but errors due to the usb <->
sata bridge chip not groking some command.

Being able to directly see the command which is tripping up the
bridge in a dmesg output without needing to tell the user to
take extra action is very helpful at this point in the uas'
driver development.

Regards,

Hans

[PATCH 19/21] uas: Drop COMMAND_COMPLETED flag

[Hans de Goede]

It was only used to sanity check against completing the same cmnd twice,
but that is the case we're likely operating on free-ed memory, and doing
sanity checks on free-ed memory is not really helpful.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 220f4c7..e8d2bf1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -72,9 +72,8 @@ enum {
 	COMMAND_INFLIGHT        = (1 << 8),
 	DATA_IN_URB_INFLIGHT    = (1 << 9),
 	DATA_OUT_URB_INFLIGHT   = (1 << 10),
-	COMMAND_COMPLETED       = (1 << 11),
-	COMMAND_ABORTED         = (1 << 12),
-	IS_IN_WORK_LIST         = (1 << 13),
+	COMMAND_ABORTED         = (1 << 11),
+	IS_IN_WORK_LIST         = (1 << 12),
 };
 
 /* Overrides scsi_pointer */
@@ -229,7 +228,7 @@ 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 tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ",
+		    "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ",
 		    caller, uas_get_tag(cmnd),
 		    (ci->state & SUBMIT_STATUS_URB)     ? " s-st"  : "",
 		    (ci->state & ALLOC_DATA_IN_URB)     ? " a-in"  : "",
@@ -241,7 +240,6 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
 		    (ci->state & COMMAND_INFLIGHT)      ? " CMD"   : "",
 		    (ci->state & DATA_IN_URB_INFLIGHT)  ? " IN"    : "",
 		    (ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT"   : "",
-		    (ci->state & COMMAND_COMPLETED)     ? " done"  : "",
 		    (ci->state & COMMAND_ABORTED)       ? " abort" : "",
 		    (ci->state & IS_IN_WORK_LIST)       ? " work"  : "");
 	scsi_print_command(cmnd);
@@ -277,8 +275,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 			      DATA_OUT_URB_INFLIGHT |
 			      COMMAND_ABORTED))
 		return -EBUSY;
-	WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
-	cmdinfo->state |= COMMAND_COMPLETED;
 	devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
 	uas_free_unsubmitted_urbs(cmnd);
 	cmnd->scsi_done(cmnd);
-- 
2.1.0

[PATCH 21/21] uas: Remove protype hardware usb interface info

[Hans de Goede]

We've removed all hack from the driver for pre-production hardware.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 23c0b97..a7f16c4 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -818,8 +818,6 @@ static struct usb_device_id uas_usb_ids[] = {
 #	include "unusual_uas.h"
 	{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_BULK) },
 	{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_UAS) },
-	/* 0xaa is a prototype device I happen to have access to */
-	{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, 0xaa) },
 	{ }
 };
 MODULE_DEVICE_TABLE(usb, uas_usb_ids);
-- 
2.1.0