Messages in this thread | | | Date | Tue, 22 Mar 2022 21:52:14 +0800 | Subject | Re: [PATCH] USB:Fix ehci infinite suspend-resume loop issue in zhaoxin | From | "" <> |
| |
On 2022/3/16 10:18, WeitaoWang-oc@zhaoxin.com wrote: > On 2022/3/15 11:18, Alan Stern wrote: >> On Tue, Mar 15, 2022 at 08:39:09PM +0800, WeitaoWang-oc@zhaoxin.com wrote: >>> On 2022/3/14 10:24, Alan Stern wrote: >>>>> + t1 = ehci_readl(ehci, &ehci->regs->status); >>>>> + ehci_writel(ehci, t1 & STS_PCD, &ehci->regs->status); >>>>> + ehci_readl(ehci, &ehci->regs->status); >>>> >>>> You should not clear the STS_PCD bit. What if some other port had a >>>> status change at the same time? Then because you cleared the >>>> port-change-detect bit, the system would not realize that the other port >>>> needed to be handled. >>> >>> I really didn't think about this case. >>> >>>> Leaving the STS_PCD bit turned on will cause the driver to do a little >>>> extra work, but it shouldn't cause any harm. >>>> >>> I have encountered the following situation if EHCI runtime suspend is >>> enabled by default. >>> >>> >>> >>> 1.Wake from system to disk and boot OS. >> >> You're talking about resuming after hibernation, right? > > You're right. >>> 2.EHCI will entry runtime suspend after enumerated by driver during boot >>> phase of suspend to disk >> >> I'm not sure what you mean by "boot phase of suspend to disk". This is >> while the restore kernel is starting up at the beginning of resume from >> hibernation, right? >> > You understood exactly what I was saying. > > >>> 3.EHCI will be placed to freeze state and ehci_resume is called after image >>> is loaded. >> >> ehci_resume is called to leave runtime suspend. Going into the freeze >> state doesn't require any changes. >> >>> 4.If PCD flag is set(caused by patch), then HCD_FLAG_RH_RUNNING will be set. >>> >>> 5.Pci_pm_freeze_noirq is called to check ehci root hub state and return >>> value is -EBUSY. which will cause >>> quiesce phase of suspend to disk fail. >> >> You're talking about check_root_hub_suspended() in hcd-pci.c, right? >> > It's right. >> You know, I'm not at all certain that the callbacks for freeze and >> freeze_noirq should ever return anything other than 0. It's okay for >> them to call check_root_hub_suspended(), but they should ignore its >> return value. >> >> Can you check if the patch below helps? >> >> Alan Stern >> >> >> Index: usb-devel/drivers/usb/core/hcd-pci.c >> =================================================================== >> --- usb-devel.orig/drivers/usb/core/hcd-pci.c >> +++ usb-devel/drivers/usb/core/hcd-pci.c >> @@ -575,6 +575,12 @@ static int hcd_pci_resume(struct device >> return resume_common(dev, PM_EVENT_RESUME); >> } >> +static int hcd_pci_freeze_check(struct device *dev) >> +{ >> + (void) check_root_hub_suspended(dev); >> + return 0; >> +} >> + >> static int hcd_pci_restore(struct device *dev) >> { >> return resume_common(dev, PM_EVENT_RESTORE); >> @@ -586,6 +592,7 @@ static int hcd_pci_restore(struct device >> #define hcd_pci_suspend_noirq NULL >> #define hcd_pci_resume_noirq NULL >> #define hcd_pci_resume NULL >> +#define hcd_pci_freeze_check NULL >> #define hcd_pci_restore NULL >> #endif /* CONFIG_PM_SLEEP */ >> @@ -616,8 +623,8 @@ const struct dev_pm_ops usb_hcd_pci_pm_o >> .suspend_noirq = hcd_pci_suspend_noirq, >> .resume_noirq = hcd_pci_resume_noirq, >> .resume = hcd_pci_resume, >> - .freeze = check_root_hub_suspended, >> - .freeze_noirq = check_root_hub_suspended, >> + .freeze = hcd_pci_freeze_check, >> + .freeze_noirq = hcd_pci_freeze_check, > > This patch can fix pci driver's fail check in freeze_noirq phase. > > Restoring system state from a hibernation image can continue and success. > > Weitao Wang
This patch seems work for our platform, but I don't know if there will be side-effects for others. So I want to take your previous suggestion and not to clear PCD, As ehci runtime suspend enabled at OS startup is not a real requirement. I'll resubmit revised patch without the whitespace damage. Thanks for your great help.
Best Wishes Weitao Wang >> .thaw_noirq = NULL, >> .thaw = NULL, >> .poweroff = hcd_pci_suspend, >> . >>
| |