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

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

[Peter Chen]

From: Li Jun <jun.li@nxp.com>

While remove host(e.g. for USB role switch from host to device), if
runtime pm is enabled by user, below oops are occurs at dwc3
and cdns3 platform. Keep the xhci-plat device being active during
remove host fixes them.

oops1:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000240
Mem abort info:
  ESR = 0x96000004
xhci-hcd xhci-hcd.1.auto: // Halt the HC
xhci-hcd xhci-hcd.1.auto: // Reset the HC
xhci-hcd xhci-hcd.1.auto: Wait for controller to be ready for doorbell rings
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000004
xhci-hcd xhci-hcd.1.auto: // Disabling event ring interrupts
  CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=00000001b7b18000
xhci-hcd xhci-hcd.1.auto: cleaning up memory
xhci-hcd xhci-hcd.1.auto: Freed event ring
xhci-hcd xhci-hcd.1.auto: Freed command ring
[0000000000000240] pgd=0000000000000000
Internal error: Oops: 96000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.4.3-00107-g64d454a-dirty #1219
Hardware name: FSL i.MX8MP EVK (DT)
Workqueue: pm pm_runtime_work
pstate: 60000005 (nZCv daif -PAN -UAO)
pc : xhci_suspend+0x34/0x698
lr : xhci_plat_runtime_suspend+0x2c/0x38
sp : ffff800011ddbbc0
x29: ffff800011ddbbc0 x28: 0000000000000000
x27: 0000000000000008 x26: ffff800011b28000
x25: ffff80001012b328 x24: ffff800011ddbd48
x23: 0000000000000000 x22: ffff80001076ed78
x21: ffff000177896000 x20: ffff0001714ebce4
x19: ffff000177896000 x18: ffffffffffffffff
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800011b288c8 x14: 0000000000000261
x13: 0000000000000001 x12: 0000000000000001
x11: 0000000000000000 x10: 00000000000009c0
x9 : ffff800011ddbd40 x8 : fefefefefefefeff
x7 : 0000000000000000 x6 : 000000003ca92688
x5 : 00ffffffffffffff x4 : 001b6b0b00000000
x3 : ffff0001714ebc10 x2 : 0000000000000000
x1 : 0000000000000001 x0 : ffff000177896250
Call trace:
 xhci_suspend+0x34/0x698
 xhci_plat_runtime_suspend+0x2c/0x38
 pm_generic_runtime_suspend+0x28/0x40
 __rpm_callback+0xd8/0x138
 rpm_callback+0x24/0x98
 rpm_suspend+0xe0/0x448
 rpm_idle+0x124/0x140
 pm_runtime_work+0xa0/0xf8
 process_one_work+0x1dc/0x370
 worker_thread+0x48/0x468
 kthread+0xf0/0x120
 ret_from_fork+0x10/0x1c

oops2:
usb 2-1: USB disconnect, device number 2
xhci-hcd xhci-hcd.1.auto: remove, state 4
usb usb2: USB disconnect, device number 1
xhci-hcd xhci-hcd.1.auto: USB bus 2 deregistered
xhci-hcd xhci-hcd.1.auto: remove, state 4
usb usb1: USB disconnect, device number 1
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000138
Mem abort info:
  ESR = 0x96000004
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000004
  CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=00000008b05e8000
