| Message ID | 20190322181127.60640-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: host: ohci-platform: Implement ohci_platform_shutdown | expand |
On Fri, 22 Mar 2019, Tony Lindgren wrote: > If OHCI is runtime suspended, we can currently get an "imprecise > external abort" on reboot with ohci-platform loaded when PM runtime > is implemented for the SoC. > > Let's fix this by implementing ohci_platform_shutdown with PM runtime > calls clocking the hardware before calling hcd->driver->shutdown. > > Fixes: 0aa0b93e7af6 ("usb: host: ohci-platform: Add basic runtime PM support") > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/usb/host/ohci-platform.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c > --- a/drivers/usb/host/ohci-platform.c > +++ b/drivers/usb/host/ohci-platform.c > @@ -240,6 +240,22 @@ static int ohci_platform_probe(struct platform_device *dev) > return err; > } > > +static void ohci_platform_shutdown(struct platform_device *pdev) > +{ > + struct usb_hcd *hcd = platform_get_drvdata(pdev); > + int err; > + > + err = pm_runtime_get_sync(&pdev->dev); > + if (err < 0) > + pm_runtime_put_noidle(&pdev->dev); > + > + if (hcd->driver->shutdown) > + hcd->driver->shutdown(hcd); > + > + if (!err) > + pm_runtime_put_sync(&pdev->dev); > +} How about putting these runtime PM additions into usb_hcd_platform_shutdown instead, so they will apply to all platform controller drivers? Also, are you certain you want the pm_runtime_put_sync at the end? If the system is shutting down anyway, why waste time doing an extra runtime suspend? Alan Stern > + > static int ohci_platform_remove(struct platform_device *dev) > { > struct usb_hcd *hcd = platform_get_drvdata(dev); > @@ -326,7 +342,7 @@ static struct platform_driver ohci_platform_driver = { > .id_table = ohci_platform_table, > .probe = ohci_platform_probe, > .remove = ohci_platform_remove, > - .shutdown = usb_hcd_platform_shutdown, > + .shutdown = ohci_platform_shutdown, > .driver = { > .name = "ohci-platform", > .pm = &ohci_platform_pm_ops, >
* Alan Stern <stern@rowland.harvard.edu> [190322 18:37]: > How about putting these runtime PM additions into > usb_hcd_platform_shutdown instead, so they will apply to all platform > controller drivers? OK let's do that then. > Also, are you certain you want the pm_runtime_put_sync at the end? If > the system is shutting down anyway, why waste time doing an extra > runtime suspend? Well mostly to keep the calls paired. But maybe there are also kexec reboot cases where we'd want to have things properly disabled for PM before kexec. Regards, Tony
On Fri, 22 Mar 2019, Tony Lindgren wrote: > * Alan Stern <stern@rowland.harvard.edu> [190322 18:37]: > > How about putting these runtime PM additions into > > usb_hcd_platform_shutdown instead, so they will apply to all platform > > controller drivers? > > OK let's do that then. > > > Also, are you certain you want the pm_runtime_put_sync at the end? If > > the system is shutting down anyway, why waste time doing an extra > > runtime suspend? > > Well mostly to keep the calls paired. But maybe there are > also kexec reboot cases where we'd want to have things > properly disabled for PM before kexec. I'm not sure that makes sense. You can't actually disable anything for runtime PM from within a driver; all you can do is tell the runtime PM core that _you're_ not using the device any more. But if some other part of the system is still using it, it will remain at full power. Alan Stern
* Alan Stern <stern@rowland.harvard.edu> [190322 20:03]: > On Fri, 22 Mar 2019, Tony Lindgren wrote: > > > * Alan Stern <stern@rowland.harvard.edu> [190322 18:37]: > > > How about putting these runtime PM additions into > > > usb_hcd_platform_shutdown instead, so they will apply to all platform > > > controller drivers? > > > > OK let's do that then. > > > > > Also, are you certain you want the pm_runtime_put_sync at the end? If > > > the system is shutting down anyway, why waste time doing an extra > > > runtime suspend? > > > > Well mostly to keep the calls paired. But maybe there are > > also kexec reboot cases where we'd want to have things > > properly disabled for PM before kexec. > > I'm not sure that makes sense. You can't actually disable anything for > runtime PM from within a driver; all you can do is tell the runtime PM > core that _you're_ not using the device any more. But if some other > part of the system is still using it, it will remain at full power. Right, it's just a usecount and whatever happens after that is ouf of the driver control. And in the shutdown case PM runtime does not have much of a chance to do anything here :) Not much point checking the pm_runtime_get_sync() for errors either then.. We can just add a comment there. If the patch below looks OK to you I'll post with an updated description. Regards, Tony 8< ------------------- diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -3017,6 +3017,9 @@ usb_hcd_platform_shutdown(struct platform_device *dev) { struct usb_hcd *hcd = platform_get_drvdata(dev); + /* No need for pm_runtime_put(), we're shutting down */ + pm_runtime_get_sync(&dev->dev); + if (hcd->driver->shutdown) hcd->driver->shutdown(hcd); }
On Fri, 22 Mar 2019, Tony Lindgren wrote: > * Alan Stern <stern@rowland.harvard.edu> [190322 20:03]: > > On Fri, 22 Mar 2019, Tony Lindgren wrote: > > > > > * Alan Stern <stern@rowland.harvard.edu> [190322 18:37]: > > > > How about putting these runtime PM additions into > > > > usb_hcd_platform_shutdown instead, so they will apply to all platform > > > > controller drivers? > > > > > > OK let's do that then. > > > > > > > Also, are you certain you want the pm_runtime_put_sync at the end? If > > > > the system is shutting down anyway, why waste time doing an extra > > > > runtime suspend? > > > > > > Well mostly to keep the calls paired. But maybe there are > > > also kexec reboot cases where we'd want to have things > > > properly disabled for PM before kexec. > > > > I'm not sure that makes sense. You can't actually disable anything for > > runtime PM from within a driver; all you can do is tell the runtime PM > > core that _you're_ not using the device any more. But if some other > > part of the system is still using it, it will remain at full power. > > Right, it's just a usecount and whatever happens after that is > ouf of the driver control. And in the shutdown case PM runtime > does not have much of a chance to do anything here :) > > Not much point checking the pm_runtime_get_sync() for errors > either then.. We can just add a comment there. > > If the patch below looks OK to you I'll post with an updated > description. > > Regards, > > Tony > > 8< ------------------- > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -3017,6 +3017,9 @@ usb_hcd_platform_shutdown(struct platform_device *dev) > { > struct usb_hcd *hcd = platform_get_drvdata(dev); > > + /* No need for pm_runtime_put(), we're shutting down */ > + pm_runtime_get_sync(&dev->dev); > + > if (hcd->driver->shutdown) > hcd->driver->shutdown(hcd); > } Yes, that will be okay. Assuming it fixes your problem, of course. :-) Alan Stern
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c --- a/drivers/usb/host/ohci-platform.c +++ b/drivers/usb/host/ohci-platform.c @@ -240,6 +240,22 @@ static int ohci_platform_probe(struct platform_device *dev) return err; } +static void ohci_platform_shutdown(struct platform_device *pdev) +{ + struct usb_hcd *hcd = platform_get_drvdata(pdev); + int err; + + err = pm_runtime_get_sync(&pdev->dev); + if (err < 0) + pm_runtime_put_noidle(&pdev->dev); + + if (hcd->driver->shutdown) + hcd->driver->shutdown(hcd); + + if (!err) + pm_runtime_put_sync(&pdev->dev); +} + static int ohci_platform_remove(struct platform_device *dev) { struct usb_hcd *hcd = platform_get_drvdata(dev); @@ -326,7 +342,7 @@ static struct platform_driver ohci_platform_driver = { .id_table = ohci_platform_table, .probe = ohci_platform_probe, .remove = ohci_platform_remove, - .shutdown = usb_hcd_platform_shutdown, + .shutdown = ohci_platform_shutdown, .driver = { .name = "ohci-platform", .pm = &ohci_platform_pm_ops,
If OHCI is runtime suspended, we can currently get an "imprecise external abort" on reboot with ohci-platform loaded when PM runtime is implemented for the SoC. Let's fix this by implementing ohci_platform_shutdown with PM runtime calls clocking the hardware before calling hcd->driver->shutdown. Fixes: 0aa0b93e7af6 ("usb: host: ohci-platform: Add basic runtime PM support") Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/usb/host/ohci-platform.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)