[PATCH v2] usb: ohci-platform: fix usb disconnect issue after s4

[PATCH v2] usb: ohci-platform: fix usb disconnect issue after s4

[Yinbo Zhu]

Avoid retaining bogus hardware states during resume-from-hibernation.
Previously we had reset the hardware as part of preparing to reinstate
the snapshot image. But we can do better now with the new PM framework,
since we know exactly which resume operations are from hibernation.

According to the commit 'cd1965db054e ("USB: ohci: move ohci_pci_{
suspend,resume} to ohci-hcd.c")' and commit '6ec4beb5c701 ("USB: new
flag for resume-from-hibernation")', the flag "hibernated" is for
resume-from-hibernation and it should be true when usb resume from disk.

When this flag "hibernated" is set, the drivers will reset the hardware
to get rid of any existing state and make sure resume from hibernation
re-enumerates everything for ohci.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v2:
		1. Revise the commit log infomation.	
		2. Wrap the ohci_platform_renew() function with two 
		   helpers that are ohci_platform_renew_hibernated() 
		   and ohci_platform_renew().

 drivers/usb/host/ohci-platform.c | 49 ++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 0adae6265127..56cb424d3bb0 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -289,7 +289,7 @@ static int ohci_platform_suspend(struct device *dev)
 	return ret;
 }
 
-static int ohci_platform_resume(struct device *dev)
+static int ohci_platform_renew(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct usb_ohci_pdata *pdata = dev_get_platdata(dev);
@@ -297,6 +297,7 @@ static int ohci_platform_resume(struct device *dev)
 
 	if (pdata->power_on) {
 		int err = pdata->power_on(pdev);
+
 		if (err < 0)
 			return err;
 	}
@@ -309,6 +310,38 @@ static int ohci_platform_resume(struct device *dev)
 
 	return 0;
 }
+
+static int ohci_platform_renew_hibernated(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct usb_ohci_pdata *pdata = dev_get_platdata(dev);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	if (pdata->power_on) {
+		int err = pdata->power_on(pdev);
+
+		if (err < 0)
+			return err;
+	}
+
+	ohci_resume(hcd, true);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int ohci_platform_resume(struct device *dev)
+{
+	return ohci_platform_renew(dev);
+}
+
+static int ohci_platform_restore(struct device *dev)
+{
+	return ohci_platform_renew_hibernated(dev);
+}
 #endif /* CONFIG_PM_SLEEP */
 
 static const struct of_device_id ohci_platform_ids[] = {
@@ -325,8 +358,16 @@ static const struct platform_device_id ohci_platform_table[] = {
 };
 MODULE_DEVICE_TABLE(platform, ohci_platform_table);
 
-static SIMPLE_DEV_PM_OPS(ohci_platform_pm_ops, ohci_platform_suspend,
-	ohci_platform_resume);
+#ifdef CONFIG_PM_SLEEP
+static const struct dev_pm_ops ohci_platform_pm_ops = {
+	.suspend = ohci_platform_suspend,
+	.resume = ohci_platform_resume,
+	.freeze = ohci_platform_suspend,
+	.thaw = ohci_platform_resume,
+	.poweroff = ohci_platform_suspend,
+	.restore = ohci_platform_restore,
+};
+#endif
 
 static struct platform_driver ohci_platform_driver = {
 	.id_table	= ohci_platform_table,
@@ -335,7 +376,9 @@ static struct platform_driver ohci_platform_driver = {
 	.shutdown	= usb_hcd_platform_shutdown,
 	.driver		= {
 		.name	= "ohci-platform",
+#ifdef CONFIG_PM_SLEEP
 		.pm	= &ohci_platform_pm_ops,
+#endif
 		.of_match_table = ohci_platform_ids,
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	}
-- 
2.31.1

Re: [PATCH v2] usb: ohci-platform: fix usb disconnect issue after s4

[Alan Stern]

On Wed, Aug 31, 2022 at 12:59:10PM +0800, Yinbo Zhu wrote:
> Avoid retaining bogus hardware states during resume-from-hibernation.
> Previously we had reset the hardware as part of preparing to reinstate
> the snapshot image. But we can do better now with the new PM framework,
> since we know exactly which resume operations are from hibernation.
> 
> According to the commit 'cd1965db054e ("USB: ohci: move ohci_pci_{
> suspend,resume} to ohci-hcd.c")' and commit '6ec4beb5c701 ("USB: new
> flag for resume-from-hibernation")', the flag "hibernated" is for
> resume-from-hibernation and it should be true when usb resume from disk.
> 
> When this flag "hibernated" is set, the drivers will reset the hardware
> to get rid of any existing state and make sure resume from hibernation
> re-enumerates everything for ohci.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v2:
> 		1. Revise the commit log infomation.	
> 		2. Wrap the ohci_platform_renew() function with two 
> 		   helpers that are ohci_platform_renew_hibernated() 
> 		   and ohci_platform_renew().
> 
>  drivers/usb/host/ohci-platform.c | 49 ++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
> index 0adae6265127..56cb424d3bb0 100644
> --- a/drivers/usb/host/ohci-platform.c
> +++ b/drivers/usb/host/ohci-platform.c
> @@ -289,7 +289,7 @@ static int ohci_platform_suspend(struct device *dev)
>  	return ret;
>  }
>  
> -static int ohci_platform_resume(struct device *dev)
> +static int ohci_platform_renew(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
>  	struct usb_ohci_pdata *pdata = dev_get_platdata(dev);
> @@ -297,6 +297,7 @@ static int ohci_platform_resume(struct device *dev)
>  
>  	if (pdata->power_on) {
>  		int err = pdata->power_on(pdev);
> +
>  		if (err < 0)
>  			return err;
>  	}
> @@ -309,6 +310,38 @@ static int ohci_platform_resume(struct device *dev)
>  
>  	return 0;
>  }
> +
> +static int ohci_platform_renew_hibernated(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct usb_ohci_pdata *pdata = dev_get_platdata(dev);
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	if (pdata->power_on) {
> +		int err = pdata->power_on(pdev);
> +
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	ohci_resume(hcd, true);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static int ohci_platform_resume(struct device *dev)
> +{
> +	return ohci_platform_renew(dev);
> +}
> +
> +static int ohci_platform_restore(struct device *dev)
> +{
> +	return ohci_platform_renew_hibernated(dev);
> +}

I really do not see any point in these helper routines.  Why not just 
use these names (ohci_platform_resume and ohci_platform_restore) for the 
actual routines and forget about the _renew names?

Although if it were me, I'd do it more like this:

static int ohci_platform_resume_common(struct device *dev, bool hibernated)
{ ... }

static int ohci_platform_resume(struct device *dev)
{
	return ohci_platform_resume_common(dev, false);
}

static int ohci_platform_restore(struct device *dev)
{
	return ohci_platform_resume_common(dev, true);
}

Alan Stern