[0000000000000138] pgd=0000000000000000, p4d=0000000000000000
Internal error: Oops: 96000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 7 Comm: kworker/u8:0 Not tainted 5.6.0-rc4-next-20200304-03578-gd6235ff42e2b #101
Hardware name: Freescale i.MX8QXP MEK (DT)
Workqueue: 1-0050 tcpm_state_machine_work
pstate: 20000005 (nzCv daif -PAN -UAO)
pc : xhci_free_dev+0x214/0x270
lr : xhci_plat_runtime_resume+0x78/0x88
sp : ffff80001006b5b0
x29: ffff80001006b5b0 x28: 0000000000000002
x27: ffff00083b74fd48 x26: ffff00083a365580
x25: ffff800010141458 x24: 0000000000000000
x23: 0000000000000000 x22: ffff000837e58138
x21: 0000000000000004 x20: ffff000837e58000
x19: ffff000837e58250 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000001
x15: 0000000000000004 x14: ffffffffffffffff
x13: 0000000000004ed8 x12: ffff8000123d1000
x11: ffff800012158000 x10: ffff8000123d1360
x9 : ffff800010c74b00 x8 : 0000000000000007
x7 : 00000000000012c2 x6 : ffff8000123d1000
x5 : 0000000000000001 x4 : 0000000000000000
x3 : 0000000000000000 x2 : 0000000000000138
x1 : 0000000000000000 x0 : 0000000000000138
Call trace:
 xhci_free_dev+0x214/0x270
 xhci_plat_runtime_resume+0x78/0x88
 pm_generic_runtime_resume+0x30/0x48
 __rpm_callback+0x90/0x148
 rpm_callback+0x28/0x88
 rpm_resume+0x568/0x758
 rpm_resume+0x260/0x758
 rpm_resume+0x260/0x758
 __pm_runtime_resume+0x40/0x88
 device_release_driver_internal+0xa0/0x1c8
 device_release_driver+0x1c/0x28
 bus_remove_device+0xd4/0x158
 device_del+0x15c/0x3a0
 usb_disable_device+0xb0/0x268
 usb_disconnect+0xcc/0x300
 usb_remove_hcd+0xf4/0x1dc
 xhci_plat_remove+0x78/0xe0
 platform_drv_remove+0x30/0x50
 device_release_driver_internal+0xfc/0x1c8
 device_release_driver+0x1c/0x28
 bus_remove_device+0xd4/0x158
 device_del+0x15c/0x3a0
 platform_device_del.part.0+0x20/0x90
 platform_device_unregister+0x28/0x40
 cdns3_host_exit+0x20/0x40
 cdns3_role_stop+0x60/0x90
 cdns3_role_set+0x64/0xd8
 usb_role_switch_set_role.part.0+0x3c/0x68
 usb_role_switch_set_role+0x20/0x30
 tcpm_mux_set+0x60/0xf8
 tcpm_reset_port+0xa4/0xf0
 tcpm_detach.part.0+0x28/0x50
 tcpm_state_machine_work+0x12ac/0x2360
 process_one_work+0x1c8/0x470
 worker_thread+0x50/0x428
 kthread+0xfc/0x128
 ret_from_fork+0x10/0x18
Code: c8037c02 35ffffa3 17ffe7c3 f9800011 (c85f7c01)
---[ end trace 45b1a173d2679e44 ]---

Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: <stable@vger.kernel.org>
Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime PM")
Reviewed-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 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);
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
 	usb_remove_hcd(shared_hcd);
-- 
2.17.1

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

[Manu Gautam]

Hi,

On 5/12/2020 8:05 AM, Peter Chen wrote:
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime PM")
> Reviewed-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  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?


>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
>  	usb_remove_hcd(shared_hcd);

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Peter Chen]

 
> 
> On 5/12/2020 8:05 AM, Peter Chen wrote:
> > Cc: Baolin Wang <baolin.wang@linaro.org>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime PM")
> > Reviewed-by: Peter Chen <peter.chen@nxp.com>
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  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

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

[Alan Stern]

On Tue, 12 May 2020, Peter Chen wrote:

> From: Li Jun <jun.li@nxp.com>
> 
> While remove host(e.g. for USB role switch from host to device), if
> runtime pm is enabled by user, below oops are occurs at dwc3
> and cdns3 platform. Keep the xhci-plat device being active during
> remove host fixes them.

...

> 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);
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
>  	usb_remove_hcd(shared_hcd);

You mustn't add a pm_runtime_get call without a corresponding 
pm_runtime_put call.

With just this one call, if the role switched from host to device and 
then back to host, then the host would never be able to go into runtime 
suspend.

In this case the correspondence between the get's and the put's will 
probably be obscure; some comments would help.

Alan Stern

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

[Mathias Nyman]

