Re: PCI PM: Restore standard config registers of all devices early

Linus Torvalds
2009-02-02T23:19:48Z

On Mon, 2 Feb 2009, Rafael J. Wysocki wrote:
> >
> > b) disable_device_irq()'s: things are live, but device interrupts
> > are turned off by essentially looping over the irq_desc_ptr[]
> > table.
>
> Well, do we actually need to turn off all device interrupts?
>
> Shared interrupts are the source of the problem, so perhaps we can
> only disable interrupts of devices that use interrupt pins at this point
> (MSI/MSI-X need not be disabled, for example, and the timer interrupts most
> probably too)?

We could try that, yes.

> > d) disable CPU interrupts.
>
> At what point do we disable the other CPUs?

I left it out, because I don't much care or think it matters. So take your
pick. I'd suggest keeping the current setup, and literally just insert the
new point between "device_power_off()" and "sysdev_suspend()", with _zero_
other changes.

> Well, it means reworking the entire suspend sequence (again) or we will
> break assumptions made by some existing drivers (interrupts off during
> suspend_late and resume_early). And that affects all drivers, not only PCI.

No it doesn't.

No changes AT ALL to the suspend sequence. We do everything in the same
order: look at my patch. The only difference is that instead of doing that
"cli" we do the "for_each_irq(disable_irq)" instead (and do the 'cli' a
bit later).

ZERO effect on drivers. The calling convention is 100% the same as far as
the driver is concerned: ->suspend() is called with interrupts on and a
fully working machine, and ->suspend_late() called with interrupts off.

The only difference is the _mechanism_ of turning interrupts off. NOTHING
else.

> I first would like to understand what _exactly_ breaks on the iBook reported to
> have problems.

I bet it's code like the USB one:

int usb_hcd_pci_resume(struct pci_dev *dev)
{

#ifdef CONFIG_PPC_PMAC
/* Reenable ASIC clocks for USB */
if (machine_is(powermac)) {
struct device_node *of_node;

of_node = pci_device_to_OF_node(dev);
if (of_node)
pmac_call_feature(PMAC_FTR_USB_ENABLE,
of_node, 0, 1);
}
#endif
..
retval = pci_enable_device(dev);

and now pci_enable_device() calls pci_raw_set_power_state(), which does:

if (dev->current_state == state) {
/* we're already there */
return 0;
} else ..

which means that it doesn't actually _do_ anything, because it thinks that
'current_state' was already PCI_D0. But if the device was totally turned
off, that's wrong.

(background: pci_restore_standard_config() will have done
pci_raw_set_power_state(PCI_D0) with the device clocks off, which wouldn't
actualyl have _done_ anythign to the device, but then it does

dev->current_state = PCI_D0;

Maybe the simplest thing to do migth be to replace that with a

pci_update_current_state(dev, PCI_D0);

instead, to try to read back the state explicitly in
pci_restore_standard_config()).

Best test:

Ben, does this trivial patch make any difference for those powermacs?

Linus
---
drivers/pci/pci.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 17bd932..97e1c38 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1419,7 +1419,7 @@ int pci_restore_standard_config(struct pci_dev *dev)
}
}

- dev->current_state = PCI_D0;
+ pci_update_current_state(dev, PCI_D0);

return 0;
}