| Message ID | 20191029170022.57528-1-mika.westerberg@linux.intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v3,1/2] PCI: pciehp: Do not disable interrupt twice on suspend | expand |
Hi Bjorn, Friendly reminder. If there are no objections it would be nice to get these two into v5.5 so we would have mostly working native PCIe hotlug then :) On Tue, Oct 29, 2019 at 08:00:21PM +0300, 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> > Tested-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> > --- > No changes from previous version. > > 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 b3122c151b80..56daad828c9e 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -253,7 +253,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 > @@ -261,7 +261,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; > } > > @@ -279,6 +291,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) > > return 0; > } > +#endif > > static int pciehp_resume(struct pcie_device *dev) > { > @@ -292,6 +305,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); > @@ -318,10 +337,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, > .resume = pciehp_resume, > - .runtime_suspend = pciehp_suspend, > +#endif > + .runtime_suspend = pciehp_runtime_suspend, > .runtime_resume = pciehp_runtime_resume, > #endif /* PM */ > }; > -- > 2.23.0
On Tue, Oct 29, 2019 at 08:00:21PM +0300, 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> > Tested-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> Applied both to pci/hotplug for v5.5, thanks! If it's convenient for you, a cover letter on multi-patch series would make a good place for responses like this that apply to the whole series. > --- > No changes from previous version. > > 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 b3122c151b80..56daad828c9e 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -253,7 +253,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 > @@ -261,7 +261,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; > } > > @@ -279,6 +291,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) > > return 0; > } > +#endif > > static int pciehp_resume(struct pcie_device *dev) > { > @@ -292,6 +305,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); > @@ -318,10 +337,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, > .resume = pciehp_resume, > - .runtime_suspend = pciehp_suspend, > +#endif > + .runtime_suspend = pciehp_runtime_suspend, > .runtime_resume = pciehp_runtime_resume, > #endif /* PM */ > }; > -- > 2.23.0 >
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index b3122c151b80..56daad828c9e 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -253,7 +253,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 @@ -261,7 +261,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; } @@ -279,6 +291,7 @@ static int pciehp_resume_noirq(struct pcie_device *dev) return 0; } +#endif static int pciehp_resume(struct pcie_device *dev) { @@ -292,6 +305,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); @@ -318,10 +337,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, .resume = pciehp_resume, - .runtime_suspend = pciehp_suspend, +#endif + .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> Tested-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> --- No changes from previous version. drivers/pci/hotplug/pciehp_core.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)