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

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

[Hans de Goede]

Hi Greg, et al,

Here is v2 of my uas error handling rewrite + cleanups series.

Note this has been rebased on top of the

"[PATCH fix for 3.17] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands"

patch which I've just send, so if that does not make 3.17, you should still
apply that one first to avoid conflicts.

Changes since v1:
-Rebased on top of:
 "uas: Add a quirk for rejecting ATA_12 and ATA_16 commands"
-"uas: zap_pending: data urbs should have completed at this time":
 Modified WARN_ON check to not have side effects
-Added 3 new patches:
 "uas: Cleanup uas_log_cmd_state usage"
 "uas: Log error codes when logging errors"
 "uas: Add response iu handling"

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

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

[Hans de Goede]

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 75d2ccd..8c7d4a2 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -156,7 +156,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;
@@ -183,7 +183,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);
 }
@@ -285,7 +285,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 |
@@ -624,7 +624,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

[PATCH v2 02/24] 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 8c7d4a2..4cf4d58 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
  *
@@ -52,11 +52,9 @@ struct uas_dev_info {
 	struct usb_anchor data_urbs;
 	unsigned long flags;
 	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;
@@ -207,7 +205,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);
 }
 
@@ -350,13 +347,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;
@@ -536,56 +526,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
@@ -787,118 +727,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);
-}

[PATCH v2 03/24] 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 4cf4d58..0d97602 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -129,6 +129,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,
@@ -143,6 +147,7 @@ static void uas_do_work(struct work_struct *work)
 		else
 			schedule_work(&devinfo->work);
 	}
+out:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
@@ -322,6 +327,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",
@@ -330,27 +340,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) {
@@ -391,6 +391,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);
 }
@@ -404,6 +405,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;
@@ -413,7 +415,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,
@@ -426,6 +434,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);
 }
 
@@ -732,6 +741,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);
@@ -742,14 +752,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);
 
@@ -1045,8 +1062,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

[PATCH v2 04/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 0d97602..ac1424c 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -259,13 +259,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"  : "",
@@ -516,10 +532,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);
@@ -690,17 +703,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

--
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 v2 05/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 ac1424c..b58d133 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -30,6 +30,8 @@
 #include "uas-detect.h"
 #include "scsiglue.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.
@@ -56,7 +58,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;
@@ -316,6 +318,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;
 }
@@ -341,7 +344,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);
 
@@ -359,21 +362,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)
@@ -674,6 +673,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));
@@ -696,19 +696,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) {
@@ -738,6 +735,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;
@@ -873,7 +871,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)
@@ -893,7 +890,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

--
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 v2 06/24] 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 b58d133..8614ddf 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -371,6 +371,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)
@@ -436,6 +442,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 v2 07/24] 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 8614ddf..68853b5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,8 +78,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 */
@@ -100,28 +99,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 =
@@ -281,8 +258,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"  : "",
@@ -296,7 +273,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"  : "");
 }
 
@@ -308,8 +284,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;
@@ -341,6 +316,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;
@@ -387,7 +364,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__);
@@ -415,6 +393,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 v2 08/24] 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 68853b5..70b44f0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -288,8 +288,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);
@@ -418,9 +416,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);
@@ -450,6 +450,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 v2 09/24] 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 70b44f0..08edb6b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -62,7 +62,6 @@ struct uas_dev_info {
 	spinlock_t lock;
 	struct work_struct work;
 	struct list_head inflight_list;
-	struct list_head dead_list;
 };
 
 enum {
@@ -130,35 +129,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;
@@ -170,7 +140,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;
@@ -182,11 +152,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);
@@ -765,11 +735,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);
@@ -948,7 +918,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)
@@ -1076,11 +1045,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 v2 10/24] 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@redhat.com>
---
 drivers/usb/storage/uas.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08edb6b..85bbc1d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 	struct uas_cmd_info *cmdinfo;
 	struct uas_cmd_info *temp;
 	unsigned long flags;
