Re: fixing usb suspend/resuming

Re: fixing usb suspend/resuming

[Alexander Gran]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

Am Freitag, 28. Mai 2004 16:42 schrieben Sie:
> If you have, then where's your patch?  :)

attached ;) (I hope this is correctly inlined now. If not I think I have to 
patch kmail first...)

> What kernel?

2.6.7-rc1-mm1

> > I think you'd need to include a
> > pci_save_state, pci_disable_device, pci_set_power_state
> > before suspending and
> > pci_set_state, pci_enable_device, (pci_set_power_state?)
> > while resuming.
>
> Odd, that's what I see drivers/usb/core/hcd-pci.c doing right now.

[Urgs] I didn't look there....

However I just debugged a bit more:
My uhci's do not support PM at all, so i disabled the uhci driver.
(The ehci does, however I don't know how windows handles suspend then...).
When I want acpi to go to S3 (echo 3 > /proc/acpi/sleep), the driver want's to 
enter S2, which the device does not support:
Stopping tasks: 
===================================================================|
radeonfb: suspending to state: 2...
agpgart: Found an AGP 2.0 compliant device at 0000:00:00.0.
agpgart: Putting AGP V2 device at 0000:00:00.0 into 0x mode
agpgart: Putting AGP V2 device at 0000:01:00.0 into 0x mode
ehci_hcd 0000:00:1d.7: suspend D0 --> D2
ehci_hcd 0000:00:1d.7: PCI suspend fail, -5
ehci_hcd 0000:00:1d.7: resume from state D0

This seems to be an acpi problem. I'm no acpi god, and no idea how it works. I 
found that every call before acpi has state 3, every afterwards has state 2.

However, even when the driver is put to S3, it is not resumed afterwards, as 
it fiddles around with dev->power.power_state. 
drivers/base/power/resume.c:dpm_resume doe not resume it, then.
Additionally I suppose, there is a pci_enable_device missing in the resume 
code (See patch below).
I removed the power_state code, and added pci_enable_device. The driver 
survives a resume now, but the handshakes fail afterwards (even after reload 
the module). Dunno why ;( ?
At Least suspend-to-disk (pmdisk) works.



-- 
Encrypted Mails welcome.
PGP-Key at http://zodiac.dnsalias.org/misc/pgpkey.asc | Key-ID: 0x6D7DD291



[-- Attachment #2: usb_suspend-resume.patch --]
[-- Type: text/x-diff, Size: 3308 bytes --]

diff -rpu linux-2.6.7-rc1-mm1/drivers/usb/core/hcd-pci.c linux-2.6.7-rc1-ag1/drivers/usb/core/hcd-pci.c
--- linux-2.6.7-rc1-mm1/drivers/usb/core/hcd-pci.c	2004-05-23 07:54:17.000000000 +0200
+++ linux-2.6.7-rc1-ag1/drivers/usb/core/hcd-pci.c	2004-05-29 18:08:32.000000000 +0200
@@ -318,7 +318,7 @@ int usb_hcd_pci_suspend (struct pci_dev 
 			
 			if (has_pci_pm)
 				retval = pci_set_power_state (dev, state);
-			dev->dev.power.power_state = state;
+			
 			if (retval < 0) {
 				dev_dbg (&dev->dev,
 						"PCI suspend fail, %d\n",
@@ -344,6 +344,7 @@ int usb_hcd_pci_resume (struct pci_dev *
 	int			has_pci_pm;
 
 	hcd = pci_get_drvdata(dev);
+	pci_restore_state(dev,hcd->pci_state);
 	has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
 	if (has_pci_pm)
 		dev_dbg(hcd->self.controller, "resume from state D%d\n",
@@ -357,8 +358,8 @@ int usb_hcd_pci_resume (struct pci_dev *
 	hcd->state = USB_STATE_RESUMING;
 
 	if (has_pci_pm)
-		pci_set_power_state (dev, 0);
-	dev->dev.power.power_state = 0;
+		pci_enable_device(dev);
+
 	retval = request_irq (dev->irq, usb_hcd_irq, SA_SHIRQ,
 				hcd->description, hcd);
 	if (retval < 0) {
@@ -367,7 +368,6 @@ int usb_hcd_pci_resume (struct pci_dev *
 		return retval;
 	}
 	pci_set_master (dev);
-	pci_restore_state (dev, hcd->pci_state);
 #ifdef	CONFIG_USB_SUSPEND
 	pci_enable_wake (dev, dev->current_state, 0);
 	pci_enable_wake (dev, 4, 0);
diff -rpu linux-2.6.7-rc1-mm1/drivers/usb/host/ehci-hcd.c linux-2.6.7-rc1-ag1/drivers/usb/host/ehci-hcd.c
--- linux-2.6.7-rc1-mm1/drivers/usb/host/ehci-hcd.c	2004-05-23 07:54:22.000000000 +0200
+++ linux-2.6.7-rc1-ag1/drivers/usb/host/ehci-hcd.c	2004-05-29 18:08:59.000000000 +0200
@@ -662,8 +662,6 @@ static int ehci_resume (struct usb_hcd *
 	/* FIXME lock root hub */
 	retval = ehci_hub_resume (hcd);
 #endif
-	if (retval == 0)
-		hcd->self.controller->power.power_state = 0;
 	return retval;
 }
 
diff -rpu linux-2.6.7-rc1-mm1/drivers/usb/host/ehci-hub.c linux-2.6.7-rc1-ag1/drivers/usb/host/ehci-hub.c
--- linux-2.6.7-rc1-mm1/drivers/usb/host/ehci-hub.c	2004-05-23 07:55:01.000000000 +0200
+++ linux-2.6.7-rc1-ag1/drivers/usb/host/ehci-hub.c	2004-05-29 18:10:37.000000000 +0200
@@ -33,12 +33,9 @@
 static int ehci_hub_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
-	struct usb_device	*root = hcd_to_bus (&ehci->hcd)->root_hub;
 	int			port;
 	int			status = 0;
 
-	if (root->dev.power.power_state != 0)
-		return 0;
 	if (time_before (jiffies, ehci->next_statechange))
 		return -EAGAIN;
 
@@ -74,7 +71,6 @@ static int ehci_hub_suspend (struct usb_
 	ehci_work (ehci, 0);
 	(void) handshake (&ehci->regs->status, STS_HALT, STS_HALT, 2000);
 
-	root->dev.power.power_state = 3;
 	ehci->next_statechange = jiffies + msecs_to_jiffies(10);
 	spin_unlock_irq (&ehci->lock);
 	return status;
@@ -89,8 +85,6 @@ static int ehci_hub_resume (struct usb_h
 	u32			temp;
 	int			i;
 
-	if (!root->dev.power.power_state)
-		return 0;
 	if (time_before (jiffies, ehci->next_statechange))
 		return -EAGAIN;
 
@@ -138,7 +132,6 @@ static int ehci_hub_resume (struct usb_h
 	if (temp)
 		writel (ehci->command | temp, &ehci->regs->command);
 
-	root->dev.power.power_state = 0;
 	ehci->next_statechange = jiffies + msecs_to_jiffies(5);
 	ehci->hcd.state = USB_STATE_RUNNING;
 	return 0;

Re: fixing usb suspend/resuming

[David Brownell]

Hi,

Somehow batch of mail to me got delayed, and yours was in it.
So I thought I'd follow up on this, since I've noticed the
same thing in other contexts:


Alexander Gran wrote:
> When I want acpi to go to S3 (echo 3 > /proc/acpi/sleep), the driver want's to 
> enter S2, which the device does not support:

I'm not clear on the intended relationship between PCI device state
numbers and ACPI device states in Linux ... but it's clear from the
specs (ACPI ch2, the mostly-generic bit) that ACPI "3" != PCI "3".

   Power Off   == ACPI 3
               == PCI 4 (D3cold)
   Low Power   == ACPI 2
               == PCI 3 (D3hot; or maybe D1 or D2, depending)

And in much the same way "USB Suspend" is an ACPI low power
state (2) ... not an ACPI power off state (3).  And for USB
host controllers, PCI D3hot is expected to support USB suspend.

I'm suspecting that something is mistranslating between ACPI
power state numbering and PCI power state numbering

I'd _certainly_ expect that the numbers passed to PCI suspend
and resume calls would match the PCI state numbers, not the
ACPI numbers!  But those numbers aren't documented in the
Linux sources, so probably different people are making rather
different assumptions.  After all, "3 == 3" and "2 == 2".

That's all different from the ACPI system power states, too.
(Which is what I'd expect /proc/acpi/sleep to affect.)

- Dave


> Stopping tasks: 
> ===================================================================|
> radeonfb: suspending to state: 2...
> agpgart: Found an AGP 2.0 compliant device at 0000:00:00.0.
> agpgart: Putting AGP V2 device at 0000:00:00.0 into 0x mode
> agpgart: Putting AGP V2 device at 0000:01:00.0 into 0x mode
> ehci_hcd 0000:00:1d.7: suspend D0 --> D2
> ehci_hcd 0000:00:1d.7: PCI suspend fail, -5
> ehci_hcd 0000:00:1d.7: resume from state D0
> 
> This seems to be an acpi problem. I'm no acpi god, and no idea how it works. I 
> found that every call before acpi has state 3, every afterwards has state 2.

Re: fixing usb suspend/resuming

[Alexander Gran]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am Samstag, 17. Juli 2004 19:32 schrieb David Brownell:
>> I'm not clear on the intended relationship between PCI device state
> numbers and ACPI device states in Linux ... but it's clear from the
> specs (ACPI ch2, the mostly-generic bit) that ACPI "3" != PCI "3".

ermm, ok. It is even more complicated. Section 2 of the ACPI specs speaks of 
Global Power states and device power states. 
So we ge the follwing, with G= ACPI Global power state, S = (ACPI?) Sleep 
State, D = ACPI Device power state,
G3 = Mechanical off = everything D3
G2=S5=soft off = everything D3
G1=S4 = Hibernation = everything D3
G1=S3/S2=Sleeping = parts in D2/D1/(D0?)
G0=S0=Working = evrything in D0

/proc/acpi/sleep talks about the S states, so when I want STR, which is S3 i 
echo 3 > /proc/acpi/sleep.
The PCI state would also be 3 for most devices, I think. Pci state 1 or 2 
cannot be expected to be supported.

> I'm suspecting that something is mistranslating between ACPI
> power state numbering and PCI power state numbering

ACK.

> I'd _certainly_ expect that the numbers passed to PCI suspend
> and resume calls would match the PCI state numbers, not the
> ACPI numbers!  But those numbers aren't documented in the
> Linux sources, so probably different people are making rather
> different assumptions.  After all, "3 == 3" and "2 == 2".
>
> That's all different from the ACPI system power states, too.
> (Which is what I'd expect /proc/acpi/sleep to affect.)

I think that file doesn't expect Global power states, as thats only on, 
sleeping, off, but sleep states. However I don't see the translation into PCI 
sleep states either. But there must be some translation, as a 3 is translatet 
into a 2 here (which than failes..)

I suppose some cleanup and documentation is required here.

regards
Alex

- -- 
Encrypted Mails welcome.
PGP-Key at http://zodiac.dnsalias.org/misc/pgpkey.asc | Key-ID: 0x6D7DD291
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA+3fp/aHb+2190pERArKfAJ4mShRA2zHafuPrsWKTPVVFUumHQwCfYb3M
9x0AqFQqhhPILjYDEnxlXH4=
=d/FA
-----END PGP SIGNATURE-----

Re: fixing usb suspend/resuming

[David Brownell]

On Monday 19 July 2004 00:27, Alexander Gran wrote:
> Am Samstag, 17. Juli 2004 19:32 schrieb David Brownell:

> > I'm suspecting that something is mistranslating between ACPI
> > power state numbering and PCI power state numbering
> 
> ACK.

See http://bugme.osdl.org/show_bug.cgi?id=2886 ... basically
it looks like this problem would show up with any of a dozen
or so different drivers, few of which are widely used on systems
that use suspend/resume much (laptops!).

- Dave

Re: fixing usb suspend/resuming

[Pavel Machek]

Hi!

> > > I'm suspecting that something is mistranslating between ACPI
> > > power state numbering and PCI power state numbering
> > 
> > ACK.
> 
> See http://bugme.osdl.org/show_bug.cgi?id=2886 ... basically
> it looks like this problem would show up with any of a dozen
> or so different drivers, few of which are widely used on systems
> that use suspend/resume much (laptops!).

Ben H. has some ideas how to fix this. Anyway, storing S-state or D-state in
integer is bad because someone will get it wrong.

Plus, some PCI drivers (ide disk?) want to do different thing on S3 and swsusp:
it does not make much sense to spindown before swsusp.
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: fixing usb suspend/resuming

[Nigel Cunningham]

Hi.

On Thu, 2004-07-29 at 18:35, Pavel Machek wrote:
> Plus, some PCI drivers (ide disk?) want to do different thing on S3 and swsusp:
> it does not make much sense to spindown before swsusp.

Regarding the spinning down before suspending to disk, I have a patch in
my version that adds support for excluding part of the device tree when
calling drivers_suspend. I take the bdevs we're writing the image to,
trace the structures to get the relevant device tree entry/ies and then
move (in the correct order) those devices and their parents from the
active devices list to a 'dont' touch' list (I don't call it that in
code). I can then safely suspend the remaining devices without powering
down the ones being used for suspend. I'm not sure of the context of
your conversation but if it's helpful, I'll send a patch.

Nigel

Re: fixing usb suspend/resuming

[Oliver Neukum]

> Regarding the spinning down before suspending to disk, I have a patch in
> my version that adds support for excluding part of the device tree when
> calling drivers_suspend. I take the bdevs we're writing the image to,
> trace the structures to get the relevant device tree entry/ies and then
> move (in the correct order) those devices and their parents from the
> active devices list to a 'dont' touch' list (I don't call it that in

How do you deal with md, loop, etc... ?

	Regards
		Oliver

Re: fixing usb suspend/resuming

[Nigel Cunningham]

Hi.

On Thu, 2004-07-29 at 22:51, Oliver Neukum wrote:
> > Regarding the spinning down before suspending to disk, I have a patch in
> > my version that adds support for excluding part of the device tree when
> > calling drivers_suspend. I take the bdevs we're writing the image to,
> > trace the structures to get the relevant device tree entry/ies and then
> > move (in the correct order) those devices and their parents from the
> > active devices list to a 'dont' touch' list (I don't call it that in
> 
> How do you deal with md, loop, etc... ?

The loop thread is NOFREEZE, so it should work fine. Until you said it,
I hadn't considered md, but it shouldn't be too hard to add some more
code to check if the devices are part of raid arrays. The other devices
could be given the same treatment.

As far as setting them up again at boot time, I've just added proper
initrd support, so one will be able to do any configuration needed from
an initrd (provided filesystems aren't mounted), get suspend to check if
it needs to resume and then carry on in the rest of the initrd mounting
filesystems and so on.

Regards,

Nigel

Re: fixing usb suspend/resuming

[David Brownell]

On Thursday 29 July 2004 05:17, Nigel Cunningham wrote:
> 
> Regarding the spinning down before suspending to disk, I have a patch in
> my version that adds support for excluding part of the device tree when
> calling drivers_suspend. 

I've always suspected such a patch would be needed ... :)

The drivers/base/power code is a bit too simplistic right now.
Among other things it keeps deadlocking when suspend()
or resume() routines try to remove devices.  That makes
it needlessly hard to handle some common situations.

-  Dave

Re: fixing usb suspend/resuming

[Pavel Machek]

Hi!

> > Plus, some PCI drivers (ide disk?) want to do different thing on S3 and swsusp:
> > it does not make much sense to spindown before swsusp.
> 
> Regarding the spinning down before suspending to disk, I have a patch in
> my version that adds support for excluding part of the device tree when
> calling drivers_suspend. I take the bdevs we're writing the image
> to,

Well, its more complicated, I believe. You can't just leave those
devices running, because they could DMA and damage the image... So you
need something like

suspend_fast_ill_resume_you_soon().

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: fixing usb suspend/resuming

[Nigel Cunningham]

Hi.

On Fri, 2004-07-30 at 07:02, Pavel Machek wrote:
> Well, its more complicated, I believe. You can't just leave those
> devices running, because they could DMA and damage the image... So you

I'm assuming (and believe I have achieved) that the only process doing
anything significant is suspend, in which case the image isn't going to
get damaged.

> need something like
> 
> suspend_fast_ill_resume_you_soon().

Don't understand what you're saying here, sorry.

Nigel

Re: fixing usb suspend/resuming

[Pavel Machek]

Hi!

> > Well, its more complicated, I believe. You can't just leave those
> > devices running, because they could DMA and damage the image... So you
> 
> I'm assuming (and believe I have achieved) that the only process doing
> anything significant is suspend, in which case the image isn't going to
> get damaged.

Well, only suspend is doing something significant, but driver might
take arbitrary time to do its DMA... Like

Freeing some memory... [write starts]
Suspending devices... [but you did not suspend disk!]
Atomic copy... [oops, that disk was *still* doing DMA]

> > need something like
> > 
> > suspend_fast_ill_resume_you_soon().
> 
> Don't understand what you're saying here, sorry.

Well, I believe we really need to suspend *all* devices. We just do
not need to spin the disks down and make screens blank; we still need
drivers to be stopped so that no activity happens during atomic copy.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: fixing usb suspend/resuming

[Nigel Cunningham]

Hi.

On Fri, 2004-07-30 at 08:39, Pavel Machek wrote:
> > I'm assuming (and believe I have achieved) that the only process doing
> > anything significant is suspend, in which case the image isn't going to
> > get damaged.
> 
> Well, only suspend is doing something significant, but driver might
> take arbitrary time to do its DMA... Like
> 
> Freeing some memory... [write starts]
> Suspending devices... [but you did not suspend disk!]
> Atomic copy... [oops, that disk was *still* doing DMA]

Sorry. Should have mentioned that one of my freezer changes includes
running sync between freezing userspace and freezing the kernel threads,
that I double check there are no dirty buffers afterwards and wait on
the completion of all suspend's I/O between after each part of the image
is written/read (have to be paranoid because of the way I reuse LRU
pages).

> > > need something like
> > > 
> > > suspend_fast_ill_resume_you_soon().
> > 
> > Don't understand what you're saying here, sorry.
> 
> Well, I believe we really need to suspend *all* devices. We just do
> not need to spin the disks down and make screens blank; we still need
> drivers to be stopped so that no activity happens during atomic copy.

Ah with you now. The typo got me (_ill_).

Re: fixing usb suspend/resuming

[David Brownell]

On Thursday 29 July 2004 01:35, Pavel Machek wrote:

> > See http://bugme.osdl.org/show_bug.cgi?id=2886 ... basically
> > it looks like this problem would show up with any of a dozen
> > or so different drivers, few of which are widely used on systems
> > that use suspend/resume much (laptops!).
> 
> Ben H. has some ideas how to fix this. Anyway, storing S-state or D-state in
> integer is bad because someone will get it wrong.

Right, there seems to be agreement that passing an ACPI S-state u32 down to
drivers expecting non-ACPI D-state u32 is a bad idea..  Drivers should see
the right bus-specific D-states ... see

http://www.mail-archive.com/linux-usb-devel%40lists.sourceforge.net/msg26528.html

which touches on some of the issues with what he explained to me.

- Dave.