Re: [PATCH 1/1] usb: host: xhci-plat: keep runtime active when remove host

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux