Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Alan Stern]

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.

Alan Stern

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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 {

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Bjorn Helgaas]

[+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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Alan Stern]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Thu, Jun 15, 2017 at 2:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 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?

00:12.0 0c03: 1022:7808 (rev 39) (prog-if 20 [EHCI])
        Subsystem: 1028:0732
        Flags: bus master, 66MHz, medium devsel, latency 32, IRQ 18
        Memory at fe769000 (32-bit, non-prefetchable) [size=256]
        Capabilities: [c0] Power Management version 2
        Capabilities: [e4] Debug port: BAR=1 offset=00e0
        Kernel driver in use: ehci-pci

Here's the diff that can make it work:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1a14ca8965e6..7bd278535ab3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2208,6 +2208,12 @@ void pci_pm_init(struct pci_dev *dev)
        }

        pmc &= PCI_PM_CAP_PME_MASK;
+
+       if (unlikely(dev->vendor == 0x1022 && dev->device == 0x7808))
{
+               dev_info(&dev->dev, "PME# does not work under D3,
disabling it\n");
+               pmc &= ~(PCI_PM_CAP_PME_D3 | PCI_PM_CAP_PME_D3cold);
+       }
+
        if (pmc) {
                dev_printk(KERN_DEBUG, &dev->dev,
                         "PME# supported from%s%s%s%s%s\n",

If you think this is OK, I'll resend the patch.

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Bjorn Helgaas]

On Thu, Jun 15, 2017 at 03:02:25PM +0800, Kai-Heng Feng wrote:
> On Thu, Jun 15, 2017 at 2:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > 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?
> 
> 00:12.0 0c03: 1022:7808 (rev 39) (prog-if 20 [EHCI])
>         Subsystem: 1028:0732
>         Flags: bus master, 66MHz, medium devsel, latency 32, IRQ 18
>         Memory at fe769000 (32-bit, non-prefetchable) [size=256]
>         Capabilities: [c0] Power Management version 2
>         Capabilities: [e4] Debug port: BAR=1 offset=00e0
>         Kernel driver in use: ehci-pci
> 
> Here's the diff that can make it work:
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1a14ca8965e6..7bd278535ab3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2208,6 +2208,12 @@ void pci_pm_init(struct pci_dev *dev)
>         }
> 
>         pmc &= PCI_PM_CAP_PME_MASK;
> +
> +       if (unlikely(dev->vendor == 0x1022 && dev->device == 0x7808))
> {
> +               dev_info(&dev->dev, "PME# does not work under D3,
> disabling it\n");
> +               pmc &= ~(PCI_PM_CAP_PME_D3 | PCI_PM_CAP_PME_D3cold);
> +       }
> +
>         if (pmc) {
>                 dev_printk(KERN_DEBUG, &dev->dev,
>                          "PME# supported from%s%s%s%s%s\n",
> 
> If you think this is OK, I'll resend the patch.

Thanks for checking this out!

My suggestion:

  - Open a bug report at bugzilla.kernel.org, Drivers/PCI component,
    attach "lspci -vv" output for entire system.

  - Reorganize your diff above as a "final" quirk that updates
    dev->pme_support and puts a note in dmesg.  I think this could go
    in arch/x86/pci/fixup.c since I think this is only possible on
    x86.

  - Include the bug URL, errata URL, and "Description" and "Potential
    Effect on System" text in your changelog.

Bjorn

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Alan Stern]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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
>

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Alan Stern]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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
>

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Bjorn Helgaas]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Alan Stern]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Rafael J. Wysocki]

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

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

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
>

[PATCH] PCI / PM: Avoid using device_may_wakeup() for runtime PM

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

pci_target_state() calls device_may_wakeup() which checks whether
or not the device may wake up the system from sleep states, but
pci_target_state() is used for runtime PM too.

Since runtime PM is expected to always enable remote wakeup if
possible, modify pci_target_state() to take additional argument
indicating whether or not it should look for a state from which
the device can signal wakeup and pass "true" to it from the code
related to runtime PM.

Reported-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1982,12 +1982,13 @@ EXPORT_SYMBOL(pci_wake_from_d3);
 /**
  * pci_target_state - find an appropriate low power state for a given PCI dev
  * @dev: PCI device
+ * @wakeup: Whether or not wakeup functionality will be enabled for the device.
  *
  * Use underlying platform code to find a supported low power state for @dev.
  * If the platform can't manage @dev, return the deepest state from which it
  * can generate wake events, based on any available PME info.
  */
