[PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Kai-Heng Feng]

USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
core: Drop run_wake flag from struct dev_pm_info").

The device in question is not power managed by platform firmware,
furthermore, it only supports PME# from D3cold:
Capabilities: [78] Power Management version 3
       Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
       Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-

Before commit de3ef1eb1cd0, the device never gets runtime suspended.
After that commit, the device gets runtime suspended, so it does not
respond to any PME#.

usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
device_can_wakeup() in pci_dev_run_wake() always returns true.

So pci_dev_run_wake() needs to check PME wakeup capability as its first
condition.

In addition, change wakeup flag passed to pci_target_state() from false
to true, because we want to find the deepest state that the device can
still generate PME#.

Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct dev_pm_info")
Cc: stable@vger.kernel.org # 4.13+
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v3: State the reason why the wakeup flag gets changed.

v2: Explicitly check dev->pme_support.

 drivers/pci/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..52821a21fc07 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2125,16 +2125,16 @@ bool pci_dev_run_wake(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;
 
-	if (device_can_wakeup(&dev->dev))
-		return true;
-
 	if (!dev->pme_support)
 		return false;
 
 	/* PME-capable in principle, but not from the target power state */
-	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
+	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
 		return false;
 
+	if (device_can_wakeup(&dev->dev))
+		return true;
+
 	while (bus->parent) {
 		struct pci_dev *bridge = bus->self;
 
-- 
2.15.1

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Kai Heng Feng]

Hi Bjorn and Rafael,

> On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>  
> wrote:
>
> USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> core: Drop run_wake flag from struct dev_pm_info").
>
> The device in question is not power managed by platform firmware,
> furthermore, it only supports PME# from D3cold:
> Capabilities: [78] Power Management version 3
>        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
>        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
>
> Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> After that commit, the device gets runtime suspended, so it does not
> respond to any PME#.
>
> usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
> device_can_wakeup() in pci_dev_run_wake() always returns true.
>
> So pci_dev_run_wake() needs to check PME wakeup capability as its first
> condition.
>
> In addition, change wakeup flag passed to pci_target_state() from false
> to true, because we want to find the deepest state that the device can
> still generate PME#.
>
> Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct  
> dev_pm_info")
> Cc: stable@vger.kernel.org # 4.13+
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v3: State the reason why the wakeup flag gets changed.
>
> v2: Explicitly check dev->pme_support.

If this patch is good enough, I am hoping it can get merged in v4.17.

Kai-Heng

>
>  drivers/pci/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..52821a21fc07 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2125,16 +2125,16 @@ bool pci_dev_run_wake(struct pci_dev *dev)
>  {
>  	struct pci_bus *bus = dev->bus;
>
> -	if (device_can_wakeup(&dev->dev))
> -		return true;
> -
>  	if (!dev->pme_support)
>  		return false;
>
>  	/* PME-capable in principle, but not from the target power state */
> -	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
> +	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
>  		return false;
>
> +	if (device_can_wakeup(&dev->dev))
> +		return true;
> +
>  	while (bus->parent) {
>  		struct pci_dev *bridge = bus->self;
>
> -- 
> 2.15.1

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Rafael J. Wysocki]

On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
> Hi Bjorn and Rafael,
> 
> > On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>  
> > wrote:
> >
> > USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> > core: Drop run_wake flag from struct dev_pm_info").
> >
> > The device in question is not power managed by platform firmware,
> > furthermore, it only supports PME# from D3cold:
> > Capabilities: [78] Power Management version 3
> >        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> >        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> >
> > Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> > After that commit, the device gets runtime suspended, so it does not
> > respond to any PME#.
> >
> > usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
> > device_can_wakeup() in pci_dev_run_wake() always returns true.
> >
> > So pci_dev_run_wake() needs to check PME wakeup capability as its first
> > condition.
> >
> > In addition, change wakeup flag passed to pci_target_state() from false
> > to true, because we want to find the deepest state that the device can
> > still generate PME#.
> >
> > Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct  
> > dev_pm_info")
> > Cc: stable@vger.kernel.org # 4.13+
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v3: State the reason why the wakeup flag gets changed.
> >
> > v2: Explicitly check dev->pme_support.
> 
> If this patch is good enough, I am hoping it can get merged in v4.17.

OK

Bjorn, if you want to take this:

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

Otherwise please let me know and I'll queue it up.


> >
> >  drivers/pci/pci.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index f6a4dd10d9b0..52821a21fc07 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2125,16 +2125,16 @@ bool pci_dev_run_wake(struct pci_dev *dev)
> >  {
> >  	struct pci_bus *bus = dev->bus;
> >
> > -	if (device_can_wakeup(&dev->dev))
> > -		return true;
> > -
> >  	if (!dev->pme_support)
> >  		return false;
> >
> >  	/* PME-capable in principle, but not from the target power state */
> > -	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
> > +	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
> >  		return false;
> >
> > +	if (device_can_wakeup(&dev->dev))
> > +		return true;
> > +
> >  	while (bus->parent) {
> >  		struct pci_dev *bridge = bus->self;
> >
> 

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Kai Heng Feng]

> On Apr 13, 2018, at 3:29 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
>> Hi Bjorn and Rafael,
>>
>>> On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> wrote:
>>>
>>> USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
>>> core: Drop run_wake flag from struct dev_pm_info").
>>>
>>> The device in question is not power managed by platform firmware,
>>> furthermore, it only supports PME# from D3cold:
>>> Capabilities: [78] Power Management version 3
>>>        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
>>>        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
>>>
>>> Before commit de3ef1eb1cd0, the device never gets runtime suspended.
>>> After that commit, the device gets runtime suspended, so it does not
>>> respond to any PME#.
>>>
>>> usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
>>> device_can_wakeup() in pci_dev_run_wake() always returns true.
>>>
>>> So pci_dev_run_wake() needs to check PME wakeup capability as its first
>>> condition.
>>>
>>> In addition, change wakeup flag passed to pci_target_state() from false
>>> to true, because we want to find the deepest state that the device can
>>> still generate PME#.
>>>
>>> Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct
>>> dev_pm_info")
>>> Cc: stable@vger.kernel.org # 4.13+
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>> v3: State the reason why the wakeup flag gets changed.
>>>
>>> v2: Explicitly check dev->pme_support.
>>
>> If this patch is good enough, I am hoping it can get merged in v4.17.
>
> OK
>
> Bjorn, if you want to take this:
>
>  Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Otherwise please let me know and I'll queue it up.

Hi, Rafael,
Can this patch be merged into your tree?

Kai-Heng

>
>
>>>  drivers/pci/pci.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index f6a4dd10d9b0..52821a21fc07 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -2125,16 +2125,16 @@ bool pci_dev_run_wake(struct pci_dev *dev)
>>>  {
>>>  	struct pci_bus *bus = dev->bus;
>>>
>>> -	if (device_can_wakeup(&dev->dev))
>>> -		return true;
>>> -
>>>  	if (!dev->pme_support)
>>>  		return false;
>>>
>>>  	/* PME-capable in principle, but not from the target power state */
>>> -	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
>>> +	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
>>>  		return false;
>>>
>>> +	if (device_can_wakeup(&dev->dev))
>>> +		return true;
>>> +
>>>  	while (bus->parent) {
>>>  		struct pci_dev *bridge = bus->self;

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Rafael J. Wysocki]

On Thursday, April 26, 2018 8:54:14 AM CEST Kai Heng Feng wrote:
> 
> > On Apr 13, 2018, at 3:29 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
> >> Hi Bjorn and Rafael,
> >>
> >>> On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>> wrote:
> >>>
> >>> USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> >>> core: Drop run_wake flag from struct dev_pm_info").
> >>>
> >>> The device in question is not power managed by platform firmware,
> >>> furthermore, it only supports PME# from D3cold:
> >>> Capabilities: [78] Power Management version 3
> >>>        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> >>>        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> >>>
> >>> Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> >>> After that commit, the device gets runtime suspended, so it does not
> >>> respond to any PME#.
> >>>
> >>> usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
> >>> device_can_wakeup() in pci_dev_run_wake() always returns true.
> >>>
> >>> So pci_dev_run_wake() needs to check PME wakeup capability as its first
> >>> condition.
> >>>
> >>> In addition, change wakeup flag passed to pci_target_state() from false
> >>> to true, because we want to find the deepest state that the device can
> >>> still generate PME#.
> >>>
> >>> Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct
> >>> dev_pm_info")
> >>> Cc: stable@vger.kernel.org # 4.13+
> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>> ---
> >>> v3: State the reason why the wakeup flag gets changed.
> >>>
> >>> v2: Explicitly check dev->pme_support.
> >>
> >> If this patch is good enough, I am hoping it can get merged in v4.17.
> >
> > OK
> >
> > Bjorn, if you want to take this:
> >
> >  Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Otherwise please let me know and I'll queue it up.
> 
> Hi, Rafael,
> Can this patch be merged into your tree?

If Bjorn tells me to do that, then yes, it can.

Thanks,
Rafael

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Bjorn Helgaas]

On Fri, Apr 13, 2018 at 09:29:56AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
> > Hi Bjorn and Rafael,
> > 
> > > On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>  
> > > wrote:
> > >
> > > USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> > > core: Drop run_wake flag from struct dev_pm_info").
> > >
> > > The device in question is not power managed by platform firmware,
> > > furthermore, it only supports PME# from D3cold:
> > > Capabilities: [78] Power Management version 3
> > >        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> > >        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> > >
> > > Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> > > After that commit, the device gets runtime suspended, so it does not
> > > respond to any PME#.

Apologies for my lack of PM expertise.  I don't think the device would
*respond* to PME#, would it?  I would think the device would
potentially *generate* a PME#.

And I guess since this device can generate PME# only from D3cold, the
implication is that runtime suspending the device may put it into D1,
D2, or D3hot, but not D3cold?  Is that an axiom of the runtime suspend
design?

> > > usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
> > > device_can_wakeup() in pci_dev_run_wake() always returns true.

I think "mandatorily" means "always" or "unconditionally", right?

> > > So pci_dev_run_wake() needs to check PME wakeup capability as its first
> > > condition.
> > >
> > > In addition, change wakeup flag passed to pci_target_state() from false
> > > to true, because we want to find the deepest state that the device can
> > > still generate PME#.

Is this a separate bug fix?  I don't understand how it fits in here
because the wakeup flag means "Whether or not wakeup functionality
will be enabled for the device", and you're not changing anything
about whether wakeup functionality will be enabled.

> > > Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct  
> > > dev_pm_info")
> > > Cc: stable@vger.kernel.org # 4.13+
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > ---
> > > v3: State the reason why the wakeup flag gets changed.
> > >
> > > v2: Explicitly check dev->pme_support.
> > 
> > If this patch is good enough, I am hoping it can get merged in v4.17.
> 
> OK
> 
> Bjorn, if you want to take this:
> 
>  Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Otherwise please let me know and I'll queue it up.

de3ef1eb1cd0 went through your tree, so I think this fix should go
through your tree, too.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Not directly related to this patch, but I think these comments in
pci_target_state() are slightly misleading:

   * Call the platform to choose the target state of the device
   * and enable wake-up from this state if supported.

   * Find the deepest state from which the device can generate
   * wake-up events, make it the target state and enable device
   * to generate PME#.

AFAICT, pci_target_state() does not actually "enable wake-up from this
state" or "enable device to generate PME#".

> > >  drivers/pci/pci.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index f6a4dd10d9b0..52821a21fc07 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2125,16 +2125,16 @@ bool pci_dev_run_wake(struct pci_dev *dev)
> > >  {
> > >  	struct pci_bus *bus = dev->bus;
> > >
> > > -	if (device_can_wakeup(&dev->dev))
> > > -		return true;
> > > -
> > >  	if (!dev->pme_support)
> > >  		return false;
> > >
> > >  	/* PME-capable in principle, but not from the target power state */
> > > -	if (!pci_pme_capable(dev, pci_target_state(dev, false)))
> > > +	if (!pci_pme_capable(dev, pci_target_state(dev, true)))
> > >  		return false;
> > >
> > > +	if (device_can_wakeup(&dev->dev))
> > > +		return true;
> > > +
> > >  	while (bus->parent) {
> > >  		struct pci_dev *bridge = bus->self;
> > >
> > 
> 
> 

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Rafael J. Wysocki]

On Thursday, April 26, 2018 3:55:45 PM CEST Bjorn Helgaas wrote:
> On Fri, Apr 13, 2018 at 09:29:56AM +0200, Rafael J. Wysocki wrote:
> > On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
> > > Hi Bjorn and Rafael,
> > > 
> > > > On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng <kai.heng.feng@canonical.com>  
> > > > wrote:
> > > >
> > > > USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> > > > core: Drop run_wake flag from struct dev_pm_info").
> > > >
> > > > The device in question is not power managed by platform firmware,
> > > > furthermore, it only supports PME# from D3cold:
> > > > Capabilities: [78] Power Management version 3
> > > >        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> > > >        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> > > >
> > > > Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> > > > After that commit, the device gets runtime suspended, so it does not
> > > > respond to any PME#.
> 
> Apologies for my lack of PM expertise.  I don't think the device would
> *respond* to PME#, would it?  I would think the device would
> potentially *generate* a PME#.

Right.

> And I guess since this device can generate PME# only from D3cold, the
> implication is that runtime suspending the device may put it into D1,
> D2, or D3hot, but not D3cold?  Is that an axiom of the runtime suspend
> design?

No, it isn't.

Runtime PM is expected to only put devices into D-states from where they
can generate PME.

Before the problematic change it would just hold the device in question in D0,
but after that change the device will be suspended (in which case it will end
up in D3hot which is incorrect).

> > > > usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
> > > > device_can_wakeup() in pci_dev_run_wake() always returns true.
> 
> I think "mandatorily" means "always" or "unconditionally", right?
> 
> > > > So pci_dev_run_wake() needs to check PME wakeup capability as its first
> > > > condition.
> > > >
> > > > In addition, change wakeup flag passed to pci_target_state() from false
> > > > to true, because we want to find the deepest state that the device can
> > > > still generate PME#.
> 
> Is this a separate bug fix?  I don't understand how it fits in here
> because the wakeup flag means "Whether or not wakeup functionality
> will be enabled for the device", and you're not changing anything
> about whether wakeup functionality will be enabled.

For runtime PM the "wakeup" argument of pci_target_state() should always be
"true", so technically this may be regarded as a separate issue, but this
change is needed as a functional fix for the device in question along with
the reordering.

Since technically there is a state from which the device can signal PME,
device_can_wakeup() returns "true" for it, but this isn't sufficient for
pci_dev_run_wake() to return "true" (because that state is D3cold and
the platform cannot power-manage the device, so the device cannot be put
into D3cold directly).  That's the first thing that needs to be changed.

On top of that, we need to look for a state from which the device can
generate PME.

> > > > Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct  
> > > > dev_pm_info")
> > > > Cc: stable@vger.kernel.org # 4.13+
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > ---
> > > > v3: State the reason why the wakeup flag gets changed.
> > > >
> > > > v2: Explicitly check dev->pme_support.
> > > 
> > > If this patch is good enough, I am hoping it can get merged in v4.17.
> > 
> > OK
> > 
> > Bjorn, if you want to take this:
> > 
> >  Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Otherwise please let me know and I'll queue it up.
> 
> de3ef1eb1cd0 went through your tree, so I think this fix should go
> through your tree, too.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

OK

> Not directly related to this patch, but I think these comments in
> pci_target_state() are slightly misleading:
> 
>    * Call the platform to choose the target state of the device
>    * and enable wake-up from this state if supported.
> 
>    * Find the deepest state from which the device can generate
>    * wake-up events, make it the target state and enable device
>    * to generate PME#.
> 
> AFAICT, pci_target_state() does not actually "enable wake-up from this
> state" or "enable device to generate PME#".

Right, the comments appear to be stale, I'll send a patch to update them.

Thanks,
Rafael

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Kai Heng Feng]

Hi Rafael,

> On Apr 26, 2018, at 10:36 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, April 26, 2018 3:55:45 PM CEST Bjorn Helgaas wrote:
>> On Fri, Apr 13, 2018 at 09:29:56AM +0200, Rafael J. Wysocki wrote:
>>> On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
>>>> Hi Bjorn and Rafael,
>>>>
>>>>> On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng  
>>>>> <kai.heng.feng@canonical.com>
>>>>> wrote:
>>>>>
>>>>> USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
>>>>> core: Drop run_wake flag from struct dev_pm_info").
>>>>>
>>>>> The device in question is not power managed by platform firmware,
>>>>> furthermore, it only supports PME# from D3cold:
>>>>> Capabilities: [78] Power Management version 3
>>>>>        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
>>>>>        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
>>>>>
>>>>> Before commit de3ef1eb1cd0, the device never gets runtime suspended.
>>>>> After that commit, the device gets runtime suspended, so it does not
>>>>> respond to any PME#.
>>
>> Apologies for my lack of PM expertise.  I don't think the device would
>> *respond* to PME#, would it?  I would think the device would
>> potentially *generate* a PME#.
>

Do you want me to send another version with updated commit message?

I can also split it to two commits if you desire.

Kai-Heng

> Right.
>
>> And I guess since this device can generate PME# only from D3cold, the
>> implication is that runtime suspending the device may put it into D1,
>> D2, or D3hot, but not D3cold?  Is that an axiom of the runtime suspend
>> design?
>
> No, it isn't.
>
> Runtime PM is expected to only put devices into D-states from where they
> can generate PME.
>
> Before the problematic change it would just hold the device in question  
> in D0,
> but after that change the device will be suspended (in which case it will  
> end
> up in D3hot which is incorrect).
>
>>>>> usb_hcd_pci_probe() mandatorily calls device_wakeup_enable(), hence
>>>>> device_can_wakeup() in pci_dev_run_wake() always returns true.
>>
>> I think "mandatorily" means "always" or "unconditionally", right?
>>
>>>>> So pci_dev_run_wake() needs to check PME wakeup capability as its first
>>>>> condition.
>>>>>
>>>>> In addition, change wakeup flag passed to pci_target_state() from false
>>>>> to true, because we want to find the deepest state that the device can
>>>>> still generate PME#.
>>
>> Is this a separate bug fix?  I don't understand how it fits in here
>> because the wakeup flag means "Whether or not wakeup functionality
>> will be enabled for the device", and you're not changing anything
>> about whether wakeup functionality will be enabled.
>
> For runtime PM the "wakeup" argument of pci_target_state() should always be
> "true", so technically this may be regarded as a separate issue, but this
> change is needed as a functional fix for the device in question along with
> the reordering.
>
> Since technically there is a state from which the device can signal PME,
> device_can_wakeup() returns "true" for it, but this isn't sufficient for
> pci_dev_run_wake() to return "true" (because that state is D3cold and
> the platform cannot power-manage the device, so the device cannot be put
> into D3cold directly).  That's the first thing that needs to be changed.
>
> On top of that, we need to look for a state from which the device can
> generate PME.
>
>>>>> Fixes: de3ef1eb1cd0 ("PM / core: Drop run_wake flag from struct
>>>>> dev_pm_info")
>>>>> Cc: stable@vger.kernel.org # 4.13+
>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>> ---
>>>>> v3: State the reason why the wakeup flag gets changed.
>>>>>
>>>>> v2: Explicitly check dev->pme_support.
>>>>
>>>> If this patch is good enough, I am hoping it can get merged in v4.17.
>>>
>>> OK
>>>
>>> Bjorn, if you want to take this:
>>>
>>>  Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Otherwise please let me know and I'll queue it up.
>>
>> de3ef1eb1cd0 went through your tree, so I think this fix should go
>> through your tree, too.
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> OK
>
>> Not directly related to this patch, but I think these comments in
>> pci_target_state() are slightly misleading:
>>
>>    * Call the platform to choose the target state of the device
>>    * and enable wake-up from this state if supported.
>>
>>    * Find the deepest state from which the device can generate
>>    * wake-up events, make it the target state and enable device
>>    * to generate PME#.
>>
>> AFAICT, pci_target_state() does not actually "enable wake-up from this
>> state" or "enable device to generate PME#".
>
> Right, the comments appear to be stale, I'll send a patch to update them.
>
> Thanks,
> Rafael

Re: [PATCH v3] PCI / PM: Always check PME wakeup capability for runtime wakeup support

[Rafael J. Wysocki]

On Friday, May 4, 2018 9:36:01 AM CEST Kai Heng Feng wrote:
> Hi Rafael,
> 
> > On Apr 26, 2018, at 10:36 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Thursday, April 26, 2018 3:55:45 PM CEST Bjorn Helgaas wrote:
> >> On Fri, Apr 13, 2018 at 09:29:56AM +0200, Rafael J. Wysocki wrote:
> >>> On Friday, April 13, 2018 8:58:11 AM CEST Kai Heng Feng wrote:
> >>>> Hi Bjorn and Rafael,
> >>>>
> >>>>> On Apr 1, 2018, at 12:40 AM, Kai-Heng Feng  
> >>>>> <kai.heng.feng@canonical.com>
> >>>>> wrote:
> >>>>>
> >>>>> USB controller ASM1042 stops working after commit de3ef1eb1cd0 ("PM /
> >>>>> core: Drop run_wake flag from struct dev_pm_info").
> >>>>>
> >>>>> The device in question is not power managed by platform firmware,
> >>>>> furthermore, it only supports PME# from D3cold:
> >>>>> Capabilities: [78] Power Management version 3
> >>>>>        Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> >>>>>        Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> >>>>>
> >>>>> Before commit de3ef1eb1cd0, the device never gets runtime suspended.
> >>>>> After that commit, the device gets runtime suspended, so it does not
> >>>>> respond to any PME#.
> >>
> >> Apologies for my lack of PM expertise.  I don't think the device would
> >> *respond* to PME#, would it?  I would think the device would
> >> potentially *generate* a PME#.
> >
> 
> Do you want me to send another version with updated commit message?

Yes, please.

> I can also split it to two commits if you desire.

No, that's not necessary.

Thanks,
Rafael