On 12.5.2020 7.03, Peter Chen wrote:
> 
>> 
>> On 5/12/2020 8:05 AM, Peter Chen wrote:
>>> Cc: Baolin Wang <baolin.wang@linaro.org> Cc: Mathias Nyman 
>>> <mathias.nyman@linux.intel.com> Cc: <stable@vger.kernel.org> 
>>> Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime 
>>> PM") Reviewed-by: Peter Chen <peter.chen@nxp.com> Signed-off-by: 
>>> Li Jun <jun.li@nxp.com> --- 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 

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

[Peter Chen]

 
> >>
> >> On 5/12/2020 8:05 AM, Peter Chen wrote:
> >>> Cc: Baolin Wang <baolin.wang@linaro.org> Cc: Mathias Nyman
> >>> <mathias.nyman@linux.intel.com> Cc: <stable@vger.kernel.org>
> >>> Fixes: b0c69b4bace3 ("usb: host: plat: Enable xHCI plat runtime
> >>> PM") Reviewed-by: Peter Chen <peter.chen@nxp.com> Signed-off-by:
> >>> Li Jun <jun.li@nxp.com> --- 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.

Sorry, I may let you misunderstand. I mean call pm_runtime_set_suspended will
NOT call xhci_plat_runtime_suspend , and only let its parents be runtime suspend.
And if call pm_runtime_put_sync instead of pm_runtime_set_suspended , the
xhci_plat_runtime_suspend is called, although it doesn't cause any issues, but
hcd has been removed at the time, it is should not call xhci_suspend after that.

> 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.

This issue occurs plug out Type-C-to-A cable with USB3 device connected from Type-C
port. From what I see it most probably dues to disconnect event triggers roothub auto-suspend,
and this auto-suspend is async, if it is scheduled later than hcd is removed, this oops occurs.

Below are several logs I capture:

1. whole debug message when this issue occurs:

[   93.889693] xhci-hcd xhci-hcd.1.auto: remove, state 1
[   93.889911] xhci-hcd xhci-hcd.1.auto: Port change event, 2-1, id 2, portsc: 0x4202c0
[   93.894774] xhci-hcd xhci-hcd.1.auto: handle_port_status: starting port polling.
[   93.894830] xhci-hcd xhci-hcd.1.auto: roothub graceful disconnect
[   93.894853] usb usb2: USB disconnect, device number 1
[   93.896876] hub 2-0:1.0: state 0 ports 1 chg 0000 evt 0002
[   93.899940] usb 2-1: USB disconnect, device number 2
[   93.904985] usb 2-1: unregistering device
[   93.904996] usb 2-1: unregistering interface 2-1:1.0
[   93.954023] usb 2-1: usb_disable_device nuking all URBs
[   93.954050] xhci-hcd xhci-hcd.1.auto: xhci_drop_endpoint called for udev 00000000e6468cd7
[   93.954138] xhci-hcd xhci-hcd.1.auto: drop ep 0x81, slot id 1, new drop flags = 0x8, new add flags = 0x0
[   93.954146] xhci-hcd xhci-hcd.1.auto: xhci_drop_endpoint called for udev 00000000e6468cd7
[   93.954197] xhci-hcd xhci-hcd.1.auto: drop ep 0x2, slot id 1, new drop flags = 0x18, new add flags = 0x0
[   93.954652] usb usb2-port1: usb_disconnect: call pm_runtime_put
[   93.954786] xhci-hcd xhci-hcd.1.auto: // Ding dong!
[   93.954851] usb usb2: unregistering device
[   93.954863] usb usb2: unregistering interface 2-0:1.0
[   93.955252] usb usb2: usb_disable_device nuking all URBs
[   93.955264] xHCI xhci_drop_endpoint called for root hub
[   93.955269] xHCI xhci_check_bandwidth called for root hub
[   93.955901] xhci-hcd xhci-hcd.1.auto: usb_remove_hcd:3016
[   93.955914] xhci-hcd xhci-hcd.1.auto: USB bus 2 deregistered
[   93.966119] xhci-hcd xhci-hcd.1.auto: usb_remove_hcd:3037
[   93.966130] xhci-hcd xhci-hcd.1.auto: remove, state 4
[   93.971434] xhci-hcd xhci-hcd.1.auto: roothub graceful disconnect
[   93.971452] usb usb1: USB disconnect, device number 1
[   93.976783] xhci-hcd xhci-hcd.1.auto: at xhci_plat_runtime_suspend
[   93.976807] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000240
[   93.979340] usb usb1: unregistering device
[   93.985784] usb usb1: unregistering interface 1-0:1.0
[   93.986064] Mem abort info:
[   93.989013]   ESR = 0x96000004
[   93.992222]   EC = 0x25: DABT (current EL), IL = 32 bits
[   93.997735]   SET = 0, FnV = 0
[   94.000840]   EA = 0, S1PTW = 0
[   94.004125] Data abort info:
[   94.007220]   ISV = 0, ISS = 0x00000004
[   94.011255]   CM = 0, WnR = 0
[   94.014424] user pgtable: 4k pages, 48-bit VAs, pgdp=00000008b6dd3000
[   94.021025] [0000000000000240] pgd=0000000000000000, p4d=0000000000000000
[   94.028025] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[   94.033605] Modules linked in:
[   94.036677] CPU: 0 PID: 121 Comm: kworker/0:2 Not tainted 5.6.0-rc4-next-20200304-03579-gfe93ddfb2c17-dirty #120
[   94.047102] Hardware name: Freescale i.MX8QXP MEK (DT)
[   94.052261] Workqueue: pm pm_runtime_work
[   94.056280] pstate: 60000005 (nZCv daif -PAN -UAO)
[   94.061080] pc : xhci_suspend+0x30/0x698
[   94.065013] lr : xhci_plat_runtime_suspend+0x80/0x90
[   94.069976] sp : ffff80001243bbb0
[   94.073294] x29: ffff80001243bbb0 x28: ffff800011c56000 
[   94.078611] x27: 0000000000000008 x26: ffff80001243bd38 
[   94.083928] x25: ffff8000101412d8 x24: 0000000000000000 
[   94.089244] x23: 0000000000000000 x22: ffff000837e8e104 
[   94.094562] x21: ffff80001085d058 x20: ffff000836b42000 
[   94.099878] x19: ffff000836b42250 x18: 0000000000000010 
[   94.105194] x17: 0000000000000000 x16: 0000000000000000 
[   94.110512] x15: ffff00083ace0470 x14: 8810000000000000 
[   94.115828] x13: 0000000000001b58 x12: ffff800011ef1000 
[   94.121145] x11: ffff800011c78000 x10: ffff800011ef1360 
[   94.126462] x9 : ffff800010c73d28 x8 : 0000000000000007 
[   94.131778] x7 : 0000000000000531 x6 : ffff800011ef1000 
[   94.137095] x5 : 0000000000000000 x4 : ffff00083f99b1b8 
[   94.142412] x3 : ffff00083f9aa208 x2 : fd7e7f70d3ccaf00 
[   94.147729] x1 : 0000000000000001 x0 : 0000000000000000 
[   94.153044] Call trace:
[   94.155505]  xhci_suspend+0x30/0x698
[   94.159090]  xhci_plat_runtime_suspend+0x80/0x90
[   94.163722]  pm_generic_runtime_suspend+0x30/0x48
[   94.168437]  __rpm_callback+0x90/0x148
[   94.172189]  rpm_callback+0x28/0x88
[   94.175682]  rpm_suspend+0x100/0x5f0
[   94.179260]  rpm_idle+0x28c/0x420
[   94.182579]  pm_runtime_work+0xa0/0xc0
[   94.186334]  process_one_work+0x1c8/0x470
[   94.190345]  worker_thread+0x50/0x428
[   94.194013]  kthread+0xfc/0x128
[   94.197158]  ret_from_fork+0x10/0x18
[   94.200741] Code: 34001f00 7100101f 54003141 f9400660 (b9424000) 
[   94.206844] ---[ end trace 3ba7d185e60c149e ]---


2 & 3 are related debug message and trace message (I can't get the whole debug message due to
console is stuck when the issue occurs)

2.

soot@imx8qmmek:~# echo 500 > /sys/bus/usb/devices/usb2/power/autosuspend_delay_m 
usb/devices/usb2/power/autosuspend_delay_ms[  206.901805] xhci-hcd xhci-hcd.1.auto: remove, state 1
[  206.906941] usb usb2: USB disconnect, device number 1ower/autosuspend_delay_ms
[  206.958838] usb 2-1: USB disconnect, device number 2
[  206.994172] xhci-hcd xhci-hcd.1.auto: USB bus 2 deregistered
[  207.000017] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  207.005213] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000240
[  207.014082] usb usb1: USB disconnect, device number 1
[  207.014106] Mem abort info:
[  207.022005]   ESR = 0x96000004
[  207.025138]   EC = 0x25: DABT (current EL), IL = 32 bits
[  207.030510]   SET = 0, FnV = 0
[  207.033598]   EA = 0, S1PTW = 0
[  207.036777] Data abort info:
[  207.039699]   ISV = 0, ISS = 0x00000004
[  207.043569]   CM = 0, WnR = 0
[  207.046549] user pgtable: 4k pages, 48-bit VAs, pgdp=00000008b6f19000

