diff mbox

usb: host: ehci: workaround PME bug on AMD EHCI controller

Message ID CAAd53p580X5_G6a05aAueHVxEoy5hpQrs6s2gtJA+gPEf2PqLg@mail.gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Kai-Heng Feng June 13, 2017, 4:21 a.m. UTC
On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> Let's get some help from people who understand PCI well.
>
> Here's the general problem: Kai-Heng has a PCI-based USB host
> controller that advertises wakeup capability from D3, but it doesn't
> assert PME# from D3 when it should.  For "lspci -vv" output, see
>
>         http://marc.info/?l=linux-usb&m=149570231732519&w=2
>
> On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
>
>> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
>> <kai.heng.feng@canonical.com> wrote:
>> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>> >>
>> >> Is this really the right solution?  Maybe it would be better to allow
>> >> the controller to go into D3 provided no wakeup signal is needed.  You
>> >> could do:
>> >>
>> >>                 device_set_wakeup_capable(&pdev->dev, 0);
>> >
>> > This doesn't work.
>> > After applying this function, still nothing happens when devices get plugged in.
>> > IIUC this function disable the wakeup function, but what I want to do
>> > here is to have PME signal works even when runtime PM is enabled.
>
> This may indicate a bug in either the PCI or USB stacks (or both!).  If
> a driver requires wakeup capability from runtime suspend but the device
> does not provide it, the PCI core should not allow the device to go
> into runtime suspend.  Or is that the driver's responsibility?
>
>> > I also saw some legacy PCI PM stuff, so I also tried:
>> > device_set_wakeup_capable(&pdev->dev, 1);
>> > ...doesn't work either.
>> >
>> >>
>> >> Another alternative is to put the controller into D2 instead of D3, but
>> >> (1) I don't know how to do that, and (2) we don't know if wakeup
>> >> signalling works any better in D2 than it does in D3.
>> >
>> > I'll try if D2 works.
>>
>> Put the device into D2 instead of D3 can make the wakeup signaling
>> work, i.e. USB devices can be correctly detected after plugged into
>> EHCI port.
>>
>> Do you think this alternative an acceptable workaround?
>
> Yes, it is.  The difficulty is that I don't know how to tell the PCI
> core that the device should go in D2 during runtime suspend instead of
> D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
>

FWIW, this is the diff I used to make the controller transits to D2
instead of D3:

Comments

Bjorn Helgaas June 13, 2017, 5:28 p.m. UTC | #1
[+cc Rafael, linux-pm]

