Re: [PATCH] USB:Fix ehci infinite suspend-resume loop issue in zhaoxin

[WeitaoWang-oc@xxxxxxxxxxx]

On 2022/3/16 10:18, WeitaoWang-oc@xxxxxxxxxxx wrote:
> On 2022/3/15 11:18, Alan Stern wrote:
> > On Tue, Mar 15, 2022 at 08:39:09PM +0800, WeitaoWang-oc@xxxxxxxxxxx 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,
> > .