Re: xhci crash at xhci_disable_hub_port_wake when system suspend.

[Peter Chen <peter.chen@kernel.org>]

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