3.

    kworker/0:5-411   [000] d..1   206.991613: rpm_idle: 2-1:1.0 flags-1 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.991626: rpm_return_int: rpm_idle+0xa8/0x420:2-1:1.0 ret=0
     kworker/0:5-411   [000] d..1   206.991629: rpm_suspend: 2-1:1.0 flags-9 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.991634: rpm_idle: 2-1 flags-1 cnt-2  dep-0  auto-0 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.991636: rpm_return_int: rpm_idle+0xa8/0x420:2-1 ret=-11
     kworker/0:5-411   [000] d..1   206.991638: rpm_return_int: rpm_suspend+0x174/0x5f0:2-1:1.0 ret=0
     kworker/0:5-411   [000] ....   206.991839: rpm_usage: 2-1 flags-c cnt-1  dep-0  auto-0 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.992523: rpm_resume: 2-1 flags-4 cnt-2  dep-0  auto-0 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.992534: rpm_return_int: rpm_resume+0x114/0x758:2-1 ret=1
     kworker/0:5-411   [000] ....   206.992548: rpm_usage: 2-1 flags-4 cnt-1  dep-0  auto-0 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.992618: rpm_idle: usb2 flags-1 cnt-0  dep-0  auto-1 p-0 irq-0 child-1
     kworker/0:5-411   [000] d..1   206.992627: rpm_return_int: rpm_idle+0xa8/0x420:usb2 ret=-16
     kworker/0:5-411   [000] ....   206.992690: rpm_usage: usb2-port1 flags-5 cnt-2  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] ....   206.992892: rpm_usage: usb2-port1 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.992924: rpm_idle: 2-0:1.0 flags-4 cnt-0  dep-0  auto-1 p-0 irq-0 child-1
     kworker/0:5-411   [000] d..1   206.992928: rpm_return_int: rpm_idle+0xa8/0x420:2-0:1.0 ret=0
     kworker/0:5-411   [000] d..1   206.992931: rpm_suspend: 2-0:1.0 flags-c cnt-0  dep-0  auto-1 p-0 irq-0 child-1
     kworker/0:5-411   [000] d..1   206.992936: rpm_idle: usb2 flags-1 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.992944: rpm_return_int: rpm_idle+0x2c8/0x420:usb2 ret=0
     kworker/0:5-411   [000] d..1   206.992946: rpm_return_int: rpm_suspend+0x174/0x5f0:2-0:1.0 ret=0
     kworker/0:5-411   [000] d..1   206.993033: rpm_idle: usb2 flags-2 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/0:5-411   [000] d..1   206.993038: rpm_return_int: rpm_idle+0xa8/0x420:usb2 ret=-16
    kworker/u8:5-409   [002] d..1   206.993112: rpm_resume: 2-0:1.0 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-1
    kworker/u8:5-409   [002] d..1   206.993118: rpm_idle: 2-0:1.0 flags-1 cnt-1  dep-0  auto-1 p-0 irq-0 child-1
    kworker/u8:5-409   [002] d..1   206.993121: rpm_return_int: rpm_idle+0xa8/0x420:2-0:1.0 ret=-11
    kworker/u8:5-409   [002] d..1   206.993123: rpm_return_int: rpm_resume+0x114/0x758:2-0:1.0 ret=1
    kworker/u8:5-409   [002] d..1   206.993135: rpm_idle: 2-0:1.0 flags-4 cnt-0  dep-0  auto-1 p-0 irq-0 child-1
    kworker/u8:5-409   [002] d..1   206.993138: rpm_return_int: rpm_idle+0xa8/0x420:2-0:1.0 ret=0
    kworker/u8:5-409   [002] d..1   206.993140: rpm_suspend: 2-0:1.0 flags-c cnt-0  dep-0  auto-1 p-0 irq-0 child-1
    kworker/u8:5-409   [002] d..1   206.993143: rpm_idle: usb2 flags-1 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [002] d..1   206.993159: rpm_return_int: rpm_idle+0x2c8/0x420:usb2 ret=0
    kworker/u8:5-409   [002] d..1   206.993161: rpm_return_int: rpm_suspend+0x174/0x5f0:2-0:1.0 ret=0
    kworker/u8:5-409   [002] d..1   206.993165: rpm_resume: usb2 flags-4 cnt-1  dep-0  auto-1 p-1 irq-0 child-0
    kworker/u8:5-409   [002] d..1   206.993167: rpm_return_int: rpm_resume+0x114/0x758:usb2 ret=1
    kworker/u8:5-409   [003] d..1   206.993257: rpm_resume: usb1-port1 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993261: rpm_return_int: rpm_resume+0x114/0x758:usb1-port1 ret=1
    kworker/u8:5-409   [003] d..1   206.993275: rpm_idle: usb2-port1 flags-5 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993288: rpm_return_int: rpm_idle+0x2c8/0x420:usb2-port1 ret=0
    kworker/u8:5-409   [003] d..1   206.993291: rpm_idle: usb1-port1 flags-5 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993295: rpm_return_int: rpm_idle+0x2c8/0x420:usb1-port1 ret=0
    kworker/u8:5-409   [003] d..1   206.993383: rpm_suspend: usb2 flags-c cnt-0  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993392: rpm_return_int: rpm_suspend+0x174/0x5f0:usb2 ret=0
     kworker/3:2-208   [003] d..1   206.993770: rpm_idle: usb1-port1 flags-2 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/3:2-208   [003] d..1   206.993775: rpm_return_int: rpm_idle+0xa8/0x420:usb1-port1 ret=0
     kworker/3:2-208   [003] d..1   206.993778: rpm_suspend: usb1-port1 flags-a cnt-0  dep-0  auto-1 p-0 irq-0 child-0
     kworker/3:2-208   [003] d..1   206.993785: rpm_return_int: rpm_suspend+0x174/0x5f0:usb1-port1 ret=-11
    kworker/u8:5-409   [003] d..1   206.993956: rpm_resume: usb2 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993964: rpm_return_int: rpm_resume+0x114/0x758:usb2 ret=1
    kworker/u8:5-409   [003] d..1   206.993977: rpm_idle: usb2 flags-4 cnt-0  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [003] d..1   206.993982: rpm_return_int: rpm_idle+0xa8/0x420:usb2 ret=-16
    kworker/u8:5-409   [003] d..1   206.994064: rpm_idle: xhci-hcd.1.auto flags-1 cnt-0  dep-0  auto-1 p-0 irq-0 child
