Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Cabiddu, Giovanni]

Hi Mario, Bjorn and Alex,

On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> AMD BIOS team has root caused an issue that NVME storage failed to come
> back from suspend to a lack of a call to _REG when NVME device was probed.
> 
> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
> added support for calling _REG when transitioning D-states, but this only
> works if the device actually "transitions" D-states.
> 
> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
> devices") added support for runtime PM on PCI devices, but never actually
> 'explicitly' sets the device to D0.
> 
> To make sure that devices are in D0 and that platform methods such as
> _REG are called, explicitly set all devices into D0 during initialization.
> 
> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
Through a bisect, we identified that this patch, in v6.16-rc1,
introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
with -EACCES.

Upon further investigation, the -EACCES appears to originate from the
rpm_resume() function, which is called by pm_runtime_resume_and_get()
within vfio_pci_core_enable(). Here is the exact call trace:

    drivers/base/power/runtime.c: rpm_resume()
    drivers/base/power/runtime.c: __pm_runtime_resume()
    include/linux/pm_runtime.h: pm_runtime_resume_and_get()
    drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
    drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
    drivers/vfio/vfio_main.c: device->ops->open_device()
    drivers/vfio/vfio_main.c: vfio_df_device_first_open()
    drivers/vfio/vfio_main.c: vfio_df_open()
    drivers/vfio/group.c: vfio_df_group_open()
    drivers/vfio/group.c: vfio_device_open_file()
    drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
    drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)

Is this a known issue that affects other devices? Is there any ongoing
discussion or fix in progress?

Thanks,

-- 
Giovanni

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Mario Limonciello]

On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
> Hi Mario, Bjorn and Alex,
> 
> On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> AMD BIOS team has root caused an issue that NVME storage failed to come
>> back from suspend to a lack of a call to _REG when NVME device was probed.
>>
>> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
>> added support for calling _REG when transitioning D-states, but this only
>> works if the device actually "transitions" D-states.
>>
>> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
>> devices") added support for runtime PM on PCI devices, but never actually
>> 'explicitly' sets the device to D0.
>>
>> To make sure that devices are in D0 and that platform methods such as
>> _REG are called, explicitly set all devices into D0 during initialization.
>>
>> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
> Through a bisect, we identified that this patch, in v6.16-rc1,
> introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
> devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
> with -EACCES.
> 
> Upon further investigation, the -EACCES appears to originate from the
> rpm_resume() function, which is called by pm_runtime_resume_and_get()
> within vfio_pci_core_enable(). Here is the exact call trace:
> 
>      drivers/base/power/runtime.c: rpm_resume()
>      drivers/base/power/runtime.c: __pm_runtime_resume()
>      include/linux/pm_runtime.h: pm_runtime_resume_and_get()
>      drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
>      drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
>      drivers/vfio/vfio_main.c: device->ops->open_device()
>      drivers/vfio/vfio_main.c: vfio_df_device_first_open()
>      drivers/vfio/vfio_main.c: vfio_df_open()
>      drivers/vfio/group.c: vfio_df_group_open()
>      drivers/vfio/group.c: vfio_device_open_file()
>      drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
>      drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
> 
> Is this a known issue that affects other devices? Is there any ongoing
> discussion or fix in progress?
> 
> Thanks,
> 

This is the first I've heard about an issue with that patch.

Does setting the VFIO parameter disable_idle_d3 help?

If so; this feels like an imbalance of runtime PM calls in the VFIO 
stack that this patch exposed.

Alex, any ideas?

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Cabiddu, Giovanni]

On Wed, Jun 11, 2025 at 06:50:59AM -0700, Mario Limonciello wrote:
> On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
> > Hi Mario, Bjorn and Alex,
> > 
> > On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > AMD BIOS team has root caused an issue that NVME storage failed to come
> > > back from suspend to a lack of a call to _REG when NVME device was probed.
> > > 
> > > commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
> > > added support for calling _REG when transitioning D-states, but this only
> > > works if the device actually "transitions" D-states.
> > > 
> > > commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
> > > devices") added support for runtime PM on PCI devices, but never actually
> > > 'explicitly' sets the device to D0.
> > > 
> > > To make sure that devices are in D0 and that platform methods such as
> > > _REG are called, explicitly set all devices into D0 during initialization.
> > > 
> > > Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > Through a bisect, we identified that this patch, in v6.16-rc1,
> > introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
> > devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
> > with -EACCES.
> > 
> > Upon further investigation, the -EACCES appears to originate from the
> > rpm_resume() function, which is called by pm_runtime_resume_and_get()
> > within vfio_pci_core_enable(). Here is the exact call trace:
> > 
> >      drivers/base/power/runtime.c: rpm_resume()
> >      drivers/base/power/runtime.c: __pm_runtime_resume()
> >      include/linux/pm_runtime.h: pm_runtime_resume_and_get()
> >      drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
> >      drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
> >      drivers/vfio/vfio_main.c: device->ops->open_device()
> >      drivers/vfio/vfio_main.c: vfio_df_device_first_open()
> >      drivers/vfio/vfio_main.c: vfio_df_open()
> >      drivers/vfio/group.c: vfio_df_group_open()
> >      drivers/vfio/group.c: vfio_device_open_file()
> >      drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
> >      drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
> > 
> > Is this a known issue that affects other devices? Is there any ongoing
> > discussion or fix in progress?
> > 
> > Thanks,
> > 
> 
> This is the first I've heard about an issue with that patch.
> 
> Does setting the VFIO parameter disable_idle_d3 help?

It does, ... a bit. With disable_idle_d3=1 the ioctl() is successful, but a
subsequent read on that file descriptor fails with -EIO.

    ioctl(5, VFIO_GROUP_GET_DEVICE_FD, 0x7ffd2b38abf0) = 6
    pread64(6, 0x7ffd2b38ab06, 2, 7696581394436) = -1 EIO (Input/output error)

> 
> If so; this feels like an imbalance of runtime PM calls in the VFIO stack
> that this patch exposed.
> 
> Alex, any ideas?
> 

Regards,

-- 
Giovanni

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Alex Williamson]

On Wed, 11 Jun 2025 06:50:59 -0700
Mario Limonciello <superm1@kernel.org> wrote:

> On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
> > Hi Mario, Bjorn and Alex,
> > 
> > On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:  
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> AMD BIOS team has root caused an issue that NVME storage failed to come
> >> back from suspend to a lack of a call to _REG when NVME device was probed.
> >>
> >> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
> >> added support for calling _REG when transitioning D-states, but this only
> >> works if the device actually "transitions" D-states.
> >>
> >> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
> >> devices") added support for runtime PM on PCI devices, but never actually
> >> 'explicitly' sets the device to D0.
> >>
> >> To make sure that devices are in D0 and that platform methods such as
> >> _REG are called, explicitly set all devices into D0 during initialization.
> >>
> >> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---  
> > Through a bisect, we identified that this patch, in v6.16-rc1,
> > introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
> > devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
> > with -EACCES.
> > 
> > Upon further investigation, the -EACCES appears to originate from the
> > rpm_resume() function, which is called by pm_runtime_resume_and_get()
> > within vfio_pci_core_enable(). Here is the exact call trace:
> > 
> >      drivers/base/power/runtime.c: rpm_resume()
> >      drivers/base/power/runtime.c: __pm_runtime_resume()
> >      include/linux/pm_runtime.h: pm_runtime_resume_and_get()
> >      drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
> >      drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
> >      drivers/vfio/vfio_main.c: device->ops->open_device()
> >      drivers/vfio/vfio_main.c: vfio_df_device_first_open()
> >      drivers/vfio/vfio_main.c: vfio_df_open()
> >      drivers/vfio/group.c: vfio_df_group_open()
> >      drivers/vfio/group.c: vfio_device_open_file()
> >      drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
> >      drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
> > 
> > Is this a known issue that affects other devices? Is there any ongoing
> > discussion or fix in progress?
> > 
> > Thanks,
> >   
> 
> This is the first I've heard about an issue with that patch.
> 
> Does setting the VFIO parameter disable_idle_d3 help?
> 
> If so; this feels like an imbalance of runtime PM calls in the VFIO 
> stack that this patch exposed.
> 
> Alex, any ideas?

Does the device in question have a PM capability?  I note that
4d4c10f763d7 makes the sequence:

       pm_runtime_forbid(&dev->dev);
       pm_runtime_set_active(&dev->dev);
       pm_runtime_enable(&dev->dev);

Dependent on the presence of a PM capability.  The PM capability is
optional on SR-IOV VFs.  This feels like a bug in the original patch,
we should be able to use pm_runtime ops on a device without
specifically checking if the device supports PCI PM.

vfio-pci also has a somewhat unique sequence versus other drivers, we
don't call pci_enable_device() until the user opens the device, but we
want to put the device into low power before that occurs.  Historically
PCI-core left device in an unknown power state between driver uses, so
we've needed to manually move the device to D0 before calling
pm_runtime_allow() and pm_runtime_put() (see
vfio_pci_core_register_device()).  Possibly this is redundant now but
we're using pci_set_power_state() which shouldn't interact with
pm_runtime, so my initial guess is that we might be unbalanced because
this is a VF w/o a PM capability and we've missed the expected
pm_runtime initialization sequence.  Thanks,

Alex

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Cabiddu, Giovanni]

On Wed, Jun 11, 2025 at 10:00:02AM -0600, Alex Williamson wrote:
> On Wed, 11 Jun 2025 06:50:59 -0700
> Mario Limonciello <superm1@kernel.org> wrote:
> 
> > On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
> > > Hi Mario, Bjorn and Alex,
> > > 
> > > On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:  
> > >> From: Mario Limonciello <mario.limonciello@amd.com>
> > >>
> > >> AMD BIOS team has root caused an issue that NVME storage failed to come
> > >> back from suspend to a lack of a call to _REG when NVME device was probed.
> > >>
> > >> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
> > >> added support for calling _REG when transitioning D-states, but this only
> > >> works if the device actually "transitions" D-states.
> > >>
> > >> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
> > >> devices") added support for runtime PM on PCI devices, but never actually
> > >> 'explicitly' sets the device to D0.
> > >>
> > >> To make sure that devices are in D0 and that platform methods such as
> > >> _REG are called, explicitly set all devices into D0 during initialization.
> > >>
> > >> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >> ---  
> > > Through a bisect, we identified that this patch, in v6.16-rc1,
> > > introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
> > > devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
> > > with -EACCES.
> > > 
> > > Upon further investigation, the -EACCES appears to originate from the
> > > rpm_resume() function, which is called by pm_runtime_resume_and_get()
> > > within vfio_pci_core_enable(). Here is the exact call trace:
> > > 
> > >      drivers/base/power/runtime.c: rpm_resume()
> > >      drivers/base/power/runtime.c: __pm_runtime_resume()
> > >      include/linux/pm_runtime.h: pm_runtime_resume_and_get()
> > >      drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
> > >      drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
> > >      drivers/vfio/vfio_main.c: device->ops->open_device()
> > >      drivers/vfio/vfio_main.c: vfio_df_device_first_open()
> > >      drivers/vfio/vfio_main.c: vfio_df_open()
> > >      drivers/vfio/group.c: vfio_df_group_open()
> > >      drivers/vfio/group.c: vfio_device_open_file()
> > >      drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
> > >      drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
> > > 
> > > Is this a known issue that affects other devices? Is there any ongoing
> > > discussion or fix in progress?
> > > 
> > > Thanks,
> > >   
> > 
> > This is the first I've heard about an issue with that patch.
> > 
> > Does setting the VFIO parameter disable_idle_d3 help?
> > 
> > If so; this feels like an imbalance of runtime PM calls in the VFIO 
> > stack that this patch exposed.
> > 
> > Alex, any ideas?
> 
> Does the device in question have a PM capability?  I note that
> 4d4c10f763d7 makes the sequence:
> 
>        pm_runtime_forbid(&dev->dev);
>        pm_runtime_set_active(&dev->dev);
>        pm_runtime_enable(&dev->dev);
> 
> Dependent on the presence of a PM capability.  The PM capability is
> optional on SR-IOV VFs.  This feels like a bug in the original patch,
> we should be able to use pm_runtime ops on a device without
> specifically checking if the device supports PCI PM.
> 
> vfio-pci also has a somewhat unique sequence versus other drivers, we
> don't call pci_enable_device() until the user opens the device, but we
> want to put the device into low power before that occurs.  Historically
> PCI-core left device in an unknown power state between driver uses, so
> we've needed to manually move the device to D0 before calling
> pm_runtime_allow() and pm_runtime_put() (see
> vfio_pci_core_register_device()).  Possibly this is redundant now but
> we're using pci_set_power_state() which shouldn't interact with
> pm_runtime, so my initial guess is that we might be unbalanced because
> this is a VF w/o a PM capability and we've missed the expected
> pm_runtime initialization sequence.  Thanks,

Yes, for Intel QAT, the issue occurs with a VF without the PM capability.

Thanks,

-- 
Giovanni

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Mario Limonciello]

On 6/11/2025 9:13 AM, Cabiddu, Giovanni wrote:
> On Wed, Jun 11, 2025 at 10:00:02AM -0600, Alex Williamson wrote:
>> On Wed, 11 Jun 2025 06:50:59 -0700
>> Mario Limonciello <superm1@kernel.org> wrote:
>>
>>> On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
>>>> Hi Mario, Bjorn and Alex,
>>>>
>>>> On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>
>>>>> AMD BIOS team has root caused an issue that NVME storage failed to come
>>>>> back from suspend to a lack of a call to _REG when NVME device was probed.
>>>>>
>>>>> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
>>>>> added support for calling _REG when transitioning D-states, but this only
>>>>> works if the device actually "transitions" D-states.
>>>>>
>>>>> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
>>>>> devices") added support for runtime PM on PCI devices, but never actually
>>>>> 'explicitly' sets the device to D0.
>>>>>
>>>>> To make sure that devices are in D0 and that platform methods such as
>>>>> _REG are called, explicitly set all devices into D0 during initialization.
>>>>>
>>>>> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>> Through a bisect, we identified that this patch, in v6.16-rc1,
>>>> introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
>>>> devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
>>>> with -EACCES.
>>>>
>>>> Upon further investigation, the -EACCES appears to originate from the
>>>> rpm_resume() function, which is called by pm_runtime_resume_and_get()
>>>> within vfio_pci_core_enable(). Here is the exact call trace:
>>>>
>>>>       drivers/base/power/runtime.c: rpm_resume()
>>>>       drivers/base/power/runtime.c: __pm_runtime_resume()
>>>>       include/linux/pm_runtime.h: pm_runtime_resume_and_get()
>>>>       drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
>>>>       drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
>>>>       drivers/vfio/vfio_main.c: device->ops->open_device()
>>>>       drivers/vfio/vfio_main.c: vfio_df_device_first_open()
>>>>       drivers/vfio/vfio_main.c: vfio_df_open()
>>>>       drivers/vfio/group.c: vfio_df_group_open()
>>>>       drivers/vfio/group.c: vfio_device_open_file()
>>>>       drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
>>>>       drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
>>>>
>>>> Is this a known issue that affects other devices? Is there any ongoing
>>>> discussion or fix in progress?
>>>>
>>>> Thanks,
>>>>    
>>>
>>> This is the first I've heard about an issue with that patch.
>>>
>>> Does setting the VFIO parameter disable_idle_d3 help?
>>>
>>> If so; this feels like an imbalance of runtime PM calls in the VFIO
>>> stack that this patch exposed.
>>>
>>> Alex, any ideas?
>>
>> Does the device in question have a PM capability?  I note that
>> 4d4c10f763d7 makes the sequence:
>>
>>         pm_runtime_forbid(&dev->dev);
>>         pm_runtime_set_active(&dev->dev);
>>         pm_runtime_enable(&dev->dev);
>>
>> Dependent on the presence of a PM capability.  The PM capability is
>> optional on SR-IOV VFs.  This feels like a bug in the original patch,
>> we should be able to use pm_runtime ops on a device without
>> specifically checking if the device supports PCI PM.
>>
>> vfio-pci also has a somewhat unique sequence versus other drivers, we
>> don't call pci_enable_device() until the user opens the device, but we
>> want to put the device into low power before that occurs.  Historically
>> PCI-core left device in an unknown power state between driver uses, so
>> we've needed to manually move the device to D0 before calling
>> pm_runtime_allow() and pm_runtime_put() (see
>> vfio_pci_core_register_device()).  Possibly this is redundant now but
>> we're using pci_set_power_state() which shouldn't interact with
>> pm_runtime, so my initial guess is that we might be unbalanced because
>> this is a VF w/o a PM capability and we've missed the expected
>> pm_runtime initialization sequence.  Thanks,
> 
> Yes, for Intel QAT, the issue occurs with a VF without the PM capability.
> 
> Thanks,
> 

