diff mbox series

PCI/portdrv: Disallow runtime suspend when waekup is required but PME service isn't supported

Message ID 20210809042414.107430-1-kai.heng.feng@canonical.com
State New
Headers show
Series PCI/portdrv: Disallow runtime suspend when waekup is required but PME service isn't supported | expand

Commit Message

Kai-Heng Feng Aug. 9, 2021, 4:24 a.m. UTC
Some platforms cannot detect ethernet hotplug once its upstream port is
runtime suspended because PME isn't enabled in _OSC. The issue can be
workarounded by "pcie_ports=native".

The vendor confirmed that the PME in _OSC is disabled intentionally for
stability issues on the other OS, so we should also honor the PME
setting here.

Disallow port runtime suspend when any child device requires wakeup, so
pci_pme_list_scan() can still read the PME status from the devices
behind the port.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213873
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/portdrv_pci.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Lukas Wunner Aug. 9, 2021, 9:47 a.m. UTC | #1
[cc += Mika]

On Mon, Aug 09, 2021 at 12:24:12PM +0800, Kai-Heng Feng wrote:
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213873

The last comment on this bugzilla says "BIOS will fix this."
and the status is RESOLVED WILL_NOT_FIX.

Why is the patch still necessary?


> Some platforms cannot detect ethernet hotplug once its upstream port is
> runtime suspended because PME isn't enabled in _OSC.

If PME is not handled natively, why does the NIC runtime suspend?
Shouldn't this be fixed in the NIC driver by keeping the device
runtime active if PME cannot be used?


> Disallow port runtime suspend when any child device requires wakeup, so
> pci_pme_list_scan() can still read the PME status from the devices
> behind the port.

pci_pme_list_scan() is for broken devices which fail to signal PME.
Is this NIC really among them or does PME fail merely because it's not
granted to OSPM?


> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -59,14 +59,30 @@ static int pcie_port_runtime_suspend(struct device *dev)
>  	return pcie_port_device_runtime_suspend(dev);
>  }
>  
> +static int pcie_port_wakeup_check(struct device *dev, void *data)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (!pdev)
> +		return 0;
> +
> +	return pdev->wakeup_prepared;
> +}
> +
>  static int pcie_port_runtime_idle(struct device *dev)
>  {
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (!pcie_port_find_device(pdev, PCIE_PORT_SERVICE_PME) &&
> +	    device_for_each_child(dev, NULL, pcie_port_wakeup_check))
> +		return -EBUSY;
> +
>  	/*
>  	 * Assume the PCI core has set bridge_d3 whenever it thinks the port
>  	 * should be good to go to D3.  Everything else, including moving
>  	 * the port to D3, is handled by the PCI core.
>  	 */
> -	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> +	return pdev->bridge_d3 ? 0 : -EBUSY;

If an additional check is necessary for this issue, it should be
integrated into pci_dev_check_d3cold() instead of pcie_port_runtime_idle().

Thanks,

Lukas
Kai-Heng Feng Aug. 9, 2021, 10:40 a.m. UTC | #2
On Mon, Aug 9, 2021 at 5:47 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> [cc += Mika]
>
> On Mon, Aug 09, 2021 at 12:24:12PM +0800, Kai-Heng Feng wrote:
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213873
>
> The last comment on this bugzilla says "BIOS will fix this."
> and the status is RESOLVED WILL_NOT_FIX.
>
> Why is the patch still necessary?

Well, let me update the bug report.

>
>
> > Some platforms cannot detect ethernet hotplug once its upstream port is
> > runtime suspended because PME isn't enabled in _OSC.
>
> If PME is not handled natively, why does the NIC runtime suspend?
> Shouldn't this be fixed in the NIC driver by keeping the device
> runtime active if PME cannot be used?

That means we need to fix every user of pci_dev_run_wake(), or fix the
issue in pci_dev_run_wake() helper itself.
However, I am afraid that implementing the fix in pci_dev_run_wake()
may break the while loop check:
bool pci_dev_run_wake(struct pci_dev *dev)
{
 ...
        while (bus->parent) {
                struct pci_dev *bridge = bus->self;

                if (device_can_wakeup(&bridge->dev))
                        return true;

                bus = bus->parent;
...
}

So I took the current approach.

>
>
> > Disallow port runtime suspend when any child device requires wakeup, so
> > pci_pme_list_scan() can still read the PME status from the devices
> > behind the port.
>
> pci_pme_list_scan() is for broken devices which fail to signal PME.
> Is this NIC really among them or does PME fail merely because it's not
> granted to OSPM?

The latter, PME IRQ isn't enabled because it's not granted by BIOS _OSC.

>
>
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -59,14 +59,30 @@ static int pcie_port_runtime_suspend(struct device *dev)
> >       return pcie_port_device_runtime_suspend(dev);
> >  }
> >
> > +static int pcie_port_wakeup_check(struct device *dev, void *data)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +     if (!pdev)
> > +             return 0;
> > +
> > +     return pdev->wakeup_prepared;
> > +}
> > +
> >  static int pcie_port_runtime_idle(struct device *dev)
> >  {
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +     if (!pcie_port_find_device(pdev, PCIE_PORT_SERVICE_PME) &&
> > +         device_for_each_child(dev, NULL, pcie_port_wakeup_check))
> > +             return -EBUSY;
> > +
> >       /*
> >        * Assume the PCI core has set bridge_d3 whenever it thinks the port
> >        * should be good to go to D3.  Everything else, including moving
> >        * the port to D3, is handled by the PCI core.
> >        */
> > -     return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> > +     return pdev->bridge_d3 ? 0 : -EBUSY;
>
> If an additional check is necessary for this issue, it should be
> integrated into pci_dev_check_d3cold() instead of pcie_port_runtime_idle().

I think PME IRQ and D3cold are different things here.
The root port of the affected NIC doesn't support D3cold because
there's no power resource.

Kai-Heng

>
> Thanks,
>
> Lukas
Lukas Wunner Aug. 9, 2021, 3 p.m. UTC | #3
[cc += Rafael]

On Mon, Aug 09, 2021 at 06:40:41PM +0800, Kai-Heng Feng wrote:
> On Mon, Aug 9, 2021 at 5:47 PM Lukas Wunner <lukas@wunner.de> wrote:
> > On Mon, Aug 09, 2021 at 12:24:12PM +0800, Kai-Heng Feng wrote:
> > > Some platforms cannot detect ethernet hotplug once its upstream port is
> > > runtime suspended because PME isn't enabled in _OSC.
> >
> > If PME is not handled natively, why does the NIC runtime suspend?
> > Shouldn't this be fixed in the NIC driver by keeping the device
> > runtime active if PME cannot be used?
> 
> That means we need to fix every user of pci_dev_run_wake(), or fix the
> issue in pci_dev_run_wake() helper itself.

If PME is not granted to the OS, the only consequence is that the PME
port service is not instantiated at the root port.  But PME is still
enabled for downstream devices.  Maybe that's a mistake?  I think the
ACPI spec is a little unclear what to do if PME control is *not* granted.
It only specifies what to do if PME control is *granted*:

   "If the OS successfully receives control of this feature, it must
    handle power management events as described in the PCI Express Base
    Specification."

   "If firmware allows the OS control of this feature, then in the context
    of the _OSC method it must ensure that all PMEs are routed to root port
    interrupts as described in the PCI Express Base Specification.
    Additionally, after control is transferred to the OS, firmware must not
    update the PME Status field in the Root Status register or the PME
    Interrupt Enable field in the Root Control register. If control of this
    feature was requested and denied or was not requested, firmware returns
    this bit set to 0."

Perhaps something like the below is appropriate, I'm not sure.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 091b4a4..7e64185 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3099,7 +3099,7 @@ void pci_pm_init(struct pci_dev *dev)
 	}
 
 	pmc &= PCI_PM_CAP_PME_MASK;
