login
Search
Hi, After the recent discussion with Linus, I have the following three fixes of the PCI PM framework. The first patch fixes the bug that bridges (and PCIe ports) are disabled during suspend, althouth they shouldn't. The second one makes the PCI PM core handle devices more carefully (details in the changelog). [Note to Linus: devices are still put into low power states with interrupts on after this patch. Moving that to the late suspend phase will be the next step.] The last patch makes the warning in pci_legacy_suspend() more useful. Thanks, Rafael --
From: Rafael J. Wysocki <rjw@sisk.pl> The warning in pci_legacy_suspend() would be much more useful if it printed the name of the function that did the wrong thing. Make it do so. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/pci/pci-driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: linux-2.6/drivers/pci/pci-driver.c =================================================================== --- linux-2.6.orig/drivers/pci/pci-driver.c +++ linux-2.6/drivers/pci/pci-driver.c @@ -365,7 +365,9 @@ static int pci_legacy_suspend(struct dev if (pci_dev->state_saved) goto Fixup; - if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0)) + if (WARN_ONCE(pci_dev->current_state != PCI_D0, + "PCI PM: Device state not saved by %pF\n", + drv->suspend)) goto Fixup; } --
Hi again. Am I right in thinking that WARN_ONCE will only warn about the first driver that has the problem? If so, wouldn't it be better to make it warn about all drivers that have the problem, but only once per device? Regards, Nigel --
That would be better, but I don't think we are going to see many of these anyway. :-) Thanks, Rafael --
From: Rafael J. Wysocki <rjw@sisk.pl>
It is a mistake to disable and enable PCI bridges and PCI Express
ports during suspend-resume (at least at the time when it is
currently done). Disabling them may lead to problems with accessing
devices behind them and they should be automatically enabled when
their standard config spaces are restored. Fix this by not attempting
to disable bridges during suspend and enable them during resume.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/pci/pci-driver.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -428,16 +428,18 @@ static int pci_pm_default_resume(struct
{
pci_fixup_device(pci_fixup_resume, pci_dev);
- if (!pci_is_bridge(pci_dev))
- pci_enable_wake(pci_dev, PCI_D0, false);
+ if (pci_is_bridge(pci_dev))
+ return 0;
+ pci_enable_wake(pci_dev, PCI_D0, false);
return pci_pm_reenable_device(pci_dev);
}
static void pci_pm_default_suspend_generic(struct pci_dev *pci_dev)
{
- /* If device is enabled at this point, disable it */
- pci_disable_enabled_device(pci_dev);
+ /* If a non-bridge device is enabled at this point, disable it */
+ if (!pci_is_bridge(pci_dev))
+ pci_disable_enabled_device(pci_dev);
/*
* Save state with interrupts enabled, because in principle the bus the
* device is on may be put into a low power state after this code runs.
--
Are you sure? This goes from doing reenable_device to not doing it for bridges, seemingly contradicting changelog? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html --
Can you explain what you mean, please? Thanks, Rafael --
It looks to me like the patch does not match the changelog. Changelog says "enable bridge during resume", but code does return 0 if it seems bridge. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html --
Ah, I see. The changelog was supposed to mean that the bridges would not be disabled during suspend and also would not be enabled during resume (which shouldn't be necessary, since they would be automatically enabled as a result of restoring their configuration registers). Thanks, -- Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it? --- Brian Kernighan --
You can add my acked-by: when you resubmit with fixed changelog... :-). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html --
It's merged, so that wouldn't be of any use. Thanks, Rafael --
From: Rafael J. Wysocki <rjw@sisk.pl> Currently, the PM core always attempts to manage devices with drivers that use the new PM framework. In particular, it attempts to disable the devices (which is unnecessary), to save their state (which may be undesirable if the driver has done that already) and to put them into low power states (again, this may be undesirable if the driver has already put the device into a low power state). That need not be the right thing to do, so make the core be more careful in this respect. Generally, there are the following categories of devices to consider: * bridge devices without drivers * non-bridge devices without drivers * bridge devices with drivers * non-bridge devices with drivers and each of them should be handled differently. For bridge devices without drivers the PCI PM core will save their state on suspend and restore it (early) during resume, after putting them into D0 if necessary. It will not attepmt to do anything else to these devices. For non-bridge devices without drivers the PCI PM core will disable them and save their state on suspend. During resume, it will put them into D0, if necessary, restore their state (early) and reenable them. For bridge devices without drivers the PCI PM core will only save their state on suspend if the driver hasn't done that already. Still, the core will restore their state (early) during resume, after putting them into D0, if necessary. For non-bridge devices with drivers the PCI PM core will only save their state on suspend if the driver hasn't done that already. Also, if the state of the device hasn't been saved by the driver, the core will attempt to put the device into a low power state. During resume the core will restore the state of the device (early), after putting it into D0, if necessary. For all devices the core will disable wake-up during resume. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/pci/pci-driver.c | 138 ...
Hi Rafael. Just a couple of typos in the comment: I'm not pretending to understand the gory details of the code itself, and so don't want you to add a Reviewed-by or such like for me, thanks. Regards, Nigel --
Thanks for the fixes, I'll resend the patch with updated changelog. Best, Rafael --
Appended. Thanks, Rafael --- From: Rafael J. Wysocki <rjw@sisk.pl> Subject: PCI PM: Let the core be more careful with respect to drivers using new framework Currently, the PM core always attempts to manage devices with drivers that use the new PM framework. In particular, it attempts to disable the devices (which is unnecessary), to save their state (which may be undesirable if the driver has done that already) and to put them into low power states (again, this may be undesirable if the driver has already put the device into a low power state). That need not be the right thing to do, so make the core be more careful in this respect. Generally, there are the following categories of devices to consider: * bridge devices without drivers * non-bridge devices without drivers * bridge devices with drivers * non-bridge devices with drivers and each of them should be handled differently. For bridge devices without drivers the PCI PM core will save their state on suspend and restore it (early) during resume, after putting them into D0 if necessary. It will not attempt to do anything else to these devices. For non-bridge devices without drivers the PCI PM core will disable them and save their state on suspend. During resume, it will put them into D0, if necessary, restore their state (early) and reenable them. For bridge devices with drivers the PCI PM core will only save their state on suspend if the driver hasn't done that already. Still, the core will restore their state (early) during resume, after putting them into D0, if necessary. For non-bridge devices with drivers the PCI PM core will only save their state on suspend if the driver hasn't done that already. Also, if the state of the device hasn't been saved by the driver, the core will attempt to put the device into a low power state. During resume the core will restore the state of the device (early), after putting it into D0, if necessary. For all devices the core will disable wake-up during ...
Linus, do you want these as part of the next pull request? If so, I'll check them out, pull them in, and let them run the linux-next build gauntlet for a day before sending them. Otherwise I'll just send a pull request today with the 8 or so fixes I have queued now. -- Jesse Barnes, Intel Open Source Technology Center --
I want them eventually, but I don't know if we got confirmation that this particular series actually fixed the PCI-E bridge issue? The one that Parag reported.. It would be good to have confirmation. Maybe we do already and I missed that email? Linus --
An additional patch against the PCIe port driver will be necessary to fix this issue, but I'm waiting for feedback from Parag about the patches already posted to find out what's the minimal fix. Thanks, Rafael --