-static pci_power_t pci_target_state(struct pci_dev *dev)
+static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 {
 	pci_power_t target_state = PCI_D3hot;
 
@@ -2024,7 +2025,7 @@ static pci_power_t pci_target_state(stru
 	if (dev->current_state == PCI_D3cold)
 		target_state = PCI_D3cold;
 
-	if (device_may_wakeup(&dev->dev)) {
+	if (wakeup) {
 		/*
 		 * Find the deepest state from which the device can generate
 		 * wake-up events, make it the target state and enable device
@@ -2050,13 +2051,14 @@ static pci_power_t pci_target_state(stru
  */
 int pci_prepare_to_sleep(struct pci_dev *dev)
 {
-	pci_power_t target_state = pci_target_state(dev);
+	bool wakeup = device_may_wakeup(&dev->dev);
+	pci_power_t target_state = pci_target_state(dev, wakeup);
 	int error;
 
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
+	pci_enable_wake(dev, target_state, wakeup);
 
 	error = pci_set_power_state(dev, target_state);
 
@@ -2089,7 +2091,7 @@ EXPORT_SYMBOL(pci_back_from_sleep);
  */
 int pci_finish_runtime_suspend(struct pci_dev *dev)
 {
-	pci_power_t target_state = pci_target_state(dev);
+	pci_power_t target_state = pci_target_state(dev, true);
 	int error;
 
 	if (target_state == PCI_POWER_ERROR)
@@ -2128,7 +2130,7 @@ bool pci_dev_run_wake(struct pci_dev *de
 		return false;
 
 	/* PME-capable in principle, but not from the intended sleep state */
-	if (!pci_pme_capable(dev, pci_target_state(dev)))
+	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
 		return false;
 
 	while (bus->parent) {
@@ -2163,9 +2165,10 @@ EXPORT_SYMBOL_GPL(pci_dev_run_wake);
 bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
 {
 	struct device *dev = &pci_dev->dev;
+	bool wakeup = device_may_wakeup(dev);
 
 	if (!pm_runtime_suspended(dev)
-	    || pci_target_state(pci_dev) != pci_dev->current_state
+	    || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
 	    || platform_pci_need_resume(pci_dev)
 	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
 		return false;
@@ -2183,7 +2186,7 @@ bool pci_dev_keep_suspended(struct pci_d
 	spin_lock_irq(&dev->power.lock);
 
 	if (pm_runtime_suspended(dev) && pci_dev->current_state < PCI_D3cold &&
-	    !device_may_wakeup(dev))
+	    !wakeup)
 		__pci_pme_active(pci_dev, false);
 
 	spin_unlock_irq(&dev->power.lock);

[PATCH v2] PCI / PM: Avoid using device_may_wakeup() for runtime PM

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

pci_target_state() calls device_may_wakeup() which checks whether
or not the device may wake up the system from sleep states, but
pci_target_state() is used for runtime PM too.

Since runtime PM is expected to always enable remote wakeup if
possible, modify pci_target_state() to take additional argument
indicating whether or not it should look for a state from which
the device can signal wakeup and pass either the return value
of device_can_wakeup(), or "false" (if the device itself is not
wakeup-capable) to it from the code related to runtime PM.

While at it, fix the comment in pci_dev_run_wake() which is not
about sleep states.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:

Passing "true" as the second argument to pci_target_state() for runtime PM
might trigger suboptimal state choices to be made, so pass the return value
of device_can_wakeup() to it instead and pass "false" to it in pci_dev_run_wake(),
because that assumes device_can_wakeup() to return "false" already.

---
 drivers/pci/pci.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1982,12 +1982,13 @@ EXPORT_SYMBOL(pci_wake_from_d3);
 /**
  * pci_target_state - find an appropriate low power state for a given PCI dev
  * @dev: PCI device
+ * @wakeup: Whether or not wakeup functionality will be enabled for the device.
  *
  * Use underlying platform code to find a supported low power state for @dev.
  * If the platform can't manage @dev, return the deepest state from which it
  * can generate wake events, based on any available PME info.
  */
-static pci_power_t pci_target_state(struct pci_dev *dev)
+static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 {
 	pci_power_t target_state = PCI_D3hot;
 
@@ -2024,7 +2025,7 @@ static pci_power_t pci_target_state(stru
 	if (dev->current_state == PCI_D3cold)
 		target_state = PCI_D3cold;
 
-	if (device_may_wakeup(&dev->dev)) {
+	if (wakeup) {
 		/*
 		 * Find the deepest state from which the device can generate
 		 * wake-up events, make it the target state and enable device
@@ -2050,13 +2051,14 @@ static pci_power_t pci_target_state(stru
  */
 int pci_prepare_to_sleep(struct pci_dev *dev)
 {
-	pci_power_t target_state = pci_target_state(dev);
+	bool wakeup = device_may_wakeup(&dev->dev);
+	pci_power_t target_state = pci_target_state(dev, wakeup);
 	int error;
 
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
+	pci_enable_wake(dev, target_state, wakeup);
 
 	error = pci_set_power_state(dev, target_state);
 
@@ -2089,9 +2091,10 @@ EXPORT_SYMBOL(pci_back_from_sleep);
  */
 int pci_finish_runtime_suspend(struct pci_dev *dev)
 {
-	pci_power_t target_state = pci_target_state(dev);
+	pci_power_t target_state;
 	int error;
 
+	target_state = pci_target_state(dev, device_can_wakeup(&dev->dev));
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
@@ -2127,8 +2130,8 @@ bool pci_dev_run_wake(struct pci_dev *de
 	if (!dev->pme_support)
 		return false;
 
-	/* PME-capable in principle, but not from the intended sleep state */
-	if (!pci_pme_capable(dev, pci_target_state(dev)))
+	/* PME-capable in principle, but not from the target power state */
+	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
 		return false;
 
 	while (bus->parent) {
@@ -2163,9 +2166,10 @@ EXPORT_SYMBOL_GPL(pci_dev_run_wake);
 bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
 {
 	struct device *dev = &pci_dev->dev;
+	bool wakeup = device_may_wakeup(dev);
 
 	if (!pm_runtime_suspended(dev)
-	    || pci_target_state(pci_dev) != pci_dev->current_state
+	    || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
 	    || platform_pci_need_resume(pci_dev)
 	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
 		return false;
@@ -2183,7 +2187,7 @@ bool pci_dev_keep_suspended(struct pci_d
 	spin_lock_irq(&dev->power.lock);
 
 	if (pm_runtime_suspended(dev) && pci_dev->current_state < PCI_D3cold &&
-	    !device_may_wakeup(dev))
+	    !wakeup)
 		__pci_pme_active(pci_dev, false);
 
 	spin_unlock_irq(&dev->power.lock);

Re: [PATCH v2] PCI / PM: Avoid using device_may_wakeup() for runtime PM

[Rafael J. Wysocki]

On Friday, June 23, 2017 02:58:11 PM Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> pci_target_state() calls device_may_wakeup() which checks whether
> or not the device may wake up the system from sleep states, but
> pci_target_state() is used for runtime PM too.
> 
> Since runtime PM is expected to always enable remote wakeup if
> possible, modify pci_target_state() to take additional argument
> indicating whether or not it should look for a state from which
> the device can signal wakeup and pass either the return value
> of device_can_wakeup(), or "false" (if the device itself is not
> wakeup-capable) to it from the code related to runtime PM.
> 
> While at it, fix the comment in pci_dev_run_wake() which is not
> about sleep states.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
> 
> Passing "true" as the second argument to pci_target_state() for runtime PM
> might trigger suboptimal state choices to be made, so pass the return value
> of device_can_wakeup() to it instead and pass "false" to it in pci_dev_run_wake(),
> because that assumes device_can_wakeup() to return "false" already.

This was sent a week ago without any response so far.

Any concerns?

> ---
>  drivers/pci/pci.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -1982,12 +1982,13 @@ EXPORT_SYMBOL(pci_wake_from_d3);
>  /**
>   * pci_target_state - find an appropriate low power state for a given PCI dev
>   * @dev: PCI device
> + * @wakeup: Whether or not wakeup functionality will be enabled for the device.
>   *
>   * Use underlying platform code to find a supported low power state for @dev.
>   * If the platform can't manage @dev, return the deepest state from which it
>   * can generate wake events, based on any available PME info.
>   */
> -static pci_power_t pci_target_state(struct pci_dev *dev)
> +static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>  {
>  	pci_power_t target_state = PCI_D3hot;
>  
> @@ -2024,7 +2025,7 @@ static pci_power_t pci_target_state(stru
>  	if (dev->current_state == PCI_D3cold)
>  		target_state = PCI_D3cold;
>  
> -	if (device_may_wakeup(&dev->dev)) {
> +	if (wakeup) {
>  		/*
>  		 * Find the deepest state from which the device can generate
>  		 * wake-up events, make it the target state and enable device
> @@ -2050,13 +2051,14 @@ static pci_power_t pci_target_state(stru
>   */
>  int pci_prepare_to_sleep(struct pci_dev *dev)
>  {
> -	pci_power_t target_state = pci_target_state(dev);
> +	bool wakeup = device_may_wakeup(&dev->dev);
> +	pci_power_t target_state = pci_target_state(dev, wakeup);
>  	int error;
>  
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> -	pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
> +	pci_enable_wake(dev, target_state, wakeup);
>  
>  	error = pci_set_power_state(dev, target_state);
>  
> @@ -2089,9 +2091,10 @@ EXPORT_SYMBOL(pci_back_from_sleep);
>   */
>  int pci_finish_runtime_suspend(struct pci_dev *dev)
>  {
> -	pci_power_t target_state = pci_target_state(dev);
> +	pci_power_t target_state;
>  	int error;
>  
> +	target_state = pci_target_state(dev, device_can_wakeup(&dev->dev));
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> @@ -2127,8 +2130,8 @@ bool pci_dev_run_wake(struct pci_dev *de
>  	if (!dev->pme_support)
>  		return false;
>  
> -	/* PME-capable in principle, but not from the intended sleep state */
> -	if (!pci_pme_capable(dev, pci_target_state(dev)))
> +	/* PME-capable in principle, but not from the target power state */
> +	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
>  		return false;
>  
>  	while (bus->parent) {
> @@ -2163,9 +2166,10 @@ EXPORT_SYMBOL_GPL(pci_dev_run_wake);
>  bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  {
>  	struct device *dev = &pci_dev->dev;
> +	bool wakeup = device_may_wakeup(dev);
>  
>  	if (!pm_runtime_suspended(dev)
> -	    || pci_target_state(pci_dev) != pci_dev->current_state
> +	    || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
>  	    || platform_pci_need_resume(pci_dev)
>  	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
>  		return false;
> @@ -2183,7 +2187,7 @@ bool pci_dev_keep_suspended(struct pci_d
>  	spin_lock_irq(&dev->power.lock);
>  
>  	if (pm_runtime_suspended(dev) && pci_dev->current_state < PCI_D3cold &&
> -	    !device_may_wakeup(dev))
> +	    !wakeup)
>  		__pci_pme_active(pci_dev, false);
>  
>  	spin_unlock_irq(&dev->power.lock);
> 

Re: [PATCH v2] PCI / PM: Avoid using device_may_wakeup() for runtime PM

[Mika Westerberg]

On Fri, Jun 30, 2017 at 12:37:00AM +0200, Rafael J. Wysocki wrote:
> On Friday, June 23, 2017 02:58:11 PM Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > pci_target_state() calls device_may_wakeup() which checks whether
> > or not the device may wake up the system from sleep states, but
> > pci_target_state() is used for runtime PM too.
> > 
> > Since runtime PM is expected to always enable remote wakeup if
> > possible, modify pci_target_state() to take additional argument
> > indicating whether or not it should look for a state from which
> > the device can signal wakeup and pass either the return value
> > of device_can_wakeup(), or "false" (if the device itself is not
> > wakeup-capable) to it from the code related to runtime PM.
> > 
> > While at it, fix the comment in pci_dev_run_wake() which is not
> > about sleep states.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > 
> > -> v2:
> > 
> > Passing "true" as the second argument to pci_target_state() for runtime PM
> > might trigger suboptimal state choices to be made, so pass the return value
> > of device_can_wakeup() to it instead and pass "false" to it in pci_dev_run_wake(),
> > because that assumes device_can_wakeup() to return "false" already.
> 
> This was sent a week ago without any response so far.
> 
> Any concerns?

No concerns from me.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2] PCI / PM: Avoid using device_may_wakeup() for runtime PM

[Bjorn Helgaas]

On Fri, Jun 23, 2017 at 02:58:11PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> pci_target_state() calls device_may_wakeup() which checks whether
> or not the device may wake up the system from sleep states, but
> pci_target_state() is used for runtime PM too.
> 
> Since runtime PM is expected to always enable remote wakeup if
> possible, modify pci_target_state() to take additional argument
> indicating whether or not it should look for a state from which
> the device can signal wakeup and pass either the return value
> of device_can_wakeup(), or "false" (if the device itself is not
> wakeup-capable) to it from the code related to runtime PM.
> 
> While at it, fix the comment in pci_dev_run_wake() which is not
> about sleep states.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Applied with Mika's reviewed-by to pci/pm for v4.13, thanks!

> ---
> 
> -> v2:
> 
> Passing "true" as the second argument to pci_target_state() for runtime PM
> might trigger suboptimal state choices to be made, so pass the return value
> of device_can_wakeup() to it instead and pass "false" to it in pci_dev_run_wake(),
> because that assumes device_can_wakeup() to return "false" already.
> 
> ---
>  drivers/pci/pci.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -1982,12 +1982,13 @@ EXPORT_SYMBOL(pci_wake_from_d3);
>  /**
>   * pci_target_state - find an appropriate low power state for a given PCI dev
>   * @dev: PCI device
> + * @wakeup: Whether or not wakeup functionality will be enabled for the device.
>   *
>   * Use underlying platform code to find a supported low power state for @dev.
>   * If the platform can't manage @dev, return the deepest state from which it
>   * can generate wake events, based on any available PME info.
>   */
> -static pci_power_t pci_target_state(struct pci_dev *dev)
> +static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>  {
>  	pci_power_t target_state = PCI_D3hot;
>  
> @@ -2024,7 +2025,7 @@ static pci_power_t pci_target_state(stru
>  	if (dev->current_state == PCI_D3cold)
>  		target_state = PCI_D3cold;
>  
> -	if (device_may_wakeup(&dev->dev)) {
> +	if (wakeup) {
>  		/*
>  		 * Find the deepest state from which the device can generate
>  		 * wake-up events, make it the target state and enable device
> @@ -2050,13 +2051,14 @@ static pci_power_t pci_target_state(stru
>   */
>  int pci_prepare_to_sleep(struct pci_dev *dev)
>  {
> -	pci_power_t target_state = pci_target_state(dev);
> +	bool wakeup = device_may_wakeup(&dev->dev);
> +	pci_power_t target_state = pci_target_state(dev, wakeup);
>  	int error;
>  
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> -	pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
> +	pci_enable_wake(dev, target_state, wakeup);
>  
>  	error = pci_set_power_state(dev, target_state);
>  
> @@ -2089,9 +2091,10 @@ EXPORT_SYMBOL(pci_back_from_sleep);
>   */
>  int pci_finish_runtime_suspend(struct pci_dev *dev)
>  {
> -	pci_power_t target_state = pci_target_state(dev);
> +	pci_power_t target_state;
>  	int error;
>  
> +	target_state = pci_target_state(dev, device_can_wakeup(&dev->dev));
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> @@ -2127,8 +2130,8 @@ bool pci_dev_run_wake(struct pci_dev *de
>  	if (!dev->pme_support)
>  		return false;
>  
> -	/* PME-capable in principle, but not from the intended sleep state */
> -	if (!pci_pme_capable(dev, pci_target_state(dev)))
> +	/* PME-capable in principle, but not from the target power state */
> +	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
>  		return false;
>  
>  	while (bus->parent) {
> @@ -2163,9 +2166,10 @@ EXPORT_SYMBOL_GPL(pci_dev_run_wake);
>  bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  {
>  	struct device *dev = &pci_dev->dev;
> +	bool wakeup = device_may_wakeup(dev);
>  
>  	if (!pm_runtime_suspended(dev)
> -	    || pci_target_state(pci_dev) != pci_dev->current_state
> +	    || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
>  	    || platform_pci_need_resume(pci_dev)
>  	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
>  		return false;
> @@ -2183,7 +2187,7 @@ bool pci_dev_keep_suspended(struct pci_d
>  	spin_lock_irq(&dev->power.lock);
>  
>  	if (pm_runtime_suspended(dev) && pci_dev->current_state < PCI_D3cold &&
> -	    !device_may_wakeup(dev))
> +	    !wakeup)
>  		__pci_pme_active(pci_dev, false);
>  
>  	spin_unlock_irq(&dev->power.lock);
>