OHCI problems with suspend/resume

Hi!

In 2.6.0-test1, OHCI is non-functional after first suspend/resume, and kills machine during secon suspend/resume cycle.

What happens is that ohci_irq gets ohci->hcca == NULL, and kills machine. Why is ohci->hcca == NULL? ohci_stop was called from hcd_panic() and freed ohci->hcca.

I believe that we should

1) not free ohci->hcca so that system has better chance surviving hcd_panic()

and

2) inform user when hcd panics.

This patch does both, please apply,

[Okay, you probably want to kill the ifdefs, and kill the code, leaving just a comment...]
Pavel

@@ -1486,6 +1486,7 @@
 static void hcd_panic (void *_hcd)
 {
 	struct usb_hcd *hcd = _hcd;
+	printk(KERN_CRIT "Host controller panicked\n");
 	hcd->driver->stop (hcd);
 }
 

@@ -627,12 +632,18 @@
 	
 	remove_debug_files (ohci);
 	ohci_mem_cleanup (ohci);
+#if 0
+	/* Freeing hcca at this point is bad idea, because ohci_irq
+	   can't cope with ohci->hcca being NULL, and therefore will
+	   crash the machine after hcd_panic()
+	 */
 	if (ohci->hcca) {
 		pci_free_consistent (ohci->hcd.pdev, sizeof *ohci->hcca,
 					ohci->hcca, ohci->hcca_dma);
 		ohci->hcca = NULL;
 		ohci->hcca_dma = 0;
 	}
+#endif
 }
 
 /*-------------------------------------------------------------------------*/

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Hmm, last time I tested suspend/resume it worked fine. That was 2.5.67, but the OHCI code hasn't had any relevant changes since then.

Evidently your system used different suspend/resume paths than mine did ... :)

Then the problem is that an IRQ is still coming in after the HCD panicked.

Not ever????

It's freed in exactly one place, after everything should be shut down. If it wasn't shut down, that was the problem.

Could you instead figure out why it wasn't shut down?

The OHCI code does that already on the normal panic path (controller delivers a Unrecoverable Error interrupt), but you're right that this would be better as a generic KERN_CRIT diagnostic. (But one saying which HCD panicked, rather than leaving folk to guess which of the N it applied to...) And I'd print that message sooner, not waiting for that task to be scheduled.

I'll merge that fix in with one I have in my queue.

- Dave

Hi!

Can you try echo 4 > /proc/acpi/sleep? echo 3 breaks it, too, but that is little harder to set up.

Actually, as PCI interrupts are shared, I do not find that too surprising.

In case of hcd_panic(), where is interrupt deallocated?

That would be good. I definitely had another failure path, where it did not tell me that hcd is no K.O...

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Good morning to you too!

I usually test with "apm -s" ... since I've yet to come up with an ACPI configuration that works properly. IRQ misconfiguration for USB is still a blocking issue for many people (not just me).

Going through ACPI would certainly explain some breakage; it's been sufficiently troublesome with USB that it's not gotten much testing at all. I happened to notice this morning that ACPI's USB IRQ problems are one of the longest-standing open 2.6 bugs:
http://bugme.osdl.org/show_bug.cgi?id=10 ... and it's now been migrated into the 2.4.22-pre series (sigh).

Could you try reproducing this failure using just APM? I could believe there's a generic PM issue (I've been expecting 2.6-test to eventually start shaking PM out); but given the amount of trouble ACPI has caused, we should first rule that factor out.

I do. Sharing is irrelevant. If it's been cleaned up, then the IRQ should no longer be bound to that device.

I'll have to check that out, but I noticed that one of my usual development machines (on which suspend/resume can actually be made to work) is now unusable due to some PCI initialization issue with Cardbus.

I'll likely submit that to Greg in the next few days, cc you.

- Dave

Hi!

