[PATCH] usb_storage: make usb-stor-scan task non-freezable

[PATCH] usb_storage: make usb-stor-scan task non-freezable

[Seth Forshee]

The following patch is in response to a consistently reproducible
failure to freeze tasks prior to restoring a hibernation image on a
Toshiba NB505 netbook. This machine has a built-in USB card reader.
Since the usb-stor-scan task is freezable but the code in
quiesce_and_remove_host() that waits for scanning to complete is not,
khubd can fail to freeze when processing the disconnect for the card
reader.

It seems that both should either be freezable or not freezable. Since
there doesn't currently seem to be any freezable way to wait on a
completion, I started with the simpler approach of making usb-stor-scan
non-freezable. If it would be preferable to make both freezable I can
take that approach instead.

Thanks,
Seth


>From 27e456c0d6512ba599f7560139555374a45342be Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Wed, 13 Jul 2011 23:02:48 -0500
Subject: [PATCH] usb_storage: make usb-stor-scan task non-freezable

The usb-stor-scan task is freezable, but the code in
quiesce_and_remove_host() that waits for scanning to complete is
not. This can cause task freezing to fail, which is happening
consistently on at least one model of netbook with a built-in USB
card reader when freezing tasks prior to restoring a hibernation
image.

This patch makes the usb-stor-scan task non-freezable to prevent
this from happening. Tested across 200 S4 cycles without a single
failure to freeze tasks.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/usb/storage/usb.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 0ca0958..f59fc90 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -831,12 +831,11 @@ static int usb_stor_scan_thread(void * __us)
 
 	dev_dbg(dev, "device found\n");
 
-	set_freezable();
 	/* Wait for the timeout to expire or for a disconnect */
 	if (delay_use > 0) {
 		dev_dbg(dev, "waiting for device to settle "
 				"before scanning\n");
-		wait_event_freezable_timeout(us->delay_wait,
+		wait_event_interruptible_timeout(us->delay_wait,
 				test_bit(US_FLIDX_DONT_SCAN, &us->dflags),
 				delay_use * HZ);
 	}
-- 
1.7.4.1

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Alan Stern]

On Mon, 18 Jul 2011, Seth Forshee wrote:

> The following patch is in response to a consistently reproducible
> failure to freeze tasks prior to restoring a hibernation image on a
> Toshiba NB505 netbook. This machine has a built-in USB card reader.
> Since the usb-stor-scan task is freezable but the code in
> quiesce_and_remove_host() that waits for scanning to complete is not,
> khubd can fail to freeze when processing the disconnect for the card
> reader.

What card-reader disconnect?

> It seems that both should either be freezable or not freezable. Since
> there doesn't currently seem to be any freezable way to wait on a
> completion, I started with the simpler approach of making usb-stor-scan
> non-freezable. If it would be preferable to make both freezable I can
> take that approach instead.

I'm not sure what the best approach is.  usb-stor-scan has to be
freezable, because the scanning code registers new child device
structures, which isn't allowed during suspend or hibernation.

Alan Stern

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Seth Forshee]

On Mon, Jul 18, 2011 at 05:12:35PM -0400, Alan Stern wrote:
> On Mon, 18 Jul 2011, Seth Forshee wrote:
> 
> > The following patch is in response to a consistently reproducible
> > failure to freeze tasks prior to restoring a hibernation image on a
> > Toshiba NB505 netbook. This machine has a built-in USB card reader.
> > Since the usb-stor-scan task is freezable but the code in
> > quiesce_and_remove_host() that waits for scanning to complete is not,
> > khubd can fail to freeze when processing the disconnect for the card
> > reader.
> 
> What card-reader disconnect?

The call trace (below) shows that the code is processing a device
disconnection when this happens. I don't know what triggers it. I take
it from your response that this isn't expected (sorry, I'm not really
all that familiar with USB)?

