[PATCH 1/2] net: wwan: iosm: Let PCI core handle PCI power transition

pci_pm_suspend_noirq() and pci_pm_resume_noirq() already handle power
transition for system-wide suspend and resume, so it's not necessary to
do it in the driver.

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 49 ++-------------------------
 1 file changed, 2 insertions(+), 47 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 2fe88b8be3481..d73894e2a84ed 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -363,67 +363,22 @@ static int __maybe_unused ipc_pcie_resume_s2idle(struct iosm_pcie *ipc_pcie)
 
 int __maybe_unused ipc_pcie_suspend(struct iosm_pcie *ipc_pcie)
 {
-	struct pci_dev *pdev;
-	int ret;
-
-	pdev = ipc_pcie->pci;
-
-	/* Execute D3 one time. */
-	if (pdev->current_state != PCI_D0) {
-		dev_dbg(ipc_pcie->dev, "done for PM=%d", pdev->current_state);
-		return 0;
-	}
-
 	/* The HAL shall ask the shared memory layer whether D3 is allowed. */
 	ipc_imem_pm_suspend(ipc_pcie->imem);
 
-	/* Save the PCI configuration space of a device before suspending. */
-	ret = pci_save_state(pdev);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_save_state error=%d", ret);
-		return ret;
-	}
-
-	/* Set the power state of a PCI device.
-	 * Transition a device to a new power state, using the device's PCI PM
-	 * registers.
-	 */
-	ret = pci_set_power_state(pdev, PCI_D3cold);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-		return ret;
-	}
-
 	dev_dbg(ipc_pcie->dev, "SUSPEND done");
-	return ret;
+	return 0;
 }
 
 int __maybe_unused ipc_pcie_resume(struct iosm_pcie *ipc_pcie)
 {
-	int ret;
-
-	/* Set the power state of a PCI device.
-	 * Transition a device to a new power state, using the device's PCI PM
-	 * registers.
-	 */
-	ret = pci_set_power_state(ipc_pcie->pci, PCI_D0);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-		return ret;
-	}
-
-	pci_restore_state(ipc_pcie->pci);
-
 	/* The HAL shall inform the shared memory layer that the device is
 	 * active.
 	 */
 	ipc_imem_pm_resume(ipc_pcie->imem);
 
 	dev_dbg(ipc_pcie->dev, "RESUME done");
-	return ret;
+	return 0;
 }
 
 static int __maybe_unused ipc_pcie_suspend_cb(struct device *dev)

-- 
2.33.1

We are seeing spurious wakeup caused by Intel 7560 WWAN on AMD laptops.
This prevent those laptops to stay in s2idle state.

From what I can understand, the intention of ipc_pcie_suspend() is to
put the device to D3cold, and ipc_pcie_suspend_s2idle() is to keep the
device at D0. However, the device can still be put to D3hot/D3cold by
PCI core.

So explicitly let PCI core know this device should stay at D0, to solve
the spurious wakeup.

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index d73894e2a84ed..af1d0e837fe99 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -340,6 +340,9 @@ static int __maybe_unused ipc_pcie_suspend_s2idle(struct iosm_pcie *ipc_pcie)
 
 	ipc_imem_pm_s2idle_sleep(ipc_pcie->imem, true);
 
+	/* Let PCI core know this device should stay at D0 */
+	pci_save_state(ipc_pcie->pci);
+
 	return 0;
 }
 

-- 
2.33.1

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller [off-list ref]:

On Fri, 24 Dec 2021 16:19:13 +0800 you wrote:

pci_pm_suspend_noirq() and pci_pm_resume_noirq() already handle power
transition for system-wide suspend and resume, so it's not necessary to
do it in the driver.

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 49 ++-------------------------
 1 file changed, 2 insertions(+), 47 deletions(-)

Here is the summary with links:
  - [1/2] net: wwan: iosm: Let PCI core handle PCI power transition
    https://git.kernel.org/netdev/net-next/c/8f58e29ed7fc
  - [2/2] net: wwan: iosm: Keep device at D0 for s2idle case
    https://git.kernel.org/netdev/net-next/c/f4dd5174e273

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[+cc Rafael, in case you have insight about the PCI_D0 question below;
Vaibhav, since this is related to your generic PM conversions]

