diff mbox series

[v2] PCI/PM: enable runtime PM later during device scanning

Message ID 20230605203519.bc4232207449.Idbaa55b93f780838af44ebccb84c36f60716df04@changeid
State New
Headers show
Series [v2] PCI/PM: enable runtime PM later during device scanning | expand

Commit Message

Johannes Berg June 5, 2023, 6:35 p.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

We found that that the following race is possible if userspace
enables runtime PM/auto-suspend immediately when a device shows
up in sysfs, if there's any call to pci_rescan_bus() during
normal system state (i.e. userspace is already active):

 - we rescan the PCI bus (*)
 - this creates the new PCI device including its sysfs
   representation
 - udev sees the new device, and the (OS-specific) scripting
   enables runtime PM by writing to power/control; this can
   happen _before_ the next step - this will runtime-suspend
   the device which saves the config space, including the BARs
   that weren't assigned yet
 - the bus rescan assigns resources to the devices and writes
   them to the config space of the device
   (but not the runtime-pm saved copy, course)
 - the driver binds and this disallows runtime PM, so the device
   is resumed, restoring the (incomplete!) config space
 - the device cannot work due to BARs not being configured

Fix this by allowing runtime PM only once the device has been
fully added. Also, with a warning, reject runtime PM on a not-
added device; this shouldn't happen anymore now.

Note that the comment that was there (that I'm replacing) was
indicating that pci_device_add() wouldn't be called at this
place yet, but in fact it's called much earlier during the whole
scan/probe process, which in part causes this problem, but it
doesn't seem possible to defer it until here either.

(*) In the case we encountered, this happened due to some reset
    of the iwlwifi device that the driver then needs to recover
    from by rescanning the bus since the device was reset and
    the system doesn't know about it yet.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v2: use pm_runtime_get_noresume()/pm_runtime_put_noidle()
    instead as advised by Rafael
---
 drivers/pci/bus.c        | 8 ++++++--
 drivers/pci/pci-driver.c | 3 +++
 drivers/pci/pci.c        | 1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Johannes Berg June 6, 2023, 7:22 a.m. UTC | #1 
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
Lukas Wunner June 7, 2023, 7:49 a.m. UTC | #2 
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
Johannes Berg June 7, 2023, 7:58 a.m. UTC | #3 
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 mbox series

Patch

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);