Message-ID: <aPaWcYgduGIHno3x@google.com>
Date: Mon, 20 Oct 2025 13:07:13 -0700
From: Brian Norris <briannorris@...omium.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-pm@...r.kernel.org, Lukas Wunner <lukas@...ner.de>,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v2] PCI/PM: Prevent runtime suspend before devices are
 fully initialized

Hi,

On Sat, Oct 18, 2025 at 01:27:13PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 17, 2025 at 9:22 PM Brian Norris <briannorris@...omium.org> wrote:
> >
> > Today, it's possible for a PCI device to be created and
> > runtime-suspended before it is fully initialized. When that happens, the
> > device will remain in D0, but the suspend process may save an
> > intermediate version of that device's state -- for example, without
> > appropriate BAR configuration. When the device later resumes, we'll
> > restore invalid PCI state and the device may not function.
> >
> > Prevent runtime suspend for PCI devices by deferring pm_runtime_enable()
> > until we've fully initialized the device.
> >
> > More details on how exactly this may occur:
> >
> > 1. PCI device is created by pci_scan_slot() or similar
> > 2. As part of pci_scan_slot(), pci_pm_init() enables runtime PM; the
> >    device starts "active" and we initially prevent (pm_runtime_forbid())
> >    suspend -- but see [*] footnote
> > 3. Underlying 'struct device' is added to the system (device_add());
> >    runtime PM can now be configured by user space
> > 4. PCI device receives BAR configuration
> >    (pci_assign_unassigned_bus_resources(), etc.)
> > 5. PCI device is added to the system in pci_bus_add_device()
> >
> > The device may potentially suspend between #3 and #4.
> >
> > [*] By default, pm_runtime_forbid() prevents suspending a device; but by
> > design, this can be overridden by user space policy via
> >
> >   echo auto > /sys/bus/pci/devices/.../power/control
> >
> > Thus, the above #3/#4 sequence is racy with user space (udev or
> > similar).
> >
> > Notably, many PCI devices are enumerated at subsys_initcall time and so
> > will not race with user space. However, there are several scenarios
> > where PCI devices are created later on, such as with hotplug or when
> > drivers (pwrctrl or controller drivers) are built as modules.
> >
> > Signed-off-by: Brian Norris <briannorris@...omium.org>
> > Cc: <stable@...r.kernel.org>
> 
> Can you please add a Link: pointer to the discussion on the previous
> version of the patch?

Ha, it sounds like you want me to get flamed by Linus :) I don't even
know how "Link:" is supposed to be used any more.

But in case Bjorn wants to apply this as-is, here's a reference to v1,
where that discussion happened:

Link: https://lore.kernel.org/all/20251016155335.1.I60a53c170a8596661883bd2b4ef475155c7aa72b@changeid/

If I send a v3, I'll include that.

<soap-box while we're at it:>
I really wish this proposal didn't also get flamed out:

Subject: [Ksummit-discuss] Allowing something Change-Id (or something like it) in kernel commits
https://lore.kernel.org/all/CAD=FV=UPjPpUyFTPjF-Ogzj_6LJLE4PTxMhCoCEDmH1LXSSmpQ@mail.gmail.com/

If we did that, then no one would have to ask for series-history links,
because changes would already include such IDs so anyone could follow
them automatically.

As a compromise, I've been doing this:
https://lore.kernel.org/all/CAD=FV=VLMFxFt55oB4ERTFw3xnH4czUY5tXiqfY14NKZ8gqojA@mail.gmail.com/

i.e., my patches tend to have Message-Ids that look like
<$timestamp.$revision.$changeid@...ngeid>, and so you can cleanly
track this patch, including past and future versions, with:

https://lore.kernel.org/all/?q=I60a53c170a8596661883bd2b4ef475155c7aa72b%40changeid
</soap-box>

> With that
> 
> Reviewed-by: Rafael J. Wysocki (Intel) <rafael@...nel.org>

Thanks!

Brian

Powered by blists - more mailing lists