On Fri, Dec 24, 2021 at 04:19:13PM +0800, Kai-Heng Feng wrote:

pci_pm_suspend_noirq() and pci_pm_resume_noirq() already handle power
transition for system-wide suspend and resume, so it's not necessary to
do it in the driver.

I see DaveM has already applied this, but it looks good to me, thanks
for doing this!

One minor question below...

I don't understand the intent of this early exit, and it's not obvious
to me that pci_pm_suspend_noirq() bails out early when
(pdev->current_state != PCI_D0).

 	/* The HAL shall ask the shared memory layer whether D3 is allowed. */
 	ipc_imem_pm_suspend(ipc_pcie->imem);
 
-	/* Save the PCI configuration space of a device before suspending. */
-	ret = pci_save_state(pdev);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_save_state error=%d", ret);
-		return ret;
-	}
-
-	/* Set the power state of a PCI device.
-	 * Transition a device to a new power state, using the device's PCI PM
-	 * registers.
-	 */
-	ret = pci_set_power_state(pdev, PCI_D3cold);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-		return ret;
-	}
-
 	dev_dbg(ipc_pcie->dev, "SUSPEND done");
-	return ret;
+	return 0;
 }
 
 int __maybe_unused ipc_pcie_resume(struct iosm_pcie *ipc_pcie)
 {
-	int ret;
-
-	/* Set the power state of a PCI device.
-	 * Transition a device to a new power state, using the device's PCI PM
-	 * registers.
-	 */
-	ret = pci_set_power_state(ipc_pcie->pci, PCI_D0);
-
-	if (ret) {
-		dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-		return ret;
-	}
-
-	pci_restore_state(ipc_pcie->pci);
-
 	/* The HAL shall inform the shared memory layer that the device is
 	 * active.
 	 */
 	ipc_imem_pm_resume(ipc_pcie->imem);
 
 	dev_dbg(ipc_pcie->dev, "RESUME done");
-	return ret;
+	return 0;
 }
 
 static int __maybe_unused ipc_pcie_suspend_cb(struct device *dev)
-- 
2.33.1

[+cc Rafael, Vaibhav]

On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote:

This is a weird and non-obvious way to say "this device should stay at
D0".  It's also fairly expensive since pci_save_state() does a lot of
slow PCI config reads.

 	return 0;
 }
 
-- 
2.33.1

On Thu, Dec 30, 2021 at 4:12 AM Bjorn Helgaas [off-list ref] wrote:

[+cc Rafael, in case you have insight about the PCI_D0 question below;
Vaibhav, since this is related to your generic PM conversions]

On Fri, Dec 24, 2021 at 04:19:13PM +0800, Kai-Heng Feng wrote:

quoted

pci_pm_suspend_noirq() and pci_pm_resume_noirq() already handle power
transition for system-wide suspend and resume, so it's not necessary to
do it in the driver.

I see DaveM has already applied this, but it looks good to me, thanks
for doing this!

One minor question below...

quoted

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 49 ++-------------------------
 1 file changed, 2 insertions(+), 47 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 2fe88b8be3481..d73894e2a84ed 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -363,67 +363,22 @@ static int __maybe_unused ipc_pcie_resume_s2idle(struct iosm_pcie *ipc_pcie)

 int __maybe_unused ipc_pcie_suspend(struct iosm_pcie *ipc_pcie)
 {
-     struct pci_dev *pdev;
-     int ret;
-
-     pdev = ipc_pcie->pci;
-
-     /* Execute D3 one time. */
-     if (pdev->current_state != PCI_D0) {
-             dev_dbg(ipc_pcie->dev, "done for PM=%d", pdev->current_state);
-             return 0;
-     }

I don't understand the intent of this early exit, and it's not obvious
to me that pci_pm_suspend_noirq() bails out early when
(pdev->current_state != PCI_D0).

Yes, I think this can be removed too. Please let me send v2.

Kai-Heng

quoted

      /* The HAL shall ask the shared memory layer whether D3 is allowed. */
      ipc_imem_pm_suspend(ipc_pcie->imem);

-     /* Save the PCI configuration space of a device before suspending. */
-     ret = pci_save_state(pdev);
-
-     if (ret) {
-             dev_err(ipc_pcie->dev, "pci_save_state error=%d", ret);
-             return ret;
-     }
-
-     /* Set the power state of a PCI device.
-      * Transition a device to a new power state, using the device's PCI PM
-      * registers.
-      */
-     ret = pci_set_power_state(pdev, PCI_D3cold);
-
-     if (ret) {
-             dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-             return ret;
-     }
-
      dev_dbg(ipc_pcie->dev, "SUSPEND done");
-     return ret;
+     return 0;
 }

 int __maybe_unused ipc_pcie_resume(struct iosm_pcie *ipc_pcie)
 {
-     int ret;
-
-     /* Set the power state of a PCI device.
-      * Transition a device to a new power state, using the device's PCI PM
-      * registers.
-      */
-     ret = pci_set_power_state(ipc_pcie->pci, PCI_D0);
-
-     if (ret) {
-             dev_err(ipc_pcie->dev, "pci_set_power_state error=%d", ret);
-             return ret;
-     }
-
-     pci_restore_state(ipc_pcie->pci);
-
      /* The HAL shall inform the shared memory layer that the device is
       * active.
       */
      ipc_imem_pm_resume(ipc_pcie->imem);

      dev_dbg(ipc_pcie->dev, "RESUME done");
-     return ret;
+     return 0;
 }

 static int __maybe_unused ipc_pcie_suspend_cb(struct device *dev)