-0
   
     kworker/3:2-208   [003] d..1   207.005157: rpm_idle: xhci-hcd.1.auto flags-2 cnt-0  dep-0  auto-1 p-0 irq-0 child
-0
     kworker/3:2-208   [003] d..1   207.005169: rpm_return_int: rpm_idle+0xa8/0x420:xhci-hcd.1.auto ret=0
     kworker/3:2-208   [003] d..1   207.005172: rpm_suspend: xhci-hcd.1.auto flags-a cnt-0  dep-0  auto-1 p-0 irq-0 ch
ild-0
    kworker/u8:5-409   [002] d..1   207.022142: rpm_resume: 1-0:1.0 flags-4 cnt-1  dep-0  auto-1 p-0 irq-0 child-1
    kworker/u8:5-409   [002] d..1   207.022153: rpm_resume: usb1 flags-0 cnt-1  dep-0  auto-1 p-0 irq-0 child-0
    kworker/u8:5-409   [002] d..1   207.022155: rpm_resume: xhci-hcd.1.auto flags-0 cnt-1  dep-0  auto-1 p-0 irq-0 chi
ld-0



> 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()
> 

I think this makes sense and tested at my platform.

Peter

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

[Peter Chen]

 
 
> > 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);
> >  	xhci->xhc_state |= XHCI_STATE_REMOVING;
> >
> >  	usb_remove_hcd(shared_hcd);
> 
> You mustn't add a pm_runtime_get call without a corresponding pm_runtime_put
> call.
> 
> With just this one call, if the role switched from host to device and then back to host,
> then the host would never be able to go into runtime suspend.
> 