Got it, thanks Alex!  I think this should help return it to previous 
behavior for devices without runtime PM and still fix the problem it 
needed to.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3dd44d1ad829..c495c3c692f5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3221,15 +3221,17 @@ void pci_pm_init(struct pci_dev *dev)

         /* find PCI PM capability in list */
         pm = pci_find_capability(dev, PCI_CAP_ID_PM);
-       if (!pm)
+       if (!pm) {
+               goto poweron;
                 return;
+       }
         /* Check device's ability to generate PME# */
         pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);

         if ((pmc & PCI_PM_CAP_VER_MASK) > 3) {
                 pci_err(dev, "unsupported PM cap regs version (%u)\n",
                         pmc & PCI_PM_CAP_VER_MASK);
-               return;
+               goto poweron;
         }

         dev->pm_cap = pm;
@@ -3274,6 +3276,7 @@ void pci_pm_init(struct pci_dev *dev)
         pci_read_config_word(dev, PCI_STATUS, &status);
         if (status & PCI_STATUS_IMM_READY)
                 dev->imm_ready = 1;
+poweron:
         pci_pm_power_up_and_verify_state(dev);
         pm_runtime_forbid(&dev->dev);
         pm_runtime_set_active(&dev->dev);

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Giovanni Cabiddu]

On Wed, Jun 11, 2025 at 01:45:49PM -0700, Mario Limonciello wrote:
> On 6/11/2025 9:13 AM, Cabiddu, Giovanni wrote:
> > On Wed, Jun 11, 2025 at 10:00:02AM -0600, Alex Williamson wrote:
> > > On Wed, 11 Jun 2025 06:50:59 -0700
> > > Mario Limonciello <superm1@kernel.org> wrote:
> > > 
> > > > On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
> > > > > Hi Mario, Bjorn and Alex,
> > > > > 
> > > > > On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
> > > > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > 
> > > > > > AMD BIOS team has root caused an issue that NVME storage failed to come
> > > > > > back from suspend to a lack of a call to _REG when NVME device was probed.
> > > > > > 
> > > > > > commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
> > > > > > added support for calling _REG when transitioning D-states, but this only
> > > > > > works if the device actually "transitions" D-states.
> > > > > > 
> > > > > > commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
> > > > > > devices") added support for runtime PM on PCI devices, but never actually
> > > > > > 'explicitly' sets the device to D0.
> > > > > > 
> > > > > > To make sure that devices are in D0 and that platform methods such as
> > > > > > _REG are called, explicitly set all devices into D0 during initialization.
> > > > > > 
> > > > > > Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > ---
> > > > > Through a bisect, we identified that this patch, in v6.16-rc1,
> > > > > introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
> > > > > devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
> > > > > with -EACCES.
> > > > > 
> > > > > Upon further investigation, the -EACCES appears to originate from the
> > > > > rpm_resume() function, which is called by pm_runtime_resume_and_get()
> > > > > within vfio_pci_core_enable(). Here is the exact call trace:
> > > > > 
> > > > >       drivers/base/power/runtime.c: rpm_resume()
> > > > >       drivers/base/power/runtime.c: __pm_runtime_resume()
> > > > >       include/linux/pm_runtime.h: pm_runtime_resume_and_get()
> > > > >       drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
> > > > >       drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
> > > > >       drivers/vfio/vfio_main.c: device->ops->open_device()
> > > > >       drivers/vfio/vfio_main.c: vfio_df_device_first_open()
> > > > >       drivers/vfio/vfio_main.c: vfio_df_open()
> > > > >       drivers/vfio/group.c: vfio_df_group_open()
> > > > >       drivers/vfio/group.c: vfio_device_open_file()
> > > > >       drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
> > > > >       drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
> > > > > 
> > > > > Is this a known issue that affects other devices? Is there any ongoing
> > > > > discussion or fix in progress?
> > > > > 
> > > > > Thanks,
> > > > 
> > > > This is the first I've heard about an issue with that patch.
> > > > 
> > > > Does setting the VFIO parameter disable_idle_d3 help?
> > > > 
> > > > If so; this feels like an imbalance of runtime PM calls in the VFIO
> > > > stack that this patch exposed.
> > > > 
> > > > Alex, any ideas?
> > > 
> > > Does the device in question have a PM capability?  I note that
> > > 4d4c10f763d7 makes the sequence:
> > > 
> > >         pm_runtime_forbid(&dev->dev);
> > >         pm_runtime_set_active(&dev->dev);
> > >         pm_runtime_enable(&dev->dev);
> > > 
> > > Dependent on the presence of a PM capability.  The PM capability is
> > > optional on SR-IOV VFs.  This feels like a bug in the original patch,
> > > we should be able to use pm_runtime ops on a device without
> > > specifically checking if the device supports PCI PM.
> > > 
> > > vfio-pci also has a somewhat unique sequence versus other drivers, we
> > > don't call pci_enable_device() until the user opens the device, but we
> > > want to put the device into low power before that occurs.  Historically
> > > PCI-core left device in an unknown power state between driver uses, so
> > > we've needed to manually move the device to D0 before calling
> > > pm_runtime_allow() and pm_runtime_put() (see
> > > vfio_pci_core_register_device()).  Possibly this is redundant now but
> > > we're using pci_set_power_state() which shouldn't interact with
> > > pm_runtime, so my initial guess is that we might be unbalanced because
> > > this is a VF w/o a PM capability and we've missed the expected
> > > pm_runtime initialization sequence.  Thanks,
> > 
> > Yes, for Intel QAT, the issue occurs with a VF without the PM capability.
> > 
> > Thanks,
> > 
> 
> Got it, thanks Alex!  I think this should help return it to previous
> behavior for devices without runtime PM and still fix the problem it needed
> to.
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3dd44d1ad829..c495c3c692f5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3221,15 +3221,17 @@ void pci_pm_init(struct pci_dev *dev)
> 
>         /* find PCI PM capability in list */
>         pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> -       if (!pm)
> +       if (!pm) {
> +               goto poweron;
>                 return;
> +       }
>         /* Check device's ability to generate PME# */
>         pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);
> 
>         if ((pmc & PCI_PM_CAP_VER_MASK) > 3) {
>                 pci_err(dev, "unsupported PM cap regs version (%u)\n",
>                         pmc & PCI_PM_CAP_VER_MASK);
> -               return;
> +               goto poweron;
>         }
> 
>         dev->pm_cap = pm;
> @@ -3274,6 +3276,7 @@ void pci_pm_init(struct pci_dev *dev)
>         pci_read_config_word(dev, PCI_STATUS, &status);
>         if (status & PCI_STATUS_IMM_READY)
>                 dev->imm_ready = 1;
> +poweron:
>         pci_pm_power_up_and_verify_state(dev);
>         pm_runtime_forbid(&dev->dev);
>         pm_runtime_set_active(&dev->dev);

