[PATCH 0/3] Fix cold plugged USB device on certain PCIe USB cards

[PATCH 0/3] Fix cold plugged USB device on certain PCIe USB cards

[Kishon Vijay Abraham I]

Cold plugged USB device was not detected on certain PCIe USB cards
(like Inateck card connected to AM64 EVM or connected to J7200 EVM).

Re-plugging the USB device always gets it enumerated.

This issue was discussed in
https://lore.kernel.org/r/772e4001-178e-4918-032c-6e625bdded24@ti.com
and
https://bugzilla.kernel.org/show_bug.cgi?id=214021

So the suggested solution is to register both root hubs along with the
second hcd for xhci.

RFC Patch series can be found at [1]

Changes from RFC:
1) Mathias identified potential issues with the RFC patch [2] and suggested
   the solution to use HCD flags. This series implements it.

[1] -> https://lore.kernel.org/linux-usb/20210824105302.25382-1-kishon@ti.com/ 
[2] -> https://lore.kernel.org/linux-usb/06693934-28f2-d59e-b004-62cabd3f9e8e@linux.intel.com

Kishon Vijay Abraham I (3):
  usb: core: hcd: Modularize HCD stop configuration in usb_stop_hcd()
  usb: core: hcd: Add support for deferring roothub registration
  xhci: Set HCD flag to defer primary roothub registration

 drivers/usb/core/hcd.c  | 67 +++++++++++++++++++++++++++--------------
 drivers/usb/host/xhci.c |  2 +-
 include/linux/usb/hcd.h |  2 ++
 3 files changed, 48 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH 1/3] usb: core: hcd: Modularize HCD stop configuration in usb_stop_hcd()

[Kishon Vijay Abraham I]

No functional change. Since configuration to stop HCD is invoked from
multiple places, group all of them in usb_stop_hcd().

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/core/hcd.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 0f8b7c93310e..775f0456f0ad 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2760,6 +2760,26 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
 	usb_put_dev(rhdev);
 }
 
+/**
+ * usb_stop_hcd - Halt the HCD
+ * @hcd: the usb_hcd that has to be halted
+ *
+ * Stop the timer and invoke ->stop() callback on the HCD
+ */
+static void usb_stop_hcd(struct usb_hcd *hcd)
+{
+	hcd->rh_pollable = 0;
+	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	del_timer_sync(&hcd->rh_timer);
+
+	hcd->driver->stop(hcd);
+	hcd->state = HC_STATE_HALT;
+
+	/* In case the HCD restarted the timer, stop it again. */
+	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	del_timer_sync(&hcd->rh_timer);
+}
+
 /**
  * usb_add_hcd - finish generic HCD structure initialization and register
  * @hcd: the usb_hcd structure to initialize
@@ -2946,13 +2966,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	return retval;
 
 err_register_root_hub:
-	hcd->rh_pollable = 0;
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
-	hcd->driver->stop(hcd);
-	hcd->state = HC_STATE_HALT;
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
+	usb_stop_hcd(hcd);
 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
@@ -3022,16 +3036,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	 * interrupt occurs), but usb_hcd_poll_rh_status() won't invoke
 	 * the hub_status_data() callback.
 	 */
-	hcd->rh_pollable = 0;
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
-
-	hcd->driver->stop(hcd);
-	hcd->state = HC_STATE_HALT;
-
-	/* In case the HCD restarted the timer, stop it again. */
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
+	usb_stop_hcd(hcd);
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		if (hcd->irq > 0)
-- 
2.17.1

Re: [PATCH 1/3] usb: core: hcd: Modularize HCD stop configuration in usb_stop_hcd()

[Alan Stern]

