> -----Original Message----- > From: Peter Chen <peter.chen@xxxxxxxxxx> > Sent: Wednesday, January 12, 2022 8:48 PM > To: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > Cc: Frank Li <frank.li@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jun Li > <jun.li@xxxxxxx>; Zhi Li <lznuaa@xxxxxxxxx> > Subject: Re: xhci crash at xhci_disable_hub_port_wake when system suspend. > > On 22-01-10 23:07:48, Mathias Nyman 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@xxxxxxxxxx > > >> 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? > > > > 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? > > Yes, it is no longer needed if xhci host is not runtime suspended. Looks like existing non xhci-plat user(xhci-tegra) is also calling xhci_resume() before calling xhci_suspend(), so it's safe to remove it. Li Jun > > -- > > Thanks, > Peter Chen