--
2.33.1

On Thu, Dec 30, 2021 at 4:18 AM Bjorn Helgaas [off-list ref] wrote:

[+cc Rafael, Vaibhav]

On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote:

quoted

We are seeing spurious wakeup caused by Intel 7560 WWAN on AMD laptops.
This prevent those laptops to stay in s2idle state.

From what I can understand, the intention of ipc_pcie_suspend() is to
put the device to D3cold, and ipc_pcie_suspend_s2idle() is to keep the
device at D0. However, the device can still be put to D3hot/D3cold by
PCI core.

So explicitly let PCI core know this device should stay at D0, to solve
the spurious wakeup.

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index d73894e2a84ed..af1d0e837fe99 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -340,6 +340,9 @@ static int __maybe_unused ipc_pcie_suspend_s2idle(struct iosm_pcie *ipc_pcie)

      ipc_imem_pm_s2idle_sleep(ipc_pcie->imem, true);

+     /* Let PCI core know this device should stay at D0 */
+     pci_save_state(ipc_pcie->pci);

This is a weird and non-obvious way to say "this device should stay at
D0".  It's also fairly expensive since pci_save_state() does a lot of
slow PCI config reads.

Yes, so I was waiting for feedback from IOSM devs what's the expected
PCI state for the s2idle case.

Dave, can you drop it from netdev until IOSM devs confirm this patch is correct?

Kai-Heng

quoted

      return 0;
 }

--
2.33.1

-----Original Message-----
From: Kai-Heng Feng <redacted>
Sent: Thursday, December 30, 2021 6:31 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Kumar, M Chetan <redacted>; linuxwwan
[off-list ref]; linux-pci@vger.kernel.org; linux-
pm@vger.kernel.org; Loic Poulain [off-list ref]; Sergey
Ryazanov [off-list ref]; Johannes Berg
[off-list ref]; David S. Miller [off-list ref];
Jakub Kicinski [off-list ref]; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; Rafael J. Wysocki [off-list ref]; Vaibhav
Gupta [off-list ref]
Subject: Re: [PATCH 2/2] net: wwan: iosm: Keep device at D0 for s2idle case

On Thu, Dec 30, 2021 at 4:18 AM Bjorn Helgaas [off-list ref] wrote:

quoted

[+cc Rafael, Vaibhav]

On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote:

quoted

We are seeing spurious wakeup caused by Intel 7560 WWAN on AMD

laptops.

quoted

quoted

This prevent those laptops to stay in s2idle state.

From what I can understand, the intention of ipc_pcie_suspend() is
to put the device to D3cold, and ipc_pcie_suspend_s2idle() is to
keep the device at D0. However, the device can still be put to
D3hot/D3cold by PCI core.

So explicitly let PCI core know this device should stay at D0, to
solve the spurious wakeup.

Did you get a chance to check the cause of spurious wakeup ? Was there any
information device is trying to send while platform is entering suspend/
host sw missed to unsubscribe certain notifications which resulted in wake event.

