Re: xhci crash at xhci_disable_hub_port_wake when system suspend.

[Zhi Li <lznuaa@gmail.com>]

On Mon, Jan 10, 2022 at 3:06 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 10.1.2022 14.33, Peter Chen wrote:
> > On 22-01-07 15:58:26, Frank Li wrote:
> >> Mathias Nyman
> >>
> >>      Recently we found a crash at xhci_disable_hub_port_wake when system suspend if enable remote wake up.
> >>
> >>      Basial flow is.
> >>
> >>      1. run time suspend call xhci_suspend, xhci parent devices gate the clock.
> >>      2. echo mem >/sys/power/state, system _device_suspend call xhci_suspend
> >>      3. xhci_suspend call xhci_disable_hub_port_wake, which access register, but clock already gated by run time pm.
> >>
> >>      Why find this issue now, that is because previous power domain driver will call run time resume before it. But the below commit remove it.
> >>
> >> c1df456d0f06eb9275c1cd4c66548fc5738ea428
> >> Author: Ulf Hansson ulf.hansson@linaro.org
> >> Date:   Thu Mar 4 20:28:43 2021 +0100
> >>
> >>     PM: domains: Don't runtime resume devices at genpd_prepare()
> >>
> >>
> >>
> >> According to HCD_FLAG_HW_ACCESSIBLE logic, xhci should not access hardware when second time call xhci_suspend without call xhci_resume.
> >>
> >>         xhci_disable_hub_port_wake(xhci, &xhci->usb3_rhub, do_wakeup);
> >>         xhci_disable_hub_port_wake(xhci, &xhci->usb2_rhub, do_wakeup);
> >>
> >>         if (!HCD_HW_ACCESSIBLE(hcd))
> >>                 return 0;
> >>
> >>         .....
> >>         clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> >>         clear_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);
> >>
> >> I am not sure if it is safe to move xhci_disable_hub_port_wake after HCD_HW_ACCESSIBLE check, Or need add additional run_time_resume before it.
>
> We probably need to runtime resume first in case we need to adjust the wakeup settings
>
> >
> > Frank, I prefer adding runtime resume at xhci-plat.c like below, let's see what
> > Mathias says.
> >
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index c1edcc9b13ce..47a5a10381a7 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -440,6 +440,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> >       ret = xhci_priv_suspend_quirk(hcd);
> >       if (ret)
> >               return ret;
> > +
> > +     if (pm_runtime_suspended(dev))
> > +             pm_runtime_resume(dev);
> >       /*
> >        * xhci_suspend() needs `do_wakeup` to know whether host is allowed
> >        * to do wakeup during suspend.
> >
>
> Yes, looks like a solution to me.
> Just checked that driver-api/pm/devices.rst also suggest calling
> pm_runtime_resume() in ->suspend callback if device needs to adjust wakeup
> capabilities.
>
> Frank Li, does this work for you?

Yes, patch already sent.

>
> Peter, if we now make sure xhci host is not runtime suspended at system suspend,
> does it mean that the !HCD_HW_ACCESSIBLE(hcd) check you added to xhci_suspend()
> is no longer needed?
>
> 18a367e8947d usb: xhci: omit duplicate actions when suspending a runtime suspended host
>
> Thanks
> -Mathias