lkml.org 
[lkml]   [2019]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] PCI/PM: Report runtime wakeup is not supported if bridge isn't bound to driver
From
Date


> 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

>
>
>

\
 
 \ /
  Last update: 2019-12-30 08:22    [W:0.042 / U:3.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site