I may not consider carefully for other cases, for my case, the xhci-plat device
will be removed, and re-created. But if we remove the driver using modprobe,
it may have issues.

> In this case the correspondence between the get's and the put's will probably be
> obscure; some comments would help.
> 

I explained at the reply for Mathias's, but I am not 100% it is the root cause.

Peter

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

[Jun Li]

​
...
> 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()

I think it's better to keep runtime active during driver removal,
how about this:

pm_runtime_get_sync()
<remove and put both hcd's>
pm_runtime_disable()
pm_runtime_put_noidle()
pm_runtime_set_suspended()

thanks
Li Jun

> -Mathias 

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

[Alan Stern]

On Wed, May 13, 2020 at 11:08:13AM +0000, Peter Chen wrote:
>  
>  
> > > 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);
> > >  	xhci->xhc_state |= XHCI_STATE_REMOVING;
> > >
> > >  	usb_remove_hcd(shared_hcd);
> > 
> > You mustn't add a pm_runtime_get call without a corresponding pm_runtime_put
> > call.
> > 
> > With just this one call, if the role switched from host to device and then back to host,
> > then the host would never be able to go into runtime suspend.
> > 
> 
> I may not consider carefully for other cases, for my case, the xhci-plat device
> will be removed, and re-created. But if we remove the driver using modprobe,
> it may have issues.
> 
> > In this case the correspondence between the get's and the put's will probably be
> > obscure; some comments would help.
> > 
> 
> I explained at the reply for Mathias's, but I am not 100% it is the root cause.

