* xhci crash at xhci_disable_hub_port_wake when system suspend.
@ 2022-01-07 15:58 Frank Li
2022-01-10 12:33 ` Peter Chen
0 siblings, 1 reply; 6+ messages in thread
From: Frank Li @ 2022-01-07 15:58 UTC (permalink / raw)
To: linux-usb, mathias.nyman; +Cc: peter.chen, Jun Li, Zhi Li
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.
best regards
Frank Li
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: xhci crash at xhci_disable_hub_port_wake when system suspend. 2022-01-07 15:58 xhci crash at xhci_disable_hub_port_wake when system suspend Frank Li @ 2022-01-10 12:33 ` Peter Chen 2022-01-10 21:07 ` Mathias Nyman 0 siblings, 1 reply; 6+ messages in thread From: Peter Chen @ 2022-01-10 12:33 UTC (permalink / raw) To: Frank Li; +Cc: linux-usb, mathias.nyman, Jun Li, Zhi Li 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. 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. -- Thanks, Peter Chen ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: xhci crash at xhci_disable_hub_port_wake when system suspend. 2022-01-10 12:33 ` Peter Chen @ 2022-01-10 21:07 ` Mathias Nyman 2022-01-10 21:37 ` Zhi Li 2022-01-12 12:48 ` Peter Chen 0 siblings, 2 replies; 6+ messages in thread From: Mathias Nyman @ 2022-01-10 21:07 UTC (permalink / raw) To: Peter Chen, Frank Li; +Cc: linux-usb, Jun Li, Zhi Li 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? 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xhci crash at xhci_disable_hub_port_wake when system suspend. 2022-01-10 21:07 ` Mathias Nyman @ 2022-01-10 21:37 ` Zhi Li 2022-01-12 12:48 ` Peter Chen 1 sibling, 0 replies; 6+ messages in thread From: Zhi Li @ 2022-01-10 21:37 UTC (permalink / raw) To: Mathias Nyman; +Cc: Peter Chen, Frank Li, linux-usb, Jun Li 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xhci crash at xhci_disable_hub_port_wake when system suspend. 2022-01-10 21:07 ` Mathias Nyman 2022-01-10 21:37 ` Zhi Li @ 2022-01-12 12:48 ` Peter Chen 2022-01-12 13:36 ` Jun Li 1 sibling, 1 reply; 6+ messages in thread From: Peter Chen @ 2022-01-12 12:48 UTC (permalink / raw) To: Mathias Nyman; +Cc: Frank Li, linux-usb, Jun Li, Zhi Li 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@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? > > 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. -- Thanks, Peter Chen ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: xhci crash at xhci_disable_hub_port_wake when system suspend. 2022-01-12 12:48 ` Peter Chen @ 2022-01-12 13:36 ` Jun Li 0 siblings, 0 replies; 6+ messages in thread From: Jun Li @ 2022-01-12 13:36 UTC (permalink / raw) To: Peter Chen, Mathias Nyman; +Cc: Frank Li, linux-usb, Zhi Li > -----Original Message----- > From: Peter Chen <peter.chen@kernel.org> > Sent: Wednesday, January 12, 2022 8:48 PM > To: Mathias Nyman <mathias.nyman@linux.intel.com> > Cc: Frank Li <frank.li@nxp.com>; linux-usb@vger.kernel.org; Jun Li > <jun.li@nxp.com>; Zhi Li <lznuaa@gmail.com> > 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@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? > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-12 13:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-07 15:58 xhci crash at xhci_disable_hub_port_wake when system suspend Frank Li 2022-01-10 12:33 ` Peter Chen 2022-01-10 21:07 ` Mathias Nyman 2022-01-10 21:37 ` Zhi Li 2022-01-12 12:48 ` Peter Chen 2022-01-12 13:36 ` Jun Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).