[   22.730125] Freezing of tasks failed after 20.01 seconds (1 tasks refusing to freeze, wq_busy=0):
[   22.730159] khubd           D 0000000000000000     0    20      2 0x00800000
[   22.730174]  ffff88003c1bda20 0000000000000046 ffffffff8183f119 ffff88003c1bd9e6
[   22.730187]  ffff88003c1bdfd8 ffff88003c1bdfd8 ffff88003c1bdfd8 0000000000012a40
[   22.730199]  ffff88003ce85bc0 ffff88003ca8c4d0 00000000000003f4 7fffffffffffffff
[   22.730212] Call Trace:
[   22.730232]  [<ffffffff815ed985>] schedule_timeout+0x2a5/0x320
[   22.730247]  [<ffffffff81151564>] ? kmem_cache_free+0x114/0x120
[   22.730258]  [<ffffffff815ef21e>] ? _raw_spin_lock+0xe/0x20
[   22.730270]  [<ffffffff815ed3ef>] wait_for_common+0xdf/0x180
[   22.730282]  [<ffffffff81056f20>] ? try_to_wake_up+0x200/0x200
[   22.730294]  [<ffffffff815ed56d>] wait_for_completion+0x1d/0x20
[   22.730308]  [<ffffffffa0031765>] quiesce_and_remove_host+0x65/0xc0 [usb_storage]
[   22.730319]  [<ffffffffa00317e2>] usb_stor_disconnect+0x22/0x40 [usb_storage]
[   22.730329]  [<ffffffff8144a7e2>] usb_unbind_interface+0x52/0x180
[   22.730338]  [<ffffffff813c406c>] __device_release_driver+0x7c/0xe0
[   22.730346]  [<ffffffff813c40fc>] device_release_driver+0x2c/0x40
[   22.730353]  [<ffffffff813c3ba8>] bus_remove_device+0x78/0xb0
[   22.730362]  [<ffffffff813c118d>] device_del+0x12d/0x1b0
[   22.730370]  [<ffffffff8144851f>] usb_disable_device+0xaf/0x1d0
[   22.730378]  [<ffffffff81441320>] usb_disconnect+0xa0/0x140
[   22.730386]  [<ffffffff81442120>] hub_port_connect_change+0xa0/0x6e0
[   22.730394]  [<ffffffff81447a37>] ? usb_control_msg+0xf7/0x120
[   22.730402]  [<ffffffff81442c14>] hub_events+0x4b4/0x610
[   22.730410]  [<ffffffff81442da5>] hub_thread+0x35/0x180
[   22.730419]  [<ffffffff810815f0>] ? add_wait_queue+0x60/0x60
[   22.730426]  [<ffffffff81442d70>] ? hub_events+0x610/0x610
[   22.730434]  [<ffffffff81080b4c>] kthread+0x8c/0xa0
[   22.730442]  [<ffffffff815f8724>] kernel_thread_helper+0x4/0x10
[   22.730451]  [<ffffffff81080ac0>] ? flush_kthread_worker+0xa0/0xa0
[   22.730458]  [<ffffffff815f8720>] ? gs_change+0x13/0x13

> > It seems that both should either be freezable or not freezable. Since
> > there doesn't currently seem to be any freezable way to wait on a
> > completion, I started with the simpler approach of making usb-stor-scan
> > non-freezable. If it would be preferable to make both freezable I can
> > take that approach instead.
> 
> I'm not sure what the best approach is.  usb-stor-scan has to be
> freezable, because the scanning code registers new child device
> structures, which isn't allowed during suspend or hibernation.

Unless there's some way to ensure we won't wait on scanning during
freezing, I don't really see any option besides making the wait
freezable.

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Alan Stern]

On Mon, 18 Jul 2011, Seth Forshee wrote:

> On Mon, Jul 18, 2011 at 05:12:35PM -0400, Alan Stern wrote:
> > On Mon, 18 Jul 2011, Seth Forshee wrote:
> > 
> > > The following patch is in response to a consistently reproducible
> > > failure to freeze tasks prior to restoring a hibernation image on a
> > > Toshiba NB505 netbook. This machine has a built-in USB card reader.
> > > Since the usb-stor-scan task is freezable but the code in
> > > quiesce_and_remove_host() that waits for scanning to complete is not,
> > > khubd can fail to freeze when processing the disconnect for the card
> > > reader.
> > 
> > What card-reader disconnect?
> 
> The call trace (below) shows that the code is processing a device
> disconnection when this happens. I don't know what triggers it. I take
> it from your response that this isn't expected (sorry, I'm not really
> all that familiar with USB)?

