Re: [PATCH v5 2/5] PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev_wait()

From: Mario Limonciello
Date: Fri Sep 13 2024 - 16:57:06 EST


On 9/13/2024 02:23, Mika Westerberg wrote:
Hi again,

On Fri, Sep 13, 2024 at 07:58:07AM +0300, Mika Westerberg wrote:
Yeah, I agree now. It does not look like the methods are messing each
other here. We don't see the GPE handler being run but I don't think it
matters here. For some reason the device just is not yet ready when it
is supposed to be in D0.

Sorry for wasting your time with these suspects.

Don't worry about it. It validates that we're coming to the right conclusions about where the problem is.


One more suggestion though ;-) I realized that my hack patch to disable
I/O and MMIO did not actually do that because it looks like we don't
clear those bits ever. I wonder if you could still check if the below
changes anything? At least it should now "disable" the device to follow
the spec.

This actually causes the system to fail to boot. I guess some deadlock from other callers to pci_disable_device().

We also double checked putting the PCI_COMMAND writes just into the runtime suspend call backs instead (to narrow down if that is part of the issue here). Putting it there fixed the boot hang, but no change to the actual issue behavior.

Bjorn,

Considering this finding, are you more amenable to the polling approach?


diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f412ef73a6e4..79a566376301 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1332,6 +1332,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
if (!pci_dev->state_saved) {
pci_save_state(pci_dev);
+ pci_pm_default_suspend(pci_dev);
pci_finish_runtime_suspend(pci_dev);
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ffaaca0978cb..91f4e7a03c94 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2218,6 +2218,13 @@ static void do_pci_disable_device(struct pci_dev *dev)
pci_command &= ~PCI_COMMAND_MASTER;
pci_write_config_word(dev, PCI_COMMAND, pci_command);
}
+ /*
+ * PCI PM 1.2 sec 8.2.2 says that when a function is put into D3
+ * the OS needs to disable I/O and MMIO space in addition to bus
+ * mastering so do that here.
+ */
+ pci_command &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
+ pci_write_config_word(dev, PCI_COMMAND, pci_command);
pcibios_disable_device(dev);
}