Messages in this thread Patch in this message | | | Date | Mon, 2 Feb 2009 15:18:41 -0800 (PST) | From | Linus Torvalds <> | Subject | Re: PCI PM: Restore standard config registers of all devices early |
| |
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; }
| |