Re: [PATCH] pci: fix kexec with power state D3
From: Rafael J. Wysocki <hidden>
Date: 2009-03-08 10:08:29
Also in:
lkml
On Sunday 08 March 2009, Yinghai Lu wrote:
Impact: second kernel by kexec will have some pci devices working Found one system with 82575EB, in the kernel that is kexeced, probe igb failed with -2. it looks like the same behavior happened on forcedeth. try to check system_state to make sure if put it on D3
This is not enough, because the PM code doesn't change system_state and it uses pci_set_power_state too.
Jesse Brandeburg said that we should do that check in core code instead of every device driver.
Well, I'm not really sure. The drivers are where the bug is.
quoted hunk ↗ jump to hunk
Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/pci.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) Index: linux-2.6/drivers/pci/pci.c ===================================================================--- linux-2.6.orig/drivers/pci/pci.c +++ linux-2.6/drivers/pci/pci.c@@ -593,6 +593,14 @@ int pci_set_power_state(struct pci_dev * if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3)) return 0; + /* + * Apparently it is not possible to reinitialise from D3 hot, + * only put the device into D3 if we really go for poweroff. + */ + if (system_state != SYSTEM_POWER_OFF && + (state == PCI_D3hot || state == PCI_D3cold)) + return 0; +
This breaks suspend/hibernation, doesn't it? Surely, we want to put devices into D3 when going for suspend, for example. That's apart from the fact that the 'state == PCI_D3cold' is redundant.
quoted hunk ↗ jump to hunk
error = pci_raw_set_power_state(dev, state, true); if (state > PCI_D0 && platform_pci_power_manageable(dev)) {@@ -1124,6 +1132,15 @@ int pci_enable_wake(struct pci_dev *dev, int error = 0; bool pme_done = false; + /* + * Apparently it is not possible to reinitialise from D3 hot, + * only put the device into D3 if we really go for poweroff. + * we only need to enable wake when we are going to power off + */ + if (enable && system_state != SYSTEM_POWER_OFF && + (state == PCI_D3hot || state == PCI_D3cold)) + return 0; + if (enable && !device_may_wakeup(&dev->dev)) return -EINVAL;
I don't like this at all, sorry. I thought we were supposed to avoid using system_state in such a way, apart from the other things. Thanks, Rafael