| Message ID | 20211224081914.345292-2-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4dd5174e2739ab0aeda14b32847e587e78ff3d9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: wwan: iosm: Let PCI core handle PCI power transition | expand |
[+cc Rafael, Vaibhav] On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote: > 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 <kai.heng.feng@canonical.com> > --- > 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. > return 0; > } > > -- > 2.33.1 >
On Thu, Dec 30, 2021 at 4:18 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Rafael, Vaibhav] > > On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote: > > 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 <kai.heng.feng@canonical.com> > > --- > > 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 > > > return 0; > > } > > > > -- > > 2.33.1 > >
> -----Original Message----- > From: Kai-Heng Feng <kai.heng.feng@canonical.com> > Sent: Thursday, December 30, 2021 6:31 AM > To: Bjorn Helgaas <helgaas@kernel.org> > Cc: Kumar, M Chetan <m.chetan.kumar@intel.com>; linuxwwan > <linuxwwan@intel.com>; linux-pci@vger.kernel.org; linux- > pm@vger.kernel.org; Loic Poulain <loic.poulain@linaro.org>; Sergey > Ryazanov <ryazanov.s.a@gmail.com>; Johannes Berg > <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; > Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>; Vaibhav > Gupta <vaibhavgupta40@gmail.com> > 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 <helgaas@kernel.org> wrote: > > > > [+cc Rafael, Vaibhav] > > > > On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote: > > > 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. 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. > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > --- > > > 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: > > 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 <m.chetan.kumar@intel.com> wrote: > > > -----Original Message----- > > From: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Sent: Thursday, December 30, 2021 6:31 AM > > To: Bjorn Helgaas <helgaas@kernel.org> > > Cc: Kumar, M Chetan <m.chetan.kumar@intel.com>; linuxwwan > > <linuxwwan@intel.com>; linux-pci@vger.kernel.org; linux- > > pm@vger.kernel.org; Loic Poulain <loic.poulain@linaro.org>; Sergey > > Ryazanov <ryazanov.s.a@gmail.com>; Johannes Berg > > <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; > > Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux- > > kernel@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>; Vaibhav > > Gupta <vaibhavgupta40@gmail.com> > > 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 <helgaas@kernel.org> wrote: > > > > > > [+cc Rafael, Vaibhav] > > > > > > On Fri, Dec 24, 2021 at 04:19:14PM +0800, Kai-Heng Feng wrote: > > > > 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. > > 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. > > > > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > --- > > > > 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 > > > Dave, can you drop it from netdev until IOSM devs confirm this patch is > > correct? > > Dave, please drop this patch from netdev.
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; }