Message-ID: <6e0c511f-1127-4a35-a40c-0161de0ec752@samsung.com>
Date: Mon, 19 Jan 2026 14:13:03 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Brian Norris <briannorris@...omium.org>, Bjorn Helgaas
	<helgaas@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Lukas Wunner
	<lukas@...ner.de>, linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-pm@...r.kernel.org, Ilpo Järvinen
	<ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v4] PCI/PM: Prevent runtime suspend before devices are
 fully initialized

On 19.01.2026 13:26, Rafael J. Wysocki wrote:
> On Mon, Jan 19, 2026 at 11:01 AM Marek Szyprowski
> <m.szyprowski@...sung.com> wrote:
>> On 18.01.2026 12:59, Rafael J. Wysocki wrote:
>>> On Sun, Jan 18, 2026 at 12:53 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>>>> On Sat, Jan 17, 2026 at 2:19 AM Brian Norris <briannorris@...omium.org> wrote:
>>>>> On Thu, Jan 15, 2026 at 12:14:49PM +0100, Marek Szyprowski wrote:
>>>>>> On 14.01.2026 21:10, Brian Norris wrote:
>>>>>>> On Wed, Jan 14, 2026 at 10:46:41AM +0100, Marek Szyprowski wrote:
>>>>>>>> On 06.01.2026 23:27, Bjorn Helgaas wrote:
>>>>>>>>> On Thu, Oct 23, 2025 at 02:09:01PM -0700, Brian Norris 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.
>>>>>>> ...
>>>>>>>> This patch landed recently in linux-next as commit c796513dc54e
>>>>>>>> ("PCI/PM: Prevent runtime suspend until devices are fully initialized").
>>>>>>>> In my tests I found that it sometimes causes the "pci 0000:01:00.0:
>>>>>>>> runtime PM trying to activate child device 0000:01:00.0 but parent
>>>>>>>> (0000:00:00.0) is not active" warning on Qualcomm Robotics RB5 board
>>>>>>>> (arch/arm64/boot/dts/qcom/qrb5165-rb5.dts). This in turn causes a
>>>>>>>> lockdep warning about console lock, but this is just a consequence of
>>>>>>>> the runtime pm warning. Reverting $subject patch on top of current
>>>>>>>> linux-next hides this warning.
>>>>>>>>
>>>>>>>> Here is a kernel log:
>>>>>>>>
>>>>>>>> pci 0000:01:00.0: [17cb:1101] type 00 class 0xff0000 PCIe Endpoint
>>>>>>>> pci 0000:01:00.0: BAR 0 [mem 0x00000000-0x000fffff 64bit]
>>>>>>>> pci 0000:01:00.0: PME# supported from D0 D3hot D3cold
>>>>>>>> pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth, limited by 5.0
>>>>>>>> GT/s PCIe x1 link at 0000:00:00.0 (capable of 7.876 Gb/s with 8.0 GT/s
>>>>>>>> PCIe x1 link)
>>>>>>>> pci 0000:01:00.0: Adding to iommu group 13
>>>>>>>> pci 0000:01:00.0: ASPM: default states L0s L1
>>>>>>>> pcieport 0000:00:00.0: bridge window [mem 0x60400000-0x604fffff]: assigned
>>>>>>>> pci 0000:01:00.0: BAR 0 [mem 0x60400000-0x604fffff 64bit]: assigned
>>>>>>>> pci 0000:01:00.0: runtime PM trying to activate child device
>>>>>>>> 0000:01:00.0 but parent (0000:00:00.0) is not active
>>>>>>> Thanks for the report. I'll try to look at reproducing this, or at least
>>>>>>> getting a better mental model of exactly why this might fail (or,
>>>>>>> "warn") this way. But if you have the time and desire to try things out
>>>>>>> for me, can you give v1 a try?
>>>>>>>
>>>>>>> https://lore.kernel.org/all/20251016155335.1.I60a53c170a8596661883bd2b4ef475155c7aa72b@changeid/
>>>>>>>
>>>>>>> I'm pretty sure it would not invoke the same problem.
>>>>>> Right, this one works fine.
>>>>>>
>>>>>>> I also suspect v3
>>>>>>> might not, but I'm less sure:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/20251022141434.v3.1.I60a53c170a8596661883bd2b4ef475155c7aa72b@changeid/
>>>>>> This one too, at least I was not able to reproduce any fail.
>>>>> Thanks for testing. I'm still not sure exactly how to reproduce your
>>>>> failure, but it seems as if the root port is being allowed to suspend
>>>>> before the endpoint is added to the system, and it remains so while the
>>>>> endpoint is about to probe. device_initial_probe() will be OK with
>>>>> respect to PM, since it will wake up the port if needed. But this
>>>>> particular code is not OK, since it doesn't ensure the parent device is
>>>>> active while preparing the endpoint power state.
>>>>>
>>>>> I suppose one way to "solve" that is (untested):
>>>>>
>>>>> --- a/drivers/pci/bus.c
>>>>> +++ b/drivers/pci/bus.c
>>>>> @@ -380,8 +380,12 @@ void pci_bus_add_device(struct pci_dev *dev)
>>>>>                   put_device(&pdev->dev);
>>>>>           }
>>>>>
>>>>> +       if (dev->dev.parent)
>>>>> +               pm_runtime_get_sync(dev->dev.parent);
>>>>>           pm_runtime_set_active(&dev->dev);
>>>>>           pm_runtime_enable(&dev->dev);
>>>>> +       if (dev->dev.parent)
>>>>> +               pm_runtime_put(dev->dev.parent);
>>>>>
>>>>>           if (!dn || of_device_is_available(dn))
>>>>>                   pci_dev_allow_binding(dev);
>>>>>
>>>>> Personally, I'm more inclined to go back to v1, since it prepares the
>>>>> runtime PM status when the device is first discovered. That way, its
>>>>> ancestors are still active, avoiding these sorts of problems. I'm
>>>>> frankly not sure of all the reasons Rafael recommended I make the
>>>>> v1->v3->v4 changes, and now that they cause problems, I'm inclined to
>>>>> question them again.
>>>>>
>>>>> Rafael, do you have any thoughts?
>>>> Yeah.
>>>>
>>>> Move back pm_runtime_set_active(&dev->dev) back to pm_runtime_init()
>>> Or rather leave it there to be precise, but I think you know what I mean. :-)
>>>
>>>> because that would prevent the parent from suspending and keep
>>>> pm_runtime_enable() here because that would prevent the device itself
>>>> from suspending between pm_runtime_init() and this place.
>>>>
>>>> And I would add comments in both places.
>> Confirmed, the following change (compared to $subject patch) fixed my issue:
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 3ef60c2fbd89..7e2b7e452d51 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -381,7 +381,6 @@ void pci_bus_add_device(struct pci_dev *dev)
>>           }
>>
>>           pm_runtime_set_active(&dev->dev);
>> -       pm_runtime_enable(&dev->dev);
> That works too, but it would defeat the purpose of the original
> change, so I mean the other way around.
>
> That is, leave the pm_runtime_enable() here and move the
> pm_runtime_set_active() back to the other place.

Okay, I mixed that. This way it works too and fixes the observed issue.

Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Powered by blists - more mailing lists