Re: [PATCH] usb: MAINTAINERS: Oliver Neukum is the new uas maintainer
On Do, 2016-07-14 at 14:26 +0200, Hans de Goede wrote: > Oliver Neukum is taking over uas maintainership from me and > Gerd Hoffmann. > > Cc: Oliver Neukum > Cc: Gerd Hoffmann > Signed-off-by: Hans de Goede Acked-by: Gerd Hoffmann -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/6] uas: make work list per-device
On Di, 2013-09-17 at 13:30 -0700, Christoph Hellwig wrote: > On Fri, Sep 13, 2013 at 01:27:12PM +0200, Gerd Hoffmann wrote: > > Simplifies locking, we'll protect the list with the device spin lock. > > Also plugs races which can happen when two devices operate on the > > global list. > > > > While being at it rename the list head from "list" to "work", preparing > > for the addition of a second list. > > Why do you even the list? The list was already there when I took over maintainance ... > What would a ordered per-device workqueue not > provide? Had no reason to look into replacing the list with something else so far. Why do you suggest using a workqueue instead? Note that the list is not used in a typical request workflow. Only in case queuing an usb urb failed the request is linked into the list and the worker will try to submit the usb urb again. cheers, Gerd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/6] uas: add dead request list
This patch adds a new list where all requests which are canceled are
added to, so we don't loose them. Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.
Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 50 +++
1 file changed, 42 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3cf5a5f..f049038 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -53,6 +53,7 @@ struct uas_dev_info {
spinlock_t lock;
struct work_struct work;
struct list_head work_list;
+ struct list_head dead_list;
};
enum {
@@ -80,6 +81,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+ struct list_head dead;
};
/* I hate forward declarations, but I actually have a loop */
@@ -89,6 +91,7 @@ static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
static void uas_configure_endpoints(struct uas_dev_info *devinfo);
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_unlink_data_urbs(struct uas_dev_info *devinfo,
struct uas_cmd_info *cmdinfo)
@@ -150,16 +153,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
+ uas_log_cmd_state(cmnd, __func__);
+ WARN_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
cmdinfo->state &= ~IS_IN_WORK_LIST;
- if (devinfo->resetting) {
- /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
- cmdinfo->state &= ~COMMAND_INFLIGHT;
- }
- uas_try_complete(cmnd, __func__);
list_del(&cmdinfo->work);
+ list_add_tail(&cmdinfo->dead, &devinfo->dead_list);
}
spin_unlock_irqrestore(&devinfo->lock, flags);
}
@@ -176,6 +175,28 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
schedule_work(&devinfo->work);
}
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+ 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->dead_list, dead) {
+ struct scsi_pointer *scp = (void *)cmdinfo;
+ struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
+ SCp);
+ uas_log_cmd_state(cmnd, __func__);
+ WARN_ON(!(cmdinfo->state & COMMAND_ABORTED));
+ /* all urbs are killed, clear inflight bits */
+ cmdinfo->state &= ~(COMMAND_INFLIGHT |
+ DATA_IN_URB_INFLIGHT |
+ DATA_OUT_URB_INFLIGHT);
+ uas_try_complete(cmnd, __func__);
+ }
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
{
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -263,6 +284,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");
cmnd->result = DID_ABORT << 16;
+ list_del(&cmdinfo->dead);
}
cmnd->scsi_done(cmnd);
return 0;
@@ -292,7 +314,13 @@ static void uas_stat_cmplt(struct urb *urb)
u16 tag;
if (urb->status) {
- dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
+ if (urb->status == -ENOENT) {
+ dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
+ urb->stream_id);
+ } else {
+ dev_err(&urb->dev->dev, "stat urb: status %d\n",
+ urb->status);
+ }
usb_free_urb(urb);
return;
}
@@ -743,7 +771,9 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
[PATCH v3 3/6] uas: make work list per-device
Simplifies locking, we'll protect the list with the device spin lock.
Also plugs races which can happen when two devices operate on the
global list.
While being at it rename the list head from "list" to "work", preparing
for the addition of a second list.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 106 +++---
1 file changed, 44 insertions(+), 62 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index fc08ee9..3cf5a5f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -51,6 +51,8 @@ struct uas_dev_info {
unsigned uas_sense_old:1;
struct scsi_cmnd *cmnd;
spinlock_t lock;
+ struct work_struct work;
+ struct list_head work_list;
};
enum {
@@ -77,7 +79,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
- struct list_head list;
+ struct list_head work;
};
/* I hate forward declarations, but I actually have a loop */
@@ -88,10 +90,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller);
static void uas_configure_endpoints(struct uas_dev_info *devinfo);
static void uas_free_streams(struct uas_dev_info *devinfo);
-static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
-static LIST_HEAD(uas_work_list);
-
static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
struct uas_cmd_info *cmdinfo)
{
@@ -118,75 +116,66 @@ static void uas_unlink_data_urbs(struct uas_dev_info
*devinfo,
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 uas_cmd_info *temp;
- struct list_head list;
unsigned long flags;
int err;
- spin_lock_irq(&uas_work_lock);
- list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
-
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ spin_lock_irqsave(&devinfo->lock, flags);
+ list_for_each_entry_safe(cmdinfo, temp, &devinfo->work_list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
- struct scsi_cmnd *cmnd = container_of(scp,
- struct scsi_cmnd, SCp);
- struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
- spin_lock_irqsave(&devinfo->lock, flags);
+ struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
+ SCp);
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
- if (!err)
+ if (!err) {
cmdinfo->state &= ~IS_IN_WORK_LIST;
- spin_unlock_irqrestore(&devinfo->lock, flags);
- if (err) {
- list_del(&cmdinfo->list);
- spin_lock_irq(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
- spin_unlock_irq(&uas_work_lock);
- schedule_work(&uas_work);
+ list_del(&cmdinfo->work);
+ } else {
+ schedule_work(&devinfo->work);
}
}
+ spin_unlock_irqrestore(&devinfo->lock, flags);
}
static void uas_abort_work(struct uas_dev_info *devinfo)
{
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
- struct list_head list;
unsigned long flags;
- spin_lock_irq(&uas_work_lock);
- list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
-
spin_lock_irqsave(&devinfo->lock, flags);
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ list_for_each_entry_safe(cmdinfo, temp, &devinfo->work_list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
- struct scsi_cmnd *cmnd = container_of(scp,
- struct scsi_cmnd, SCp);
- struct uas_dev_info *di = (void *)cmnd->device->hostdata;
-
- if (di == devinfo) {
- cmdinfo->state |= COMMAND_ABORTED;
- cmdinfo->state &= ~IS_IN_WORK_LIST;
- if (devinfo->resetting) {
- /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
- cmdinfo->state &= ~COMMAND_INFLIGHT;
-
[PATCH v3 2/6] uas: properly reinitialize in uas_eh_bus_reset_handler
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..fc08ee9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
static DECLARE_WORK(uas_work, uas_do_work);
static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+ uas_free_streams(devinfo);
err = usb_reset_device(udev);
+ if (!err)
+ uas_configure_endpoints(devinfo);
devinfo->resetting = 0;
if (err) {
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/6] uas: replace BUG_ON() + WARN_ON() with WARN_ON_ONCE()
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f049038..046eedf 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -154,7 +154,7 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
uas_log_cmd_state(cmnd, __func__);
- WARN_ON(cmdinfo->state & COMMAND_ABORTED);
+ WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
cmdinfo->state &= ~IS_IN_WORK_LIST;
list_del(&cmdinfo->work);
@@ -169,7 +169,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(!spin_is_locked(&devinfo->lock));
+ WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
list_add_tail(&cmdinfo->work, &devinfo->work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
schedule_work(&devinfo->work);
@@ -187,7 +187,7 @@ 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(!(cmdinfo->state & COMMAND_ABORTED));
+ WARN_ON_ONCE(!(cmdinfo->state & COMMAND_ABORTED));
/* all urbs are killed, clear inflight bits */
cmdinfo->state &= ~(COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
@@ -271,13 +271,13 @@ 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(!spin_is_locked(&devinfo->lock));
+ WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
DATA_OUT_URB_INFLIGHT |
UNLINK_DATA_URBS))
return -EBUSY;
- BUG_ON(cmdinfo->state & COMMAND_COMPLETED);
+ 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);
@@ -398,8 +398,9 @@ static void uas_data_cmplt(struct urb *urb)
sdb = scsi_out(cmnd);
cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
}
- BUG_ON(sdb == NULL);
- if (urb->status) {
+ if (sdb == NULL) {
+ WARN_ON_ONCE(1);
+ } else if (urb->status) {
/* error: no data transfered */
sdb->resid = sdb->length;
} else {
@@ -573,7 +574,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
int err;
- WARN_ON(!spin_is_locked(&devinfo->lock));
+ WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & SUBMIT_STATUS_URB) {
err = uas_submit_sense_urb(cmnd->device->host, gfp,
cmdinfo->stream);
@@ -771,7 +772,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
uas_log_cmd_state(cmnd, __func__);
spin_lock_irqsave(&devinfo->lock, flags);
- WARN_ON(cmdinfo->state & COMMAND_ABORTED);
+ WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
list_add_tail(&cmdinfo->dead, &devinfo->dead_list);
if (cmdinfo->state & IS_IN_WORK_LIST) {
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] uas: add dead request list
On Di, 2013-09-03 at 10:39 -0700, Sarah Sharp wrote: > Don't you need to send an ABORT TASK message to the device to cancel the > outstanding request for that stream ID? I don't see that in this code. > I see lots of URB cancellation code, but nothing to remove the request > from the device-side queue. It is there. uas_eh_abort_handler() cancels a single request. There is also uas_eh_device_reset_handler() which will try a LOGICAL UNIT RESET. Those might not work though, depending on the failure mode. If your usb3 streams stop working you can't cancel scsi requests that way. > Or does it simply ensure that SCSI bus reset works? The scsi layer invokes the uas_eh_bus_reset_handler() as last resort, when everything else fails. So, yes, there we'll have the sledge hammer approach to recover: cancel all usb urbs, abort all requests, full usb device reset + re-initialization. But we hardly have another chance when the less invasive methods to cancel a requests didn't work ... > Plus, as Joe mentioned, this code is full of BUG_ON(), which is not > friendly to users, and doesn't increase my confidence that the driver is > ready to have CONFIG_BROKEN removed. Huh? Why you are thinking BUG_ON() is a indicator for bad code quality? I'm using BUG_ON() like assert() in userspace, i.e. they are extra sanity checks which should never ever trigger. I can switch them to less disruptive WARN_ON() if that is the preferred way these says. cheers, Gerd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] uas: rename work list lock + list field
This patch prepares for the addition of another list and renames the
work list lock and the list_head field in struct uas_cmd_info.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 50 +++
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index fc08ee9..db09bda 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -77,7 +77,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
- struct list_head list;
+ struct list_head work;
};
/* I hate forward declarations, but I actually have a loop */
@@ -89,7 +89,7 @@ static void uas_configure_endpoints(struct uas_dev_info
*devinfo);
static void uas_free_streams(struct uas_dev_info *devinfo);
static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
+static DEFINE_SPINLOCK(uas_lists_lock);
static LIST_HEAD(uas_work_list);
static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
@@ -124,11 +124,11 @@ static void uas_do_work(struct work_struct *work)
unsigned long flags;
int err;
- spin_lock_irq(&uas_work_lock);
+ spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
+ spin_unlock_irq(&uas_lists_lock);
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -139,10 +139,10 @@ static void uas_do_work(struct work_struct *work)
cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
- list_del(&cmdinfo->list);
- spin_lock_irq(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
- spin_unlock_irq(&uas_work_lock);
+ list_del(&cmdinfo->work);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
+ spin_unlock_irq(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -155,12 +155,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct list_head list;
unsigned long flags;
- spin_lock_irq(&uas_work_lock);
+ spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
+ spin_unlock_irq(&uas_lists_lock);
spin_lock_irqsave(&devinfo->lock, flags);
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -178,10 +178,10 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
- list_del(&cmdinfo->list);
- spin_lock_irq(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
- spin_unlock_irq(&uas_work_lock);
+ list_del(&cmdinfo->work);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
+ spin_unlock_irq(&uas_lists_lock);
}
}
spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -288,10 +288,10 @@ static void uas_xfer_data(struct urb *urb, struct
scsi_cmnd *cmnd,
cmdinfo->state |= direction | SUBMIT_STATUS_URB;
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
if (err) {
- spin_lock(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
+ spin_lock(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
- spin_unlock(&uas_work_lock);
+ spin_unlock(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -694,10 +694,10 @@ static int uas_queuecommand_l
[PATCH 4/5] uas: add dead request list
This patch adds a new list where all requests which are canceled are
added to, so we don't loose them. Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.
Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 70 +--
1 file changed, 62 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index db09bda..2b3ca29 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,6 +78,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+ struct list_head dead;
};
/* I hate forward declarations, but I actually have a loop */
@@ -87,10 +88,12 @@ static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
static void uas_configure_endpoints(struct uas_dev_info *devinfo);
static void uas_free_streams(struct uas_dev_info *devinfo);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
static DECLARE_WORK(uas_work, uas_do_work);
static DEFINE_SPINLOCK(uas_lists_lock);
static LIST_HEAD(uas_work_list);
+static LIST_HEAD(uas_dead_list);
static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
struct uas_cmd_info *cmdinfo)
@@ -167,15 +170,13 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct uas_dev_info *di = (void *)cmnd->device->hostdata;
if (di == devinfo) {
+ uas_log_cmd_state(cmnd, __func__);
+ BUG_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->dead, &uas_dead_list);
+ spin_unlock_irq(&uas_lists_lock);
cmdinfo->state &= ~IS_IN_WORK_LIST;
- if (devinfo->resetting) {
- /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
- cmdinfo->state &= ~COMMAND_INFLIGHT;
- }
- uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
list_del(&cmdinfo->work);
@@ -187,6 +188,43 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
spin_unlock_irqrestore(&devinfo->lock, flags);
}
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+ struct uas_cmd_info *cmdinfo;
+ struct uas_cmd_info *temp;
+ struct list_head list;
+ unsigned long flags;
+
+ spin_lock_irq(&uas_lists_lock);
+ list_replace_init(&uas_dead_list, &list);
+ spin_unlock_irq(&uas_lists_lock);
+
+ spin_lock_irqsave(&devinfo->lock, flags);
+ list_for_each_entry_safe(cmdinfo, temp, &list, dead) {
+ struct scsi_pointer *scp = (void *)cmdinfo;
+ struct scsi_cmnd *cmnd = container_of(scp,
+ struct scsi_cmnd, SCp);
+ struct uas_dev_info *di = (void *)cmnd->device->hostdata;
+
+ if (di == devinfo) {
+ uas_log_cmd_state(cmnd, __func__);
+ BUG_ON(!(cmdinfo->state & COMMAND_ABORTED));
+ /* all urbs are killed, clear inflight bits */
+ cmdinfo->state &= ~(COMMAND_INFLIGHT |
+ DATA_IN_URB_INFLIGHT |
+ DATA_OUT_URB_INFLIGHT);
+ uas_try_complete(cmnd, __func__);
+ } else {
+ /* not our uas device, relink into list */
+ list_del(&cmdinfo->dead);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->dead, &uas_dead_list);
+ spin_unlock_irq(&uas_lists_lock);
+ }
+ }
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
{
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -274,6 +312,9 @@ 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");
cmnd->result = DID_ABORT <&l
[PATCH 2/5] uas: properly reinitialize in uas_eh_bus_reset_handler
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..fc08ee9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
static DECLARE_WORK(uas_work, uas_do_work);
static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+ uas_free_streams(devinfo);
err = usb_reset_device(udev);
+ if (!err)
+ uas_configure_endpoints(devinfo);
devinfo->resetting = 0;
if (err) {
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] uas: rename work list lock + list field
This patch prepares for the addition of another list and renames the
work list lock and the list_head field in struct uas_cmd_info.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 50 +++
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f89202f..a63972a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -77,7 +77,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
- struct list_head list;
+ struct list_head work;
};
/* I hate forward declarations, but I actually have a loop */
@@ -89,7 +89,7 @@ static void uas_configure_endpoints(struct uas_dev_info
*devinfo);
static void uas_free_streams(struct uas_dev_info *devinfo);
static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
+static DEFINE_SPINLOCK(uas_lists_lock);
static LIST_HEAD(uas_work_list);
static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
@@ -124,11 +124,11 @@ static void uas_do_work(struct work_struct *work)
unsigned long flags;
int err;
- spin_lock_irq(&uas_work_lock);
+ spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
+ spin_unlock_irq(&uas_lists_lock);
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -139,10 +139,10 @@ static void uas_do_work(struct work_struct *work)
cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
- list_del(&cmdinfo->list);
- spin_lock_irq(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
- spin_unlock_irq(&uas_work_lock);
+ list_del(&cmdinfo->work);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
+ spin_unlock_irq(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -155,12 +155,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct list_head list;
unsigned long flags;
- spin_lock_irq(&uas_work_lock);
+ spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
- spin_unlock_irq(&uas_work_lock);
+ spin_unlock_irq(&uas_lists_lock);
spin_lock_irqsave(&devinfo->lock, flags);
- list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+ list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -178,10 +178,10 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
- list_del(&cmdinfo->list);
- spin_lock_irq(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
- spin_unlock_irq(&uas_work_lock);
+ list_del(&cmdinfo->work);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
+ spin_unlock_irq(&uas_lists_lock);
}
}
spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -288,10 +288,10 @@ static void uas_xfer_data(struct urb *urb, struct
scsi_cmnd *cmnd,
cmdinfo->state |= direction | SUBMIT_STATUS_URB;
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
if (err) {
- spin_lock(&uas_work_lock);
- list_add_tail(&cmdinfo->list, &uas_work_list);
+ spin_lock(&uas_lists_lock);
+ list_add_tail(&cmdinfo->work, &uas_work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
- spin_unlock(&uas_work_lock);
+ spin_unlock(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -694,10 +694,10 @@ static int uas_queuecommand_l
[PATCH 2/5] uas: properly reinitialize in uas_eh_bus_reset_handler
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..f89202f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
static DECLARE_WORK(uas_work, uas_do_work);
static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,11 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+ uas_free_streams(devinfo);
err = usb_reset_device(udev);
+ if (!err) {
+ uas_configure_endpoints(devinfo);
+ }
devinfo->resetting = 0;
if (err) {
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] uas: add dead request list
This patch adds a new list where all requests which are canceled are
added to, so we don't loose them. Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.
Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.
Signed-off-by: Gerd Hoffmann
---
drivers/usb/storage/uas.c | 69 +--
1 file changed, 61 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a63972a..9dfb8f9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,6 +78,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+ struct list_head dead;
};
/* I hate forward declarations, but I actually have a loop */
@@ -87,10 +88,12 @@ static void uas_do_work(struct work_struct *work);
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
static void uas_configure_endpoints(struct uas_dev_info *devinfo);
static void uas_free_streams(struct uas_dev_info *devinfo);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
static DECLARE_WORK(uas_work, uas_do_work);
static DEFINE_SPINLOCK(uas_lists_lock);
static LIST_HEAD(uas_work_list);
+static LIST_HEAD(uas_dead_list);
static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
struct uas_cmd_info *cmdinfo)
@@ -167,15 +170,13 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct uas_dev_info *di = (void *)cmnd->device->hostdata;
if (di == devinfo) {
+ uas_log_cmd_state(cmnd, __func__);
+ BUG_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->dead, &uas_dead_list);
+ spin_unlock_irq(&uas_lists_lock);
cmdinfo->state &= ~IS_IN_WORK_LIST;
- if (devinfo->resetting) {
- /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
- cmdinfo->state &= ~COMMAND_INFLIGHT;
- }
- uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
list_del(&cmdinfo->work);
@@ -187,6 +188,43 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
spin_unlock_irqrestore(&devinfo->lock, flags);
}
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+ struct uas_cmd_info *cmdinfo;
+ struct uas_cmd_info *temp;
+ struct list_head list;
+ unsigned long flags;
+
+ spin_lock_irq(&uas_lists_lock);
+ list_replace_init(&uas_dead_list, &list);
+ spin_unlock_irq(&uas_lists_lock);
+
+ spin_lock_irqsave(&devinfo->lock, flags);
+ list_for_each_entry_safe(cmdinfo, temp, &list, dead) {
+ struct scsi_pointer *scp = (void *)cmdinfo;
+ struct scsi_cmnd *cmnd = container_of(scp,
+ struct scsi_cmnd, SCp);
+ struct uas_dev_info *di = (void *)cmnd->device->hostdata;
+
+ if (di == devinfo) {
+ uas_log_cmd_state(cmnd, __func__);
+ BUG_ON(!(cmdinfo->state & COMMAND_ABORTED));
+ /* all urbs are killed, clear inflight bits */
+ cmdinfo->state &= ~(COMMAND_INFLIGHT |
+ DATA_IN_URB_INFLIGHT |
+ DATA_OUT_URB_INFLIGHT);
+ uas_try_complete(cmnd, __func__);
+ } else {
+ /* not our uas device, relink into list */
+ list_del(&cmdinfo->dead);
+ spin_lock_irq(&uas_lists_lock);
+ list_add_tail(&cmdinfo->dead, &uas_dead_list);
+ spin_unlock_irq(&uas_lists_lock);
+ }
+ }
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
{
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -274,6 +312,9 @@ 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");
cmnd->result = DID_ABORT <&l