+	int err;
 
 	spin_lock_irqsave(&devinfo->lock, flags);
 	list_for_each_entry_safe(cmdinfo, temp, &devinfo->dead_list, list) {
@@ -152,12 +153,11 @@ 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__);
+		err = uas_try_complete(cmnd, __func__);
+		WARN_ON(err != 0);
 	}
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 }
-- 
2.1.0

[PATCH v2 11/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 85bbc1d..36c42b8 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -61,7 +61,6 @@ struct uas_dev_info {
 	struct scsi_cmnd *cmnd[MAX_CMNDS];
 	spinlock_t lock;
 	struct work_struct work;
-	struct list_head inflight_list;
 };
 
 enum {
@@ -87,7 +86,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 */
@@ -103,18 +101,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;
@@ -143,15 +144,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 err;
+	int i, err;
 
 	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;
@@ -260,7 +263,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;
@@ -707,7 +709,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;
 }
@@ -917,7 +918,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

--
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 v2 12/24] 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 36c42b8..1781b36 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -428,12 +428,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);
 }
 
@@ -511,7 +508,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 v2 13/24] 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 1781b36..ce2d8fc 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -257,12 +257,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;
@@ -712,6 +711,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;
@@ -793,6 +833,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 v2 14/24] 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@redhat.com>
---
 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 ce2d8fc..fca8775 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -249,6 +249,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;
@@ -263,6 +282,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;
 }
@@ -738,6 +758,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

[PATCH v2 15/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 fca8775..f4fe816 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -667,6 +667,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;
+
 	if ((devinfo->flags & US_FL_NO_ATA_1X) &&
 			(cmnd->cmnd[0] == ATA_12 || cmnd->cmnd[0] == ATA_16)) {
 		memcpy(cmnd->sense_buffer, usb_stor_sense_invalidCDB,
@@ -1005,6 +1009,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);
@@ -1019,10 +1071,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;
 	}
 
@@ -1060,9 +1111,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

--
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 v2 16/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 f4fe816..187e7bf 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -921,7 +921,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

--
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 v2 17/24] 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 187e7bf..90afe4a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -318,10 +318,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);
 		}
@@ -428,7 +425,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

[PATCH v2 18/24] 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 90afe4a..652b97b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -232,8 +232,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"  : "",
@@ -247,6 +247,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

[PATCH v2 19/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 652b97b..c6f7870 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -74,9 +74,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 */
@@ -232,7 +231,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"  : "",
@@ -244,7 +243,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);
@@ -280,8 +278,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

--
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 v2 20/24] 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@redhat.com>
---
 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 c6f7870..cbd4312 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -32,20 +32,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;
@@ -56,7 +42,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;
@@ -187,29 +172,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.
@@ -339,12 +301,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);
@@ -900,8 +857,6 @@ static int uas_configure_endpoints(struct uas_dev_info *devinfo)
 	struct usb_device *udev = devinfo->udev;
 	int r;
 
-	devinfo->uas_sense_old = 0;
-
 	r = uas_find_endpoints(devinfo->intf->cur_altsetting, eps);
 	if (r)
 		return r;
-- 
2.1.0

[PATCH v2 21/24] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 cbd4312..a77c5e0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -830,8 +830,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

--
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 v2 22/24] uas: Cleanup uas_log_cmd_state usage

[Hans de Goede]

Instead of doing:

uas_log_cmd_state(cmnd, __func__)
scmd_printk(KERN_ERR, cmnd, "error doing foo %d\n", err)

On error, resulting in 2 log calls for a single error, make uas_log_cmd_state
take a status code, and change calls like the above to:

uas_log_cmd_state(cmnd, "error doing foo", err)

Also change various sanity checks (which should never trigger) from:
"scmd_printk(KERN_ERR, cmnd, "sanity foo failed\n")" to calling the new
uas_log_cmd_state(), so that when they do trigger we get more info.

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

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a77c5e0..0b3d3a5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,7 +78,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 static void uas_do_work(struct work_struct *work);
 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);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
