pci_set_power_state() failure and breaking suspend

pci_set_power_state() failure and breaking suspend

[Benjamin Herrenschmidt]

So I noticed a small regression that I think might uncover a deeper
issue...

Recently, ohci1394 grew some "proper" error handling in its suspend
function, something that looks like:

        err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
        if (err)
                goto out;

First, it breaks some old PowerBooks where the internal OHCI had PM
feature exposed on PCI (the pmac specific code that follows those lines
is enough on those machines).

That can easily be fixed by removing the if (err) goto out; statement
and having the pmac code set err to 0 in certain conditions, and I'll be
happy to submit a patch for this.

However, this raises the question of do we actually want to prevent
machines to suspend when they have a PCI device that don't have the PCI
PM capability ? I'm asking that because I can easily imagine that sort
of construct growing into more drivers (sounds logical if you don't
think) and I can even imagine somebody thinking it's a good idea to slap
a __must_check on pci_set_power_state() ... 

I think the answer is not simple. For a lot of devices, it will be
harmless, they will be either unaffected by the sleep transition or
powered down and back up, and the driver will cope just fine. I suppose
there can be issues for devices that do not support it, though none
obvious comes to mind.

It's one of those policies that are hard to define. But I think the best
approach for now is to not fail the suspend when pci_set_power_state()
returns an error.

Ben.

Re: pci_set_power_state() failure and breaking suspend

[Benjamin Herrenschmidt]

On Tue, 2006-10-24 at 16:54 +1000, Benjamin Herrenschmidt wrote:
> So I noticed a small regression that I think might uncover a deeper
> issue...
> 
> Recently, ohci1394 grew some "proper" error handling in its suspend
> function, something that looks like:
> 
>         err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
>         if (err)
>                 goto out;
> 
> First, it breaks some old PowerBooks where the internal OHCI had PM
> feature exposed on PCI (the pmac specific code that follows those lines
> is enough on those machines).

If I could type, the above would have read...

First, it breaks some old PowerBooks where the internal OHCI has no PM
feature exposed on PCI....

Ben.

Re: pci_set_power_state() failure and breaking suspend

[Stefan Richter]

Benjamin Herrenschmidt wrote:
>> Recently, ohci1394 grew some "proper" error handling in its suspend
>> function, something that looks like:
>> 
>>         err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
>>         if (err)
>>                 goto out;
...
> First, it breaks some old PowerBooks where the internal OHCI has no PM
> feature exposed on PCI....

Just out of curiosity: Are these HCs actual PCI hardware or are they
glued onto an extra PCI interface? Does the startup message of ohci1394
log OHCI 1.1 or 1.0 compliance for them? Probably the latter; OHCI 1.1
was released in January 2000.

OHCI 1.1 says "PCI based 1394 Open Host Controllers /should/ implement
PCI Power Management, and implementations that support PCI Power
Management /shall/ exhibit behavior consistent with this Annex."
(Emphasis mine.) I.e. a compliant HC does either not support power
management at all or supports it including the pci_set_power_state()
triggered state transitions.