Okay, but there's something you may not know: pm_runtime_set_suspended() 
should be called _after_ pm_runtime_disable().  If you try to set the 
state to "suspended" while the device is enabled for runtime PM, the 
call may very well fail.

In this case, if all you want to do is block callbacks to the xHCI 
suspend routine, pm_runtime_disable() is probably all you need.

Alan Stern

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

[Peter Chen]

On 20-05-13 14:45:42, Jun Li wrote:
> ​
> ...
> > 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()
> 
> I think it's better to keep runtime active during driver removal,
> how about this:
> 
> pm_runtime_get_sync()
> <remove and put both hcd's>
> pm_runtime_disable()
> pm_runtime_put_noidle()
> pm_runtime_set_suspended()
> 

I think it is more reasonable since for some DRD controllers if
DRD core is suspended, access the xHCI register (eg, we remove
xhci-plat-hcd module at the time) may hang the system. Alan &
Mathias, what's your opinion?

-- 

Thanks,
Peter Chen

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

[Alan Stern]

On Thu, May 14, 2020 at 01:31:59AM +0000, Peter Chen wrote:
> On 20-05-13 14:45:42, Jun Li wrote:
> > ​
> > ...
> > > 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()
> > 
> > I think it's better to keep runtime active during driver removal,
> > how about this:
> > 
> > pm_runtime_get_sync()
> > <remove and put both hcd's>
> > pm_runtime_disable()
> > pm_runtime_put_noidle()
> > pm_runtime_set_suspended()
> > 
> 
> I think it is more reasonable since for some DRD controllers if
> DRD core is suspended, access the xHCI register (eg, we remove
> xhci-plat-hcd module at the time) may hang the system. Alan &
> Mathias, what's your opinion?

Jun's suggestion looks good to me.

Alan Stern

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

[Mathias Nyman]

