| Message ID | 20211224081914.345292-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] net: wwan: iosm: Let PCI core handle PCI power transition | expand |
Hello: This series was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: 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 <kai.heng.feng@canonical.com> > --- > 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!
[+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... > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > 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). > /* 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:12 AM Bjorn Helgaas <helgaas@kernel.org> 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: > > 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... > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > 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 > > > /* 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 > >
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)
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 <kai.heng.feng@canonical.com> --- drivers/net/wwan/iosm/iosm_ipc_pcie.c | 49 ++------------------------- 1 file changed, 2 insertions(+), 47 deletions(-)