On 12.5.2020 7.03, Peter Chen wrote:
>
>>
>> On 5/12/2020 8:05 AM, Peter Chen wrote:
>>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxx> Cc: Mathias Nyman
>>> <mathias.nyman@xxxxxxxxxxxxxxx> Cc: <stable@xxxxxxxxxxxxxxx>
>>> Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime
>>> PM") Reviewed-by: Peter Chen <peter.chen@xxxxxxx> Signed-off-by:
>>> Li Jun <jun.li@xxxxxxx> --- drivers/usb/host/xhci-plat.c | 1 + 1
>>> file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.c
>>> b/drivers/usb/host/xhci-plat.c index 1d4f6f85f0fe..f38d53528c96
>>> 100644 --- a/drivers/usb/host/xhci-plat.c +++
>>> b/drivers/usb/host/xhci-plat.c @@ -362,6 +362,7 @@ static int
>>> xhci_plat_remove(struct platform_device *dev) struct clk
>>> *reg_clk = xhci->reg_clk; struct usb_hcd *shared_hcd =
>>> xhci->shared_hcd;
>>>
>>> + pm_runtime_get_sync(&dev->dev);
>> Where is corresponding _put() call?
>>
>
> At the end of this function, there is a
> pm_runtime_set_suspended(&dev->dev). Calling
> pm_runtime_put_sync(&dev->dev) will cause xhci_plat_runtime_suspend
> is called which is not expected.
>
> Peter
>
This seems odd,
I don't understand why pm_runtime_set_suspended() would call pm_runtime_put_sync()?
I thought pm_runtime_set_suspended() is used to let pm core know the hardware is suspended,
and inform the parent of this, possibly allowing parent to runtime suspend.
Not sure if it's needed in the remove function.
It makes sense to allow the parent to suspend, but xhci isn't really suspended.
Yes is stopped, but we can't resume our way back to a active state.
Could it be that the runtime suspend call you are seeing is because we are removing all child devices,
disconnecting and freeing everything, including roothubs and hcd's.
Maybe runtime pm should be disabled a bit earlier.
Currently probe and remove looks like:
xhci_plat_probe()
pm_runtime_set_active()
pm_runtime_enable()
pm_runtime_get_noresume()
...
pm_runtime_put_noidle()
pm_runtime_forbid()
xhci_plat_remove()
<remove and put both hcd's>
pm_runtime_set_suspended()
pm_runtime_disable()
Would it make sense to change xhci_plat_remove() to
xhci_plat_remove()
pm_runtime_disable()
<remove and put both hcd's>
pm_runtime_set_suspended()
or possibly wrapping the remove in a runtime get/put:
xhci_plat_remove()
pm_runtime_get_noresume()
pm_runtime_disable()
<remove and put both hcd's>
pm_runtime_set_suspended()
pm_runtime_put_noidle()
-Mathias
