Hi,
On 16-01-18 10:57, Oliver Neukum wrote:
Hi,
looking at your last patch I noticed something.
I think it fails to work if we hit a peculiar race condition.
It looks to me like we can get the following:
task A
pre_reset()
--> calling scsi_block_requests()
task B
shutdown()
--> setting the shutdown flag
post_reset()
--> checking the flag -> do nothing -> deadlock
So we need in addition something like the below patch.
What do you think?
Interesting theory, I guess you're right that we've a race,
but it seems to me that the proper fix would be to call
usb_lock_device_for_reset() from the shutdown callback,
something which the usb_device_reset docs say we should
have done from day 1...
Note please test this actually works before upstreaming this :)
Also at the point shutdown runs all block queues have been
flushed (including the on disk caches) and no new block io
requests can get queued, so keeping the queue blocked if we
hit the race should not be a problem.
Regards,
Hans
Regards
Oliver
From 68cc8940d7dde8e2a48217d8086cd692809e7980 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
Date: Mon, 15 Jan 2018 13:56:59 +0100
Subject: [PATCH] UAS: fix race between post_reset() and shutdown()
Disconnect due to shutdown can happen after pre_reset() has been called. Thus the host
must be unblocked even in the shutdown case.
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/storage/uas.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 338133a239df..d5ed1e581fb5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1046,10 +1046,11 @@ 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;
+ int err = 1;
+ /* We may have lost the race and shutdown after pre_reset() */
if (devinfo->shutdown)
- return 0;
+ goto skip;
err = uas_configure_endpoints(devinfo);
if (err && err != ENODEV)
@@ -1062,6 +1063,7 @@ static int uas_post_reset(struct usb_interface *intf)
scsi_report_bus_reset(shost, 0);
spin_unlock_irqrestore(shost->host_lock, flags);
+skip:
scsi_unblock_requests(shost);
return err ? 1 : 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