Actually, you do not need acpi to test swsusp (echo 4 > /proc/acpi/sleep is the easiest way, but there's syscall to do the same).

That's okay, ACPI in 2.4.22-pre just replaced older ACPI and even-more-buggy ACPI in 2.4.21.

ACPI is not much involved. If you want, I can (attempt to) make swsusp work on your machine....

ohci_stop() does not seem to unregister IRQ...

Okay, thanx.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

I experience similar problems with UHCI:

Before suspending to S3 I unload usb modules and afterwards I reload them, but it does not work again. (For testing I also plugged in an USB card reader with an LED.
The LED is on except when I resume from S3)

Here is some dmesg output

After booting:

drivers/usb/core/usb.c: registered new driver usbfs drivers/usb/core/usb.c: registered new driver hub drivers/usb/host/uhci-hcd.c: USB Universal Host Controller Interface driver v2.1 uhci-hcd 0000:00:11.2: VIA Technologies, In USB
uhci-hcd 0000:00:11.2: irq 10, io base 0000dc00
uhci-hcd 0000:00:11.2: new USB bus registered, assigned bus number 1 drivers/usb/host/uhci-hcd.c: detected 2 ports
uhci-hcd 0000:00:11.2: root hub device address 1
usb usb1: new device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: VIA Technologies, In USB
usb usb1: Manufacturer: Linux 2.6.0-test1-bk2 uhci-hcd
usb usb1: SerialNumber: 0000:00:11.2
drivers/usb/core/usb.c: usb_hotplug
usb usb1: usb_new_device - registering interface 1-0:0 drivers/usb/core/usb.c: usb_hotplug
hub 1-0:0: usb_device_probe
hub 1-0:0: usb_device_probe - got id
hub 1-0:0: USB hub found
hub 1-0:0: 2 ports detected
hub 1-0:0: standalone hub
hub 1-0:0: ganged power switching
hub 1-0:0: global over-current protection
hub 1-0:0: Port indicators are not supported
hub 1-0:0: power on to power good time: 2ms
hub 1-0:0: hub controller current requirement: 0mA
hub 1-0:0: local power source is good
hub 1-0:0: no over-current condition exists
hub 1-0:0: enabling power on all ports
hub 1-0:0: port 1, status 100, change 3, 12 Mb/s
hub 1-0:0: port 2, status 301, change 3, 1.5 Mb/s
hub 1-0:0: debounce: port 2: delay 100ms stable 4 status 0x301
hub 1-0:0: new USB device on port 2, assigned address 2
usb 1-2: new device strings: Mfr=1, Product=2, SerialNumber=0
usb 1-2: Product: USB-PS/2 Optical Mouse
usb 1-2: Manufacturer: Logitech
drivers/usb/core/usb.c: usb_hotplug
usb 1-2: usb_new_device - registering interface 1-2:0 drivers/usb/core/usb.c: usb_hotplug
hub 1-0:0: port 1 enable change, status 100
hid 1-2:0: usb_device_probe
hid 1-2:0: usb_device_probe - got id
input: USB HID v1.10 Mouse [Logitech USB-PS/2 Optical Mouse] on usb-0000:00:11.2-2 drivers/usb/core/usb.c: registered new driver hid drivers/usb/input/hid-core.c: v2.0:USB HID core driver

rmmod uhci_hcd hid usbcore:

uhci-hcd 0000:00:11.2: remove, state 3
uhci-hcd 0000:00:11.2: roothub graceful disconnect
usb usb1: USB disconnect, address 1
usb 1-2: USB disconnect, address 2
drivers/usb/core/usb.c: nuking urbs assigned to 1-2
usb 1-2: unregistering interfaces
drivers/usb/core/usb.c: nuking urbs assigned to 1-2
usb 1-2: hcd_unlink_urb df5047c0 fail -22
usb 1-2: hcd_unlink_urb df504740 fail -22 drivers/usb/core/usb.c: usb_hotplug
usb 1-2: unregistering device
drivers/usb/core/usb.c: usb_hotplug drivers/usb/core/usb.c: nuking urbs assigned to usb1 uhci-hcd 0000:00:11.2: shutdown urb df504940 pipe 40408180 ep1in-intr usb usb1: unregistering interfaces
drivers/usb/core/usb.c: nuking urbs assigned to usb1
usb usb1: hcd_unlink_urb df504940 fail -22 drivers/usb/core/usb.c: usb_hotplug
usb usb1: unregistering device
drivers/usb/core/usb.c: usb_hotplug
uhci-hcd 0000:00:11.2: USB bus 1 deregistered drivers/usb/core/usb.c: deregistering driver hid drivers/usb/core/usb.c: deregistering driver usbfs drivers/usb/core/hub.c: hub_thread exiting drivers/usb/core/usb.c: deregistering driver hub

echo 3 > /proc/acpi/sleep
modprobe usbcore uhci_hcd

(Sometimes the optical mouse blinks for a moment)

drivers/usb/core/usb.c: registered new driver usbfs drivers/usb/core/usb.c: registered new driver hub drivers/usb/host/uhci-hcd.c: USB Universal Host Controller Interface driver v2.1 uhci-hcd 0000:00:11.2: VIA Technologies, In USB
uhci-hcd 0000:00:11.2: irq 10, io base 0000dc00
uhci-hcd 0000:00:11.2: new USB bus registered, assigned bus number 1 drivers/usb/host/uhci-hcd.c: detected 2 ports
uhci-hcd 0000:00:11.2: root hub device address 1
usb usb1: new device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: VIA Technologies, In USB
usb usb1: Manufacturer: Linux 2.6.0-test1-bk2 uhci-hcd
usb usb1: SerialNumber: 0000:00:11.2
drivers/usb/core/usb.c: usb_hotplug
usb usb1: usb_new_device - registering interface 1-0:0 drivers/usb/core/usb.c: usb_hotplug
hub 1-0:0: usb_device_probe
hub 1-0:0: usb_device_probe - got id
hub 1-0:0: USB hub found
hub 1-0:0: 2 ports detected
hub 1-0:0: standalone hub
hub 1-0:0: ganged power switching
hub 1-0:0: global over-current protection
hub 1-0:0: Port indicators are not supported
hub 1-0:0: power on to power good time: 2ms
hub 1-0:0: hub controller current requirement: 0mA
hub 1-0:0: local power source is good
hub 1-0:0: no over-current condition exists
hub 1-0:0: enabling power on all ports
hub 1-0:0: port 1, status 100, change 3, 12 Mb/s
hub 1-0:0: port 2, status 301, change 3, 1.5 Mb/s
hub 1-0:0: debounce: port 2: delay 100ms stable 4 status 0x301
hub 1-0:0: new USB device on port 2, assigned address 2 drivers/usb/core/message.c: usb_control/bulk_msg: timeout

-Dominik Brugger

Hello again,

I have a few things to add:

1) If uhci_hcd was never loaded before suspend, one has to press a key on the keyboard after echo 3 > /proc/acpi/sleep in order to get the machine suspended. Wakeup does not work in that case, reboot required.