In our internal test (x86 platform) we had not noticed such spurious wakeup but would
like to cross check by running few more tests.

quoted

quoted

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c

b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index d73894e2a84ed..af1d0e837fe99 100644

--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -340,6 +340,9 @@ static int __maybe_unused

ipc_pcie_suspend_s2idle(struct iosm_pcie *ipc_pcie)

      ipc_imem_pm_s2idle_sleep(ipc_pcie->imem, true);

+     /* Let PCI core know this device should stay at D0 */
+     pci_save_state(ipc_pcie->pci);

This is a weird and non-obvious way to say "this device should stay at
D0".  It's also fairly expensive since pci_save_state() does a lot of
slow PCI config reads.

Yes, so I was waiting for feedback from IOSM devs what's the expected PCI
state for the s2idle case.

D3 is the expected state. 

Dave, can you drop it from netdev until IOSM devs confirm this patch is
correct?

Dave, please drop this patch from netdev.

On Mon, 3 Jan 2022 15:28:18 +0000 Kumar, M Chetan wrote:

quoted

Dave, can you drop it from netdev until IOSM devs confirm this patch is
correct?  

Dave, please drop this patch from netdev.

YMMV but these sort of requests aren't usually acted on. netdev doesn't
rebase so revert is needed, and the developers involved are best at
writing commit messages for those since they have all the context. 
So sending a revert patch, with Link to the discussion and context
explained is the best way.

On Mon, Jan 3, 2022 at 11:28 PM Kumar, M Chetan
[off-list ref] wrote:

quoted

-----Original Message-----
From: Kai-Heng Feng <redacted>
Sent: Thursday, December 30, 2021 6:31 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Kumar, M Chetan <redacted>; linuxwwan
[off-list ref]; linux-pci@vger.kernel.org; linux-
pm@vger.kernel.org; Loic Poulain [off-list ref]; Sergey
Ryazanov [off-list ref]; Johannes Berg
[off-list ref]; David S. Miller [off-list ref];
Jakub Kicinski [off-list ref]; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; Rafael J. Wysocki [off-list ref]; Vaibhav
Gupta [off-list ref]
Subject: Re: [PATCH 2/2] net: wwan: iosm: Keep device at D0 for s2idle case

On Thu, Dec 30, 2021 at 4:18 AM Bjorn Helgaas [off-list ref] wrote:

quoted

[+cc Rafael, Vaibhav]

On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote:

quoted

We are seeing spurious wakeup caused by Intel 7560 WWAN on AMD

laptops.

quoted

quoted

This prevent those laptops to stay in s2idle state.

From what I can understand, the intention of ipc_pcie_suspend() is
to put the device to D3cold, and ipc_pcie_suspend_s2idle() is to
keep the device at D0. However, the device can still be put to
D3hot/D3cold by PCI core.

So explicitly let PCI core know this device should stay at D0, to
solve the spurious wakeup.

Did you get a chance to check the cause of spurious wakeup ? Was there any
information device is trying to send while platform is entering suspend/
host sw missed to unsubscribe certain notifications which resulted in wake event.

Can you please let me know how to check it?

In our internal test (x86 platform) we had not noticed such spurious wakeup but would
like to cross check by running few more tests.

Sure, let me know what tests you want me to run.

quoted

quoted

quoted

Signed-off-by: Kai-Heng Feng <redacted>
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c

b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index d73894e2a84ed..af1d0e837fe99 100644

--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c

@@ -340,6 +340,9 @@ static int __maybe_unused

ipc_pcie_suspend_s2idle(struct iosm_pcie *ipc_pcie)

      ipc_imem_pm_s2idle_sleep(ipc_pcie->imem, true);

+     /* Let PCI core know this device should stay at D0 */
+     pci_save_state(ipc_pcie->pci);

This is a weird and non-obvious way to say "this device should stay at
D0".  It's also fairly expensive since pci_save_state() does a lot of
slow PCI config reads.

Yes, so I was waiting for feedback from IOSM devs what's the expected PCI
state for the s2idle case.

D3 is the expected state.

Is it D3hot or D3cold?

Kai-Heng

quoted

Dave, can you drop it from netdev until IOSM devs confirm this patch is
correct?

Dave, please drop this patch from netdev.