Re: xhci crash at xhci_disable_hub_port_wake when system suspend.

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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