But why is there a disconnect at this time?  Maybe you could find out 
if you collect the kernel log from the boot kernel (serial console or 
network console).

It appears that you had the bad luck to begin restoring from
hibernation just after the card reader was disconnected!  If the
restore had been started a few seconds later instead, after the
disconnect processing was finished, it would have worked.

> > > It seems that both should either be freezable or not freezable. Since
> > > there doesn't currently seem to be any freezable way to wait on a
> > > completion, I started with the simpler approach of making usb-stor-scan
> > > non-freezable. If it would be preferable to make both freezable I can
> > > take that approach instead.

BTW, I didn't say anything before but this is wrong -- both threads
already _are_ freezable.  The problem is that the khubd thread has to
wait for the usb-stor-scan thread to finish before it can be frozen,
and this doesn't work if usb-stor-scan has already been frozen.

> > I'm not sure what the best approach is.  usb-stor-scan has to be
> > freezable, because the scanning code registers new child device
> > structures, which isn't allowed during suspend or hibernation.
> 
> Unless there's some way to ensure we won't wait on scanning during
> freezing, I don't really see any option besides making the wait
> freezable.

Maybe it would be better to come up with a freezable version of 
wait_for_completion().  You should ask for advice on the linux-pm 
mailing list.

Alan Stern

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Seth Forshee]

On Tue, Jul 19, 2011 at 10:26:39AM -0400, Alan Stern wrote:
> On Mon, 18 Jul 2011, Seth Forshee wrote:
> 
> > On Mon, Jul 18, 2011 at 05:12:35PM -0400, Alan Stern wrote:
> > > On Mon, 18 Jul 2011, Seth Forshee wrote:
> > > 
> > > > The following patch is in response to a consistently reproducible
> > > > failure to freeze tasks prior to restoring a hibernation image on a
> > > > Toshiba NB505 netbook. This machine has a built-in USB card reader.
> > > > Since the usb-stor-scan task is freezable but the code in
> > > > quiesce_and_remove_host() that waits for scanning to complete is not,
> > > > khubd can fail to freeze when processing the disconnect for the card
> > > > reader.
> > > 
> > > What card-reader disconnect?
> > 
> > The call trace (below) shows that the code is processing a device
> > disconnection when this happens. I don't know what triggers it. I take
> > it from your response that this isn't expected (sorry, I'm not really
> > all that familiar with USB)?
> 
> But why is there a disconnect at this time?  Maybe you could find out 
> if you collect the kernel log from the boot kernel (serial console or 
> network console).

I'm not sure which kernel you mean by 'boot' kernel. The machine doesn't
have a serial port, and network console seems to stop too early to get
any interesting data during hibernation. I took photographs of the
console on the LCD as the machine hibernated, but I didn't really see
anything interesting there.

Getting logs after the failed restore is easy though, and that log looks
more interesting. There are some errors, followed later by an status
change event showing that the PORT_CONNECT and PORT_PE bits have been
cleared. Here's the relevant secton of the log. These errors and the
disconnect don't appear in a normal boot.

