lkml.org 
[lkml]   [2009]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: PCI PM: Restore standard config registers of all devices early
    Date
    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


    \
     
     \ /
      Last update: 2009-02-03 00:49    [W:0.030 / U:59.644 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site