On Wed, Aug 25, 2021 at 04:21:30PM +0530, Kishon Vijay Abraham I wrote:
> No functional change. Since configuration to stop HCD is invoked from
> multiple places, group all of them in usb_stop_hcd().
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/usb/core/hcd.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 0f8b7c93310e..775f0456f0ad 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2760,6 +2760,26 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
>  	usb_put_dev(rhdev);
>  }
>  
> +/**
> + * usb_stop_hcd - Halt the HCD
> + * @hcd: the usb_hcd that has to be halted
> + *
> + * Stop the timer and invoke ->stop() callback on the HCD

This comment is now somewhat out of context.  It should be rephrased 
to say "Stop the root-hub polling timer and invoke the HCD's ->stop 
callback."

Apart from that,

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

> + */
> +static void usb_stop_hcd(struct usb_hcd *hcd)
> +{
> +	hcd->rh_pollable = 0;
> +	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> +	del_timer_sync(&hcd->rh_timer);
> +
> +	hcd->driver->stop(hcd);
> +	hcd->state = HC_STATE_HALT;
> +
> +	/* In case the HCD restarted the timer, stop it again. */
> +	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> +	del_timer_sync(&hcd->rh_timer);
> +}
> +
>  /**
>   * usb_add_hcd - finish generic HCD structure initialization and register
>   * @hcd: the usb_hcd structure to initialize
> @@ -2946,13 +2966,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	return retval;
>  
>  err_register_root_hub:
> -	hcd->rh_pollable = 0;
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
> -	hcd->driver->stop(hcd);
> -	hcd->state = HC_STATE_HALT;
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
> +	usb_stop_hcd(hcd);
>  err_hcd_driver_start:
>  	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
>  		free_irq(irqnum, hcd);
> @@ -3022,16 +3036,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
>  	 * interrupt occurs), but usb_hcd_poll_rh_status() won't invoke
>  	 * the hub_status_data() callback.
>  	 */
> -	hcd->rh_pollable = 0;
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
> -
> -	hcd->driver->stop(hcd);
> -	hcd->state = HC_STATE_HALT;
> -
> -	/* In case the HCD restarted the timer, stop it again. */
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
> +	usb_stop_hcd(hcd);
>  
>  	if (usb_hcd_is_primary_hcd(hcd)) {
>  		if (hcd->irq > 0)
> -- 
> 2.17.1
> 

[PATCH 2/3] usb: core: hcd: Add support for deferring roothub registration

[Kishon Vijay Abraham I]

It has been observed with certain PCIe USB cards (like Inateck connected
to AM64 EVM or J7200 EVM) that as soon as the primary roothub is
registered, port status change is handled even before xHC is running
leading to cold plug USB devices not detected. For such cases, registering
both the root hubs along with the second HCD is required. Add support for
deferring roothub registration in usb_add_hcd(), so that both primary and
secondary roothubs are registered along with the second HCD.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hcd.c  | 28 +++++++++++++++++++++++-----
 include/linux/usb/hcd.h |  2 ++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 775f0456f0ad..ba0493d22d13 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2795,6 +2795,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 {
 	int retval;
 	struct usb_device *rhdev;
+	struct usb_hcd *shared_hcd;
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
 		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
@@ -2956,17 +2957,34 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 
 	/* starting here, usbcore will pay attention to this root hub */
-	retval = register_root_hub(hcd);
-	if (retval != 0)
-		goto err_register_root_hub;
+	shared_hcd = hcd->shared_hcd;
+	if (!usb_hcd_is_primary_hcd(hcd) && shared_hcd &&
+	    HCD_DEFER_RH_REGISTER(shared_hcd)) {
+		retval = register_root_hub(shared_hcd);
+		if (retval != 0)
+			goto err_register_shared_root_hub;
 
-	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
-		usb_hcd_poll_rh_status(hcd);
+		if (shared_hcd->uses_new_polling && HCD_POLL_RH(shared_hcd))
+			usb_hcd_poll_rh_status(shared_hcd);
+	}
+
+	if (!HCD_DEFER_RH_REGISTER(hcd)) {
+		retval = register_root_hub(hcd);
+		if (retval != 0)
+			goto err_register_root_hub;
+
+		if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
+			usb_hcd_poll_rh_status(hcd);
+	}
 
 	return retval;
 
 err_register_root_hub:
 	usb_stop_hcd(hcd);
+err_register_shared_root_hub:
+	if (!usb_hcd_is_primary_hcd(hcd) && shared_hcd &&
+	    shared_hcd->flags & HCD_FLAG_DEFER_PRI_RH_REGISTER)
+		usb_stop_hcd(shared_hcd);
 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 548a028f2dab..6a357ba72f5d 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -124,6 +124,7 @@ struct usb_hcd {
 #define HCD_FLAG_RH_RUNNING		5	/* root hub is running? */
 #define HCD_FLAG_DEAD			6	/* controller has died? */
 #define HCD_FLAG_INTF_AUTHORIZED	7	/* authorize interfaces? */
+#define HCD_FLAG_DEFER_PRI_RH_REGISTER	8	/* Defer roothub registration */
 
 	/* The flags can be tested using these macros; they are likely to
 	 * be slightly faster than test_bit().
@@ -134,6 +135,7 @@ struct usb_hcd {
 #define HCD_WAKEUP_PENDING(hcd)	((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
 #define HCD_RH_RUNNING(hcd)	((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
 #define HCD_DEAD(hcd)		((hcd)->flags & (1U << HCD_FLAG_DEAD))
+#define HCD_DEFER_RH_REGISTER(hcd) ((hcd)->flags & (1U << HCD_FLAG_DEFER_PRI_RH_REGISTER))
 
 	/*
 	 * Specifies if interfaces are authorized by default
-- 
2.17.1

Re: [PATCH 2/3] usb: core: hcd: Add support for deferring roothub registration

[Alan Stern]

On Wed, Aug 25, 2021 at 04:21:31PM +0530, Kishon Vijay Abraham I wrote:
> It has been observed with certain PCIe USB cards (like Inateck connected
> to AM64 EVM or J7200 EVM) that as soon as the primary roothub is
> registered, port status change is handled even before xHC is running
> leading to cold plug USB devices not detected. For such cases, registering
> both the root hubs along with the second HCD is required. Add support for
> deferring roothub registration in usb_add_hcd(), so that both primary and
> secondary roothubs are registered along with the second HCD.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/core/hcd.c  | 28 +++++++++++++++++++++++-----
>  include/linux/usb/hcd.h |  2 ++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 775f0456f0ad..ba0493d22d13 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2795,6 +2795,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  {
>  	int retval;
>  	struct usb_device *rhdev;
> +	struct usb_hcd *shared_hcd;
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
>  		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> @@ -2956,17 +2957,34 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	/* starting here, usbcore will pay attention to this root hub */

That comment is now wrong.  This is the point in the code where 
usbcore will start paying attention to the _shared_ (primary) hcd's 
roothub (if its HCD_DEFER_PRI_RH_REGISTER flag is set).

> -	retval = register_root_hub(hcd);
> -	if (retval != 0)
> -		goto err_register_root_hub;
> +	shared_hcd = hcd->shared_hcd;
> +	if (!usb_hcd_is_primary_hcd(hcd) && shared_hcd &&
> +	    HCD_DEFER_RH_REGISTER(shared_hcd)) {
> +		retval = register_root_hub(shared_hcd);
> +		if (retval != 0)
> +			goto err_register_shared_root_hub;
>  
> -	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
> -		usb_hcd_poll_rh_status(hcd);
> +		if (shared_hcd->uses_new_polling && HCD_POLL_RH(shared_hcd))
> +			usb_hcd_poll_rh_status(shared_hcd);
> +	}
> +

_This_ is where usbcore will start paying attention to the hcd's own 
root hub (if its HCD_DEFER_PRI_RH_REGISTER flag isn't set).

> +	if (!HCD_DEFER_RH_REGISTER(hcd)) {
> +		retval = register_root_hub(hcd);
> +		if (retval != 0)
> +			goto err_register_root_hub;
> +
> +		if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
> +			usb_hcd_poll_rh_status(hcd);
> +	}
>  
>  	return retval;
>  
>  err_register_root_hub:
>  	usb_stop_hcd(hcd);
> +err_register_shared_root_hub:
> +	if (!usb_hcd_is_primary_hcd(hcd) && shared_hcd &&
> +	    shared_hcd->flags & HCD_FLAG_DEFER_PRI_RH_REGISTER)
> +		usb_stop_hcd(shared_hcd);
>  err_hcd_driver_start:
>  	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
>  		free_irq(irqnum, hcd);
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 548a028f2dab..6a357ba72f5d 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -124,6 +124,7 @@ struct usb_hcd {
>  #define HCD_FLAG_RH_RUNNING		5	/* root hub is running? */
>  #define HCD_FLAG_DEAD			6	/* controller has died? */
>  #define HCD_FLAG_INTF_AUTHORIZED	7	/* authorize interfaces? */
> +#define HCD_FLAG_DEFER_PRI_RH_REGISTER	8	/* Defer roothub registration */
>  
>  	/* The flags can be tested using these macros; they are likely to
>  	 * be slightly faster than test_bit().
> @@ -134,6 +135,7 @@ struct usb_hcd {
>  #define HCD_WAKEUP_PENDING(hcd)	((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
>  #define HCD_RH_RUNNING(hcd)	((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
>  #define HCD_DEAD(hcd)		((hcd)->flags & (1U << HCD_FLAG_DEAD))
> +#define HCD_DEFER_RH_REGISTER(hcd) ((hcd)->flags & (1U << HCD_FLAG_DEFER_PRI_RH_REGISTER))

It's awkward to have slightly different names for the flag and the 
test.  How about getting rid of the "_PRI" part of the flag name?  It 
isn't really needed, because we obviously won't defer registering the 
secondary hcd's root hub -- there's no place to defer it to.

Alan Stern

>  	 * Specifies if interfaces are authorized by default
> -- 
> 2.17.1
> 

[PATCH 3/3] xhci: Set HCD flag to defer primary roothub registration

[Kishon Vijay Abraham I]

Set "HCD_FLAG_DEFER_PRI_RH_REGISTER" to hcd->flags in xhci_run() to defer
registering primary roothub in usb_add_hcd(). This will make sure both
primary roothub and secondary roothub will be registered along with the
second HCD. This is required for cold plugged USB devices to be detected
in certain PCIe USB cards (like Inateck USB card connected to AM64 EVM
or J7200 EVM).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3618070eba78..9b7d968022c8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -644,7 +644,6 @@ int xhci_run(struct usb_hcd *hcd)
 	/* Start the xHCI host controller running only after the USB 2.0 roothub
 	 * is setup.
 	 */
-
 	hcd->uses_new_polling = 1;
 	if (!usb_hcd_is_primary_hcd(hcd))
 		return xhci_run_finished(xhci);
@@ -692,6 +691,7 @@ int xhci_run(struct usb_hcd *hcd)
 		if (ret)
 			xhci_free_command(xhci, command);
 	}
+	set_bit(HCD_FLAG_DEFER_PRI_RH_REGISTER, &hcd->flags);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"Finished xhci_run for USB2 roothub");
 
-- 
2.17.1

Re: [PATCH 3/3] xhci: Set HCD flag to defer primary roothub registration

[Kishon Vijay Abraham I]

Hi,

On 25/08/21 4:21 pm, Kishon Vijay Abraham I wrote:
> Set "HCD_FLAG_DEFER_PRI_RH_REGISTER" to hcd->flags in xhci_run() to defer
> registering primary roothub in usb_add_hcd(). This will make sure both
> primary roothub and secondary roothub will be registered along with the
> second HCD. This is required for cold plugged USB devices to be detected
> in certain PCIe USB cards (like Inateck USB card connected to AM64 EVM
> or J7200 EVM).
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 3618070eba78..9b7d968022c8 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -644,7 +644,6 @@ int xhci_run(struct usb_hcd *hcd)
>  	/* Start the xHCI host controller running only after the USB 2.0 roothub
>  	 * is setup.
>  	 */
> -

I just realized I've left a spurious space here. While fixing Alan's
comment, I'll fix this one.

Regards,
Kishon
>  	hcd->uses_new_polling = 1;
>  	if (!usb_hcd_is_primary_hcd(hcd))
>  		return xhci_run_finished(xhci);
> @@ -692,6 +691,7 @@ int xhci_run(struct usb_hcd *hcd)
>  		if (ret)
>  			xhci_free_command(xhci, command);
>  	}
> +	set_bit(HCD_FLAG_DEFER_PRI_RH_REGISTER, &hcd->flags);
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"Finished xhci_run for USB2 roothub");
>  
>