usb: host: ohci-platform: Implement ohci_platform_shutdown

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Tony Lindgren]

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);
+}
+
 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,

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Alan Stern]

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

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Tony Lindgren]

* 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);
 }

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Alan Stern]

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

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Tony Lindgren]

* 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

usb: host: ohci-platform: Implement ohci_platform_shutdown

[Alan Stern]

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,
>