[    1.900069] usb 1-8: new high speed USB device number 3 using ehci_hcd
[    1.960234] ehci_hcd 0000:00:1d.7: port 8 high speed
[    1.960247] ehci_hcd 0000:00:1d.7: GetStatus port:8 status 001005 0  ACK POWER sig=se0 PE CONNECT
[    1.960802]  sda: sda1 sda2 sda3 sda4 < sda5 sda6 >
[    1.962122] sd 0:0:0:0: [sda] Attached SCSI disk
[    1.964697] Initializing USB Mass Storage driver...
[    1.964827] usb-storage 1-4:1.0: usb_probe_interface
[    1.964852] usb-storage 1-4:1.0: usb_probe_interface - got id
[    1.964922] usbcore: registered new interface driver usb-storage
[    1.964930] USB Mass Storage support registered.
[    1.967261] ums-realtek 1-4:1.0: usb_probe_interface
[    1.967274] ums-realtek 1-4:1.0: usb_probe_interface - got id
[    1.984640] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 1
[    1.984739] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 2
[    1.984871] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 3
[    1.984995] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 4
[    1.985120] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 5
[    1.985246] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 6
[    1.985370] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 7
[    1.985496] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 8
[    1.985619] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 9
[    1.985746] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 10
[    1.985869] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 11
[    1.985996] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 12
[    1.986120] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 13
[    1.986246] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 14
[    1.986371] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 15
[    1.986496] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 16
[    1.986620] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 17
[    1.986746] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 18
[    1.986870] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 19
[    1.986997] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 20
[    1.987120] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 21
[    1.987246] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 22
[    1.987370] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 23
[    1.987496] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 24
[    1.987620] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 25
[    1.987746] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 26
[    1.987870] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 27
[    1.987996] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 28
[    1.988120] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 29
[    1.988246] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 30
[    1.988370] ehci_hcd 0000:00:1d.7: detected XactErr len 0/13 retry 31
[    1.988496] ehci_hcd 0000:00:1d.7: devpath 4 ep2in 3strikes
[    1.988635] scsi4 : usb-storage 1-4:1.0
[    1.988983] usbcore: registered new interface driver ums-realtek
[    2.290054] usb usb2: suspend_rh (auto-stop)
[    2.290086] usb usb4: suspend_rh (auto-stop)
[    2.290112] usb usb3: suspend_rh (auto-stop)
[    2.290138] usb usb5: suspend_rh (auto-stop)
[    2.527488] PM: Starting manual resume from disk
[    2.527498] PM: Hibernation image partition 8:6 present
[    2.527503] PM: Looking for hibernation image.
[    2.614099] PM: Image signature found, resuming
[    2.615567] PM: Marking nosave pages: 000000000009d000 - 0000000000100000
[    2.615580] PM: Basic memory bitmaps created
[    2.615586] PM: Preparing processes for restore.
[    2.615591] Freezing user space processes ... (elapsed 0.01 seconds) done.
[    2.630126] Freezing remaining freezable tasks ... 
[    2.899934] usb 1-8: skipped 1 descriptor after configuration
[    2.899947] usb 1-8: skipped 5 descriptors after interface
[    2.899959] usb 1-8: skipped 1 descriptor after endpoint
[    2.899973] usb 1-8: skipped 17 descriptors after interface
[    2.901022] usb 1-8: default language 0x0409
[    2.911410] usb 1-8: udev 3, busnum 1, minor = 2
[    2.911422] usb 1-8: New USB device found, idVendor=0bda, idProduct=5801
[    2.911432] usb 1-8: New USB device strings: Mfr=3, Product=1, SerialNumber=2
[    2.911443] usb 1-8: Product: USB Camera
[    2.911451] usb 1-8: Manufacturer: XTB5SFGS
[    2.911459] usb 1-8: SerialNumber: 200901010001
[    2.911676] usb 1-8: usb_probe_device
[    2.911685] usb 1-8: configuration #1 chosen from 1 choice
[    2.916408] usb 1-8: adding 1-8:1.0 (config #1, interface 0)
[    2.917762] usb 1-8: adding 1-8:1.1 (config #1, interface 1)
[    2.917874] hub 3-0:1.0: state 7 ports 2 chg 0000 evt 0000
[    2.917893] hub 5-0:1.0: state 7 ports 2 chg 0000 evt 0000
[    2.917902] hub 1-0:1.0: state 7 ports 8 chg 0000 evt 0010
[    2.917919] ehci_hcd 0000:00:1d.7: GetStatus port:4 status 001002 0  ACK POWER sig=se0 CSC
[    2.917935] hub 1-0:1.0: port 4, status 0100, change 0001, 12 Mb/s
[    2.917943] usb 1-4: USB disconnect, device number 2
[    2.917948] usb 1-4: unregistering device
[    2.917954] usb 1-4: unregistering interface 1-4:1.0
[   22.650106] 
[   22.650122] Freezing of tasks failed after 20.01 seconds (1 tasks refusing to freeze, wq_busy=0):

> It appears that you had the bad luck to begin restoring from
> hibernation just after the card reader was disconnected!  If the
> restore had been started a few seconds later instead, after the
> disconnect processing was finished, it would have worked.

It's definitely a case of unfortunate timing.

> > > > It seems that both should either be freezable or not freezable. Since
> > > > there doesn't currently seem to be any freezable way to wait on a
> > > > completion, I started with the simpler approach of making usb-stor-scan
> > > > non-freezable. If it would be preferable to make both freezable I can
> > > > take that approach instead.
> 
> BTW, I didn't say anything before but this is wrong -- both threads
> already _are_ freezable.  The problem is that the khubd thread has to
> wait for the usb-stor-scan thread to finish before it can be frozen,
> and this doesn't work if usb-stor-scan has already been frozen.

Right, what I meant was to make the wait on the completion freezable. I
should have been clearer.

> > > I'm not sure what the best approach is.  usb-stor-scan has to be
> > > freezable, because the scanning code registers new child device
> > > structures, which isn't allowed during suspend or hibernation.
> > 
> > Unless there's some way to ensure we won't wait on scanning during
> > freezing, I don't really see any option besides making the wait
> > freezable.
> 
> Maybe it would be better to come up with a freezable version of 
> wait_for_completion().  You should ask for advice on the linux-pm 
> mailing list.

That's what I'm planning to look into as soon as I get a chance.

Thanks for your help.

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Seth Forshee]

On Tue, Jul 19, 2011 at 10:26:39AM -0400, Alan Stern wrote:
> On Mon, 18 Jul 2011, Seth Forshee wrote:
> 
> > On Mon, Jul 18, 2011 at 05:12:35PM -0400, Alan Stern wrote:
> > > On Mon, 18 Jul 2011, Seth Forshee wrote:
> > > 
> > > > The following patch is in response to a consistently reproducible
> > > > failure to freeze tasks prior to restoring a hibernation image on a
> > > > Toshiba NB505 netbook. This machine has a built-in USB card reader.
> > > > Since the usb-stor-scan task is freezable but the code in
> > > > quiesce_and_remove_host() that waits for scanning to complete is not,
> > > > khubd can fail to freeze when processing the disconnect for the card
> > > > reader.
> > > 
> > > What card-reader disconnect?
> > 
> > The call trace (below) shows that the code is processing a device
> > disconnection when this happens. I don't know what triggers it. I take
> > it from your response that this isn't expected (sorry, I'm not really
> > all that familiar with USB)?
> 
> But why is there a disconnect at this time?  Maybe you could find out 
> if you collect the kernel log from the boot kernel (serial console or 
> network console).