2) It makes no difference whether the Logitech USB Mouse is connected or not.

-Dominik Brugger

Hi!

Strange, UHCI worked for me last time I tried. I'll try again soon...
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Hi!

Do not unload modules before suspend, they should handle that just fine.

With uhci on toshiba satelite I can do both S3 and S4, and USB survives that (at least it is able to power devices and detects plugs/unplugs).
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Good morning,

If I keep them loaded during suspend:

[...]
Suspending devices
uhci-hcd 0000:00:11.2: suspend to state 3 drivers/usb/host/uhci-hcd.c: dc00: suspend_hc
hwsleep-0257 [10] acpi_enter_sleep_state: Entering sleep state [S3]
Enabling SEP on CPU 0
Back to C!
Devices Resumed
uhci-hcd 0000:00:11.2: resume
[...]
drivers/usb/host/uhci-hcd.c: dc00: wakeup_hc

Mouse doesn't work, unplug:

hub 1-0:0: port 1, status 300, change 3, 1.5 Mb/s usb 1-1: USB disconnect, address 2 drivers/usb/core/usb.c: nuking urbs assigned to 1-1 usb 1-1: unregistering interfaces drivers/usb/core/usb.c: nuking urbs assigned to 1-1 usb 1-1: hcd_unlink_urb df7be640 fail -22 usb 1-1: hcd_unlink_urb df7be540 fail -22 drivers/usb/core/usb.c: usb_hotplug
usb 1-1: unregistering device drivers/usb/core/usb.c: usb_hotplug
hub 1-0:0: port 1 enable change, status 300 drivers/usb/host/uhci-hcd.c: dc00: suspend_hc

