* [PATCH] PCI: Fix active state requirement in PME polling
@ 2024-01-23 18:55 Alex Williamson
2024-01-23 19:40 ` Rafael J. Wysocki
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Alex Williamson @ 2024-01-23 18:55 UTC (permalink / raw)
To: bhelgaas
Cc: Alex Williamson, linux-pci, lukas, mika.westerberg, rafael, sanath.s
The commit noted in fixes added a bogus requirement that runtime PM
managed devices need to be in the RPM_ACTIVE state for PME polling.
In fact, only devices in low power states should be polled.
However there's still a requirement that the device config space must
be accessible, which has implications for both the current state of
the polled device and the parent bridge, when present. It's not
sufficient to assume the bridge remains in D0 and cases have been
observed where the bridge passes the D0 test, but the PM state
indicates RPM_SUSPENDING and config space of the polled device becomes
inaccessible during pci_pme_wakeup().
Therefore, since the bridge is already effectively required to be in
the RPM_ACTIVE state, formalize this in the code and elevate the PM
usage count to maintain the state while polling the subordinate
device.
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
Reported-by: Sanath S <sanath.s@amd.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bdbf8a94b4d0..764d7c977ef4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
if (pdev->pme_poll) {
struct pci_dev *bridge = pdev->bus->self;
struct device *dev = &pdev->dev;
- int pm_status;
+ struct device *bdev = bridge ? &bridge->dev : NULL;
+ int bref = 0;
/*
- * If bridge is in low power state, the
- * configuration space of subordinate devices
- * may be not accessible
+ * If we have a bridge, it should be in an active/D0
+ * state or the configuration space of subordinate
+ * devices may not be accessible or stable over the
+ * course of the call.
*/
- if (bridge && bridge->current_state != PCI_D0)
- continue;
+ if (bdev) {
+ bref = pm_runtime_get_if_active(bdev, true);
+ if (!bref)
+ continue;
+
+ if (bridge->current_state != PCI_D0)
+ goto put_bridge;
+ }
/*
- * If the device is in a low power state it
- * should not be polled either.
+ * The device itself should be suspended but config
+ * space must be accessible, therefore it cannot be in
+ * D3cold.
*/
- pm_status = pm_runtime_get_if_active(dev, true);
- if (!pm_status)
- continue;
-
- if (pdev->current_state != PCI_D3cold)
+ if (pm_runtime_suspended(dev) &&
+ pdev->current_state != PCI_D3cold)
pci_pme_wakeup(pdev, NULL);
- if (pm_status > 0)
- pm_runtime_put(dev);
+put_bridge:
+ if (bref > 0)
+ pm_runtime_put(bdev);
} else {
list_del(&pme_dev->list);
kfree(pme_dev);
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 18:55 [PATCH] PCI: Fix active state requirement in PME polling Alex Williamson
@ 2024-01-23 19:40 ` Rafael J. Wysocki
2024-01-23 19:50 ` Alex Williamson
2024-02-09 16:35 ` Bjorn Helgaas
2024-02-09 19:03 ` Bjorn Helgaas
2 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2024-01-23 19:40 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, linux-pci, lukas, mika.westerberg, rafael, sanath.s
On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> The commit noted in fixes added a bogus requirement that runtime PM
> managed devices need to be in the RPM_ACTIVE state for PME polling.
> In fact, only devices in low power states should be polled.
>
> However there's still a requirement that the device config space must
> be accessible, which has implications for both the current state of
> the polled device and the parent bridge, when present. It's not
> sufficient to assume the bridge remains in D0 and cases have been
> observed where the bridge passes the D0 test, but the PM state
> indicates RPM_SUSPENDING and config space of the polled device becomes
> inaccessible during pci_pme_wakeup().
>
> Therefore, since the bridge is already effectively required to be in
> the RPM_ACTIVE state, formalize this in the code and elevate the PM
> usage count to maintain the state while polling the subordinate
> device.
>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> Reported-by: Sanath S <sanath.s@amd.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bdbf8a94b4d0..764d7c977ef4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> if (pdev->pme_poll) {
> struct pci_dev *bridge = pdev->bus->self;
> struct device *dev = &pdev->dev;
> - int pm_status;
> + struct device *bdev = bridge ? &bridge->dev : NULL;
> + int bref = 0;
>
> /*
> - * If bridge is in low power state, the
> - * configuration space of subordinate devices
> - * may be not accessible
> + * If we have a bridge, it should be in an active/D0
> + * state or the configuration space of subordinate
> + * devices may not be accessible or stable over the
> + * course of the call.
> */
> - if (bridge && bridge->current_state != PCI_D0)
> - continue;
> + if (bdev) {
> + bref = pm_runtime_get_if_active(bdev, true);
> + if (!bref)
I would check bref <= 0 here.
> + continue;
> +
> + if (bridge->current_state != PCI_D0)
Isn't the power state guaranteed to be PCI_D0 at this point? If it
isn't, then why?
> + goto put_bridge;
> + }
>
> /*
> - * If the device is in a low power state it
> - * should not be polled either.
> + * The device itself should be suspended but config
> + * space must be accessible, therefore it cannot be in
> + * D3cold.
> */
> - pm_status = pm_runtime_get_if_active(dev, true);
> - if (!pm_status)
> - continue;
> -
> - if (pdev->current_state != PCI_D3cold)
> + if (pm_runtime_suspended(dev) &&
> + pdev->current_state != PCI_D3cold)
> pci_pme_wakeup(pdev, NULL);
>
> - if (pm_status > 0)
> - pm_runtime_put(dev);
> +put_bridge:
> + if (bref > 0)
> + pm_runtime_put(bdev);
> } else {
> list_del(&pme_dev->list);
> kfree(pme_dev);
> --
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 19:40 ` Rafael J. Wysocki
@ 2024-01-23 19:50 ` Alex Williamson
2024-01-23 19:59 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2024-01-23 19:50 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: bhelgaas, linux-pci, lukas, mika.westerberg, sanath.s
On Tue, 23 Jan 2024 20:40:32 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > The commit noted in fixes added a bogus requirement that runtime PM
> > managed devices need to be in the RPM_ACTIVE state for PME polling.
> > In fact, only devices in low power states should be polled.
> >
> > However there's still a requirement that the device config space must
> > be accessible, which has implications for both the current state of
> > the polled device and the parent bridge, when present. It's not
> > sufficient to assume the bridge remains in D0 and cases have been
> > observed where the bridge passes the D0 test, but the PM state
> > indicates RPM_SUSPENDING and config space of the polled device becomes
> > inaccessible during pci_pme_wakeup().
> >
> > Therefore, since the bridge is already effectively required to be in
> > the RPM_ACTIVE state, formalize this in the code and elevate the PM
> > usage count to maintain the state while polling the subordinate
> > device.
> >
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> > Reported-by: Sanath S <sanath.s@amd.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index bdbf8a94b4d0..764d7c977ef4 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> > if (pdev->pme_poll) {
> > struct pci_dev *bridge = pdev->bus->self;
> > struct device *dev = &pdev->dev;
> > - int pm_status;
> > + struct device *bdev = bridge ? &bridge->dev : NULL;
> > + int bref = 0;
> >
> > /*
> > - * If bridge is in low power state, the
> > - * configuration space of subordinate devices
> > - * may be not accessible
> > + * If we have a bridge, it should be in an active/D0
> > + * state or the configuration space of subordinate
> > + * devices may not be accessible or stable over the
> > + * course of the call.
> > */
> > - if (bridge && bridge->current_state != PCI_D0)
> > - continue;
> > + if (bdev) {
> > + bref = pm_runtime_get_if_active(bdev, true);
> > + if (!bref)
>
> I would check bref <= 0 here.
>
> > + continue;
> > +
> > + if (bridge->current_state != PCI_D0)
>
> Isn't the power state guaranteed to be PCI_D0 at this point? If it
> isn't, then why?
Both of these seem necessary to support !CONFIG_PM, where bref would be
-EINVAL and provides no indication of the current_state. Is that
incorrect? Thanks,
Alex
> > + goto put_bridge;
> > + }
> >
> > /*
> > - * If the device is in a low power state it
> > - * should not be polled either.
> > + * The device itself should be suspended but config
> > + * space must be accessible, therefore it cannot be in
> > + * D3cold.
> > */
> > - pm_status = pm_runtime_get_if_active(dev, true);
> > - if (!pm_status)
> > - continue;
> > -
> > - if (pdev->current_state != PCI_D3cold)
> > + if (pm_runtime_suspended(dev) &&
> > + pdev->current_state != PCI_D3cold)
> > pci_pme_wakeup(pdev, NULL);
> >
> > - if (pm_status > 0)
> > - pm_runtime_put(dev);
> > +put_bridge:
> > + if (bref > 0)
> > + pm_runtime_put(bdev);
> > } else {
> > list_del(&pme_dev->list);
> > kfree(pme_dev);
> > --
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 19:50 ` Alex Williamson
@ 2024-01-23 19:59 ` Rafael J. Wysocki
2024-01-23 20:39 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2024-01-23 19:59 UTC (permalink / raw)
To: Alex Williamson
Cc: Rafael J. Wysocki, bhelgaas, linux-pci, lukas, mika.westerberg, sanath.s
On Tue, Jan 23, 2024 at 8:51 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Tue, 23 Jan 2024 20:40:32 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > The commit noted in fixes added a bogus requirement that runtime PM
> > > managed devices need to be in the RPM_ACTIVE state for PME polling.
> > > In fact, only devices in low power states should be polled.
> > >
> > > However there's still a requirement that the device config space must
> > > be accessible, which has implications for both the current state of
> > > the polled device and the parent bridge, when present. It's not
> > > sufficient to assume the bridge remains in D0 and cases have been
> > > observed where the bridge passes the D0 test, but the PM state
> > > indicates RPM_SUSPENDING and config space of the polled device becomes
> > > inaccessible during pci_pme_wakeup().
> > >
> > > Therefore, since the bridge is already effectively required to be in
> > > the RPM_ACTIVE state, formalize this in the code and elevate the PM
> > > usage count to maintain the state while polling the subordinate
> > > device.
> > >
> > > Cc: Lukas Wunner <lukas@wunner.de>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > > Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> > > Reported-by: Sanath S <sanath.s@amd.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > > drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> > > 1 file changed, 22 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index bdbf8a94b4d0..764d7c977ef4 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> > > if (pdev->pme_poll) {
> > > struct pci_dev *bridge = pdev->bus->self;
> > > struct device *dev = &pdev->dev;
> > > - int pm_status;
> > > + struct device *bdev = bridge ? &bridge->dev : NULL;
> > > + int bref = 0;
> > >
> > > /*
> > > - * If bridge is in low power state, the
> > > - * configuration space of subordinate devices
> > > - * may be not accessible
> > > + * If we have a bridge, it should be in an active/D0
> > > + * state or the configuration space of subordinate
> > > + * devices may not be accessible or stable over the
> > > + * course of the call.
> > > */
> > > - if (bridge && bridge->current_state != PCI_D0)
> > > - continue;
> > > + if (bdev) {
> > > + bref = pm_runtime_get_if_active(bdev, true);
> > > + if (!bref)
> >
> > I would check bref <= 0 here.
> >
> > > + continue;
> > > +
> > > + if (bridge->current_state != PCI_D0)
> >
> > Isn't the power state guaranteed to be PCI_D0 at this point? If it
> > isn't, then why?
>
> Both of these seem necessary to support !CONFIG_PM, where bref would be
> -EINVAL and provides no indication of the current_state. Is that
> incorrect? Thanks,
Well, CONFIG_PCIE_PME depends on CONFIG_PM, so I'm not sure how
dev->pme_poll can be set without it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 19:59 ` Rafael J. Wysocki
@ 2024-01-23 20:39 ` Alex Williamson
2024-01-23 22:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2024-01-23 20:39 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: bhelgaas, linux-pci, lukas, mika.westerberg, sanath.s
On Tue, 23 Jan 2024 20:59:50 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Tue, Jan 23, 2024 at 8:51 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Tue, 23 Jan 2024 20:40:32 +0100
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
> > > <alex.williamson@redhat.com> wrote:
> > > >
> > > > The commit noted in fixes added a bogus requirement that runtime PM
> > > > managed devices need to be in the RPM_ACTIVE state for PME polling.
> > > > In fact, only devices in low power states should be polled.
> > > >
> > > > However there's still a requirement that the device config space must
> > > > be accessible, which has implications for both the current state of
> > > > the polled device and the parent bridge, when present. It's not
> > > > sufficient to assume the bridge remains in D0 and cases have been
> > > > observed where the bridge passes the D0 test, but the PM state
> > > > indicates RPM_SUSPENDING and config space of the polled device becomes
> > > > inaccessible during pci_pme_wakeup().
> > > >
> > > > Therefore, since the bridge is already effectively required to be in
> > > > the RPM_ACTIVE state, formalize this in the code and elevate the PM
> > > > usage count to maintain the state while polling the subordinate
> > > > device.
> > > >
> > > > Cc: Lukas Wunner <lukas@wunner.de>
> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > > > Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> > > > Reported-by: Sanath S <sanath.s@amd.com>
> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > ---
> > > > drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> > > > 1 file changed, 22 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index bdbf8a94b4d0..764d7c977ef4 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> > > > if (pdev->pme_poll) {
> > > > struct pci_dev *bridge = pdev->bus->self;
> > > > struct device *dev = &pdev->dev;
> > > > - int pm_status;
> > > > + struct device *bdev = bridge ? &bridge->dev : NULL;
> > > > + int bref = 0;
> > > >
> > > > /*
> > > > - * If bridge is in low power state, the
> > > > - * configuration space of subordinate devices
> > > > - * may be not accessible
> > > > + * If we have a bridge, it should be in an active/D0
> > > > + * state or the configuration space of subordinate
> > > > + * devices may not be accessible or stable over the
> > > > + * course of the call.
> > > > */
> > > > - if (bridge && bridge->current_state != PCI_D0)
> > > > - continue;
> > > > + if (bdev) {
> > > > + bref = pm_runtime_get_if_active(bdev, true);
> > > > + if (!bref)
> > >
> > > I would check bref <= 0 here.
> > >
> > > > + continue;
> > > > +
> > > > + if (bridge->current_state != PCI_D0)
> > >
> > > Isn't the power state guaranteed to be PCI_D0 at this point? If it
> > > isn't, then why?
> >
> > Both of these seem necessary to support !CONFIG_PM, where bref would be
> > -EINVAL and provides no indication of the current_state. Is that
> > incorrect? Thanks,
>
> Well, CONFIG_PCIE_PME depends on CONFIG_PM, so I'm not sure how
> dev->pme_poll can be set without it.
I only see that drivers/pci/pci.c:pci_pm_init() sets pme_poll true and
I'm not spotting a dependency on either PCIE_PME or PM to get there. I
see a few places where pme.c, governed by PCIE_PME, can set pme_poll
false though.
It's also not clear to me that we should skip scanning a device if
pm_runtime_get_if_active() returns -EINVAL for the bridge due to
power.disable_depth. If runtime PM isn't enabled on the bridge,
shouldn't we be able to test current_state and assume it won't change?
Thanks,
Alex
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 20:39 ` Alex Williamson
@ 2024-01-23 22:33 ` Rafael J. Wysocki
2024-01-24 14:21 ` Sanath S
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2024-01-23 22:33 UTC (permalink / raw)
To: Alex Williamson
Cc: Rafael J. Wysocki, bhelgaas, linux-pci, lukas, mika.westerberg, sanath.s
On Tue, Jan 23, 2024 at 9:39 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Tue, 23 Jan 2024 20:59:50 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Jan 23, 2024 at 8:51 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Tue, 23 Jan 2024 20:40:32 +0100
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > > On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
> > > > <alex.williamson@redhat.com> wrote:
> > > > >
> > > > > The commit noted in fixes added a bogus requirement that runtime PM
> > > > > managed devices need to be in the RPM_ACTIVE state for PME polling.
> > > > > In fact, only devices in low power states should be polled.
> > > > >
> > > > > However there's still a requirement that the device config space must
> > > > > be accessible, which has implications for both the current state of
> > > > > the polled device and the parent bridge, when present. It's not
> > > > > sufficient to assume the bridge remains in D0 and cases have been
> > > > > observed where the bridge passes the D0 test, but the PM state
> > > > > indicates RPM_SUSPENDING and config space of the polled device becomes
> > > > > inaccessible during pci_pme_wakeup().
> > > > >
> > > > > Therefore, since the bridge is already effectively required to be in
> > > > > the RPM_ACTIVE state, formalize this in the code and elevate the PM
> > > > > usage count to maintain the state while polling the subordinate
> > > > > device.
> > > > >
> > > > > Cc: Lukas Wunner <lukas@wunner.de>
> > > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > > > > Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> > > > > Reported-by: Sanath S <sanath.s@amd.com>
> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > ---
> > > > > drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> > > > > 1 file changed, 22 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index bdbf8a94b4d0..764d7c977ef4 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> > > > > if (pdev->pme_poll) {
> > > > > struct pci_dev *bridge = pdev->bus->self;
> > > > > struct device *dev = &pdev->dev;
> > > > > - int pm_status;
> > > > > + struct device *bdev = bridge ? &bridge->dev : NULL;
> > > > > + int bref = 0;
> > > > >
> > > > > /*
> > > > > - * If bridge is in low power state, the
> > > > > - * configuration space of subordinate devices
> > > > > - * may be not accessible
> > > > > + * If we have a bridge, it should be in an active/D0
> > > > > + * state or the configuration space of subordinate
> > > > > + * devices may not be accessible or stable over the
> > > > > + * course of the call.
> > > > > */
> > > > > - if (bridge && bridge->current_state != PCI_D0)
> > > > > - continue;
> > > > > + if (bdev) {
> > > > > + bref = pm_runtime_get_if_active(bdev, true);
> > > > > + if (!bref)
> > > >
> > > > I would check bref <= 0 here.
> > > >
> > > > > + continue;
> > > > > +
> > > > > + if (bridge->current_state != PCI_D0)
> > > >
> > > > Isn't the power state guaranteed to be PCI_D0 at this point? If it
> > > > isn't, then why?
> > >
> > > Both of these seem necessary to support !CONFIG_PM, where bref would be
> > > -EINVAL and provides no indication of the current_state. Is that
> > > incorrect? Thanks,
> >
> > Well, CONFIG_PCIE_PME depends on CONFIG_PM, so I'm not sure how
> > dev->pme_poll can be set without it.
>
> I only see that drivers/pci/pci.c:pci_pm_init() sets pme_poll true and
> I'm not spotting a dependency on either PCIE_PME or PM to get there. I
> see a few places where pme.c, governed by PCIE_PME, can set pme_poll
> false though.
All of this is a bit moot when CONFIG_PM is unset, because PME polling
was introduced as a workaround for problems with PME signaling which
is only enabled when CONFIG_PM is set. IOW, if CONFIG_PM is not set,
there is no reason for PME polling.
> It's also not clear to me that we should skip scanning a device if
> pm_runtime_get_if_active() returns -EINVAL for the bridge due to
> power.disable_depth.
Let me recap things a bit.
PME polling is run from a freezable workqueue, so it is not carried
out during system-wide PM transitions, only in the working state
proper.
This means that when runtime PM is disabled for a bridge, there is no
reason for its power state to change and all bridges start in D0.
So you are right, the endpoint device below the bridge still needs to
be scanned then, but I'm not sure about the current_state check. In
theory, it should not be necessary.
But I agree that this is a minor point, so please feel free to add
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
to the patch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 22:33 ` Rafael J. Wysocki
@ 2024-01-24 14:21 ` Sanath S
0 siblings, 0 replies; 10+ messages in thread
From: Sanath S @ 2024-01-24 14:21 UTC (permalink / raw)
To: Rafael J. Wysocki, Alex Williamson
Cc: bhelgaas, linux-pci, lukas, mika.westerberg, sanath.s
On 1/24/2024 4:03 AM, Rafael J. Wysocki wrote:
> On Tue, Jan 23, 2024 at 9:39 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
>> On Tue, 23 Jan 2024 20:59:50 +0100
>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>
>>> On Tue, Jan 23, 2024 at 8:51 PM Alex Williamson
>>> <alex.williamson@redhat.com> wrote:
>>>> On Tue, 23 Jan 2024 20:40:32 +0100
>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>>>
>>>>> On Tue, Jan 23, 2024 at 7:56 PM Alex Williamson
>>>>> <alex.williamson@redhat.com> wrote:
>>>>>> The commit noted in fixes added a bogus requirement that runtime PM
>>>>>> managed devices need to be in the RPM_ACTIVE state for PME polling.
>>>>>> In fact, only devices in low power states should be polled.
>>>>>>
>>>>>> However there's still a requirement that the device config space must
>>>>>> be accessible, which has implications for both the current state of
>>>>>> the polled device and the parent bridge, when present. It's not
>>>>>> sufficient to assume the bridge remains in D0 and cases have been
>>>>>> observed where the bridge passes the D0 test, but the PM state
>>>>>> indicates RPM_SUSPENDING and config space of the polled device becomes
>>>>>> inaccessible during pci_pme_wakeup().
>>>>>>
>>>>>> Therefore, since the bridge is already effectively required to be in
>>>>>> the RPM_ACTIVE state, formalize this in the code and elevate the PM
>>>>>> usage count to maintain the state while polling the subordinate
>>>>>> device.
>>>>>>
>>>>>> Cc: Lukas Wunner <lukas@wunner.de>
>>>>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>>>>>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>>>>>> Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
>>>>>> Reported-by: Sanath S <sanath.s@amd.com>
Gave a try on couple of thunderbolt docks, issue is resolved with this
patch.
Tested-by: Sanath S <sanath.s@amd.com>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>> ---
>>>>>> drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
>>>>>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>> index bdbf8a94b4d0..764d7c977ef4 100644
>>>>>> --- a/drivers/pci/pci.c
>>>>>> +++ b/drivers/pci/pci.c
>>>>>> @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
>>>>>> if (pdev->pme_poll) {
>>>>>> struct pci_dev *bridge = pdev->bus->self;
>>>>>> struct device *dev = &pdev->dev;
>>>>>> - int pm_status;
>>>>>> + struct device *bdev = bridge ? &bridge->dev : NULL;
>>>>>> + int bref = 0;
>>>>>>
>>>>>> /*
>>>>>> - * If bridge is in low power state, the
>>>>>> - * configuration space of subordinate devices
>>>>>> - * may be not accessible
>>>>>> + * If we have a bridge, it should be in an active/D0
>>>>>> + * state or the configuration space of subordinate
>>>>>> + * devices may not be accessible or stable over the
>>>>>> + * course of the call.
>>>>>> */
>>>>>> - if (bridge && bridge->current_state != PCI_D0)
>>>>>> - continue;
>>>>>> + if (bdev) {
>>>>>> + bref = pm_runtime_get_if_active(bdev, true);
>>>>>> + if (!bref)
>>>>> I would check bref <= 0 here.
>>>>>
>>>>>> + continue;
>>>>>> +
>>>>>> + if (bridge->current_state != PCI_D0)
>>>>> Isn't the power state guaranteed to be PCI_D0 at this point? If it
>>>>> isn't, then why?
>>>> Both of these seem necessary to support !CONFIG_PM, where bref would be
>>>> -EINVAL and provides no indication of the current_state. Is that
>>>> incorrect? Thanks,
>>> Well, CONFIG_PCIE_PME depends on CONFIG_PM, so I'm not sure how
>>> dev->pme_poll can be set without it.
>> I only see that drivers/pci/pci.c:pci_pm_init() sets pme_poll true and
>> I'm not spotting a dependency on either PCIE_PME or PM to get there. I
>> see a few places where pme.c, governed by PCIE_PME, can set pme_poll
>> false though.
> All of this is a bit moot when CONFIG_PM is unset, because PME polling
> was introduced as a workaround for problems with PME signaling which
> is only enabled when CONFIG_PM is set. IOW, if CONFIG_PM is not set,
> there is no reason for PME polling.
>
>> It's also not clear to me that we should skip scanning a device if
>> pm_runtime_get_if_active() returns -EINVAL for the bridge due to
>> power.disable_depth.
> Let me recap things a bit.
>
> PME polling is run from a freezable workqueue, so it is not carried
> out during system-wide PM transitions, only in the working state
> proper.
>
> This means that when runtime PM is disabled for a bridge, there is no
> reason for its power state to change and all bridges start in D0.
>
> So you are right, the endpoint device below the bridge still needs to
> be scanned then, but I'm not sure about the current_state check. In
> theory, it should not be necessary.
>
> But I agree that this is a minor point, so please feel free to add
>
> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
>
> to the patch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 18:55 [PATCH] PCI: Fix active state requirement in PME polling Alex Williamson
2024-01-23 19:40 ` Rafael J. Wysocki
@ 2024-02-09 16:35 ` Bjorn Helgaas
2024-02-09 17:56 ` Alex Williamson
2024-02-09 19:03 ` Bjorn Helgaas
2 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2024-02-09 16:35 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, linux-pci, lukas, mika.westerberg, rafael, sanath.s
On Tue, Jan 23, 2024 at 11:55:31AM -0700, Alex Williamson wrote:
> The commit noted in fixes added a bogus requirement that runtime PM
> managed devices need to be in the RPM_ACTIVE state for PME polling.
> In fact, only devices in low power states should be polled.
>
> However there's still a requirement that the device config space must
> be accessible, which has implications for both the current state of
> the polled device and the parent bridge, when present. It's not
> sufficient to assume the bridge remains in D0 and cases have been
> observed where the bridge passes the D0 test, but the PM state
> indicates RPM_SUSPENDING and config space of the polled device becomes
> inaccessible during pci_pme_wakeup().
>
> Therefore, since the bridge is already effectively required to be in
> the RPM_ACTIVE state, formalize this in the code and elevate the PM
> usage count to maintain the state while polling the subordinate
> device.
This apparently fixes a problem: the bugzilla says something about
disks attached to Thunderbolt/USB4 docks not working, but I doubt it's
actually specific to Thunderbolt/USB4 or to disks.
The bugzilla also indicates that d3fcd7360338 was a regression.
d3fcd7360338 appeared in v6.6, so this fix is likely a candidate for
the current release (v6.8).
I'd like to mention both the user-visible problem being fixed and
the fact that it fixes a regression here in the commit log so we can
make the case for putting this in v6.8.
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> Reported-by: Sanath S <sanath.s@amd.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bdbf8a94b4d0..764d7c977ef4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> if (pdev->pme_poll) {
> struct pci_dev *bridge = pdev->bus->self;
> struct device *dev = &pdev->dev;
> - int pm_status;
> + struct device *bdev = bridge ? &bridge->dev : NULL;
> + int bref = 0;
>
> /*
> - * If bridge is in low power state, the
> - * configuration space of subordinate devices
> - * may be not accessible
> + * If we have a bridge, it should be in an active/D0
> + * state or the configuration space of subordinate
> + * devices may not be accessible or stable over the
> + * course of the call.
> */
> - if (bridge && bridge->current_state != PCI_D0)
> - continue;
> + if (bdev) {
> + bref = pm_runtime_get_if_active(bdev, true);
> + if (!bref)
> + continue;
> +
> + if (bridge->current_state != PCI_D0)
> + goto put_bridge;
> + }
>
> /*
> - * If the device is in a low power state it
> - * should not be polled either.
> + * The device itself should be suspended but config
> + * space must be accessible, therefore it cannot be in
> + * D3cold.
> */
> - pm_status = pm_runtime_get_if_active(dev, true);
> - if (!pm_status)
> - continue;
> -
> - if (pdev->current_state != PCI_D3cold)
> + if (pm_runtime_suspended(dev) &&
> + pdev->current_state != PCI_D3cold)
> pci_pme_wakeup(pdev, NULL);
>
> - if (pm_status > 0)
> - pm_runtime_put(dev);
> +put_bridge:
> + if (bref > 0)
> + pm_runtime_put(bdev);
> } else {
> list_del(&pme_dev->list);
> kfree(pme_dev);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-02-09 16:35 ` Bjorn Helgaas
@ 2024-02-09 17:56 ` Alex Williamson
0 siblings, 0 replies; 10+ messages in thread
From: Alex Williamson @ 2024-02-09 17:56 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas, linux-pci, lukas, mika.westerberg, rafael, sanath.s
On Fri, 9 Feb 2024 10:35:21 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Jan 23, 2024 at 11:55:31AM -0700, Alex Williamson wrote:
> > The commit noted in fixes added a bogus requirement that runtime PM
> > managed devices need to be in the RPM_ACTIVE state for PME polling.
> > In fact, only devices in low power states should be polled.
> >
> > However there's still a requirement that the device config space must
> > be accessible, which has implications for both the current state of
> > the polled device and the parent bridge, when present. It's not
> > sufficient to assume the bridge remains in D0 and cases have been
> > observed where the bridge passes the D0 test, but the PM state
> > indicates RPM_SUSPENDING and config space of the polled device becomes
> > inaccessible during pci_pme_wakeup().
> >
> > Therefore, since the bridge is already effectively required to be in
> > the RPM_ACTIVE state, formalize this in the code and elevate the PM
> > usage count to maintain the state while polling the subordinate
> > device.
>
> This apparently fixes a problem: the bugzilla says something about
> disks attached to Thunderbolt/USB4 docks not working, but I doubt it's
> actually specific to Thunderbolt/USB4 or to disks.
Right, AIUI it's simply a PCIe hierarchy where a bridge was previously
scanned in response to a PME and no longer is because of the invalid
requirement added in d3fcd7360338 that the runtime power management
status of the device is active.
> The bugzilla also indicates that d3fcd7360338 was a regression.
> d3fcd7360338 appeared in v6.6, so this fix is likely a candidate for
> the current release (v6.8).
Agreed.
> I'd like to mention both the user-visible problem being fixed and
> the fact that it fixes a regression here in the commit log so we can
> make the case for putting this in v6.8.
Ok, I've not experienced the regression myself, but I can add a
paragraph describing my understanding of the bugzilla. I'd probably
just say:
This resolves a regression reported in the bugzilla below where
a Thunderbolt/USB4 hierarchy fails to scan for an attached NVMe
endpoint downstream of a bridge in a D3hot power state.
If you'd like a respin including that or if you have further
phrasing/info suggestions, please let me know. Thanks,
Alex
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> > Reported-by: Sanath S <sanath.s@amd.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index bdbf8a94b4d0..764d7c977ef4 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> > if (pdev->pme_poll) {
> > struct pci_dev *bridge = pdev->bus->self;
> > struct device *dev = &pdev->dev;
> > - int pm_status;
> > + struct device *bdev = bridge ? &bridge->dev : NULL;
> > + int bref = 0;
> >
> > /*
> > - * If bridge is in low power state, the
> > - * configuration space of subordinate devices
> > - * may be not accessible
> > + * If we have a bridge, it should be in an active/D0
> > + * state or the configuration space of subordinate
> > + * devices may not be accessible or stable over the
> > + * course of the call.
> > */
> > - if (bridge && bridge->current_state != PCI_D0)
> > - continue;
> > + if (bdev) {
> > + bref = pm_runtime_get_if_active(bdev, true);
> > + if (!bref)
> > + continue;
> > +
> > + if (bridge->current_state != PCI_D0)
> > + goto put_bridge;
> > + }
> >
> > /*
> > - * If the device is in a low power state it
> > - * should not be polled either.
> > + * The device itself should be suspended but config
> > + * space must be accessible, therefore it cannot be in
> > + * D3cold.
> > */
> > - pm_status = pm_runtime_get_if_active(dev, true);
> > - if (!pm_status)
> > - continue;
> > -
> > - if (pdev->current_state != PCI_D3cold)
> > + if (pm_runtime_suspended(dev) &&
> > + pdev->current_state != PCI_D3cold)
> > pci_pme_wakeup(pdev, NULL);
> >
> > - if (pm_status > 0)
> > - pm_runtime_put(dev);
> > +put_bridge:
> > + if (bref > 0)
> > + pm_runtime_put(bdev);
> > } else {
> > list_del(&pme_dev->list);
> > kfree(pme_dev);
> > --
> > 2.43.0
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI: Fix active state requirement in PME polling
2024-01-23 18:55 [PATCH] PCI: Fix active state requirement in PME polling Alex Williamson
2024-01-23 19:40 ` Rafael J. Wysocki
2024-02-09 16:35 ` Bjorn Helgaas
@ 2024-02-09 19:03 ` Bjorn Helgaas
2 siblings, 0 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2024-02-09 19:03 UTC (permalink / raw)
To: Alex Williamson
Cc: bhelgaas, linux-pci, lukas, mika.westerberg, rafael, sanath.s
On Tue, Jan 23, 2024 at 11:55:31AM -0700, Alex Williamson wrote:
> The commit noted in fixes added a bogus requirement that runtime PM
> managed devices need to be in the RPM_ACTIVE state for PME polling.
> In fact, only devices in low power states should be polled.
>
> However there's still a requirement that the device config space must
> be accessible, which has implications for both the current state of
> the polled device and the parent bridge, when present. It's not
> sufficient to assume the bridge remains in D0 and cases have been
> observed where the bridge passes the D0 test, but the PM state
> indicates RPM_SUSPENDING and config space of the polled device becomes
> inaccessible during pci_pme_wakeup().
>
> Therefore, since the bridge is already effectively required to be in
> the RPM_ACTIVE state, formalize this in the code and elevate the PM
> usage count to maintain the state while polling the subordinate
> device.
>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Fixes: d3fcd7360338 ("PCI: Fix runtime PM race with PME polling")
> Reported-by: Sanath S <sanath.s@amd.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218360
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Applied with Rafael's reviewed-by to for-linus for v6.8, thanks!
> ---
> drivers/pci/pci.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bdbf8a94b4d0..764d7c977ef4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2433,29 +2433,36 @@ static void pci_pme_list_scan(struct work_struct *work)
> if (pdev->pme_poll) {
> struct pci_dev *bridge = pdev->bus->self;
> struct device *dev = &pdev->dev;
> - int pm_status;
> + struct device *bdev = bridge ? &bridge->dev : NULL;
> + int bref = 0;
>
> /*
> - * If bridge is in low power state, the
> - * configuration space of subordinate devices
> - * may be not accessible
> + * If we have a bridge, it should be in an active/D0
> + * state or the configuration space of subordinate
> + * devices may not be accessible or stable over the
> + * course of the call.
> */
> - if (bridge && bridge->current_state != PCI_D0)
> - continue;
> + if (bdev) {
> + bref = pm_runtime_get_if_active(bdev, true);
> + if (!bref)
> + continue;
> +
> + if (bridge->current_state != PCI_D0)
> + goto put_bridge;
> + }
>
> /*
> - * If the device is in a low power state it
> - * should not be polled either.
> + * The device itself should be suspended but config
> + * space must be accessible, therefore it cannot be in
> + * D3cold.
> */
> - pm_status = pm_runtime_get_if_active(dev, true);
> - if (!pm_status)
> - continue;
> -
> - if (pdev->current_state != PCI_D3cold)
> + if (pm_runtime_suspended(dev) &&
> + pdev->current_state != PCI_D3cold)
> pci_pme_wakeup(pdev, NULL);
>
> - if (pm_status > 0)
> - pm_runtime_put(dev);
> +put_bridge:
> + if (bref > 0)
> + pm_runtime_put(bdev);
> } else {
> list_del(&pme_dev->list);
> kfree(pme_dev);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-02-09 19:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-23 18:55 [PATCH] PCI: Fix active state requirement in PME polling Alex Williamson
2024-01-23 19:40 ` Rafael J. Wysocki
2024-01-23 19:50 ` Alex Williamson
2024-01-23 19:59 ` Rafael J. Wysocki
2024-01-23 20:39 ` Alex Williamson
2024-01-23 22:33 ` Rafael J. Wysocki
2024-01-24 14:21 ` Sanath S
2024-02-09 16:35 ` Bjorn Helgaas
2024-02-09 17:56 ` Alex Williamson
2024-02-09 19:03 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).