On Tue, Jun 13, 2017 at 12:21:15PM +0800, Kai-Heng Feng wrote:
> On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > Let's get some help from people who understand PCI well.
> >
> > Here's the general problem: Kai-Heng has a PCI-based USB host
> > controller that advertises wakeup capability from D3, but it doesn't
> > assert PME# from D3 when it should.  For "lspci -vv" output, see
> >
> >         http://marc.info/?l=linux-usb&m=149570231732519&w=2
> >
> > On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
> >
> >> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
> >> <kai.heng.feng@canonical.com> wrote:
> >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
> >> >>
> >> >> Is this really the right solution?  Maybe it would be better to allow
> >> >> the controller to go into D3 provided no wakeup signal is needed.  You
> >> >> could do:
> >> >>
> >> >>                 device_set_wakeup_capable(&pdev->dev, 0);
> >> >
> >> > This doesn't work.
> >> > After applying this function, still nothing happens when devices get plugged in.
> >> > IIUC this function disable the wakeup function, but what I want to do
> >> > here is to have PME signal works even when runtime PM is enabled.
> >
> > This may indicate a bug in either the PCI or USB stacks (or both!).  If
> > a driver requires wakeup capability from runtime suspend but the device
> > does not provide it, the PCI core should not allow the device to go
> > into runtime suspend.  Or is that the driver's responsibility?
> >
> >> > I also saw some legacy PCI PM stuff, so I also tried:
> >> > device_set_wakeup_capable(&pdev->dev, 1);
> >> > ...doesn't work either.
> >> >
> >> >>
> >> >> Another alternative is to put the controller into D2 instead of D3, but
> >> >> (1) I don't know how to do that, and (2) we don't know if wakeup
> >> >> signalling works any better in D2 than it does in D3.
> >> >
> >> > I'll try if D2 works.
> >>
> >> Put the device into D2 instead of D3 can make the wakeup signaling
> >> work, i.e. USB devices can be correctly detected after plugged into
> >> EHCI port.
> >>
> >> Do you think this alternative an acceptable workaround?
> >
> > Yes, it is.  The difficulty is that I don't know how to tell the PCI
> > core that the device should go in D2 during runtime suspend instead of
> > D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
> >
> 
> FWIW, this is the diff I used to make the controller transits to D2
> instead of D3:
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 563901cd9c06..36663688404a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -922,6 +922,9 @@ int pci_set_power_state(struct pci_dev *dev,
> pci_power_t state)
>         if (dev->current_state == state)
>                 return 0;
> 
> +       if (state > PCI_D2 && (dev->dev_flags & PCI_DEV_FLAGS_MAX_D2))
> +               state = PCI_D2;
> +
>         __pci_start_power_transition(dev, state);
> 
>         /* This device is quirked not to be put into D3, so
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 93326974ff4b..a2c1fe62974a 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>                 if (pdev->device == 0x7808) {
>                         ehci->use_dummy_qh = 1;
>                         ehci_info(ehci, "applying AMD
> SB700/SB800/Hudson-2/3 EHCI dummy qh workaround\n");
> +
> +                       pdev->dev_flags |= PCI_DEV_FLAGS_MAX_D2;
>                 }
>                 break;
>         case PCI_VENDOR_ID_VIA:
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8039f9f0ca05..6f86f2aa8548 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -188,6 +188,7 @@ enum pci_dev_flags {
>          * the direct_complete optimization.
>          */
>         PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
> +       PCI_DEV_FLAGS_MAX_D2 = (__force pci_dev_flags_t) (1 << 12),
>  };
> 
>  enum pci_irq_reroute_variant {

The lspci output [1] shows:

  00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI Controller (rev 39) (prog-if 20 [EHCI])
    Capabilities: [c0] Power Management version 2
      Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
      Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
      Bridge: PM- B3+

The device claims it can assert PME# from D3hot.  If it can't, that
sounds like a hardware defect that should be addressed with a quirk.
Ideally we would also have a pointer to the AMD hardware erratum.

Is the following path involved here?

  pci_finish_runtime_suspend
    target_state = pci_target_state()
      if (device_may_wakup())
	if (dev->pme_support)
	  ...
    pci_set_power_state(..., target_state)

If so, I would naively expect that a quirk could clear the
PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
and pci_target_state() would then avoid selecting D3 or D3cold.  But
I'm not an expert in power management.

Bjorn

[1] http://marc.info/?l=linux-usb&m=149570231732519&w=2
Alan Stern June 14, 2017, 6:55 p.m. UTC | #2
On Tue, 13 Jun 2017, Bjorn Helgaas wrote:

> [+cc Rafael, linux-pm]
> 
> On Tue, Jun 13, 2017 at 12:21:15PM +0800, Kai-Heng Feng wrote:
> > On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > Let's get some help from people who understand PCI well.
> > >
> > > Here's the general problem: Kai-Heng has a PCI-based USB host
> > > controller that advertises wakeup capability from D3, but it doesn't
> > > assert PME# from D3 when it should.  For "lspci -vv" output, see
> > >
> > >         http://marc.info/?l=linux-usb&m=149570231732519&w=2
> > >
> > > On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
> > >
> > >> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
> > >> <kai.heng.feng@canonical.com> wrote:
> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > >> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
> > >> >>
> > >> >> Is this really the right solution?  Maybe it would be better to allow
> > >> >> the controller to go into D3 provided no wakeup signal is needed.  You
> > >> >> could do:
> > >> >>
> > >> >>                 device_set_wakeup_capable(&pdev->dev, 0);
> > >> >
> > >> > This doesn't work.
> > >> > After applying this function, still nothing happens when devices get plugged in.
> > >> > IIUC this function disable the wakeup function, but what I want to do
> > >> > here is to have PME signal works even when runtime PM is enabled.
> > >
> > > This may indicate a bug in either the PCI or USB stacks (or both!).  If
> > > a driver requires wakeup capability from runtime suspend but the device
> > > does not provide it, the PCI core should not allow the device to go
> > > into runtime suspend.  Or is that the driver's responsibility?
> > >
> > >> > I also saw some legacy PCI PM stuff, so I also tried:
> > >> > device_set_wakeup_capable(&pdev->dev, 1);
> > >> > ...doesn't work either.
> > >> >
> > >> >>
> > >> >> Another alternative is to put the controller into D2 instead of D3, but
> > >> >> (1) I don't know how to do that, and (2) we don't know if wakeup
> > >> >> signalling works any better in D2 than it does in D3.
> > >> >
> > >> > I'll try if D2 works.
> > >>
> > >> Put the device into D2 instead of D3 can make the wakeup signaling
> > >> work, i.e. USB devices can be correctly detected after plugged into
> > >> EHCI port.
> > >>
> > >> Do you think this alternative an acceptable workaround?
> > >
> > > Yes, it is.  The difficulty is that I don't know how to tell the PCI
> > > core that the device should go in D2 during runtime suspend instead of
> > > D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.

> The lspci output [1] shows:
> 
>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI Controller (rev 39) (prog-if 20 [EHCI])
>     Capabilities: [c0] Power Management version 2
>       Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
>       Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>       Bridge: PM- B3+
> 
> The device claims it can assert PME# from D3hot.  If it can't, that
> sounds like a hardware defect that should be addressed with a quirk.
> Ideally we would also have a pointer to the AMD hardware erratum.
> 
> Is the following path involved here?
> 
>   pci_finish_runtime_suspend
>     target_state = pci_target_state()
>       if (device_may_wakup())
> 	if (dev->pme_support)
> 	  ...
>     pci_set_power_state(..., target_state)
> 
> If so, I would naively expect that a quirk could clear the
> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
> and pci_target_state() would then avoid selecting D3 or D3cold.  But
> I'm not an expert in power management.

That's a good idea.  However, we should apply the quirk only when it is 
needed.  Which means we need to know the numeric values for the PCI 
IDs.  Also, this will help searching for published errata.

Kai-Heng, what does "lspci -nvs 00:12.0" show?

Alan Stern
Kai-Heng Feng June 15, 2017, 6:57 a.m. UTC | #3
On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> The lspci output [1] shows:
>
>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI Controller (rev 39) (prog-if 20 [EHCI])
>     Capabilities: [c0] Power Management version 2
>       Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
>       Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>       Bridge: PM- B3+
>
> The device claims it can assert PME# from D3hot.  If it can't, that
> sounds like a hardware defect that should be addressed with a quirk.
> Ideally we would also have a pointer to the AMD hardware erratum.

Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
with Low Speed Devices" in [1].
It points to a workaround in appendix A2 from [2].
However it says this bug only effects Low Speed devices, but it
actually also happens on High Speed devices.

[1] https://support.amd.com/TechDocs/46837.pdf
[2] https://support.amd.com/TechDocs/42413.pdf

>
> Is the following path involved here?
>
>   pci_finish_runtime_suspend
>     target_state = pci_target_state()
>       if (device_may_wakup())
>         if (dev->pme_support)
>           ...
>     pci_set_power_state(..., target_state)

Yes, it's involved.

>
> If so, I would naively expect that a quirk could clear the
> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
> and pci_target_state() would then avoid selecting D3 or D3cold.  But
> I'm not an expert in power management.

Clearing those two bits does the trick, thanks for the tip.

>
> Bjorn
>
> [1] http://marc.info/?l=linux-usb&m=149570231732519&w=2
Alan Stern June 15, 2017, 2:12 p.m. UTC | #4
On Thu, 15 Jun 2017, Kai-Heng Feng wrote:

> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > The lspci output [1] shows:
> >
> >   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI Controller (rev 39) (prog-if 20 [EHCI])
> >     Capabilities: [c0] Power Management version 2
> >       Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
> >       Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
> >       Bridge: PM- B3+
> >
> > The device claims it can assert PME# from D3hot.  If it can't, that
> > sounds like a hardware defect that should be addressed with a quirk.
> > Ideally we would also have a pointer to the AMD hardware erratum.
> 
> Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
> with Low Speed Devices" in [1].
> It points to a workaround in appendix A2 from [2].
> However it says this bug only effects Low Speed devices, but it
> actually also happens on High Speed devices.
> 
> [1] https://support.amd.com/TechDocs/46837.pdf
> [2] https://support.amd.com/TechDocs/42413.pdf

Those documents refer to a hardware bug with a workaround in the BIOS.  
Have you checked to see if your BIOS is up to date?

Alan Stern
Kai-Heng Feng June 16, 2017, 3:07 a.m. UTC | #5
On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 15 Jun 2017, Kai-Heng Feng wrote:
>
>> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >
>> > The lspci output [1] shows:
>> >
>> >   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI Controller (rev 39) (prog-if 20 [EHCI])
>> >     Capabilities: [c0] Power Management version 2
>> >       Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
>> >       Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>> >       Bridge: PM- B3+
>> >
>> > The device claims it can assert PME# from D3hot.  If it can't, that
>> > sounds like a hardware defect that should be addressed with a quirk.
>> > Ideally we would also have a pointer to the AMD hardware erratum.
>>
>> Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
>> with Low Speed Devices" in [1].
>> It points to a workaround in appendix A2 from [2].
>> However it says this bug only effects Low Speed devices, but it
>> actually also happens on High Speed devices.
>>
>> [1] https://support.amd.com/TechDocs/46837.pdf
>> [2] https://support.amd.com/TechDocs/42413.pdf
>
> Those documents refer to a hardware bug with a workaround in the BIOS.
> Have you checked to see if your BIOS is up to date?

Yes, it's up to date.

>
> Alan Stern
>
Kai-Heng Feng June 16, 2017, 4:18 p.m. UTC | #6
On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> Those documents refer to a hardware bug with a workaround in the BIOS.
>> Have you checked to see if your BIOS is up to date?
>
> Yes, it's up to date.
>

Alan, I re-sent a patch but I forgot to add you to CC list:
http://marc.info/?l=linux-pci&m=149760607914628&w=2
Alan Stern June 16, 2017, 5:30 p.m. UTC | #7
On Sat, 17 Jun 2017, Kai-Heng Feng wrote:

> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> Those documents refer to a hardware bug with a workaround in the BIOS.
> >> Have you checked to see if your BIOS is up to date?
> >
> > Yes, it's up to date.
> >
> 
> Alan, I re-sent a patch but I forgot to add you to CC list:
> http://marc.info/?l=linux-pci&m=149760607914628&w=2

Thanks for letting me know.  The patch seems reasonable.

Have you tested it with system suspend?  That is, if you suspend the 
whole computer, does plugging or unplugging a USB device cause the 
system to wake up?

Alan Stern
Kai-Heng Feng June 19, 2017, 3:30 a.m. UTC | #8
On Sat, Jun 17, 2017 at 1:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
>
>> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
>> <kai.heng.feng@canonical.com> wrote:
>> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> Those documents refer to a hardware bug with a workaround in the BIOS.
>> >> Have you checked to see if your BIOS is up to date?
>> >
>> > Yes, it's up to date.
>> >
>>
>> Alan, I re-sent a patch but I forgot to add you to CC list:
>> http://marc.info/?l=linux-pci&m=149760607914628&w=2
>
> Thanks for letting me know.  The patch seems reasonable.
>
> Have you tested it with system suspend?  That is, if you suspend the
> whole computer, does plugging or unplugging a USB device cause the
> system to wake up?

No, the system will not wake up when plugging or unplugging.
Tried several times, nether runtime PM enabled nor runtime PM disabled
will wake up the system under S3, when (un)plugging USB devices.

>
> Alan Stern
>
Bjorn Helgaas June 19, 2017, 5:45 p.m. UTC | #9
On Mon, Jun 19, 2017 at 11:30:18AM +0800, Kai-Heng Feng wrote:
> On Sat, Jun 17, 2017 at 1:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
> >
> >> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
> >> <kai.heng.feng@canonical.com> wrote:
> >> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >> Those documents refer to a hardware bug with a workaround in the BIOS.
> >> >> Have you checked to see if your BIOS is up to date?
> >> >
> >> > Yes, it's up to date.
> >> >
> >>
> >> Alan, I re-sent a patch but I forgot to add you to CC list:
> >> http://marc.info/?l=linux-pci&m=149760607914628&w=2
> >
> > Thanks for letting me know.  The patch seems reasonable.
> >
> > Have you tested it with system suspend?  That is, if you suspend the
> > whole computer, does plugging or unplugging a USB device cause the
> > system to wake up?
> 
> No, the system will not wake up when plugging or unplugging.
> Tried several times, nether runtime PM enabled nor runtime PM disabled
> will wake up the system under S3, when (un)plugging USB devices.

Alan, I don't know what this test means for the patch
(http://marc.info/?l=linux-pci&m=149760607914628&w=2).

pci_target_state() is documented as "return the deepest state from
which the device can generate wake events."  For this device, I guess
that means D2, and the patch should accomplish that.

I don't know what's supposed to happen to this device when the system
is in S3.  I assume that if the system is in S3, most devices are in
D3.  If this device is in D3, we won't get PMEs, which I guess is what
Kai-Heng is seeing.  Is that the desired behavior?  Or do we want the
PMEs enough that we should leave the device in D2 (if that's even
possible)?

Bjorn
Alan Stern June 19, 2017, 6:32 p.m. UTC | #10
On Mon, 19 Jun 2017, Bjorn Helgaas wrote:

> > > Have you tested it with system suspend?  That is, if you suspend the
> > > whole computer, does plugging or unplugging a USB device cause the
> > > system to wake up?
> > 
> > No, the system will not wake up when plugging or unplugging.
> > Tried several times, nether runtime PM enabled nor runtime PM disabled
> > will wake up the system under S3, when (un)plugging USB devices.
> 
> Alan, I don't know what this test means for the patch
> (http://marc.info/?l=linux-pci&m=149760607914628&w=2).
> 
> pci_target_state() is documented as "return the deepest state from
> which the device can generate wake events."  For this device, I guess
> that means D2, and the patch should accomplish that.
> 
> I don't know what's supposed to happen to this device when the system
> is in S3.  I assume that if the system is in S3, most devices are in
> D3.  If this device is in D3, we won't get PMEs, which I guess is what
> Kai-Heng is seeing.  Is that the desired behavior?  Or do we want the
> PMEs enough that we should leave the device in D2 (if that's even
> possible)?

It's possible that the test was invalid.  Kai-Heng did not say whether
/sys/.../power/wakeup was set to "enabled" for both the EHCI controller
and the USB root hub beneath it, before the test was started.  If
either of them was set to "disabled" then we would not expect a plug or
unplug event to wake up the system.

In any case, the controller should be set to the lowest power setting 
that is consistent with the desired wakeup behavior.  If wakeup is set 
to "enabled" then the state should be D2 -- if possible.  That's the 
theory, anyway.  If the system supports putting devices only into D3 
during S3 sleep then there's no choice, but if we do have a choice then 
we should take it.

BTW, I just noticed that pci_target_state() uses device_may_wakeup() to
get the desired wakeup behavior.  That is correct for system sleep, but
it is wrong for runtime PM.  For runtime PM, wakeup should be enabled
whenever the hardware allows it, so the test should be
device_can_wakeup().

This means that pci_target_state() should behave differently depending
on whether it is called from pci_prepare_to_sleep() or from
pci_finish_runtime_suspend().  Probably nobody noticed this before 
because it usually doesn't make any difference.

Alan Stern
Rafael J. Wysocki June 19, 2017, 10 p.m. UTC | #11
On Monday, June 19, 2017 02:32:57 PM Alan Stern wrote:
> On Mon, 19 Jun 2017, Bjorn Helgaas wrote:
> 
> > > > Have you tested it with system suspend?  That is, if you suspend the
> > > > whole computer, does plugging or unplugging a USB device cause the
> > > > system to wake up?
> > > 
> > > No, the system will not wake up when plugging or unplugging.
> > > Tried several times, nether runtime PM enabled nor runtime PM disabled
> > > will wake up the system under S3, when (un)plugging USB devices.
> > 
> > Alan, I don't know what this test means for the patch
> > (http://marc.info/?l=linux-pci&m=149760607914628&w=2).
> > 
> > pci_target_state() is documented as "return the deepest state from
> > which the device can generate wake events."  For this device, I guess
> > that means D2, and the patch should accomplish that.
> > 
> > I don't know what's supposed to happen to this device when the system
> > is in S3.  I assume that if the system is in S3, most devices are in
> > D3.  If this device is in D3, we won't get PMEs, which I guess is what
> > Kai-Heng is seeing.  Is that the desired behavior?  Or do we want the
> > PMEs enough that we should leave the device in D2 (if that's even
> > possible)?
> 
> It's possible that the test was invalid.  Kai-Heng did not say whether
> /sys/.../power/wakeup was set to "enabled" for both the EHCI controller
> and the USB root hub beneath it, before the test was started.  If
> either of them was set to "disabled" then we would not expect a plug or
> unplug event to wake up the system.
> 
> In any case, the controller should be set to the lowest power setting 
> that is consistent with the desired wakeup behavior.  If wakeup is set 
> to "enabled" then the state should be D2 -- if possible.  That's the 
> theory, anyway.  If the system supports putting devices only into D3 
> during S3 sleep then there's no choice, but if we do have a choice then 
> we should take it.
> 
> BTW, I just noticed that pci_target_state() uses device_may_wakeup() to
> get the desired wakeup behavior.  That is correct for system sleep, but
> it is wrong for runtime PM.  For runtime PM, wakeup should be enabled
> whenever the hardware allows it, so the test should be
> device_can_wakeup().
> 
> This means that pci_target_state() should behave differently depending
> on whether it is called from pci_prepare_to_sleep() or from
> pci_finish_runtime_suspend().  Probably nobody noticed this before 
> because it usually doesn't make any difference.

Right, this is a bug.

Let me cut a fix for it.

Thanks,
Rafael
Kai-Heng Feng June 20, 2017, 2:36 a.m. UTC | #12
On Tue, Jun 20, 2017 at 2:32 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> It's possible that the test was invalid.  Kai-Heng did not say whether
> /sys/.../power/wakeup was set to "enabled" for both the EHCI controller
> and the USB root hub beneath it, before the test was started.  If
> either of them was set to "disabled" then we would not expect a plug or
> unplug event to wake up the system.

You are right, it's "disabled" on USB root hub.
Changed it to "enabled", the test result remains the same.

>
> In any case, the controller should be set to the lowest power setting
> that is consistent with the desired wakeup behavior.  If wakeup is set
> to "enabled" then the state should be D2 -- if possible.  That's the
> theory, anyway.  If the system supports putting devices only into D3
> during S3 sleep then there's no choice, but if we do have a choice then
> we should take it.
>
> BTW, I just noticed that pci_target_state() uses device_may_wakeup() to
> get the desired wakeup behavior.  That is correct for system sleep, but
> it is wrong for runtime PM.  For runtime PM, wakeup should be enabled
> whenever the hardware allows it, so the test should be
> device_can_wakeup().
>
> This means that pci_target_state() should behave differently depending
> on whether it is called from pci_prepare_to_sleep() or from
> pci_finish_runtime_suspend().  Probably nobody noticed this before
> because it usually doesn't make any difference.
>
> Alan Stern
>
diff mbox

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..36663688404a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -922,6 +922,9 @@  int pci_set_power_state(struct pci_dev *dev,
pci_power_t state)
        if (dev->current_state == state)
                return 0;

+       if (state > PCI_D2 && (dev->dev_flags & PCI_DEV_FLAGS_MAX_D2))
+               state = PCI_D2;
+
        __pci_start_power_transition(dev, state);

        /* This device is quirked not to be put into D3, so
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..a2c1fe62974a 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -181,6 +181,8 @@  static int ehci_pci_setup(struct usb_hcd *hcd)
                if (pdev->device == 0x7808) {
                        ehci->use_dummy_qh = 1;
                        ehci_info(ehci, "applying AMD
SB700/SB800/Hudson-2/3 EHCI dummy qh workaround\n");
+
+                       pdev->dev_flags |= PCI_DEV_FLAGS_MAX_D2;
                }
                break;
        case PCI_VENDOR_ID_VIA:
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..6f86f2aa8548 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -188,6 +188,7 @@  enum pci_dev_flags {
         * the direct_complete optimization.
         */
        PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
+       PCI_DEV_FLAGS_MAX_D2 = (__force pci_dev_flags_t) (1 << 12),
 };

 enum pci_irq_reroute_variant {