| Message ID | 20190618125051.2382-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] PCI: pciehp: Do not disable interrupt twice on suspend | expand |
at 20:50, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > We try to keep PCIe hotplug ports runtime suspended when entering system > suspend. Due to the fact that the PCIe portdrv sets NEVER_SKIP driver PM > flag the PM core always calls system suspend/resume hooks even if the > device is left runtime suspended. Since PCIe hotplug driver re-uses the > same function for both it ends up disabling hotplug interrupt twice and > the second time following is printed: > > pciehp 0000:03:01.0:pcie204: pcie_do_write_cmd: no response from device > > Prevent this from happening by checking whether the device is already > runtime suspended when system suspend hook is called. > > Fixes: 9c62f0bfb832 ("PCI: pciehp: Implement runtime PM callbacks") > Reported-by: Kai Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pci/hotplug/pciehp_core.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp_core.c > b/drivers/pci/hotplug/pciehp_core.c > index 6ad0d86762cb..3f8c13ddb3e8 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -248,7 +248,7 @@ static bool pme_is_native(struct pcie_device *dev) > return pcie_ports_native || host->native_pme; > } > > -static int pciehp_suspend(struct pcie_device *dev) > +static void pciehp_disable_interrupt(struct pcie_device *dev) > { > /* > * Disable hotplug interrupt so that it does not trigger > @@ -256,7 +256,19 @@ static int pciehp_suspend(struct pcie_device *dev) > */ > if (pme_is_native(dev)) > pcie_disable_interrupt(get_service_data(dev)); > +} > > +#ifdef CONFIG_PM_SLEEP > +static int pciehp_suspend(struct pcie_device *dev) > +{ > + /* > + * If the port is already runtime suspended we can keep it that > + * way. > + */ > + if (dev_pm_smart_suspend_and_suspended(&dev->port->dev)) > + return 0; > + > + pciehp_disable_interrupt(dev); > return 0; > } > > @@ -274,6 +286,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) > > return 0; > } > +#endif > > static int pciehp_resume(struct pcie_device *dev) > { > @@ -287,6 +300,12 @@ static int pciehp_resume(struct pcie_device *dev) > return 0; > } > > +static int pciehp_runtime_suspend(struct pcie_device *dev) > +{ > + pciehp_disable_interrupt(dev); > + return 0; > +} > + > static int pciehp_runtime_resume(struct pcie_device *dev) > { > struct controller *ctrl = get_service_data(dev); > @@ -313,10 +332,12 @@ static struct pcie_port_service_driver > hpdriver_portdrv = { > .remove = pciehp_remove, > > #ifdef CONFIG_PM > +#ifdef CONFIG_PM_SLEEP > .suspend = pciehp_suspend, > .resume_noirq = pciehp_resume_noirq, > +#endif > .resume = pciehp_resume, > - .runtime_suspend = pciehp_suspend, > + .runtime_suspend = pciehp_runtime_suspend, > .runtime_resume = pciehp_runtime_resume, > #endif /* PM */ > }; > -- > 2.20.1
On Tuesday, June 18, 2019 2:50:50 PM CEST Mika Westerberg wrote: > We try to keep PCIe hotplug ports runtime suspended when entering system > suspend. Due to the fact that the PCIe portdrv sets NEVER_SKIP driver PM > flag the PM core always calls system suspend/resume hooks even if the > device is left runtime suspended. Since PCIe hotplug driver re-uses the > same function for both it ends up disabling hotplug interrupt twice and > the second time following is printed: > > pciehp 0000:03:01.0:pcie204: pcie_do_write_cmd: no response from device > > Prevent this from happening by checking whether the device is already > runtime suspended when system suspend hook is called. > > Fixes: 9c62f0bfb832 ("PCI: pciehp: Implement runtime PM callbacks") > Reported-by: Kai Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/hotplug/pciehp_core.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index 6ad0d86762cb..3f8c13ddb3e8 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -248,7 +248,7 @@ static bool pme_is_native(struct pcie_device *dev) > return pcie_ports_native || host->native_pme; > } > > -static int pciehp_suspend(struct pcie_device *dev) > +static void pciehp_disable_interrupt(struct pcie_device *dev) > { > /* > * Disable hotplug interrupt so that it does not trigger > @@ -256,7 +256,19 @@ static int pciehp_suspend(struct pcie_device *dev) > */ > if (pme_is_native(dev)) > pcie_disable_interrupt(get_service_data(dev)); > +} > > +#ifdef CONFIG_PM_SLEEP > +static int pciehp_suspend(struct pcie_device *dev) > +{ > + /* > + * If the port is already runtime suspended we can keep it that > + * way. > + */ > + if (dev_pm_smart_suspend_and_suspended(&dev->port->dev)) > + return 0; > + > + pciehp_disable_interrupt(dev); > return 0; > } > > @@ -274,6 +286,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) > > return 0; > } > +#endif > > static int pciehp_resume(struct pcie_device *dev) > { > @@ -287,6 +300,12 @@ static int pciehp_resume(struct pcie_device *dev) > return 0; > } > > +static int pciehp_runtime_suspend(struct pcie_device *dev) > +{ > + pciehp_disable_interrupt(dev); > + return 0; > +} > + > static int pciehp_runtime_resume(struct pcie_device *dev) > { > struct controller *ctrl = get_service_data(dev); > @@ -313,10 +332,12 @@ static struct pcie_port_service_driver hpdriver_portdrv = { > .remove = pciehp_remove, > > #ifdef CONFIG_PM > +#ifdef CONFIG_PM_SLEEP > .suspend = pciehp_suspend, > .resume_noirq = pciehp_resume_noirq, > +#endif > .resume = pciehp_resume, > - .runtime_suspend = pciehp_suspend, > + .runtime_suspend = pciehp_runtime_suspend, > .runtime_resume = pciehp_runtime_resume, > #endif /* PM */ > }; >
On Tue, Jun 18, 2019 at 03:50:50PM +0300, Mika Westerberg wrote: > @@ -313,10 +332,12 @@ static struct pcie_port_service_driver hpdriver_portdrv = { > .remove = pciehp_remove, > > #ifdef CONFIG_PM > +#ifdef CONFIG_PM_SLEEP > .suspend = pciehp_suspend, > .resume_noirq = pciehp_resume_noirq, > +#endif > .resume = pciehp_resume, > - .runtime_suspend = pciehp_suspend, > + .runtime_suspend = pciehp_runtime_suspend, > .runtime_resume = pciehp_runtime_resume, > #endif /* PM */ > }; Hm, why isn't ".resume" part of the "#ifdef CONFIG_PM_SLEEP" section? Otherwise LGTM. Thanks, Lukas
On Sun, Aug 04, 2019 at 09:53:06PM +0200, Lukas Wunner wrote: > On Tue, Jun 18, 2019 at 03:50:50PM +0300, Mika Westerberg wrote: > > @@ -313,10 +332,12 @@ static struct pcie_port_service_driver hpdriver_portdrv = { > > .remove = pciehp_remove, > > > > #ifdef CONFIG_PM > > +#ifdef CONFIG_PM_SLEEP > > .suspend = pciehp_suspend, > > .resume_noirq = pciehp_resume_noirq, > > +#endif > > .resume = pciehp_resume, > > - .runtime_suspend = pciehp_suspend, > > + .runtime_suspend = pciehp_runtime_suspend, > > .runtime_resume = pciehp_runtime_resume, > > #endif /* PM */ > > }; > > Hm, why isn't ".resume" part of the "#ifdef CONFIG_PM_SLEEP" section? Good point. I'll move it there.
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 6ad0d86762cb..3f8c13ddb3e8 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -248,7 +248,7 @@ static bool pme_is_native(struct pcie_device *dev) return pcie_ports_native || host->native_pme; } -static int pciehp_suspend(struct pcie_device *dev) +static void pciehp_disable_interrupt(struct pcie_device *dev) { /* * Disable hotplug interrupt so that it does not trigger @@ -256,7 +256,19 @@ static int pciehp_suspend(struct pcie_device *dev) */ if (pme_is_native(dev)) pcie_disable_interrupt(get_service_data(dev)); +} +#ifdef CONFIG_PM_SLEEP +static int pciehp_suspend(struct pcie_device *dev) +{ + /* + * If the port is already runtime suspended we can keep it that + * way. + */ + if (dev_pm_smart_suspend_and_suspended(&dev->port->dev)) + return 0; + + pciehp_disable_interrupt(dev); return 0; } @@ -274,6 +286,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) return 0; } +#endif static int pciehp_resume(struct pcie_device *dev) { @@ -287,6 +300,12 @@ static int pciehp_resume(struct pcie_device *dev) return 0; } +static int pciehp_runtime_suspend(struct pcie_device *dev) +{ + pciehp_disable_interrupt(dev); + return 0; +} + static int pciehp_runtime_resume(struct pcie_device *dev) { struct controller *ctrl = get_service_data(dev); @@ -313,10 +332,12 @@ static struct pcie_port_service_driver hpdriver_portdrv = { .remove = pciehp_remove, #ifdef CONFIG_PM +#ifdef CONFIG_PM_SLEEP .suspend = pciehp_suspend, .resume_noirq = pciehp_resume_noirq, +#endif .resume = pciehp_resume, - .runtime_suspend = pciehp_suspend, + .runtime_suspend = pciehp_runtime_suspend, .runtime_resume = pciehp_runtime_resume, #endif /* PM */ };
We try to keep PCIe hotplug ports runtime suspended when entering system suspend. Due to the fact that the PCIe portdrv sets NEVER_SKIP driver PM flag the PM core always calls system suspend/resume hooks even if the device is left runtime suspended. Since PCIe hotplug driver re-uses the same function for both it ends up disabling hotplug interrupt twice and the second time following is printed: pciehp 0000:03:01.0:pcie204: pcie_do_write_cmd: no response from device Prevent this from happening by checking whether the device is already runtime suspended when system suspend hook is called. Fixes: 9c62f0bfb832 ("PCI: pciehp: Implement runtime PM callbacks") Reported-by: Kai Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pci/hotplug/pciehp_core.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)