[PATCH v1 0/2] PCI: PM: Fix handling of device that can only signal PME from D3cold

[PATCH v1 0/2] PCI: PM: Fix handling of device that can only signal PME from D3cold

[Rafael J. Wysocki]

Hi,

This series is a replacement for the following patch:

https://patchwork.kernel.org/project/linux-pm/patch/3149540.aeNJFYEL58@kreacher/

allowing devices that only can signal PME from D3cold to be put into
low-power states and signal wakeup from D3cold (if they get into it).

However, the patch above works by adding a special case to pci_pme_capable()
which actually is not necessary.

Instead of doing that, it is sufficient to make pci_target_state() handle the
case in which the device cannot signal PME from D0 consistently (patch [1/2]
in this series) and make __pci_enable_wake() enable PM signaling for devices
that can signal PME from D3cold (patch [2/2] in this series).

Please see the patch changelogs for details.

Thanks!

[PATCH v1 1/2] PCI: PM: Add special case handling for PCIe device wakeup

[Rafael J. Wysocki]

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

It is inconsistent to return PCI_D0 from pci_target_state() instead
of the original target state if 'wakeup' is true and the device
cannot signal PME from D0.

This only happens when the device cannot signal PME from the original
target state and any shallower power states (including D0) and that
case is effectively equivalent to the one in which PME signaling is
not supported at all.  Since the original target state is returned in
the latter case, make the function do that in the former one too.

Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
Fixes: 666ff6f83e1d ("PCI/PM: Avoid using device_may_wakeup() for runtime PM")
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com>
Reported-by: Koba Ko <koba.ko@canonical.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2595,16 +2595,20 @@ static pci_power_t pci_target_state(stru
 	if (dev->current_state == PCI_D3cold)
 		target_state = PCI_D3cold;
 
-	if (wakeup) {
+	if (wakeup && dev->pme_support) {
+		pci_power_t state = target_state;
+
 		/*
 		 * Find the deepest state from which the device can generate
 		 * PME#.
 		 */
-		if (dev->pme_support) {
-			while (target_state
-			      && !(dev->pme_support & (1 << target_state)))
-				target_state--;
-		}
+		while (state && !(dev->pme_support & (1 << state)))
+			state--;
+
+		if (state)
+			return state;
+		else if (dev->pme_support & 1)
+			return PCI_D0;
 	}
 
 	return target_state;

[PATCH v1 2/2] PCI: PM: Enable PME if it can be signaled from D3cold

[Rafael J. Wysocki]

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

PME signaling is only enabled by __pci_enable_wake() if the target
device can signal PME from the given target power state (to avoid
pointless reconfiguration of the device), but if the hierarchy above
the device goes into D3cold, the device itself will end up in D3cold
too, so if it can signal PME from D3cold, it should be enabled to
do so in __pci_enable_wake().

[Note that if the device does not end up in D3cold and it cannot
 signal PME from the original target power state, it will not signal
 PME, so in that case the behavior does not change.]

Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
Fixes: 5bcc2fb4e815 ("PCI PM: Simplify PCI wake-up code")
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com>
Reported-by: Koba Ko <koba.ko@canonical.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2491,7 +2491,14 @@ static int __pci_enable_wake(struct pci_
 	if (enable) {
 		int error;
 
-		if (pci_pme_capable(dev, state))
+		/*
+		 * Enable PME signaling if the device can signal PME from
+		 * D3cold regardless of whether or not it can signal PME from
+		 * the current target state, because that will allow it to
+		 * signal PME when the hierarchy above it goes into D3cold and
+		 * the device itself ends up in D3cold as a result of that.
+		 */
+		if (pci_pme_capable(dev, state) || pci_pme_capable(dev, PCI_D3cold))
 			pci_pme_active(dev, true);
 		else
 			ret = 1;

Re: [PATCH v1 1/2] PCI: PM: Add special case handling for PCIe device wakeup

[Rafael J. Wysocki]

On Thu, Jul 29, 2021 at 4:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is inconsistent to return PCI_D0 from pci_target_state() instead
> of the original target state if 'wakeup' is true and the device
> cannot signal PME from D0.
>
> This only happens when the device cannot signal PME from the original
> target state and any shallower power states (including D0) and that
> case is effectively equivalent to the one in which PME signaling is
> not supported at all.  Since the original target state is returned in
> the latter case, make the function do that in the former one too.
>
> Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
> Fixes: 666ff6f83e1d ("PCI/PM: Avoid using device_may_wakeup() for runtime PM")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com>
> Reported-by: Koba Ko <koba.ko@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The subject of this patch should be different, let me resend it.

[PATCH v1.1 1/2] PCI: PM: Avoid forcing PCI_D0 for wakeup reasons inconsistently

[Rafael J. Wysocki]

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

It is inconsistent to return PCI_D0 from pci_target_state() instead
of the original target state if 'wakeup' is true and the device
cannot signal PME from D0.

This only happens when the device cannot signal PME from the original
target state and any shallower power states (including D0) and that
case is effectively equivalent to the one in which PME singaling is
not supported at all.  Since the original target state is returned in
the latter case, make the function do that in the former one too.

Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
Fixes: 666ff6f83e1d ("PCI/PM: Avoid using device_may_wakeup() for runtime PM")
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-by: Utkarsh H Patel <utkarsh.h.patel@intel.com>
Reported-by: Koba Ko <koba.ko@canonical.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v1.1
      * Resend under a suitable subject.

---
 drivers/pci/pci.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2595,16 +2595,20 @@ static pci_power_t pci_target_state(stru
 	if (dev->current_state == PCI_D3cold)
 		target_state = PCI_D3cold;
 
-	if (wakeup) {
+	if (wakeup && dev->pme_support) {
+		pci_power_t state = target_state;
+
 		/*
 		 * Find the deepest state from which the device can generate
 		 * PME#.
 		 */
-		if (dev->pme_support) {
-			while (target_state
-			      && !(dev->pme_support & (1 << target_state)))
-				target_state--;
-		}
+		while (state && !(dev->pme_support & (1 << state)))
+			state--;
+
+		if (state)
+			return state;
+		else if (dev->pme_support & 1)
+			return PCI_D0;
 	}
 
 	return target_state;

Re: [PATCH v1.1 1/2] PCI: PM: Avoid forcing PCI_D0 for wakeup reasons inconsistently

[Mika Westerberg]

On Thu, Jul 29, 2021 at 05:54:28PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is inconsistent to return PCI_D0 from pci_target_state() instead
> of the original target state if 'wakeup' is true and the device
> cannot signal PME from D0.
> 
> This only happens when the device cannot signal PME from the original
> target state and any shallower power states (including D0) and that
> case is effectively equivalent to the one in which PME singaling is
> not supported at all.  Since the original target state is returned in
> the latter case, make the function do that in the former one too.
> 
> Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
> Fixes: 666ff6f83e1d ("PCI/PM: Avoid using device_may_wakeup() for runtime PM")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

Re: [PATCH v1 2/2] PCI: PM: Enable PME if it can be signaled from D3cold

[Mika Westerberg]

On Thu, Jul 29, 2021 at 04:49:10PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> PME signaling is only enabled by __pci_enable_wake() if the target
> device can signal PME from the given target power state (to avoid
> pointless reconfiguration of the device), but if the hierarchy above
> the device goes into D3cold, the device itself will end up in D3cold
> too, so if it can signal PME from D3cold, it should be enabled to
> do so in __pci_enable_wake().
> 
> [Note that if the device does not end up in D3cold and it cannot
>  signal PME from the original target power state, it will not signal
>  PME, so in that case the behavior does not change.]
> 
> Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
> Fixes: 5bcc2fb4e815 ("PCI PM: Simplify PCI wake-up code")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

Also this solves the reported issue so,

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

Thanks!

Re: [PATCH v1 2/2] PCI: PM: Enable PME if it can be signaled from D3cold

[Rafael J. Wysocki]

On Fri, Jul 30, 2021 at 12:28 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Jul 29, 2021 at 04:49:10PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > PME signaling is only enabled by __pci_enable_wake() if the target
> > device can signal PME from the given target power state (to avoid
> > pointless reconfiguration of the device), but if the hierarchy above
> > the device goes into D3cold, the device itself will end up in D3cold
> > too, so if it can signal PME from D3cold, it should be enabled to
> > do so in __pci_enable_wake().
> >
> > [Note that if the device does not end up in D3cold and it cannot
> >  signal PME from the original target power state, it will not signal
> >  PME, so in that case the behavior does not change.]
> >
> > Link: https://lore.kernel.org/linux-pm/3149540.aeNJFYEL58@kreacher/
> > Fixes: 5bcc2fb4e815 ("PCI PM: Simplify PCI wake-up code")
> > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Also this solves the reported issue so,
>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thank you!

I'll queue up these two patches for 5.15 barring any objections from Bjorn.