Re: [PATCH] pci: Don't call resume callback for nearly bound devices
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-11-10 14:14:44
Also in:
linux-i2c, linux-pci, lkml
On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:quoted
On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki [off-list ref] wrote:quoted
...
quoted
quoted
So instead, we can drop the pm_runtime_get_sync() and pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(), respectively, and add pm_runtine_get_noresume() to pci_pm_init(), which will prevent PM-runtime from touching the device until it has a driver that supports PM-runtime. We'll lose the theoretical ability to put unbound devices into D3 this way, but we learned some time ago that this isn't safe in all cases anyway.IOW, something like this (untested and most likely white-space-damaged).Thanks! I applied this manually to for-linus in hopes of making the the next linux-next build. Please send any testing reports and corrections to the patch and commit log!
Robert, I hate to ask even more of you, but if you have a chance, it would be very helpful if you could test the patch below. I'm pretty sure it should fix the problem you saw, and I hope to ask Linus to merge it today.
quoted hunk ↗ jump to hunk
commit dd414877b58b ("PCI/PM: Prevent runtime PM until claimed by a driver that supports it") Author: Bjorn Helgaas [off-list ref] Date: Tue Nov 9 13:36:09 2021 -0600 PCI/PM: Prevent runtime PM until claimed by a driver that supports it Previously we had a path that could call a driver's ->runtime_resume() method before calling the driver's ->probe() method, which is a problem because ->runtime_resume() often relies on initialization done in ->probe(): local_pci_probe pm_runtime_get_sync ... pci_pm_runtime_resume if (!pci_dev->driver) return 0; <-- early exit dev->driver->pm->runtime_resume(); <-- driver ->runtime_resume() pci_dev->driver = pci_drv; pci_drv->probe() <-- driver ->probe() Prior to 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver"), we took the early exit, which avoided the problem. But 2a4d9408c9e8 removed pci_dev->driver (since it's redundant with device->driver), so we no longer take the early exit, which leads to havoc in ->runtime_resume(). Similarly, we could call the driver's ->runtime_idle() method after its ->remove() method. Avoid the problem by dropping the pm_runtime_get_sync() and pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(), respectively. Add pm_runtime_get_noresume(), which uses no driver PM callbacks, to the pci_pm_init() enumeration path. This will prevent PM-runtime from touching the device until it has a driver that supports PM-runtime. Link: https://lore.kernel.org/r/CAJZ5v0impb8uscbp8LUTBMExfMoGz=cPrTWhSGh0GF_SANNKPQ@mail.gmail.com (local) Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver") Reported-by: Robert Święcki [off-list ref] Suggested-by: Rafael J. Wysocki [off-list ref] Signed-off-by: Bjorn Helgaas [off-list ref]diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 1d98c974381c..41cdf510214f 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c@@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi) struct device *dev = &pci_dev->dev; int rc; - /* - * Unbound PCI devices are always put in D0, regardless of - * runtime PM status. During probe, the device is set to - * active and the usage count is incremented. If the driver - * supports runtime PM, it should call pm_runtime_put_noidle(), - * or any other runtime PM helper function decrementing the usage - * count, in its probe routine and pm_runtime_get_noresume() in - * its remove routine. - */ - pm_runtime_get_sync(dev); rc = pci_drv->probe(pci_dev, ddi->id); if (!rc) return rc;@@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev) pcibios_free_irq(pci_dev); pci_iov_remove(pci_dev); - /* Undo the runtime PM settings in local_pci_probe() */ - pm_runtime_put_sync(dev); - /* * If the device is still on, set the power state as "unknown", * since it might change by the next time we load the driver.diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b88db815ee01..e9c38b994c73 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c@@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev) u16 pmc; pm_runtime_forbid(&dev->dev); + + /* + * Unbound PCI devices are always put in D0. If the driver supports + * runtime PM, it should call pm_runtime_put_noidle(), or any other + * runtime PM helper function decrementing the usage count, in its + * probe routine and pm_runtime_get_noresume() in its remove routine. + */ pm_runtime_set_active(&dev->dev); + pm_runtime_get_noresume(&dev->dev); pm_runtime_enable(&dev->dev); device_enable_async_suspend(&dev->dev); dev->wakeup_prepared = false;