replug:

hub 1-0:0: port 1, status 301, change 3, 1.5 Mb/s hub 1-0:0: debounce: port 1: delay 100ms stable 4 status 0x301 hub 1-0:0: new USB device on port 1, assigned address 3 drivers/usb/core/message.c: usb_control/bulk_msg: timeout

Further attempts to un/re-plug the device show no response.

(Same with Canon Powershot instead of Logitech USB Mouse)

In my case it is an Epox 8kha+ board, the problem should not be hardware specific since Suspend To RAM works under WinXP.

I will try S4 lateron.

-Dominik Brugger

This could very well be related to the problem reported by Paul Mackerras about a month or two ago, see:

http://sourceforge.net/mailarchive/message.php?msg_id=5176974

I think the hub driver's power management code may be at fault. It needs to cancel the status interrupt URB when suspending and resubmit it when waking up; right now it probably does neither one.

Or maybe I'm wrong about that. Perhaps it's okay to leave the URB active. If that's the case, then the root hub power management code needs to be changed to restart the status URB timer after a wakeup.

I'm not sure how the design is intended to work, but either way something needs to be fixed.

Alan Stern

Could well be. I need to spend some time auditing power management in the USB drivers in general. The idea here is that a sub-driver (USB device driver) should make sure it has no more pending URBs when returning from suspend() and the HCD driver should just cancel pending URBs if still any and reject any one that would be submited before it's woken up. It's especially very important on OHCI to not touch chip registers (like enabling bulk queue etc...) after the chip have been put to suspend state. On some chips, that can cause random bus mastering to main memory during sleep, which can cause all sorts of interesting results especially when the north bridge is asleep...

Ben.

Benjamin Herrenschmidt wrote, responding to Alan Stern:

I thought that patch got merged already ...

Yes, it seems like all the HCDs (and the hub driver) need attention.

Plus, the enumeration process should respect hubs' power budgets, and handle overcurrent better. I had a hub re-enumerate over forty times not that long ago, just because it enabled too many things at once and the surge currents made lots of trouble. Plenty of power, if it got turned on carefully enough... :)

Agreed, this needs work. Some USB device drivers will likely need to implement suspend()/resume() callbacks, which thoughtfully enough the driver model conversion already gave us. At one point it was planned to have it automatically traverse the devices and suspend, leaves up to root; and resume in the reverse order. Is that behaving now?

Suspend should likely enable remote wakeup ([email protected] has been asking about that), at least as a config option. That'll be useful for things like keyboards and mice.

Now's probably a good time to work on this sort of integration issue. I'll be glad to see contributions in this area.

There's already code in the OHCI driver to handle that for some of the Apple hardware, as you know. And it should kick in on at least some of the power management paths; although not all, as Pavel has noted.

- Dave

So far as I can see, there's no USB power management code at all apart from the HC drivers. That includes the hub driver.

And I know that the UHCI suspend routine could use a little work. There's a 10-Hz polling timer that it doesn't turn off.

Alan Stern

Hi!

Why the hub driver?

For basic functionality, you simply power it down (doing virtual unplug), and power it back up on resume (doing virtual plug of all devices). That should work reasonably for everything but mass-storage.

Havin enough juice in "common case", but not in "worst case" is not too legal situation, is it?

Yes.

Okay, but we need normal suspend/resume working first :-).

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

For non-basic functionality such as "remote wakeup", where you can wake the system up from its suspension by doing things like typing on the USB keyboard.

This was a perfectly legal configuration, with what I recall as four devices. It was pretty far from "worst case".

- Dave

Almost correct. It's also necessary to stop/reinitialize a periodic status request.

None of this functionality has been implemented yet.

Alan Stern

Well, partially; but it's not used consistently. Could you (or someone) explain what the plan is? I see:

- Three separate x86 PM "initiators": APM, ACPI, swsusp. (Plus ones for ARM and MIPS.)

- Two driver registration infrastructures, the driver model stuff and the older pm_*() stuff.

The pm_*() is how a handful of sound drivers and other random stuff register themselves -- and how PCI does it.

I'd sure have expected PCI to only use the driver model stuff, and I'll hope all those users will all be phased out by the time that 2.6 gets near the end of its test cycle.

The "initiators" all talk to _both_ infrastructures, but they don't talk to the driver model stuff in the same way. For example, on suspend:

- ACPI issues a NOTIFY, which can veto the suspend; then SAVE_STATE, ditto; finally POWER_DOWN.

- APM uses the pm_*() calls for a vetoable check, never issues SAVE_STATE, then goes POWER_DOWN.

- While swsusp is more like ACPI except that it doesn't support vetoing from either NOTIFY or SAVE_STATE.

That all seems more problematic to me. Seems to me that APM should issue SAVE_STATE (and RESTORE_STATE), and all three PM "initiators" should agree on vetoing.

For USB, the {SAVE,RESTORE}_STATE calls would be the most natural place to do the "kill pending urbs" calls that Alan Stern mentioned -- at least, for D3 or swsusp levels. Likely for D1 and D2, devices with pending I/O won't really be keen on from suspending.

Now, for the record I tried to suspend a test1 APM-works system, with UHCI, and it wouldn't resume -- unclear why, or if test2 will do the same.

A different APM-works system, with OHCI and test2, did suspend/resume OK; but something went wrong with OHCI even before any driver model PM calls happened, if the OHCI driver was active (doing DMA): the HCD got an "Unrecoverable Error" IRQ, which generally means that some kind DMA fault appeared.

All of which is a roundabout way of adding to what I
said: the PM infrastructure USB will need to rely on seems like it needs polishing yet! :)

- Dave

To clarify, before someone jumps all over me: look inside the PCI code, you'll see that PCI as a whole is registered using the pm_*() stuff for power management. Yes, when those callbacks get made, they morph into driver model calls.

Which means that PCI devices (like OHCI) don't fit into the suspend/resume sequence in the way one would expect. It might explain why I'm still seeing two equivalent suspend notifications in the OHCI code...

- Dave

Hi!

Ouch, PCI really should use new driver model. I have not known about that. I'll look into it.

