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

Rafael J. Wysocki
2009-02-02T23:46:19Z

On Tuesday 03 February 2009, Linus Torvalds wrote:
>
> 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.

So, perhaps we can just loop over the interrupt links and disable them all
at this point?

> > > 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.

Ah, OK.

> > 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).

Sounds good.

> 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);

Good idea anyway.

> return 0;
> }

Or perhaps put

current_state = PCI_UNKNOWN;

under that 'if (machine_is(powermac))' ?

Thanks,
Rafael