+				int status);
 
 static void uas_do_work(struct work_struct *work)
 {
@@ -139,7 +140,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 
 		cmnd = devinfo->cmnd[i];
 		cmdinfo = (void *)&cmnd->SCp;
-		uas_log_cmd_state(cmnd, __func__);
+		uas_log_cmd_state(cmnd, __func__, 0);
 		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
 		cmdinfo->state &= ~COMMAND_INFLIGHT;
 		cmnd->result = result << 16;
@@ -188,13 +189,14 @@ static int uas_get_tag(struct scsi_cmnd *cmnd)
 	return tag;
 }
 
-static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
+			      int status)
 {
 	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 ",
-		    caller, uas_get_tag(cmnd),
+		    "%s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ",
+		    prefix, status, 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"  : "",
@@ -295,7 +297,7 @@ static void uas_stat_cmplt(struct urb *urb)
 	cmdinfo = (void *)&cmnd->SCp;
 
 	if (!(cmdinfo->state & COMMAND_INFLIGHT)) {
-		scmd_printk(KERN_ERR, cmnd, "unexpected status cmplt\n");
+		uas_log_cmd_state(cmnd, "unexpected status cmplt", 0);
 		goto out;
 	}
 
@@ -313,7 +315,7 @@ static void uas_stat_cmplt(struct urb *urb)
 	case IU_ID_READ_READY:
 		if (!cmdinfo->data_in_urb ||
 				(cmdinfo->state & DATA_IN_URB_INFLIGHT)) {
-			scmd_printk(KERN_ERR, cmnd, "unexpected read rdy\n");
+			uas_log_cmd_state(cmnd, "unexpected read rdy", 0);
 			break;
 		}
 		uas_xfer_data(urb, cmnd, SUBMIT_DATA_IN_URB);
@@ -321,14 +323,13 @@ static void uas_stat_cmplt(struct urb *urb)
 	case IU_ID_WRITE_READY:
 		if (!cmdinfo->data_out_urb ||
 				(cmdinfo->state & DATA_OUT_URB_INFLIGHT)) {
-			scmd_printk(KERN_ERR, cmnd, "unexpected write rdy\n");
+			uas_log_cmd_state(cmnd, "unexpected write rdy", 0);
 			break;
 		}
 		uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
 		break;
 	default:
-		scmd_printk(KERN_ERR, cmnd,
-			"Bogus IU (%d) received on status pipe\n", iu->iu_id);
+		uas_log_cmd_state(cmnd, "bogus IU", iu->iu_id);
 	}
 out:
 	usb_free_urb(urb);