After experimenting with this device more I came to the conclusion that
the normal behavior with this machine is for the card reader to be
disconnected from the USB bus unless there's a card in the slot. During
a normal boot with an empty card slot the card reader never shows up on
the bus. But for some reason after S4 it shows up, but then some
transaction errors happen due to no card being present (I can trigger
the same errors by quickly inserting and removing a card) and the device
is disconnected. This is only after S4 though -- if I set the
hibernation mode to reboot or shutdown the errors don't happen. And I do
not see the errors if I hibernate and then boot up with noresume, so
it's not something that happens as a result of the restore process.

So at this point I'm pretty convinced that this is some kind of firmware
issue and that there isn't much hope of fixing it. It's not even
something that can be fixed by patching the AML, as no GPEs or AML
methods are coming into play when all this happens.

> Maybe it would be better to come up with a freezable version of 
> wait_for_completion().  You should ask for advice on the linux-pm 
> mailing list.

I tried this, and it doesn't work. Well, it works in the sense of
freezing khubd, but then khubd is frozen holding the mutex for the
device, and the restore hangs later while trying to suspend devices.

The only solution I've come up with is to leave usb-stor-scan freezable
without allowing it to actually freeze. We can request a fake signal be
sent when freezing and use interruptible sleep to abort the wait early
and finish up the thread's processing. This is implemented in the patch
below. Does this approach look reasonable? It's rather subtle, but it
does seem to work. I done numerous S4 cycles with and without a card
inserted and didn't get any failures.