-	if (pmc) {
+	if (pmc && pci_find_host_bridge(dev->bus)->native_pme) {
 		pci_info(dev, "PME# supported from%s%s%s%s%s\n",
 			 (pmc & PCI_PM_CAP_PME_D0) ? " D0" : "",
 			 (pmc & PCI_PM_CAP_PME_D1) ? " D1" : "",


> > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > @@ -59,14 +59,30 @@ static int pcie_port_runtime_suspend(struct device *dev)
> > >       return pcie_port_device_runtime_suspend(dev);
> > >  }
> > >
> > > +static int pcie_port_wakeup_check(struct device *dev, void *data)
> > > +{
> > > +     struct pci_dev *pdev = to_pci_dev(dev);
> > > +
> > > +     if (!pdev)
> > > +             return 0;
> > > +
> > > +     return pdev->wakeup_prepared;
> > > +}
> > > +
> > >  static int pcie_port_runtime_idle(struct device *dev)
> > >  {
> > > +     struct pci_dev *pdev = to_pci_dev(dev);
> > > +
> > > +     if (!pcie_port_find_device(pdev, PCIE_PORT_SERVICE_PME) &&
> > > +         device_for_each_child(dev, NULL, pcie_port_wakeup_check))
> > > +             return -EBUSY;
> > > +
> > >       /*
> > >        * Assume the PCI core has set bridge_d3 whenever it thinks the port
> > >        * should be good to go to D3.  Everything else, including moving
> > >        * the port to D3, is handled by the PCI core.
> > >        */
> > > -     return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> > > +     return pdev->bridge_d3 ? 0 : -EBUSY;
> >
> > If an additional check is necessary for this issue, it should be
> > integrated into pci_dev_check_d3cold() instead of pcie_port_runtime_idle().
> 
> I think PME IRQ and D3cold are different things here.
> The root port of the affected NIC doesn't support D3cold because
> there's no power resource.

If a bridge is runtime suspended to D3, the hierarchy below it is
inaccessible, which is basically the same as if it's put in D3cold,
hence the name pci_dev_check_d3cold().  That function allows a device
to block an upstream bridge from runtime suspending because the device
is not allowed to go to D3cold.  The function specifically checks whether
a device is PME-capable from D3cold.  The NIC claims it's capable but
the PME event has no effect because PME control wasn't granted to the
OS and firmware neglected to set PME Interrupt Enable in the Root Control
register.  We could check for this case and block runtime PM of bridges
based on the rationale that PME polling is needed to detect wakeup.

Thanks,

Lukas
Kai-Heng Feng Aug. 10, 2021, 3:37 p.m. UTC | #4
On Mon, Aug 9, 2021 at 11:00 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> [cc += Rafael]
>
> On Mon, Aug 09, 2021 at 06:40:41PM +0800, Kai-Heng Feng wrote:
> > On Mon, Aug 9, 2021 at 5:47 PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Mon, Aug 09, 2021 at 12:24:12PM +0800, Kai-Heng Feng wrote:
> > > > Some platforms cannot detect ethernet hotplug once its upstream port is
> > > > runtime suspended because PME isn't enabled in _OSC.
> > >
> > > If PME is not handled natively, why does the NIC runtime suspend?
> > > Shouldn't this be fixed in the NIC driver by keeping the device
> > > runtime active if PME cannot be used?
> >
> > That means we need to fix every user of pci_dev_run_wake(), or fix the
> > issue in pci_dev_run_wake() helper itself.
>
> If PME is not granted to the OS, the only consequence is that the PME
> port service is not instantiated at the root port.  But PME is still
> enabled for downstream devices.  Maybe that's a mistake?  I think the
> ACPI spec is a little unclear what to do if PME control is *not* granted.
> It only specifies what to do if PME control is *granted*:

So do you prefer to just disable runtime PM for the downstream device?

>
>    "If the OS successfully receives control of this feature, it must
>     handle power management events as described in the PCI Express Base
>     Specification."
>
>    "If firmware allows the OS control of this feature, then in the context
>     of the _OSC method it must ensure that all PMEs are routed to root port
>     interrupts as described in the PCI Express Base Specification.
>     Additionally, after control is transferred to the OS, firmware must not
>     update the PME Status field in the Root Status register or the PME
>     Interrupt Enable field in the Root Control register. If control of this
>     feature was requested and denied or was not requested, firmware returns
>     this bit set to 0."
>
> Perhaps something like the below is appropriate, I'm not sure.
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 091b4a4..7e64185 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3099,7 +3099,7 @@ void pci_pm_init(struct pci_dev *dev)
>         }
>
>         pmc &= PCI_PM_CAP_PME_MASK;
> -       if (pmc) {
> +       if (pmc && pci_find_host_bridge(dev->bus)->native_pme) {
>                 pci_info(dev, "PME# supported from%s%s%s%s%s\n",
>                          (pmc & PCI_PM_CAP_PME_D0) ? " D0" : "",
>                          (pmc & PCI_PM_CAP_PME_D1) ? " D1" : "",
>
>

I think this will also prevent non-root port devices from using PME.

[snipped]

> >
> > I think PME IRQ and D3cold are different things here.
> > The root port of the affected NIC doesn't support D3cold because
> > there's no power resource.
>
> If a bridge is runtime suspended to D3, the hierarchy below it is
> inaccessible, which is basically the same as if it's put in D3cold,
> hence the name pci_dev_check_d3cold().  That function allows a device
> to block an upstream bridge from runtime suspending because the device
> is not allowed to go to D3cold.  The function specifically checks whether
> a device is PME-capable from D3cold.  The NIC claims it's capable but
> the PME event has no effect because PME control wasn't granted to the
> OS and firmware neglected to set PME Interrupt Enable in the Root Control
> register.  We could check for this case and block runtime PM of bridges
> based on the rationale that PME polling is needed to detect wakeup.

So for this case, should we prevent the downstream devices from
runtime suspending, or let it suspend but keep the root port active in
order to make pci_pme_list_scan() work?

Kai-Heng

>
> Thanks,
>
> Lukas
Lukas Wunner Aug. 10, 2021, 4:21 p.m. UTC | #5
On Tue, Aug 10, 2021 at 11:37:12PM +0800, Kai-Heng Feng wrote:
> On Mon, Aug 9, 2021 at 11:00 PM Lukas Wunner <lukas@wunner.de> wrote:
> > If PME is not granted to the OS, the only consequence is that the PME
> > port service is not instantiated at the root port.  But PME is still
> > enabled for downstream devices.  Maybe that's a mistake?  I think the
> > ACPI spec is a little unclear what to do if PME control is *not* granted.
> > It only specifies what to do if PME control is *granted*:
> 
> So do you prefer to just disable runtime PM for the downstream device?

I honestly don't know.  I was just wondering whether it is okay
to enable PME on devices if control is not granted by the firmware.
The spec is fairly vague.  But I guess the idea is that enabling PME
on devices is correct, just handling the interrupts is done by firmware
instead of the OS.

In your case, the endpoint device claims it can signal PME from D3cold,
which is why we allow the root port above to runtime suspend to D3hot.
The lspci output you've attached to the bugzilla indicates that yes,
signaling PME in D3cold does work, but the PME interrupt is neither
handled by the OS (because it's not allowed to) nor by firmware.

So you would like to rely on PME polling instead, which only works if the
root port remains in D0.  Otherwise config space of the endpoint device
is inaccessible.

I think the proper solution is that firmware should handle the PME
interrupt.  You've said the vendor objects because they found PME
doesn't work reliably.  Well in that case the endpoint device shouldn't
indicate that it can signal PME, at least not from D3cold.  Perhaps
the vendor is able to change the endpoint device's config space so
that it doesn't claim to support PME?

If that doesn't work and thus a kernel patch is necessary, the next
question is whether changing core code is the right approach.

If you do want to change core code, I'd suggest modifying 
pci_dev_check_d3cold() so that it blocks runtime PM on upstream
bridges if PME is not handled natively AND firmware failed to enable
the PME interrupt at the root port.  The rationale is that upstream
bridges need to remain in D0 so that PME polling is possible.

An alternative would be a quirk for this specific laptop which clears
pdev->pme_support.

Thanks,

Lukas
Kai-Heng Feng Aug. 11, 2021, 5:06 a.m. UTC | #6
On Wed, Aug 11, 2021 at 12:21 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Tue, Aug 10, 2021 at 11:37:12PM +0800, Kai-Heng Feng wrote:
> > On Mon, Aug 9, 2021 at 11:00 PM Lukas Wunner <lukas@wunner.de> wrote:
> > > If PME is not granted to the OS, the only consequence is that the PME
> > > port service is not instantiated at the root port.  But PME is still
> > > enabled for downstream devices.  Maybe that's a mistake?  I think the
> > > ACPI spec is a little unclear what to do if PME control is *not* granted.
> > > It only specifies what to do if PME control is *granted*:
> >
> > So do you prefer to just disable runtime PM for the downstream device?
>
> I honestly don't know.  I was just wondering whether it is okay
> to enable PME on devices if control is not granted by the firmware.
> The spec is fairly vague.  But I guess the idea is that enabling PME
> on devices is correct, just handling the interrupts is done by firmware
> instead of the OS.

Does this imply that current ACPI doesn't handle this part?

>
> In your case, the endpoint device claims it can signal PME from D3cold,
> which is why we allow the root port above to runtime suspend to D3hot.
> The lspci output you've attached to the bugzilla indicates that yes,
> signaling PME in D3cold does work, but the PME interrupt is neither
> handled by the OS (because it's not allowed to) nor by firmware.
>
> So you would like to rely on PME polling instead, which only works if the
> root port remains in D0.  Otherwise config space of the endpoint device
> is inaccessible.

The Windows approach is to make the entire hierarchy stays at D0, I
think maybe it's a better way than relying on PME polling.

>
> I think the proper solution is that firmware should handle the PME
> interrupt.  You've said the vendor objects because they found PME
> doesn't work reliably.

The PME works, what vendor said is that enabling PME makes the system
"unstable".

> Well in that case the endpoint device shouldn't
> indicate that it can signal PME, at least not from D3cold.  Perhaps
> the vendor is able to change the endpoint device's config space so
> that it doesn't claim to support PME?

 This is not an viable option, and we have to consider that BIOS from
different vendors can exhibit the same behavior.

>
> If that doesn't work and thus a kernel patch is necessary, the next
> question is whether changing core code is the right approach.

I really don't see other way because non-granted PME is a system-wide thing...

>
> If you do want to change core code, I'd suggest modifying
> pci_dev_check_d3cold() so that it blocks runtime PM on upstream
> bridges if PME is not handled natively AND firmware failed to enable
> the PME interrupt at the root port.  The rationale is that upstream
> bridges need to remain in D0 so that PME polling is possible.

How do I know that firmware failed to enable PME IRQ?

And let me see how to make pci_dev_check_d3cold() work for this case.

>
> An alternative would be a quirk for this specific laptop which clears
> pdev->pme_support.

This won't scale, because many models are affected.

Kai-Heng

>
> Thanks,
>
> Lukas
Lukas Wunner Aug. 11, 2021, 7:11 a.m. UTC | #7
On Wed, Aug 11, 2021 at 01:06:27PM +0800, Kai-Heng Feng wrote:
> On Wed, Aug 11, 2021 at 12:21 AM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Tue, Aug 10, 2021 at 11:37:12PM +0800, Kai-Heng Feng wrote:
> > I honestly don't know.  I was just wondering whether it is okay
> > to enable PME on devices if control is not granted by the firmware.
> > The spec is fairly vague.  But I guess the idea is that enabling PME
> > on devices is correct, just handling the interrupts is done by firmware
> > instead of the OS.
> 
> Does this imply that current ACPI doesn't handle this part?

Apparently not, according to the "lspci-bridge-after-hotplug" you've
attached to the bugzilla, the PME Interrupt Enable bit wasn't set in
the Root Control register.  The kernel doesn't register an IRQ handler
for PME because firmware doesn't grant it control, so it's firmware's
job to enable and handle the IRQ (or poll the relevant register or
whatever).

  RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
                                                   ^^^^^^^^^^

> The Windows approach is to make the entire hierarchy stays at D0, I
> think maybe it's a better way than relying on PME polling.

Including the endpoint device, i.e. the NIC?


> > If you do want to change core code, I'd suggest modifying
> > pci_dev_check_d3cold() so that it blocks runtime PM on upstream
> > bridges if PME is not handled natively AND firmware failed to enable
> > the PME interrupt at the root port.  The rationale is that upstream
> > bridges need to remain in D0 so that PME polling is possible.
> 
> How do I know that firmware failed to enable PME IRQ?

Check whether PCI_EXP_RTCTL_PMEIE was set by firmware in the Root Control
register.


> > An alternative would be a quirk for this specific laptop which clears
> > pdev->pme_support.
> 
> This won't scale, because many models are affected.

We already have quirks which clear pdev->pme_support, e.g.
pci_fixup_no_d0_pme() and pci_fixup_no_msi_no_pme().
Perhaps something like that would be appropriate here.

Thanks,

Lukas
Kai-Heng Feng Aug. 12, 2021, 5:20 a.m. UTC | #8
On Wed, Aug 11, 2021 at 3:11 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Wed, Aug 11, 2021 at 01:06:27PM +0800, Kai-Heng Feng wrote:
> > On Wed, Aug 11, 2021 at 12:21 AM Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > On Tue, Aug 10, 2021 at 11:37:12PM +0800, Kai-Heng Feng wrote:
> > > I honestly don't know.  I was just wondering whether it is okay
> > > to enable PME on devices if control is not granted by the firmware.
> > > The spec is fairly vague.  But I guess the idea is that enabling PME
> > > on devices is correct, just handling the interrupts is done by firmware
> > > instead of the OS.
> >
> > Does this imply that current ACPI doesn't handle this part?
>
> Apparently not, according to the "lspci-bridge-after-hotplug" you've
> attached to the bugzilla, the PME Interrupt Enable bit wasn't set in
> the Root Control register.  The kernel doesn't register an IRQ handler
> for PME because firmware doesn't grant it control, so it's firmware's
> job to enable and handle the IRQ (or poll the relevant register or
> whatever).
>
>   RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
>                                                    ^^^^^^^^^^

OK, I'll send a patch that checks this flag for PME capability.

>
> > The Windows approach is to make the entire hierarchy stays at D0, I
> > think maybe it's a better way than relying on PME polling.
>
> Including the endpoint device, i.e. the NIC?

Yes, including the endpoint device.

>
>
> > > If you do want to change core code, I'd suggest modifying
> > > pci_dev_check_d3cold() so that it blocks runtime PM on upstream
> > > bridges if PME is not handled natively AND firmware failed to enable
> > > the PME interrupt at the root port.  The rationale is that upstream
> > > bridges need to remain in D0 so that PME polling is possible.
> >
> > How do I know that firmware failed to enable PME IRQ?
>
> Check whether PCI_EXP_RTCTL_PMEIE was set by firmware in the Root Control
> register.

I originally thought there can be a special ACPI method to query this info.

>
>
> > > An alternative would be a quirk for this specific laptop which clears
> > > pdev->pme_support.
> >
> > This won't scale, because many models are affected.
>
> We already have quirks which clear pdev->pme_support, e.g.
> pci_fixup_no_d0_pme() and pci_fixup_no_msi_no_pme().
> Perhaps something like that would be appropriate here.

OK, I'll take this approach.

Kai-Heng

>
> Thanks,
>
> Lukas
diff mbox series

Patch

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index c7ff1eea225a..e693d243c90d 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -59,14 +59,30 @@  static int pcie_port_runtime_suspend(struct device *dev)
 	return pcie_port_device_runtime_suspend(dev);
 }
 
+static int pcie_port_wakeup_check(struct device *dev, void *data)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev)
+		return 0;
+
+	return pdev->wakeup_prepared;
+}
+
 static int pcie_port_runtime_idle(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pcie_port_find_device(pdev, PCIE_PORT_SERVICE_PME) &&
+	    device_for_each_child(dev, NULL, pcie_port_wakeup_check))
+		return -EBUSY;
+
 	/*
 	 * Assume the PCI core has set bridge_d3 whenever it thinks the port
 	 * should be good to go to D3.  Everything else, including moving
 	 * the port to D3, is handled by the PCI core.
 	 */
-	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+	return pdev->bridge_d3 ? 0 : -EBUSY;
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {