Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662016 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-09 09:30 +0200
    Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-09 16:50 +0200
      Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-12 09:10 +0200
        Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-12 12:20 +0200
          Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-12 16:20 +0200
            Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-13 06:30 +0200
              Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI  controller Bjorn Helgaas <helgaas@kernel.org> - 2017-06-13 19:30 +0200
                Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-14 21:00 +0200
                  Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-15 09:10 +0200
                    Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI  controller Bjorn Helgaas <helgaas@kernel.org> - 2017-06-15 15:30 +0200
                Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-15 09:00 +0200
                  Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-15 16:20 +0200
                    Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-16 05:10 +0200
                      Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-16 18:20 +0200
                        Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-16 19:40 +0200
                          Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Kai-Heng Feng <kai.heng.feng@canonical.com> - 2017-06-19 05:40 +0200
                            Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI  controller Bjorn Helgaas <helgaas@kernel.org> - 2017-06-19 19:50 +0200
                              Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller Alan Stern <stern@rowland.harvard.edu> - 2017-06-19 20:40 +0200
                                Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-20 00:10 +0200

#1662016 — [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

As Alan Stern points out [1], the PME signal is not enabled when
controller is in D3, therefore it's not being woken up when new deivces
get plugged in.

Workaround this bug by preventing the controller enters D3 power state.

[1] https://www.spinics.net/lists/linux-usb/msg157462.html

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/host/ehci-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..616685f83954 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_NO_D3;
 		}
 		break;
 	case PCI_VENDOR_ID_VIA:
-- 
2.13.0

[toc] | [next] | [standalone]

#1662532

On Fri, 9 Jun 2017, Kai-Heng Feng wrote:

> As Alan Stern points out [1], the PME signal is not enabled when
> controller is in D3, therefore it's not being woken up when new deivces
> get plugged in.
> 
> Workaround this bug by preventing the controller enters D3 power state.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg157462.html
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/usb/host/ehci-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 93326974ff4b..616685f83954 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_NO_D3;
>  		}
>  		break;
>  	case PCI_VENDOR_ID_VIA:

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);

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.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1663276

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:
>
>> As Alan Stern points out [1], the PME signal is not enabled when
>> controller is in D3, therefore it's not being woken up when new deivces
>> get plugged in.
>>
>> Workaround this bug by preventing the controller enters D3 power state.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg157462.html
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>>  drivers/usb/host/ehci-pci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index 93326974ff4b..616685f83954 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_NO_D3;
>>               }
>>               break;
>>       case PCI_VENDOR_ID_VIA:
>
> 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.

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.

Thanks for the review.

>
> Alan Stern
>

[toc] | [prev] | [next] | [standalone]

#1663398

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.
>
> 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?

>
> Thanks for the review.
>

[toc] | [prev] | [next] | [standalone]

#1663538

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

[toc] | [prev] | [next] | [standalone]

#1664390

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 {

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1666153

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

[toc] | [prev] | [next] | [standalone]

#1666502

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1666500

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

[toc] | [prev] | [next] | [standalone]

#1666753

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

[toc] | [prev] | [next] | [standalone]

#1667393

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
>

[toc] | [prev] | [next] | [standalone]

#1667865

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

[toc] | [prev] | [next] | [standalone]

#1667915

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

[toc] | [prev] | [next] | [standalone]

#1668748

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
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1669632

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

[toc] | [prev] | [next] | [standalone]

#1669994

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web