OHCI 1.00 does not have a power management specification. It points to
the PCI Local Bus Specification Revision 2.1 though (which I don't have).

We are still working on full power management support by ohci1394 and
upper IEEE 1394 layers, so I am glad to get feedback and patches.
-- 
Stefan Richter
-=====-=-==- =-=- ==---
http://arcgraph.de/sr/

Re: pci_set_power_state() failure and breaking suspend

[Benjamin Herrenschmidt]

> Just out of curiosity: Are these HCs actual PCI hardware or are they
> glued onto an extra PCI interface? Does the startup message of ohci1394
> log OHCI 1.1 or 1.0 compliance for them? Probably the latter; OHCI 1.1
> was released in January 2000.

I don't have one at hand right now but I suspect it's 1.1. I'll ask
paulus tomorrow. They are cells inside an Apple ASIC but on a PCI bus
(on-chip PCI bus).

> OHCI 1.1 says "PCI based 1394 Open Host Controllers /should/ implement
> PCI Power Management, and implementations that support PCI Power
> Management /shall/ exhibit behavior consistent with this Annex."
> (Emphasis mine.) I.e. a compliant HC does either not support power
> management at all or supports it including the pci_set_power_state()
> triggered state transitions.

Well, the old Apple ones support power management but not the PCI PM
states. However, there are separate magic bits in the Apple ASIC that
can be used to toggle the clocks on/off (which is that the ppc specific
bits do basically, along as turning off power on the bus).

> OHCI 1.00 does not have a power management specification. It points to
> the PCI Local Bus Specification Revision 2.1 though (which I don't have).
> 
> We are still working on full power management support by ohci1394 and
> upper IEEE 1394 layers, so I am glad to get feedback and patches.

Well, the question is wether we want to make the whole machine suspend
fail because there is a 1394 chip that doesn't do PCI PM in or not...

I can send patches "fixing" it both ways (just ignoring the result from
pci_set_power_state in general, or just ignoring that result on Apple
cells).

Ben.

Re: pci_set_power_state() failure and breaking suspend

[Stefan Richter]

Benjamin Herrenschmidt wrote:
> Well, the question is wether we want to make the whole machine suspend
> fail because there is a 1394 chip that doesn't do PCI PM in or not...
> 
> I can send patches "fixing" it both ways (just ignoring the result from
> pci_set_power_state in general, or just ignoring that result on Apple
> cells).

Yes, what would be the correct way to do this? And if it the latter
option, should that be implemented in ohci1394 or in pci_set_power_state?

grep says that almost nobody checks the return code of
pci_set_power_state. But e.g. usb/core/hcd-pci.c does...

(Side note: The sole function that ohci1394's suspend and resume hooks
fulfill right now in mainline is to change power consumption of the
chip. The IEEE 1394 stack as a whole does not survive suspend + resume
yet. A still incomplete solution is in linux1394-2.6.git.)
-- 
Stefan Richter
-=====-=-==- =-=- ==---
http://arcgraph.de/sr/

Re: pci_set_power_state() failure and breaking suspend

[Benjamin Herrenschmidt]

On Tue, 2006-10-24 at 13:41 +0200, Stefan Richter wrote:
> Benjamin Herrenschmidt wrote:
> > Well, the question is wether we want to make the whole machine suspend
> > fail because there is a 1394 chip that doesn't do PCI PM in or not...
> > 
> > I can send patches "fixing" it both ways (just ignoring the result from
> > pci_set_power_state in general, or just ignoring that result on Apple
> > cells).
> 
> Yes, what would be the correct way to do this? And if it the latter
> option, should that be implemented in ohci1394 or in pci_set_power_state?
> 
> grep says that almost nobody checks the return code of
> pci_set_power_state. But e.g. usb/core/hcd-pci.c does...

Yes, and I think that's bogus too ...

> (Side note: The sole function that ohci1394's suspend and resume hooks
> fulfill right now in mainline is to change power consumption of the
> chip. The IEEE 1394 stack as a whole does not survive suspend + resume
> yet. A still incomplete solution is in linux1394-2.6.git.)

Re: pci_set_power_state() failure and breaking suspend

[Stefan Richter]

On 24 Okt, Benjamin Herrenschmidt wrote:
>> Recently, ohci1394 grew some "proper" error handling in its suspend
>> function, something that looks like:
>> 
>>         err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
>>         if (err)
>>                 goto out;
...
> First, it breaks some old PowerBooks where the internal OHCI has no PM
> feature exposed on PCI....

What about the following? Could someone compile-test it with gcc4
and CONFIG_IEEE1394_VERBOSEDEBUG=n?



From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: ieee1394: ohci1394: revert fail on error in suspend

The error checks in the suspend code were too harsh and broke
suspend on some PPC_PMACs again which have extra platform code.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/ohci1394.c
===================================================================
--- linux.orig/drivers/ieee1394/ohci1394.c	2006-10-25 08:26:44.000000000 +0200
+++ linux/drivers/ieee1394/ohci1394.c	2006-10-25 08:28:34.000000000 +0200
@@ -3553,11 +3553,16 @@ static int ohci1394_pci_suspend (struct 
 	int err;
 
 	err = pci_save_state(pdev);
-	if (err)
-		goto out;
+	if (err) {
+		printk(KERN_ERR "%s: pci_save_state failed with %d\n",
+		       OHCI1394_DRIVER_NAME, err);
+		return err;
+	}
 	err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
+#ifdef OHCI1394_DEBUG
 	if (err)
-		goto out;
+		printk(KERN_DEBUG "pci_set_power_state failed %d\n", err);
+#endif /* OHCI1394_DEBUG */
 
 /* PowerMac suspend code comes last */
 #ifdef CONFIG_PPC_PMAC
@@ -3570,8 +3575,8 @@ static int ohci1394_pci_suspend (struct 
 			pmac_call_feature(PMAC_FTR_1394_ENABLE, of_node, 0, 0);
 	}
 #endif /* CONFIG_PPC_PMAC */
-out:
-	return err;
+
+	return 0;
 }
 #endif /* CONFIG_PM */
 

Re: pci_set_power_state() failure and breaking suspend

[Stefan Richter]

I wrote:
> +		printk(KERN_DEBUG "pci_set_power_state failed %d\n", err);
                                                  ...with... ^
-- 
Stefan Richter
-=====-=-==- =-=- ==--=
http://arcgraph.de/sr/

Re: pci_set_power_state() failure and breaking suspend

[Stefan Richter]

And this should go on top of ohci1394_pci_suspend:

+	printk(KERN_INFO "%s does not fully support suspend yet\n",
+	       OHCI1394_DRIVER_NAME);
+
-- 
Stefan Richter
-=====-=-==- =-=- ==--=
http://arcgraph.de/sr/

Re: pci_set_power_state() failure and breaking suspend

[Rafael J. Wysocki]

On Tuesday, 24 October 2006 08:54, Benjamin Herrenschmidt wrote:
> So I noticed a small regression that I think might uncover a deeper
> issue...
> 
> Recently, ohci1394 grew some "proper" error handling in its suspend
> function, something that looks like:
> 
>         err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
>         if (err)
>                 goto out;
> 
> First, it breaks some old PowerBooks where the internal OHCI had PM
> feature exposed on PCI (the pmac specific code that follows those lines
> is enough on those machines).
> 
> That can easily be fixed by removing the if (err) goto out; statement
> and having the pmac code set err to 0 in certain conditions, and I'll be
> happy to submit a patch for this.
> 
> However, this raises the question of do we actually want to prevent
> machines to suspend when they have a PCI device that don't have the PCI
> PM capability ? I'm asking that because I can easily imagine that sort
> of construct growing into more drivers (sounds logical if you don't
> think) and I can even imagine somebody thinking it's a good idea to slap
> a __must_check on pci_set_power_state() ... 

As far as the suspend to RAM is concerned, I don't know.

For the suspend to disk we can ignore the error if we know that the device
in question won't do anything like a DMA transfer into memory while we're
creating the suspend image.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: pci_set_power_state() failure and breaking suspend

[Scott Wood]

Rafael J. Wysocki wrote:
> On Tuesday, 24 October 2006 08:54, Benjamin Herrenschmidt wrote:
>>However, this raises the question of do we actually want to prevent
>>machines to suspend when they have a PCI device that don't have the PCI
>>PM capability ? I'm asking that because I can easily imagine that sort
>>of construct growing into more drivers (sounds logical if you don't
>>think) and I can even imagine somebody thinking it's a good idea to slap
>>a __must_check on pci_set_power_state() ... 
> 
> 
> As far as the suspend to RAM is concerned, I don't know.
> 
> For the suspend to disk we can ignore the error if we know that the device
> in question won't do anything like a DMA transfer into memory while we're
> creating the suspend image.

I think it should be ignored for suspend-to-RAM as well; even if a 
device or two is consuming unnecessary power, it's better than not being 
able to suspend at all, causing more things to consume unnecessary power.

At most, a warning should be issued so the user knows what's going on, 
and can choose whether to suspend to disk instead (or choose to complain 
to the device manufacturer).

-Scott