Thanks,
Seth


>From a80c164b558ae4bceabda8c3f25d9dcafe772727 Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Fri, 22 Jul 2011 16:39:31 -0500
Subject: [PATCH] usb_storage: Don't freeze in usb-stor-scan

Scanning cannot be run during suspend or hibernation, but if
usb-stor-scan freezes another thread waiting on scanning to
complete may fail to freeze.

However, if usb-stor-scan is left freezable without ever actually
freezing then the freezer will wait on it to exit, and threads
waiting for scanning to finish will no longer be blocked. One
problem with this approach is that usb-stor-scan has a delay to
wait for devices to settle (which is currently the only point where
it can freeze). To work around this we can request that the freezer
send a fake signal when freezing, then use interruptible sleep to
wake the thread early when freezing happens.

To make this happen, the following changes are made to
usb-stor-scan:

 * Use set_freezable_with_signal() instead of set_freezable() to
   request a fake signal when freezing

 * Use wait_event_interruptible_timeout() instead of
   wait_event_freezable_timeout() to avoid freezing

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/usb/storage/usb.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 0ca0958..c325e69 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -831,12 +831,22 @@ static int usb_stor_scan_thread(void * __us)
 
 	dev_dbg(dev, "device found\n");
 
-	set_freezable();
-	/* Wait for the timeout to expire or for a disconnect */
+	set_freezable_with_signal();
+	/*
+	 * Wait for the timeout to expire or for a disconnect
+	 *
+	 * We can't freeze in this thread or we risk causing khubd to
+	 * fail to freeze, but we can't be non-freezable either. Nor can
+	 * khubd freeze while waiting for scanning to complete as it may
+	 * hold the device lock, causing a hang when suspending devices.
+	 * So we request a fake signal when freezing and use
+	 * interruptible sleep to kick us out of our wait early when
+	 * freezing happens.
+	 */
 	if (delay_use > 0) {
 		dev_dbg(dev, "waiting for device to settle "
 				"before scanning\n");
-		wait_event_freezable_timeout(us->delay_wait,
+		wait_event_interruptible_timeout(us->delay_wait,
 				test_bit(US_FLIDX_DONT_SCAN, &us->dflags),
 				delay_use * HZ);
 	}
-- 
1.7.4.1

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Alan Stern]

On Sat, 23 Jul 2011, Seth Forshee wrote:

> After experimenting with this device more I came to the conclusion that
> the normal behavior with this machine is for the card reader to be
> disconnected from the USB bus unless there's a card in the slot. During
> a normal boot with an empty card slot the card reader never shows up on
> the bus.

Weird.  Then the reader would never be usable.  Unless it connects 
itself to the bus when a card is inserted?

>  But for some reason after S4 it shows up, but then some
> transaction errors happen due to no card being present (I can trigger
> the same errors by quickly inserting and removing a card) and the device
> is disconnected. This is only after S4 though -- if I set the
> hibernation mode to reboot or shutdown the errors don't happen. And I do
> not see the errors if I hibernate and then boot up with noresume, so
> it's not something that happens as a result of the restore process.
> 
> So at this point I'm pretty convinced that this is some kind of firmware
> issue and that there isn't much hope of fixing it. It's not even
> something that can be fixed by patching the AML, as no GPEs or AML
> methods are coming into play when all this happens.
> 
> > Maybe it would be better to come up with a freezable version of 
> > wait_for_completion().  You should ask for advice on the linux-pm 
> > mailing list.
> 
> I tried this, and it doesn't work. Well, it works in the sense of
> freezing khubd, but then khubd is frozen holding the mutex for the
> device, and the restore hangs later while trying to suspend devices.