I tried this change and it works.

Thanks,

-- 
Giovanni

Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing - Bug report

[Alexey Kardashevskiy]

On 12/6/25 06:45, Mario Limonciello wrote:
> On 6/11/2025 9:13 AM, Cabiddu, Giovanni wrote:
>> On Wed, Jun 11, 2025 at 10:00:02AM -0600, Alex Williamson wrote:
>>> On Wed, 11 Jun 2025 06:50:59 -0700
>>> Mario Limonciello <superm1@kernel.org> wrote:
>>>
>>>> On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
>>>>> Hi Mario, Bjorn and Alex,
>>>>>
>>>>> On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>
>>>>>> AMD BIOS team has root caused an issue that NVME storage failed to come
>>>>>> back from suspend to a lack of a call to _REG when NVME device was probed.
>>>>>>
>>>>>> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
>>>>>> added support for calling _REG when transitioning D-states, but this only
>>>>>> works if the device actually "transitions" D-states.
>>>>>>
>>>>>> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
>>>>>> devices") added support for runtime PM on PCI devices, but never actually
>>>>>> 'explicitly' sets the device to D0.
>>>>>>
>>>>>> To make sure that devices are in D0 and that platform methods such as
>>>>>> _REG are called, explicitly set all devices into D0 during initialization.
>>>>>>
>>>>>> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>> Through a bisect, we identified that this patch, in v6.16-rc1,
>>>>> introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
>>>>> devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
>>>>> with -EACCES.
>>>>>
>>>>> Upon further investigation, the -EACCES appears to originate from the
>>>>> rpm_resume() function, which is called by pm_runtime_resume_and_get()
>>>>> within vfio_pci_core_enable(). Here is the exact call trace:
>>>>>
>>>>>       drivers/base/power/runtime.c: rpm_resume()
>>>>>       drivers/base/power/runtime.c: __pm_runtime_resume()
>>>>>       include/linux/pm_runtime.h: pm_runtime_resume_and_get()
>>>>>       drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
>>>>>       drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
>>>>>       drivers/vfio/vfio_main.c: device->ops->open_device()
>>>>>       drivers/vfio/vfio_main.c: vfio_df_device_first_open()
>>>>>       drivers/vfio/vfio_main.c: vfio_df_open()
>>>>>       drivers/vfio/group.c: vfio_df_group_open()
>>>>>       drivers/vfio/group.c: vfio_device_open_file()
>>>>>       drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
>>>>>       drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
>>>>>
>>>>> Is this a known issue that affects other devices? Is there any ongoing
>>>>> discussion or fix in progress?
>>>>>
>>>>> Thanks,
>>>>
>>>> This is the first I've heard about an issue with that patch.
>>>>
>>>> Does setting the VFIO parameter disable_idle_d3 help?
>>>>
>>>> If so; this feels like an imbalance of runtime PM calls in the VFIO
>>>> stack that this patch exposed.
>>>>
>>>> Alex, any ideas?
>>>
>>> Does the device in question have a PM capability?  I note that
>>> 4d4c10f763d7 makes the sequence:
>>>
>>>         pm_runtime_forbid(&dev->dev);
>>>         pm_runtime_set_active(&dev->dev);
>>>         pm_runtime_enable(&dev->dev);
>>>
>>> Dependent on the presence of a PM capability.  The PM capability is
>>> optional on SR-IOV VFs.  This feels like a bug in the original patch,
>>> we should be able to use pm_runtime ops on a device without
>>> specifically checking if the device supports PCI PM.
>>>
>>> vfio-pci also has a somewhat unique sequence versus other drivers, we
>>> don't call pci_enable_device() until the user opens the device, but we
>>> want to put the device into low power before that occurs.  Historically
>>> PCI-core left device in an unknown power state between driver uses, so
>>> we've needed to manually move the device to D0 before calling
>>> pm_runtime_allow() and pm_runtime_put() (see
>>> vfio_pci_core_register_device()).  Possibly this is redundant now but
>>> we're using pci_set_power_state() which shouldn't interact with
>>> pm_runtime, so my initial guess is that we might be unbalanced because
>>> this is a VF w/o a PM capability and we've missed the expected
>>> pm_runtime initialization sequence.  Thanks,
>>
>> Yes, for Intel QAT, the issue occurs with a VF without the PM capability.
>>
>> Thanks,
>>
> 
> Got it, thanks Alex!  I think this should help return it to previous behavior for devices without runtime PM and still fix the problem it needed to.


Seems working for me too, thanks,



> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3dd44d1ad829..c495c3c692f5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3221,15 +3221,17 @@ void pci_pm_init(struct pci_dev *dev)
> 
>          /* find PCI PM capability in list */
>          pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> -       if (!pm)
> +       if (!pm) {
> +               goto poweron;
>                  return;
> +       }
>          /* Check device's ability to generate PME# */
>          pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);
> 
>          if ((pmc & PCI_PM_CAP_VER_MASK) > 3) {
>                  pci_err(dev, "unsupported PM cap regs version (%u)\n",
>                          pmc & PCI_PM_CAP_VER_MASK);
> -               return;
> +               goto poweron;
>          }
> 
>          dev->pm_cap = pm;
> @@ -3274,6 +3276,7 @@ void pci_pm_init(struct pci_dev *dev)
>          pci_read_config_word(dev, PCI_STATUS, &status);
>          if (status & PCI_STATUS_IMM_READY)
>                  dev->imm_ready = 1;
> +poweron:
>          pci_pm_power_up_and_verify_state(dev);
>          pm_runtime_forbid(&dev->dev);
>          pm_runtime_set_active(&dev->dev);

-- 
Alexey