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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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 <[email protected]>
>> ---
>> 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
>
On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
<[email protected]> wrote:
> On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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.
>
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
> <[email protected]> wrote:
> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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
On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <[email protected]> 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
>> <[email protected]> wrote:
>> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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 {
[+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 <[email protected]> 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
> >> <[email protected]> wrote:
> >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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
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 <[email protected]> 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
> > >> <[email protected]> wrote:
> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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
On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <[email protected]> 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
On Thu, Jun 15, 2017 at 2:55 AM, Alan Stern <[email protected]> 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 <[email protected]> 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
>> > >> <[email protected]> wrote:
>> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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.
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 <[email protected]> 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 <[email protected]> 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
> >> > >> <[email protected]> wrote:
> >> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <[email protected]> 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
On Thu, 15 Jun 2017, Kai-Heng Feng wrote:
> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <[email protected]> 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
On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <[email protected]> wrote:
> On Thu, 15 Jun 2017, Kai-Heng Feng wrote:
>
>> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <[email protected]> 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
>
On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
<[email protected]> wrote:
> On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <[email protected]> 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
On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
> <[email protected]> wrote:
> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <[email protected]> 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
On Sat, Jun 17, 2017 at 1:30 AM, Alan Stern <[email protected]> wrote:
> On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
>
>> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
>> <[email protected]> wrote:
>> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <[email protected]> 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
>
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 <[email protected]> wrote:
> > On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
> >
> >> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
> >> <[email protected]> wrote:
> >> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <[email protected]> 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
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
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
On Tue, Jun 20, 2017 at 2:32 AM, Alan Stern <[email protected]> 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
>
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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);
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
-> 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);
On Friday, June 23, 2017 02:58:11 PM Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
>
> -> 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);
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > -> 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 <[email protected]>
On Fri, Jun 23, 2017 at 02:58:11PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
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);
>