IIRC, I had SAVE_STATE there at some point, and there was some problem. I do not really use APM :-(.

Good. I figured vetoing support is not critical. I should add it.

Yes, agreed.

Do you need vetoing? Otherwise it should be ready, except for APM.

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

USB drivers don't talk suspend/resume yet, so they won't notice missing features there. Regressions are a different story though.

But I can imagine that usb-storage (or is that SCSI?) might want to veto suspending devices that are being used for some kinds of i/o. Eventually it should exist.

- Dave

Hi!

For what kind of I/O? I do not see a reason for disk to veto suspend. CD-burner might want to do that, but it still would be bad idea... (Running on battery, battery goes low, and you destroy your CD *and* your filesystem.
Pavel

Horseback riding is like software...
...vgf orggre jura vgf serr.

If it's in the middle of any kind of write, suspending would seem to be unwise. Say, writing to a swap partition...

Mostly I'm just saying that if vetoing ever makes sense (and I understand that it does), USB drivers will need to understand it too.

- Dave

Hi!

We already know it is not... We stopped all user tasks for this purpose. Also the device has opportunity to simply wait inside SAVE_STATE for a write to finish.

Agreed.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

No, you wait for pending requests to complete and keep further ones in the queue. I will probably write the proper infrastructure for SCSI hosts later this month, it has to be done like IDE, with a special BIO "suspend" request so that things gets properly synchronized.

Benjamin Herrenschmidt <[email protected]>

Well... that's userland policy. You can have a notion of "severity" of the suspend request, like vetoing a user-requested suspend request but not one issuing from a very-low-battery warning...

This is a part of the PM playfield that still is mostly to be done...

Ben.

The only place where a kernel driver may veto is when doing things like firmware flashing.

If you want to veto suspend when burning a CD, it's userland policy. We still need, I beleive, to define/write a proper userland-side API for all these with the daemon that goes with it. That will be next step once we have the kernel side working properly ;)

Ben.

Hi!

Where does acpi call pm_*()? It seems like it does not and it seems like a bug to me.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Doesn't; it showed up in a 'grep' that I should have examined more closely. Sorry! But both swsusp and APM have the "using both registration schemes" issue (consistency matters here).

Why does it seem like a bug -- because it's using only the "new" infrastructure, while there are still a few drivers that only register to the old one? I'd rather see those drivers (about a dozen) have compile time #ifdef CONFIG_PM #warnings, and call them the bug...

- Dave

Hi!

Yes.

I guess I like that, too.. But at this point whole PCI relies on old-style pm_*(); that needs to be fixed, first.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Hi!

I remember the reason... SAVE_STATE expects user processes to be stopped, which is not the case in APM. Perhaps that is easy to fix these days...
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

That SAVE_STATE restriction doesn't seem to be documented...

- Dave

Hi!

It is not; but I could not convince myself that it is okay to call SAVE_STATE with userland alive, and that's why I did not add SAVE_STATE to apm.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Because it is plain wrong ;)

Ben.

No ! You may feel better stopping user processes (and actuallty you may require that for swsusp, I don't know) but the whole PM scheme is designed to make that unnecessary. I do NOT stop user processes on suspend-to-RAM on PowerMacs, I don't think neither APM nor ACPI need that (I may be wrong here, but if that is the case, then some drivers need fixing).

Ben.

Hi!

When all drivers are safe, stopping user processes on ACPI S3 will no longer be needed.

But they are not safe for now, and I believe that stopping processes makes some hw drivers safe where they were not safe before. At least if process is sleeping within some driver, stopping user processes will wait till it exits the driver. That should help for simple devices.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

You should look at Patrick Mochel's stuff that shall be getting in the official tree this month hopefully. It does that (among others)

The older pm_ model is deprecated.

PCI is broken since it does both (and thus, if we call both rounds of notifiers, we end up suspending PCI twice, the second time without any ordering constraints). In my trees, I comment out that "legacy" stuff (though I also don't call the old-style pm_* stuff anymore neither)

All that will change to a unified mecanism soon

SAVE_STATE is going away. The proper semantics are SUSPEND and POWERDOWN (+/-). The later is optional and really only needed by drivers who need to do their last powerdown step with interrupts disabled.

The SUSPEND state is responsible for blocking further IOs, and snapshoting the state. It can enter the HW suspend mode too, depending on the "state" argument you pass.

The actual policy of what shall be done to "block" IOs is function-specific. Typically, a network driver will just stop the Rx side and drop any Tx packet (it just need to call netif_stop_queue() actually, but it can drop pending packets in the Tx ring). A block driver (IDE, SCSI) must complete any pending request and keep new incoming ones held in the BIO queue. This has been implemented properly for IDE already, SCSI still need work.

The USB "device" drivers shall just rely on the Device Model infrastructure to have their suspend/resume callbacks be called at the appropriate time. If they take care of finishing pending IOs (or cancelling, as you like, depending on the driver function) and not issuing new URBs until they are resumed, there shall be no problem, the HCD should be idle when it's own suspend callback is reached. Which is why I beleive it's safe/better to actually cancel pending URBs and reject any new one in the HCD driver when it's in suspend state.

Ben.

OK, thanks for the update. It was puzzling to see this stuff in such a state ... I've got a few patches for USB PM that are worth putting back in advance of that, but it seems like real testing will need to wait until "next month". (Tomorrow!)

Yes, that's no problem. It looks like:

  static int my_suspend(struct device *_intf, u32 state, u32 level)
  {
      struct usb_interface   *intf;
      int                    retval = 0;

      intf = to_usb_interface(_intf);
      switch (level) {
      ... cases are yet to stabilize ...
      }
      return retval;

}

... similar for resume()

  static struct usb_driver my_driver {
       .owner =      THIS_MODULE,
       .name =       "MyFirstDriver",
       .probe =      my_probe,
       .disconnect = my_disconnect,
       .id_table =   my_id_table,
       .driver = {
             .suspend =    my_suspend,
             .resume =     my_resume,
        },
   };

But until the suspend()/resume() callback API stabilizes, it's mostly useful to know that it'll be exactly that simple, and that usbcore doesn't get in the way.

I suspect that USB should do some non-global PM stuff too. Hub ports can be suspended when the devices connected to them are idle for long enough ... that's not something I'd expect system-wide PM policies to address.

- Dave

Indeed, this is not addressed, though it may sense to have a way in the device model to call the suspend/resume callbacks of "childs" of a given hub only for that purpose, I don't think this is implemented yet. Maybe talk to Patrick about it.

In general "local" power management (automatic disk spin down, chipset idle-pm, etc...) is ... local to the driver, though you can probably use the power state field of struct device to store your current state if it maps to those semantics.

Ben.

Hi!

Is not disk spin-down policy, and thus belonging to userspace? Having daemon poll for inactivity of hubs once every 5 minutes and sending them to sleep should not hurt, too...
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Taking precedents into account it is the kernel's job. Screen blanking is done in kernel, as is afaik floppy motor control.

Regards
Oliver

Hi!

Floppy motor was not designed to be "always on", and screen gets physically damaged if you don't turn it off. OTOH harddrives were designed for always on, and if you spin up/down too much you shorten their lives...

				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Hi!

Can you mail me a patch? [Where does PCI do its "second round"? From a quick look I did not see that.]

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

I just comment out the init code in drivers/pci/power.c

Ben.

Hi!

Hmm, in such case whole power.c file can be
killed. pci_register_driver() should take care of registering proper callbacks.

Apm needs to be fixed to do save_state/resume.
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Hello again,

After resuming from S4 uhci_hcd works fine (in opposition to S3 under exactly the same circumstances). USB support was completely compiled into kernel and not as modules (therefore no unloading before suspend was done).

Interesting:

uhci_hcd wakes up again before power down.

Devices Resumed
Devices Resumed
uhci-hcd 0000:00:11.2: resume
uhci-hcd 0000:00:11.2: can't resume, not suspended!

(now using 2.6.0-test2-mm2)

-Dominik Brugger

OK, I can see that in 2.6.0-test2 iff there's a device connected; that's on OHCI hardware that doesn't retain power during suspend, which means it uses the restart() path. Hardware that retains power depends on slightly different logic.

Of course, the HC shouldn't have died and gone down those paths; but the "HC died" paths need to work right too.

More like not calling stop() from hcd_panic. Instead, all the devices should be disconnected, and their urbs cleaned up. That way the controller will sit in a known and "safe" state (reset) until the driver is shut down and gets stop()ped. I think that logic just "seemed to work" before, with subtle misbehaviors.

We're still working to make sure that we do all the right stuff to shut down devices, no longer relying on USB device drivers to shut themselves down properly in their disconnect() methods. Many haven't, which can easily lead to oopsing on the shutdown paths that don't get used very regularly.

Eventually I suspect that the HCD glue should grow logic to try restarting drivers after the hardware dies/resets, but first it's important to be sure they shut down properly.

With a better diagnostic though.

Here's a patch that makes things slightly better. It's still not fully functional yet -- I forgot how many FIXMEs are in those PM code paths! -- and shouldn't be merged as-is, but it works slightly better:

- Has a more informative diagnostic message (which HC died);

- When HC dies, mark the whole tree as unavailable so that new URB submissions using that HC will just fail;

- Then hcd_panic() just disconnects all the devices, still keeping the root hub around.

- OHCI-specific (should be generic, hcd-pci.c): don't try resuming a halted controller.

Where "better" means that it seems functional after the first suspend/resume cycle, and re-enumerates the device that's connected ... but there's still strangeness. And I can see how some of it would be generic.

- Dave

@@ -1487,8 +1488,26 @@
 
 static void hcd_panic (void *_hcd)
 {
-	struct usb_hcd *hcd = _hcd;
-	hcd->driver->stop (hcd);
+	struct usb_hcd		*hcd = _hcd;
+	struct usb_device	*hub = hcd->self.root_hub;
+
+	hub = usb_get_dev (hub);
+	usb_disconnect (&hub);
+
+	/* FIXME either try to restart, or arrange to clean up the 
+	 * hc-internal state, like usb_hcd_pci_remove() does
+	 */
+}
+
+void mark_gone (struct usb_device *dev)
+{
+	unsigned	i;
+
+	dev->state = USB_STATE_NOTATTACHED;
+	for (i = 0; i < dev->maxchild; i++) {
+		if (dev->children [i])
+			mark_gone (dev->children [i]);
+	}
 }
 
 /**

@@ -1501,29 +1520,12 @@
  */
 void usb_hc_died (struct usb_hcd *hcd)
 {
-	struct list_head	*devlist, *urblist;
-	struct hcd_dev		*dev;
-	struct urb		*urb;
-	unsigned long		flags;
-	
-	/* flag every pending urb as done */
-	spin_lock_irqsave (&hcd_data_lock, flags);
-	list_for_each (devlist, &hcd->dev_list) {
-		dev = list_entry (devlist, struct hcd_dev, dev_list);
-		list_for_each (urblist, &dev->urb_list) {
-			urb = list_entry (urblist, struct urb, urb_list);
-			dev_dbg (hcd->controller, "shutdown %s urb %p pipe %x, current status %d\n",
-				hcd->self.bus_name, urb, urb->pipe, urb->status);
-			if (urb->status == -EINPROGRESS)
-				urb->status = -ESHUTDOWN;
-		}
-	}
-	urb = (struct urb *) hcd->rh_timer.data;
-	if (urb)
-		urb->status = -ESHUTDOWN;
-	spin_unlock_irqrestore (&hcd_data_lock, flags);
+	dev_err (hcd->controller, "HC died; pending I/O will be aborted.\n");
 
-	/* hcd->stop() needs a task context */
+	/* prevent new submissions to devices in this tree */
+	mark_gone (hcd->self.root_hub);
+	
+	/* then usb_disconnect() them all, in a task context */
 	INIT_WORK (&hcd->work, hcd_panic, hcd);
 	(void) schedule_work (&hcd->work);
 }

@@ -199,6 +199,11 @@
 	int			retval = 0;
 	unsigned long		flags;
 
+	if (hcd->state == USB_STATE_HALT) {
+		ohci_dbg (ohci, "USB restart of halted device\n");
+		return -EL3HLT;
+	}
+
 #ifdef CONFIG_PMAC_PBOOK
 	{
 		struct device_node *of_node;

Hi!

For me it:

reports problem after first suspend, and ohci stops working

oopses in hcd_panic during second suspend (IIRC).
Pavel

Horseback riding is like software...
...vgf orggre jura vgf serr.

When you say "reports problem", what do you mean?

I can see problems _during_ the suspend, before any USB suspend calls are even made. Only if the controller has work to do, so it's scanning the schedules (DMA) ... it's fine until the suspend starts, then the controller gets an "Unrecoverable Error" IRQ, which I'm used to seeing for DMA errors. (As if something started shutting down hardware before the driver had a chance to clean up.)

If there's no DMA action before the suspend starts, the current code behaved OK for me -- on an APM D3cold suspend/resume cycle, which isn't so different from what you'll see with swsusp.

That makes me think I'll be better off waiting for some of those PM updates to get merged, before I spend much more time looking at the usb parts of these issues.

Likely because resuming a halted controller is nonsense, and the code needs to reject such attempts.

Of course, if the controller weren't going bonkers before it's told to start suspending, these particular faults wouldn't be happening ... :)

- Dave

Hi!

I fixed PCI powermangment, and OHCI problems are gone (replaced by "machine-feels-like-XT-because-keventd-spins"). I'm looking onto that.

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]