diff mbox series

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

Message ID 20210825105132.10420-3-kishon@ti.com (mailing list archive)
State Superseded
Headers show
Series Fix cold plugged USB device on certain PCIe USB cards | expand

Commit Message

Kishon Vijay Abraham I Aug. 25, 2021, 10:51 a.m. UTC 
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(-)

Comments

Alan Stern Aug. 25, 2021, 2:52 p.m. UTC | #1 
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
>
diff mbox series

Patch

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