| Message ID | 20220830111449.2300-1-zhuyinbo@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: ohci-platform: fix usb disconnect issue after s4 | expand |
On Tue, Aug 30, 2022 at 07:14:49PM +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 "cd1965db0" and "6ec4beb5c" that the flag > "hibernated" is for resume-from-hibernation and it should be true when > usb resume from disk. When writing commit ids in changelogs, please use the recommended format. For this, that paragraph would read: According to 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... > 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> What commit id does this fix? > --- > drivers/usb/host/ohci-platform.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c > index 0adae6265127..e733da2cd3b7 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, bool hibernated) I hate functions like this as it's now impossible to read the caller and understand what is going on. > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct usb_ohci_pdata *pdata = dev_get_platdata(dev); > @@ -301,7 +301,7 @@ static int ohci_platform_resume(struct device *dev) > return err; > } > > - ohci_resume(hcd, false); > + ohci_resume(hcd, hibernated); > > pm_runtime_disable(dev); > pm_runtime_set_active(dev); > @@ -309,6 +309,16 @@ static int ohci_platform_resume(struct device *dev) > > return 0; > } > + > +static int ohci_platform_resume(struct device *dev) > +{ > + return ohci_platform_renew(dev, false); See, what does "false" here mean? You can wrap the ohci_platform_renew() function with two helpers that are ohci_platform_renew_hibernated() and ohci_platform_renew() or something like that, which would explain what the difference is here. thanks, greg k-h
On Tue, Aug 30, 2022 at 07:14:49PM +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 "cd1965db0" and "6ec4beb5c" that 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. What is the "usb disconnect issue" you mention in the Subject line that this patch is supposed to fix? > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> > --- > drivers/usb/host/ohci-platform.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c > index 0adae6265127..e733da2cd3b7 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, bool hibernated) How about calling this routine ohci_platform_resume_maybe_hibernated() instead? Alan Stern > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct usb_ohci_pdata *pdata = dev_get_platdata(dev); > @@ -301,7 +301,7 @@ static int ohci_platform_resume(struct device *dev) > return err; > } > > - ohci_resume(hcd, false); > + ohci_resume(hcd, hibernated); > > pm_runtime_disable(dev); > pm_runtime_set_active(dev); > @@ -309,6 +309,16 @@ static int ohci_platform_resume(struct device *dev) > > return 0; > } > + > +static int ohci_platform_resume(struct device *dev) > +{ > + return ohci_platform_renew(dev, false); > +} > + > +static int ohci_platform_restore(struct device *dev) > +{ > + return ohci_platform_renew(dev, true); > +} > #endif /* CONFIG_PM_SLEEP */ > > static const struct of_device_id ohci_platform_ids[] = { > @@ -325,8 +335,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 +353,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 >
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c index 0adae6265127..e733da2cd3b7 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, bool hibernated) { struct usb_hcd *hcd = dev_get_drvdata(dev); struct usb_ohci_pdata *pdata = dev_get_platdata(dev); @@ -301,7 +301,7 @@ static int ohci_platform_resume(struct device *dev) return err; } - ohci_resume(hcd, false); + ohci_resume(hcd, hibernated); pm_runtime_disable(dev); pm_runtime_set_active(dev); @@ -309,6 +309,16 @@ static int ohci_platform_resume(struct device *dev) return 0; } + +static int ohci_platform_resume(struct device *dev) +{ + return ohci_platform_renew(dev, false); +} + +static int ohci_platform_restore(struct device *dev) +{ + return ohci_platform_renew(dev, true); +} #endif /* CONFIG_PM_SLEEP */ static const struct of_device_id ohci_platform_ids[] = { @@ -325,8 +335,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 +353,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, }
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 "cd1965db0" and "6ec4beb5c" that 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> --- drivers/usb/host/ohci-platform.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)