linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 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).