Ah yes.  So much for that idea.

> The only solution I've come up with is to leave usb-stor-scan freezable
> without allowing it to actually freeze. We can request a fake signal be
> sent when freezing and use interruptible sleep to abort the wait early
> and finish up the thread's processing. This is implemented in the patch
> below. Does this approach look reasonable? It's rather subtle, but it
> does seem to work. I done numerous S4 cycles with and without a card
> inserted and didn't get any failures.

This runs the risk of failing to suspend if scanning takes too long.  

On the other hand, many systems nowadays use async scanning anyway.
And that combination of events isn't too likely to happen, whereas 
you're facing a real problem right now.  So I guess this is okay.

Alan Stern

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Seth Forshee]

On Sat, Jul 23, 2011 at 09:56:15PM -0400, Alan Stern wrote:
> On Sat, 23 Jul 2011, Seth Forshee wrote:
> 
> > After experimenting with this device more I came to the conclusion that
> > the normal behavior with this machine is for the card reader to be
> > disconnected from the USB bus unless there's a card in the slot. During
> > a normal boot with an empty card slot the card reader never shows up on
> > the bus.
> 
> Weird.  Then the reader would never be usable.  Unless it connects 
> itself to the bus when a card is inserted?

Yes, the reader is connected to the bus when a card is inserted. Remove
the card and it disappears again.

> > The only solution I've come up with is to leave usb-stor-scan freezable
> > without allowing it to actually freeze. We can request a fake signal be
> > sent when freezing and use interruptible sleep to abort the wait early
> > and finish up the thread's processing. This is implemented in the patch
> > below. Does this approach look reasonable? It's rather subtle, but it
> > does seem to work. I done numerous S4 cycles with and without a card
> > inserted and didn't get any failures.
> 
> This runs the risk of failing to suspend if scanning takes too long.  

It may increase the exposure to that risk, but really the risk is
already there.

> On the other hand, many systems nowadays use async scanning anyway.
> And that combination of events isn't too likely to happen, whereas 
> you're facing a real problem right now.  So I guess this is okay.

Great, thanks.

Will the patch get picked up from my previous email, or do I need to
resend it?

Thanks,
Seth

Re: [PATCH] usb_storage: make usb-stor-scan task non-freezable

[Alan Stern]

On Tue, 26 Jul 2011, Seth Forshee wrote:

> On Sat, Jul 23, 2011 at 09:56:15PM -0400, Alan Stern wrote:
> > On Sat, 23 Jul 2011, Seth Forshee wrote:
> > 
> > > After experimenting with this device more I came to the conclusion that
> > > the normal behavior with this machine is for the card reader to be
> > > disconnected from the USB bus unless there's a card in the slot. During
> > > a normal boot with an empty card slot the card reader never shows up on
> > > the bus.
> > 
> > Weird.  Then the reader would never be usable.  Unless it connects 
> > itself to the bus when a card is inserted?
> 
> Yes, the reader is connected to the bus when a card is inserted. Remove
> the card and it disappears again.

That's the first time I've heard of such a thing.

> > > The only solution I've come up with is to leave usb-stor-scan freezable
> > > without allowing it to actually freeze. We can request a fake signal be
> > > sent when freezing and use interruptible sleep to abort the wait early
> > > and finish up the thread's processing. This is implemented in the patch
> > > below. Does this approach look reasonable? It's rather subtle, but it
> > > does seem to work. I done numerous S4 cycles with and without a card
> > > inserted and didn't get any failures.
> > 
> > This runs the risk of failing to suspend if scanning takes too long.  
> 
> It may increase the exposure to that risk, but really the risk is
> already there.

True.  Once the scanning begins there's no way to freeze the thread, 
even in the current code.

> > On the other hand, many systems nowadays use async scanning anyway.
> > And that combination of events isn't too likely to happen, whereas 
> > you're facing a real problem right now.  So I guess this is okay.
> 
> Great, thanks.
> 
> Will the patch get picked up from my previous email, or do I need to
> resend it?

You should resend it to Greg, and you can add

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern