| Message ID | 20230605203519.bc4232207449.Idbaa55b93f780838af44ebccb84c36f60716df04@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI/PM: enable runtime PM later during device scanning | expand |
On Mon, Jun 05, 2023 at 08:35:45PM +0200, Johannes Berg wrote: > v2: use pm_runtime_get_noresume()/pm_runtime_put_noidle() > instead as advised by Rafael You've changed the code but seemingly did not update the commit message and code comment. Technically you're not "allowing" runtime PM, you just stop keeping the device runtime active. A more fitting subject might thus be: PCI/PM: Keep devices runtime active during enumeration > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -1278,6 +1278,9 @@ static int pci_pm_runtime_suspend(struct device *dev) > pci_power_t prev = pci_dev->current_state; > int error; > > + if (WARN_ON(!pci_dev_is_added(pci_dev))) > + return -EBUSY; > + If this can't happen (as the commit message says), why warn? Thanks, Lukas
On Mon, 2023-06-05 at 22:50 +0200, Lukas Wunner wrote: > On Mon, Jun 05, 2023 at 08:35:45PM +0200, Johannes Berg wrote: > > v2: use pm_runtime_get_noresume()/pm_runtime_put_noidle() > > instead as advised by Rafael > > You've changed the code but seemingly did not update the commit > message and code comment. > Yeah. I actually _considered_ that, but didn't feel it was really any different (or let's say wrong) now. That said, I probably don't understand the lingo around runtime PM well enough, and am more or less conflating "runtime PM" and "runtime suspend" in my head, which is still not allowed, and indeed that's the whole point of the patch. > Technically you're not "allowing" > runtime PM, you just stop keeping the device runtime active. > > A more fitting subject might thus be: > > PCI/PM: Keep devices runtime active during enumeration *shrug* Like I said, terminology I'm not familiar with. I guess I can change it, or if anyone ends up committing it as is (rather than treating it as an extended bug report) they can :-) > > --- a/drivers/pci/pci-driver.c > > +++ b/drivers/pci/pci-driver.c > > @@ -1278,6 +1278,9 @@ static int pci_pm_runtime_suspend(struct device *dev) > > pci_power_t prev = pci_dev->current_state; > > int error; > > > > + if (WARN_ON(!pci_dev_is_added(pci_dev))) > > + return -EBUSY; > > + > > If this can't happen (as the commit message says), why warn? The code here causes quite some trouble if it _does_ happen and it was incredibly tricky to debug. johannes
On Mon, Jun 05, 2023 at 08:35:45PM +0200, Johannes Berg wrote: > @@ -3139,6 +3139,7 @@ void pci_pm_init(struct pci_dev *dev) > u16 pmc; > > pm_runtime_forbid(&dev->dev); > + pm_runtime_get_noresume(&dev->dev); > pm_runtime_set_active(&dev->dev); > pm_runtime_enable(&dev->dev); > device_enable_async_suspend(&dev->dev); > @@ -335,9 +336,12 @@ void pci_bus_add_device(struct pci_dev *dev) > int retval; > > /* > - * Can not put in pci_device_add yet because resources > - * are not assigned yet for some devices. > + * Allow runtime PM only here, since otherwise we may > + * try to suspend a device that isn't fully configured > + * yet, which causes problems. > */ > + pm_runtime_put_noidle(&dev->dev); > + > pcibios_bus_add_device(dev); > pci_fixup_device(pci_fixup_final, dev); > pci_create_sysfs_dev_files(dev); There seem to be many different callers that end up in pci_pm_init() and pci_bus_add_device(). Is it guaranteed that the two functions are always called in order? Do callers exist which only invoke the former but not the latter or vice-versa? Can it happen that a caller of the former errors out, so the latter is never called, leading to a runtime PM ref imbalance? It would be easier to ascertain correctness if you could find a function at a higher level which (indirectly) calls both pci_pm_init() and pci_bus_add_device() so that you can acquire and release the runtimw PM ref in that single function. Thanks, Lukas
On Wed, 2023-06-07 at 09:49 +0200, Lukas Wunner wrote: > On Mon, Jun 05, 2023 at 08:35:45PM +0200, Johannes Berg wrote: > > @@ -3139,6 +3139,7 @@ void pci_pm_init(struct pci_dev *dev) > > u16 pmc; > > > > pm_runtime_forbid(&dev->dev); > > + pm_runtime_get_noresume(&dev->dev); > > pm_runtime_set_active(&dev->dev); > > pm_runtime_enable(&dev->dev); > > device_enable_async_suspend(&dev->dev); > > @@ -335,9 +336,12 @@ void pci_bus_add_device(struct pci_dev *dev) > > int retval; > > > > /* > > - * Can not put in pci_device_add yet because resources > > - * are not assigned yet for some devices. > > + * Allow runtime PM only here, since otherwise we may > > + * try to suspend a device that isn't fully configured > > + * yet, which causes problems. > > */ > > + pm_runtime_put_noidle(&dev->dev); > > + > > pcibios_bus_add_device(dev); > > pci_fixup_device(pci_fixup_final, dev); > > pci_create_sysfs_dev_files(dev); > > There seem to be many different callers that end up in pci_pm_init() > and pci_bus_add_device(). > > Is it guaranteed that the two functions are always called in order? > Do callers exist which only invoke the former but not the latter or > vice-versa? Can it happen that a caller of the former errors out, > so the latter is never called, leading to a runtime PM ref imbalance? I did ask myself that too, and honestly, I'm not entirely sure - need somebody more familiar to really understand that, I think. Most places elsewhere _do_ call both, and it feels like you have to call both if you want to do something with the device. However there are a few places that seem to call the first part and then remove the device again immediately after. That also seems harmless though. > It would be easier to ascertain correctness if you could find a > function at a higher level which (indirectly) calls both pci_pm_init() > and pci_bus_add_device() so that you can acquire and release the > runtimw PM ref in that single function. > Unfortunately, there isn't such a place, since the scanning is done by various bus walks. johannes
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 5bc81cc0a2de..e06ea5449be9 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -13,6 +13,7 @@ #include <linux/ioport.h> #include <linux/proc_fs.h> #include <linux/slab.h> +#include <linux/pm_runtime.h> #include "pci.h" @@ -335,9 +336,12 @@ void pci_bus_add_device(struct pci_dev *dev) int retval; /* - * Can not put in pci_device_add yet because resources - * are not assigned yet for some devices. + * Allow runtime PM only here, since otherwise we may + * try to suspend a device that isn't fully configured + * yet, which causes problems. */ + pm_runtime_put_noidle(&dev->dev); + pcibios_bus_add_device(dev); pci_fixup_device(pci_fixup_final, dev); pci_create_sysfs_dev_files(dev); diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index ae9baf801681..8d82b4abb169 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1278,6 +1278,9 @@ static int pci_pm_runtime_suspend(struct device *dev) pci_power_t prev = pci_dev->current_state; int error; + if (WARN_ON(!pci_dev_is_added(pci_dev))) + return -EBUSY; + pci_suspend_ptm(pci_dev); /* diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 5ede93222bc1..808906ad14b9 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3139,6 +3139,7 @@ void pci_pm_init(struct pci_dev *dev) u16 pmc; pm_runtime_forbid(&dev->dev); + pm_runtime_get_noresume(&dev->dev); pm_runtime_set_active(&dev->dev); pm_runtime_enable(&dev->dev); device_enable_async_suspend(&dev->dev);