* [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
@ 2019-12-27 9:24 Kai-Heng Feng
2019-12-27 10:36 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Kai-Heng Feng @ 2019-12-27 9:24 UTC (permalink / raw)
To: bhelgaas, rafael.j.wysocki; +Cc: linux-pci, linux-kernel, Kai-Heng Feng
We have a Pericom USB add-on card that has three USB controller
functions 06:00.[0-2], connected to bridge device 05:03.0, which is
connected to another bridge device 04:00.0:
-[0000:00]-+-00.0
+-1c.6-[04-06]----00.0-[05-06]----03.0-[06]--+-00.0
| +-00.1
| \-00.2
When bridge device (05:03.0) and all three USB controller functions
(06:00.[0-2]) are runtime suspended, they don't get woken up by plugging
USB devices into the add-on card.
This is because the pcieport driver failed to probe on 04:00.0, since
the device supports neither legacy IRQ, MSI nor MSI-X. Because of that,
there's no native PCIe PME can work for devices connected to it.
So let's correctly report runtime wakeup isn't supported when any of
PCIe bridges isn't bound to pcieport driver.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205981
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/pci/pci.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 951099279192..ca686cfbd65e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2493,6 +2493,18 @@ bool pci_dev_run_wake(struct pci_dev *dev)
if (!pci_pme_capable(dev, pci_target_state(dev, true)))
return false;
+ /* If any upstream PCIe bridge isn't bound to pcieport driver, there's
+ * no IRQ for PME.
+ */
+ if (pci_is_pcie(dev)) {
+ while (bus->parent) {
+ if (!bus->self->driver)
+ return false;
+
+ bus = bus->parent;
+ }
+ }
+
if (device_can_wakeup(&dev->dev))
return true;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
2019-12-27 9:24 [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver Kai-Heng Feng
@ 2019-12-27 10:36 ` Rafael J. Wysocki
2019-12-27 17:15 ` Kai-Heng Feng
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-12-27 10:36 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: bhelgaas, rafael.j.wysocki, linux-pci, linux-kernel
On Friday, December 27, 2019 10:24:05 AM CET Kai-Heng Feng wrote:
> We have a Pericom USB add-on card that has three USB controller
> functions 06:00.[0-2], connected to bridge device 05:03.0, which is
> connected to another bridge device 04:00.0:
>
> -[0000:00]-+-00.0
> +-1c.6-[04-06]----00.0-[05-06]----03.0-[06]--+-00.0
> | +-00.1
> | \-00.2
>
> When bridge device (05:03.0) and all three USB controller functions
> (06:00.[0-2]) are runtime suspended, they don't get woken up by plugging
> USB devices into the add-on card.
>
> This is because the pcieport driver failed to probe on 04:00.0, since
> the device supports neither legacy IRQ, MSI nor MSI-X. Because of that,
> there's no native PCIe PME can work for devices connected to it.
But in that case the PME driver (drivers/pci/pcie/pme.c) should not bind
to the port in question, so the "can_wakeup" flag should not be set for
the devices under that port.
> So let's correctly report runtime wakeup isn't supported when any of
> PCIe bridges isn't bound to pcieport driver.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205981
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/pci/pci.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 951099279192..ca686cfbd65e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2493,6 +2493,18 @@ bool pci_dev_run_wake(struct pci_dev *dev)
> if (!pci_pme_capable(dev, pci_target_state(dev, true)))
> return false;
>
> + /* If any upstream PCIe bridge isn't bound to pcieport driver, there's
> + * no IRQ for PME.
> + */
> + if (pci_is_pcie(dev)) {
> + while (bus->parent) {
> + if (!bus->self->driver)
> + return false;
> +
> + bus = bus->parent;
> + }
> + }
> +
So it looks like device_can_wakeup() returns 'true' for this device, but it
should return 'false'.
Do you know why the "can_wakeup" flag is set for it?
> if (device_can_wakeup(&dev->dev))
> return true;
>
>
Moreover, even if the native PME is not supported, there can be an ACPI GPE (or
equivalent) that triggers when WAKE# is asserted by one of the PCIe devices
connected to it, so the test added by this patch cannot be used in general.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
2019-12-27 10:36 ` Rafael J. Wysocki
@ 2019-12-27 17:15 ` Kai-Heng Feng
2019-12-29 22:37 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Kai-Heng Feng @ 2019-12-27 17:15 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: bhelgaas, rafael.j.wysocki, linux-pci, linux-kernel
> On Dec 27, 2019, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Friday, December 27, 2019 10:24:05 AM CET Kai-Heng Feng wrote:
>> We have a Pericom USB add-on card that has three USB controller
>> functions 06:00.[0-2], connected to bridge device 05:03.0, which is
>> connected to another bridge device 04:00.0:
>>
>> -[0000:00]-+-00.0
>> +-1c.6-[04-06]----00.0-[05-06]----03.0-[06]--+-00.0
>> | +-00.1
>> | \-00.2
>>
>> When bridge device (05:03.0) and all three USB controller functions
>> (06:00.[0-2]) are runtime suspended, they don't get woken up by plugging
>> USB devices into the add-on card.
>>
>> This is because the pcieport driver failed to probe on 04:00.0, since
>> the device supports neither legacy IRQ, MSI nor MSI-X. Because of that,
>> there's no native PCIe PME can work for devices connected to it.
>
> But in that case the PME driver (drivers/pci/pcie/pme.c) should not bind
> to the port in question, so the "can_wakeup" flag should not be set for
> the devices under that port.
We can remove the can_wakeup flag for all its child devices once pcieport probe fails, but I think it's not intuitive.
>
>> So let's correctly report runtime wakeup isn't supported when any of
>> PCIe bridges isn't bound to pcieport driver.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205981
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pci.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 951099279192..ca686cfbd65e 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2493,6 +2493,18 @@ bool pci_dev_run_wake(struct pci_dev *dev)
>> if (!pci_pme_capable(dev, pci_target_state(dev, true)))
>> return false;
>>
>> + /* If any upstream PCIe bridge isn't bound to pcieport driver, there's
>> + * no IRQ for PME.
>> + */
>> + if (pci_is_pcie(dev)) {
>> + while (bus->parent) {
>> + if (!bus->self->driver)
>> + return false;
>> +
>> + bus = bus->parent;
>> + }
>> + }
>> +
>
> So it looks like device_can_wakeup() returns 'true' for this device, but it
> should return 'false'.
The USB controllers can assert PME#, so it actually can wakeup, in a way.
I think the logical distinction between pci_dev_run_wake() and device_can_wakeup() is that,
pci_dev_run_wake() means it can actually do runtime wakeup, while device_can_wakeup()
only means it has the capability to wakeup. Am I correct here?
>
> Do you know why the "can_wakeup" flag is set for it?
All PCI devices with PME cap calls device_set_wakeup_capable() in pci_pm_init().
>
>> if (device_can_wakeup(&dev->dev))
>> return true;
>>
>>
>
> Moreover, even if the native PME is not supported, there can be an ACPI GPE (or
> equivalent) that triggers when WAKE# is asserted by one of the PCIe devices
> connected to it, so the test added by this patch cannot be used in general.
Ok. So how to know when both native PME isn't supported and it doesn't use ACPI GPE?
I thought ACPI GPE only works for devices directly connect to Root Complex, but I can't find the reference now.
Another short-term workaround is to make pci_pme_list_scan() not skip bridge when it's in D3hot:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e87196cc1a7f..3333194a62d3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2111,7 +2111,7 @@ static void pci_pme_list_scan(struct work_struct *work)
* configuration space of subordinate devices
* may be not accessible
*/
- if (bridge && bridge->current_state != PCI_D0)
+ if (bridge && bridge->current_state == PCI_D3cold)
continue;
/*
* If the device is in D3cold it should not be
I haven't seen any case that config space is not accessible under D3hot, but I don't have PCI spec to check.
Kai-Heng
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
2019-12-27 17:15 ` Kai-Heng Feng
@ 2019-12-29 22:37 ` Rafael J. Wysocki
2019-12-30 7:21 ` Kai-Heng Feng
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-12-29 22:37 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: bhelgaas, rafael.j.wysocki, linux-pci, linux-kernel
On Friday, December 27, 2019 6:15:26 PM CET Kai-Heng Feng wrote:
>
> > On Dec 27, 2019, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Friday, December 27, 2019 10:24:05 AM CET Kai-Heng Feng wrote:
> >> We have a Pericom USB add-on card that has three USB controller
> >> functions 06:00.[0-2], connected to bridge device 05:03.0, which is
> >> connected to another bridge device 04:00.0:
> >>
> >> -[0000:00]-+-00.0
> >> +-1c.6-[04-06]----00.0-[05-06]----03.0-[06]--+-00.0
> >> | +-00.1
> >> | \-00.2
> >>
> >> When bridge device (05:03.0) and all three USB controller functions
> >> (06:00.[0-2]) are runtime suspended, they don't get woken up by plugging
> >> USB devices into the add-on card.
> >>
> >> This is because the pcieport driver failed to probe on 04:00.0, since
> >> the device supports neither legacy IRQ, MSI nor MSI-X. Because of that,
> >> there's no native PCIe PME can work for devices connected to it.
> >
> > But in that case the PME driver (drivers/pci/pcie/pme.c) should not bind
> > to the port in question, so the "can_wakeup" flag should not be set for
> > the devices under that port.
>
> We can remove the can_wakeup flag for all its child devices once pcieport probe fails, but I think it's not intuitive.
>
> >
> >> So let's correctly report runtime wakeup isn't supported when any of
> >> PCIe bridges isn't bound to pcieport driver.
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205981
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >> ---
> >> drivers/pci/pci.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 951099279192..ca686cfbd65e 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -2493,6 +2493,18 @@ bool pci_dev_run_wake(struct pci_dev *dev)
> >> if (!pci_pme_capable(dev, pci_target_state(dev, true)))
> >> return false;
> >>
> >> + /* If any upstream PCIe bridge isn't bound to pcieport driver, there's
> >> + * no IRQ for PME.
> >> + */
> >> + if (pci_is_pcie(dev)) {
> >> + while (bus->parent) {
> >> + if (!bus->self->driver)
> >> + return false;
> >> +
> >> + bus = bus->parent;
> >> + }
> >> + }
> >> +
> >
> > So it looks like device_can_wakeup() returns 'true' for this device, but it
> > should return 'false'.
>
> The USB controllers can assert PME#, so it actually can wakeup, in a way.
Well, that's questionable.
If there is no known way for the PME to be signaled, we may as well mark the
device as wakeup-incapable.
> I think the logical distinction between pci_dev_run_wake() and device_can_wakeup() is that,
> pci_dev_run_wake() means it can actually do runtime wakeup, while device_can_wakeup()
> only means it has the capability to wakeup. Am I correct here?
Kind of, but the "capability" part is not well defined, so to speak, because
if the device happens to be located below a PCIe port in a low-power state
(say D3hot), the PME "support" declared in the config space is clearly
insufficient.
> >
> > Do you know why the "can_wakeup" flag is set for it?
>
> All PCI devices with PME cap calls device_set_wakeup_capable() in pci_pm_init().
Right, I forgot about that thing.
It is inconsistent with the rest of the code, particularly with
pci_dev_run_wake(), so I'd try to drop it.
IIRC that would require some other pieces of code to be reworked to avoid
regressions, though.
> >
> >> if (device_can_wakeup(&dev->dev))
> >> return true;
> >>
> >>
> >
> > Moreover, even if the native PME is not supported, there can be an ACPI GPE (or
> > equivalent) that triggers when WAKE# is asserted by one of the PCIe devices
> > connected to it, so the test added by this patch cannot be used in general.
>
> Ok. So how to know when both native PME isn't supported and it doesn't use ACPI GPE?
If the PME driver doesn't bind to the device's root port, the native PME cannot
work.
If there is no wakeup GPE, pci_acpi_setup() will not call
device_set_wakeup_capable() for the device.
> I thought ACPI GPE only works for devices directly connect to Root Complex, but I can't find the reference now.
No, that's not the case.
It can work for any devices (even old-style PCI, non-PCIe) with PME# connected
to a dedicated WAKE# pin on the board (which then is represented as an ACPI GPE
or a GPIO IRQ).
>
> Another short-term workaround is to make pci_pme_list_scan() not skip bridge when it's in D3hot:
No, that would not be safe in general.
Basically, pci_finish_runtime_suspend() needs to enable wakeup for devices
that can do PME even though can_wakeup is not set for them, as long as
pci_pme_list_scan() can reach them. That should be sufficient to cover
all of the practically relevant cases.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
2019-12-29 22:37 ` Rafael J. Wysocki
@ 2019-12-30 7:21 ` Kai-Heng Feng
0 siblings, 0 replies; 5+ messages in thread
From: Kai-Heng Feng @ 2019-12-30 7:21 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: bhelgaas, rafael.j.wysocki, linux-pci, linux-kernel
> On Dec 30, 2019, at 06:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Friday, December 27, 2019 6:15:26 PM CET Kai-Heng Feng wrote:
>>
>>> On Dec 27, 2019, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>
>>> On Friday, December 27, 2019 10:24:05 AM CET Kai-Heng Feng wrote:
>>>> We have a Pericom USB add-on card that has three USB controller
>>>> functions 06:00.[0-2], connected to bridge device 05:03.0, which is
>>>> connected to another bridge device 04:00.0:
>>>>
>>>> -[0000:00]-+-00.0
>>>> +-1c.6-[04-06]----00.0-[05-06]----03.0-[06]--+-00.0
>>>> | +-00.1
>>>> | \-00.2
>>>>
>>>> When bridge device (05:03.0) and all three USB controller functions
>>>> (06:00.[0-2]) are runtime suspended, they don't get woken up by plugging
>>>> USB devices into the add-on card.
>>>>
>>>> This is because the pcieport driver failed to probe on 04:00.0, since
>>>> the device supports neither legacy IRQ, MSI nor MSI-X. Because of that,
>>>> there's no native PCIe PME can work for devices connected to it.
>>>
>>> But in that case the PME driver (drivers/pci/pcie/pme.c) should not bind
>>> to the port in question, so the "can_wakeup" flag should not be set for
>>> the devices under that port.
>>
>> We can remove the can_wakeup flag for all its child devices once pcieport probe fails, but I think it's not intuitive.
>>
>>>
>>>> So let's correctly report runtime wakeup isn't supported when any of
>>>> PCIe bridges isn't bound to pcieport driver.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205981
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>> ---
>>>> drivers/pci/pci.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index 951099279192..ca686cfbd65e 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2493,6 +2493,18 @@ bool pci_dev_run_wake(struct pci_dev *dev)
>>>> if (!pci_pme_capable(dev, pci_target_state(dev, true)))
>>>> return false;
>>>>
>>>> + /* If any upstream PCIe bridge isn't bound to pcieport driver, there's
>>>> + * no IRQ for PME.
>>>> + */
>>>> + if (pci_is_pcie(dev)) {
>>>> + while (bus->parent) {
>>>> + if (!bus->self->driver)
>>>> + return false;
>>>> +
>>>> + bus = bus->parent;
>>>> + }
>>>> + }
>>>> +
>>>
>>> So it looks like device_can_wakeup() returns 'true' for this device, but it
>>> should return 'false'.
>>
>> The USB controllers can assert PME#, so it actually can wakeup, in a way.
>
> Well, that's questionable.
>
> If there is no known way for the PME to be signaled, we may as well mark the
> device as wakeup-incapable.
Ok. Reasonable.
>
>> I think the logical distinction between pci_dev_run_wake() and device_can_wakeup() is that,
>> pci_dev_run_wake() means it can actually do runtime wakeup, while device_can_wakeup()
>> only means it has the capability to wakeup. Am I correct here?
>
> Kind of, but the "capability" part is not well defined, so to speak, because
> if the device happens to be located below a PCIe port in a low-power state
> (say D3hot), the PME "support" declared in the config space is clearly
> insufficient.
Ok.
>
>>>
>>> Do you know why the "can_wakeup" flag is set for it?
>>
>> All PCI devices with PME cap calls device_set_wakeup_capable() in pci_pm_init().
>
> Right, I forgot about that thing.
>
> It is inconsistent with the rest of the code, particularly with
> pci_dev_run_wake(), so I'd try to drop it.
>
> IIRC that would require some other pieces of code to be reworked to avoid
> regressions, though.
Ok. So I'll work on a v2 patch on top of your change.
>
>>>
>>>> if (device_can_wakeup(&dev->dev))
>>>> return true;
>>>>
>>>>
>>>
>>> Moreover, even if the native PME is not supported, there can be an ACPI GPE (or
>>> equivalent) that triggers when WAKE# is asserted by one of the PCIe devices
>>> connected to it, so the test added by this patch cannot be used in general.
>>
>> Ok. So how to know when both native PME isn't supported and it doesn't use ACPI GPE?
>
> If the PME driver doesn't bind to the device's root port, the native PME cannot
> work.
>
> If there is no wakeup GPE, pci_acpi_setup() will not call
> device_set_wakeup_capable() for the device.
Thanks for the info. Does adding a check on adev->wakeup.flags.valid sufficiently cover all cases for this patch?
>
>> I thought ACPI GPE only works for devices directly connect to Root Complex, but I can't find the reference now.
>
> No, that's not the case.
>
> It can work for any devices (even old-style PCI, non-PCIe) with PME# connected
> to a dedicated WAKE# pin on the board (which then is represented as an ACPI GPE
> or a GPIO IRQ).
Ok, didn't know that.
>
>>
>> Another short-term workaround is to make pci_pme_list_scan() not skip bridge when it's in D3hot:
>
> No, that would not be safe in general.
>
> Basically, pci_finish_runtime_suspend() needs to enable wakeup for devices
> that can do PME even though can_wakeup is not set for them, as long as
> pci_pme_list_scan() can reach them. That should be sufficient to cover
> all of the practically relevant cases.
Understand.
Kai-Heng
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-30 7:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27 9:24 [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver Kai-Heng Feng
2019-12-27 10:36 ` Rafael J. Wysocki
2019-12-27 17:15 ` Kai-Heng Feng
2019-12-29 22:37 ` Rafael J. Wysocki
2019-12-30 7:21 ` Kai-Heng Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.