@@ -374,17 +375,13 @@ static void uas_data_cmplt(struct urb *urb)
 
 	/* 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");
+		uas_log_cmd_state(cmnd, "unexpected data cmplt", 0);
 		goto out;
 	}
 
 	if (urb->status) {
-		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",
-				urb->status, urb->stream_id);
-		}
+		if (urb->status != -ENOENT && urb->status != -ECONNRESET)
+			uas_log_cmd_state(cmnd, "data cmplt err", urb->status);
 		/* error: no data transfered */
 		sdb->resid = sdb->length;
 	} else {
@@ -508,10 +505,7 @@ static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd,
 	err = usb_submit_urb(urb, gfp);
 	if (err) {
 		usb_unanchor_urb(urb);
-		uas_log_cmd_state(cmnd, __func__);
-		shost_printk(KERN_INFO, shost,
-			     "sense urb submission error %d stream %d\n",
-			     err, stream);
+		uas_log_cmd_state(cmnd, "sense submit err", err);
 		usb_free_urb(urb);
 		return NULL;
 	}
@@ -547,10 +541,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		err = usb_submit_urb(cmdinfo->data_in_urb, gfp);
 		if (err) {
 			usb_unanchor_urb(cmdinfo->data_in_urb);
-			uas_log_cmd_state(cmnd, __func__);
-			scmd_printk(KERN_INFO, cmnd,
-				"data in urb submission error %d stream %d\n",
-				err, cmdinfo->data_in_urb->stream_id);
+			uas_log_cmd_state(cmnd, "data in submit err", err);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
@@ -571,10 +562,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		err = usb_submit_urb(cmdinfo->data_out_urb, gfp);
 		if (err) {
 			usb_unanchor_urb(cmdinfo->data_out_urb);
-			uas_log_cmd_state(cmnd, __func__);
-			scmd_printk(KERN_INFO, cmnd,
-				"data out urb submission error %d stream %d\n",
-				err, cmdinfo->data_out_urb->stream_id);
+			uas_log_cmd_state(cmnd, "data out submit err", err);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
@@ -593,9 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 		err = usb_submit_urb(cmdinfo->cmd_urb, gfp);
 		if (err) {
 			usb_unanchor_urb(cmdinfo->cmd_urb);
-			uas_log_cmd_state(cmnd, __func__);
-			scmd_printk(KERN_INFO, cmnd,
-				    "cmd urb submission error %d\n", err);
+			uas_log_cmd_state(cmnd, "cmd submit err", err);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
 		cmdinfo->cmd_urb = NULL;
@@ -701,7 +687,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 
 	spin_lock_irqsave(&devinfo->lock, flags);
 
-	uas_log_cmd_state(cmnd, __func__);
+	uas_log_cmd_state(cmnd, __func__, 0);
 
 	/* Ensure that try_complete does not call scsi_done */
 	cmdinfo->state |= COMMAND_ABORTED;
-- 
2.1.0

[PATCH v2 23/24] uas: Log error codes when logging errors

[Hans de Goede]

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

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 0b3d3a5..1dcaeee 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -750,7 +750,8 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	usb_unlock_device(udev);
 
 	if (err) {
-		shost_printk(KERN_INFO, sdev->host, "%s FAILED\n", __func__);
+		shost_printk(KERN_INFO, sdev->host, "%s FAILED err %d\n",
+			     __func__, err);
 		return FAILED;
 	}
 
@@ -1020,13 +1021,16 @@ static int uas_post_reset(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;
+	int err;
 
 	if (devinfo->shutdown)
 		return 0;
 
-	if (uas_configure_endpoints(devinfo) != 0) {
+	err = uas_configure_endpoints(devinfo);
+	if (err) {
 		shost_printk(KERN_ERR, shost,
-			     "%s: alloc streams error after reset", __func__);
+			     "%s: alloc streams error %d after reset",
+			     __func__, err);
 		return 1;
 	}
 
@@ -1062,10 +1066,13 @@ static int uas_reset_resume(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;
+	int err;
 
-	if (uas_configure_endpoints(devinfo) != 0) {
+	err = uas_configure_endpoints(devinfo);
+	if (err) {
 		shost_printk(KERN_ERR, shost,
-			     "%s: alloc streams error after reset", __func__);
+			     "%s: alloc streams error %d after reset",
+			     __func__, err);
 		return -EIO;
 	}
 
-- 
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 v2 24/24] uas: Add response iu handling

[Hans de Goede]

If something goes wrong in our communication with an uas device we may get
a response iu in reaction to a cmnd, rather then a status iu. In this case
propagate an error upwards, rather then logging a bogus iu message.

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

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1dcaeee..bebf861 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -328,6 +328,16 @@ static void uas_stat_cmplt(struct urb *urb)
 		}
 		uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
 		break;
+	case IU_ID_RESPONSE:
+		uas_log_cmd_state(cmnd, "unexpected response iu",
+				  ((struct response_iu *)iu)->response_code);
+		/* Error, cancel data transfers */
+		data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+		cmdinfo->state &= ~COMMAND_INFLIGHT;
+		cmnd->result = DID_ERROR << 16;
+		uas_try_complete(cmnd, __func__);
+		break;
 	default:
 		uas_log_cmd_state(cmnd, "bogus IU", iu->iu_id);
 	}
-- 
2.1.0

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

[Sergei Shtylyov]

Hello.

On 9/13/2014 1:26 PM, Hans de Goede wrote:

> 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 | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 08edb6b..85bbc1d 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
>   	struct uas_cmd_info *cmdinfo;
>   	struct uas_cmd_info *temp;
>   	unsigned long flags;
> +	int err;

    Er, I don't see why this variable is necessary.

[...]
> @@ -152,12 +153,11 @@ 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__);
> +		err = uas_try_complete(cmnd, __func__);
> +		WARN_ON(err != 0);

    Why not:

		WARN_ON(uas_try_complete(cmnd, __func__));

>   	}
>   	spin_unlock_irqrestore(&devinfo->lock, flags);
>   }

WBR, Sergei

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

[Hans de Goede]

Hi,

On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 9/13/2014 1:26 PM, Hans de Goede wrote:
> 
>> 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@redhat.com>
>> ---
>>   drivers/usb/storage/uas.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 08edb6b..85bbc1d 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
>>       struct uas_cmd_info *cmdinfo;
>>       struct uas_cmd_info *temp;
>>       unsigned long flags;
>> +    int err;
> 
>    Er, I don't see why this variable is necessary.
> 
> [...]
>> @@ -152,12 +153,11 @@ 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__);
>> +        err = uas_try_complete(cmnd, __func__);
>> +        WARN_ON(err != 0);
> 
>    Why not:
> 
>         WARN_ON(uas_try_complete(cmnd, __func__));
> 

This was discussed already during v1 of this patch-set, WARN_ON may
not have a side-effect, as it may be defined as an empty macro.

Regards,

Hans

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

[James Bottomley]

On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 9/13/2014 1:26 PM, Hans de Goede wrote:
> > 
> >> 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@redhat.com>
> >> ---
> >>   drivers/usb/storage/uas.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> >> index 08edb6b..85bbc1d 100644
> >> --- a/drivers/usb/storage/uas.c
> >> +++ b/drivers/usb/storage/uas.c
> >> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
> >>       struct uas_cmd_info *cmdinfo;
> >>       struct uas_cmd_info *temp;
> >>       unsigned long flags;
> >> +    int err;
> > 
> >    Er, I don't see why this variable is necessary.
> > 
> > [...]
> >> @@ -152,12 +153,11 @@ 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__);
> >> +        err = uas_try_complete(cmnd, __func__);
> >> +        WARN_ON(err != 0);
> > 
> >    Why not:
> > 
> >         WARN_ON(uas_try_complete(cmnd, __func__));
> > 
> 
> This was discussed already during v1 of this patch-set, WARN_ON may
> not have a side-effect, as it may be defined as an empty macro.

Must have missed the discussion, but whoever said that loses all their
review points.  We're very careful to make sure that even in the case
where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
side effects are still accounted for.  This is the canonical definition
of WARN_ON in the compiled out case:

#define WARN_ON(condition) ({						\
	int __ret_warn_on = !!(condition);				\
	unlikely(__ret_warn_on);					\
})

So the compiler will eliminate the statement only if there are no side
effects.

James

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

[Hans de Goede]

Hi,

On 09/14/2014 12:29 PM, James Bottomley wrote:
> On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 9/13/2014 1:26 PM, Hans de Goede wrote:
>>>
>>>> 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@redhat.com>
>>>> ---
>>>>   drivers/usb/storage/uas.c | 10 +++++-----
>>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>>>> index 08edb6b..85bbc1d 100644
>>>> --- a/drivers/usb/storage/uas.c
>>>> +++ b/drivers/usb/storage/uas.c
>>>> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
>>>>       struct uas_cmd_info *cmdinfo;
>>>>       struct uas_cmd_info *temp;
>>>>       unsigned long flags;
>>>> +    int err;
>>>
>>>    Er, I don't see why this variable is necessary.
>>>
>>> [...]
>>>> @@ -152,12 +153,11 @@ 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__);
>>>> +        err = uas_try_complete(cmnd, __func__);
>>>> +        WARN_ON(err != 0);
>>>
>>>    Why not:
>>>
>>>         WARN_ON(uas_try_complete(cmnd, __func__));
>>>
>>
>> This was discussed already during v1 of this patch-set, WARN_ON may
>> not have a side-effect, as it may be defined as an empty macro.
> 
> Must have missed the discussion, but whoever said that loses all their
> review points.  We're very careful to make sure that even in the case
> where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
> side effects are still accounted for.  This is the canonical definition
> of WARN_ON in the compiled out case:
> 
> #define WARN_ON(condition) ({						\
> 	int __ret_warn_on = !!(condition);				\
> 	unlikely(__ret_warn_on);					\
> })
> 
> So the compiler will eliminate the statement only if there are no side
> effects.

Ah that is good to know. Still I would like to stick with the new version
(which adds the err), as I believe that that code is more readable.

AFAIK in general the kernel coding style is to favor:

err = func();
if (err)

over:

if (func())

And this is sorta the same.

Regards,

Hans

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

[James Bottomley]

On Sun, 2014-09-14 at 12:32 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/14/2014 12:29 PM, James Bottomley wrote:
> > On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> >>> Hello.
> >>>
> >>> On 9/13/2014 1:26 PM, Hans de Goede wrote:
> >>>
> >>>> 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@redhat.com>
> >>>> ---
> >>>>   drivers/usb/storage/uas.c | 10 +++++-----
> >>>>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> >>>> index 08edb6b..85bbc1d 100644
> >>>> --- a/drivers/usb/storage/uas.c
> >>>> +++ b/drivers/usb/storage/uas.c
> >>>> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
> >>>>       struct uas_cmd_info *cmdinfo;
> >>>>       struct uas_cmd_info *temp;
> >>>>       unsigned long flags;
> >>>> +    int err;
> >>>
> >>>    Er, I don't see why this variable is necessary.
> >>>
> >>> [...]
> >>>> @@ -152,12 +153,11 @@ 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__);
> >>>> +        err = uas_try_complete(cmnd, __func__);
> >>>> +        WARN_ON(err != 0);
> >>>
> >>>    Why not:
> >>>
> >>>         WARN_ON(uas_try_complete(cmnd, __func__));
> >>>
> >>
> >> This was discussed already during v1 of this patch-set, WARN_ON may
> >> not have a side-effect, as it may be defined as an empty macro.
> > 
> > Must have missed the discussion, but whoever said that loses all their
> > review points.  We're very careful to make sure that even in the case
> > where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
> > side effects are still accounted for.  This is the canonical definition
> > of WARN_ON in the compiled out case:
> > 
> > #define WARN_ON(condition) ({						\
> > 	int __ret_warn_on = !!(condition);				\
> > 	unlikely(__ret_warn_on);					\
> > })
> > 
> > So the compiler will eliminate the statement only if there are no side
> > effects.
> 
> Ah that is good to know. Still I would like to stick with the new version
> (which adds the err), as I believe that that code is more readable.
> 
> AFAIK in general the kernel coding style is to favor:
> 
> err = func();
> if (err)
> 
> over:
> 
> if (func())
> 
> And this is sorta the same.

I'm agnostic on that.  I was just combatting the impression that you had
to be careful about side effects in known macro statements.

James

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

[Oliver Neukum]

On Sun, 2014-09-14 at 11:34 +0100, James Bottomley wrote:

> I'm agnostic on that.  I was just combatting the impression that you had
> to be careful about side effects in known macro statements.

No you haven't. But it is very good practice to not have side effects
in warnings, as people, not compilers, like to remove them.

	Regards
		Oliver