On 14.5.2020 4.34, Alan Stern wrote:
> On Thu, May 14, 2020 at 01:31:59AM +0000, Peter Chen wrote:
>> On 20-05-13 14:45:42, Jun Li wrote:
>>> ​
>>> ...
>>>> 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()
>>>
>>> I think it's better to keep runtime active during driver removal,
>>> how about this:
>>>
>>> pm_runtime_get_sync()
>>> <remove and put both hcd's>
>>> pm_runtime_disable()
>>> pm_runtime_put_noidle()
>>> pm_runtime_set_suspended()
>>>
>>
>> I think it is more reasonable since for some DRD controllers if
>> DRD core is suspended, access the xHCI register (eg, we remove
>> xhci-plat-hcd module at the time) may hang the system. Alan &
>> Mathias, what's your opinion?

Makes sense to me

> 
> Jun's suggestion looks good to me.
> 
> Alan Stern
> 

Great, lets go with this then.
Jun, or Peter, could you turn this into a patch and check that it works?
I only got PCI xHC's to play with myself.

Thanks
-Mathias

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

[Peter Chen]

 
> >>>
> >>>> 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()
> >>>
> >>> I think it's better to keep runtime active during driver removal,
> >>> how about this:
> >>>
> >>> pm_runtime_get_sync()
> >>> <remove and put both hcd's>
> >>> pm_runtime_disable()
> >>> pm_runtime_put_noidle()
> >>> pm_runtime_set_suspended()
> >>>
> >>
> >> I think it is more reasonable since for some DRD controllers if DRD
> >> core is suspended, access the xHCI register (eg, we remove
> >> xhci-plat-hcd module at the time) may hang the system. Alan &
> >> Mathias, what's your opinion?
> 
> Makes sense to me
> 
> >
> > Jun's suggestion looks good to me.
> >
> > Alan Stern
> >
> 
> Great, lets go with this then.
> Jun, or Peter, could you turn this into a patch and check that it works?
> I only got PCI xHC's to play with myself.
> 
 
Jun, would you please create a patch for it. I have tested it at CDNS3 platform,
feel free add my tag.

Reviewed-by: Peter Chen <peter.chen@nxp.com>
Tested-by: Peter Chen <peter.chen@nxp.com>

Peter

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

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月14日 17:11
> To: Mathias Nyman <mathias.nyman@linux.intel.com>; Alan Stern
> <stern@rowland.harvard.edu>
> Cc: Jun Li <jun.li@nxp.com>; Manu Gautam <mgautam@codeaurora.org>;
> mathias.nyman@intel.com; linux-usb@vger.kernel.org; gregkh@linuxfoundation.org;
> dl-linux-imx <linux-imx@nxp.com>; Baolin Wang <baolin.wang@linaro.org>;
> stable@vger.kernel.org
> Subject: RE: 回复: [PATCH 1/1] usb: host: xhci-plat: keep runtime active when remove
> host
> 
> 
> > >>>
> > >>>> 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()
> > >>>
> > >>> I think it's better to keep runtime active during driver removal,
> > >>> how about this:
> > >>>
> > >>> pm_runtime_get_sync()
> > >>> <remove and put both hcd's>
> > >>> pm_runtime_disable()
> > >>> pm_runtime_put_noidle()
> > >>> pm_runtime_set_suspended()
> > >>>
> > >>
> > >> I think it is more reasonable since for some DRD controllers if DRD
> > >> core is suspended, access the xHCI register (eg, we remove
> > >> xhci-plat-hcd module at the time) may hang the system. Alan &
> > >> Mathias, what's your opinion?
> >
> > Makes sense to me
> >
> > >
> > > Jun's suggestion looks good to me.
> > >
> > > Alan Stern
> > >
> >
> > Great, lets go with this then.
> > Jun, or Peter, could you turn this into a patch and check that it works?
> > I only got PCI xHC's to play with myself.
> >
> 
> Jun, would you please create a patch for it. I have tested it at CDNS3 platform,
> feel free add my tag.
> 
> Reviewed-by: Peter Chen <peter.chen@nxp.com>
> Tested-by: Peter Chen <peter.chen@nxp.com>

Thanks, I will send out a v2 for this.

Li Jun

> 
> Peter