Fix cold plugged USB device on certain PCIe USB cards

PATCH Superseded Kishon Vijay Abraham I 3 participants 7 messages Aug 26, 2021

Subsystems (2)

USB USB xHCI driver

lore

Share & tools

lore

Kishon Vijay Abraham I

author

2021-08-26 11:14 UTC #0 lore raw

v2 cover Fix cold plugged USB device on certain PCIe USB cards

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/[email protected]
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]
v1 Patch series can be found at [3]

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.

Changes from v1:
1) Fixed code comments pointed out by Alan Stern
2) Renamed the HCD flag variable to "HCD_FLAG_DEFER_RH_REGISTER" from "HCD_FLAG_DEFER_PRI_RH_REGISTER"

[1] -> https://lore.kernel.org/linux-usb/[email protected]/
[2] -> https://lore.kernel.org/linux-usb/[email protected]
[3] -> https://lore.kernel.org/r/[email protected]

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  | 66 +++++++++++++++++++++++++++--------------
 drivers/usb/host/xhci.c |  1 +
 include/linux/usb/hcd.h |  2 ++
 3 files changed, 47 insertions(+), 22 deletions(-)

Signature · 1 line

2.17.1

Kishon Vijay Abraham I

author

2021-08-26 11:14 UTC #1 lore raw

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

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 <[email protected]>

Acked-by Alan Stern <[email protected]>

Tested-by Chris Chiu <[email protected]>

---
 drivers/usb/core/hcd.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

drivers/usb/core/hcd.c +22 ~~-17~~

index 0f8b7c93310e..00d35fe1fef0 100644
@@ -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 root-hub polling timer and invoke the HCD's ->stop callback.
+ */
+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)

Signature · 1 line

2.17.1

Kishon Vijay Abraham I

author

2021-08-26 11:14 UTC #2 lore raw

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

In reply to 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 <[email protected]>

Tested-by Chris Chiu <[email protected]>

---
 drivers/usb/core/hcd.c  | 27 ++++++++++++++++++++++-----
 include/linux/usb/hcd.h |  2 ++
 2 files changed, 24 insertions(+), 5 deletions(-)

drivers/usb/core/hcd.c +22 -5

index 00d35fe1fef0..64fdfc6692c2 100644
@@ -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);

@@ -2955,18 +2956,34 @@ int usb_add_hcd(struct usb_hcd *hcd,
 		goto err_hcd_driver_start;
 	}
 
+	/* starting here, usbcore will pay attention to the shared HCD roothub */
+	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 (shared_hcd->uses_new_polling && HCD_POLL_RH(shared_hcd))
+			usb_hcd_poll_rh_status(shared_hcd);
+	}
+
 	/* starting here, usbcore will pay attention to this root hub */
-	retval = register_root_hub(hcd);
-	if (retval != 0)
-		goto err_register_root_hub;
+	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);
+		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 && HCD_DEFER_RH_REGISTER(shared_hcd))
+		usb_stop_hcd(shared_hcd);
 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);

include/linux/usb/hcd.h +2 -0

index 548a028f2dab..2c1fc9212cf2 100644
@@ -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_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_RH_REGISTER))
 
 	/*
 	 * Specifies if interfaces are authorized by default

Signature · 1 line

2.17.1

Mathias Nyman

Acked-by on v2 2/3

2021-08-26 12:45 UTC #3 lore raw

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

Quoted 11 lines · 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 <[email protected]>
Suggested-by Mathias Nyman <[email protected]>
Tested-by Chris Chiu <[email protected]>

Acked-by Mathias Nyman <[email protected]>

Alan Stern

2021-08-26 15:16 UTC #4 lore raw

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

In reply to Kishon Vijay Abraham I

Quoted 8 lines · 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.

Quoted 4 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>
Suggested-by Mathias Nyman <[email protected]>
Tested-by Chris Chiu <[email protected]>
---

I should have noticed this problem before...

Quoted 43 lines

 drivers/usb/core/hcd.c  | 27 ++++++++++++++++++++++-----
 include/linux/usb/hcd.h |  2 ++
 2 files changed, 24 insertions(+), 5 deletions(-)

drivers/usb/core/hcd.c +17 -5

index 00d35fe1fef0..64fdfc6692c2 100644
@@ -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);

@@ -2955,18 +2956,34 @@ int usb_add_hcd(struct usb_hcd *hcd,
 		goto err_hcd_driver_start;
 	}
 
+	/* starting here, usbcore will pay attention to the shared HCD roothub */
+	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 (shared_hcd->uses_new_polling && HCD_POLL_RH(shared_hcd))
+			usb_hcd_poll_rh_status(shared_hcd);
+	}
+
 	/* starting here, usbcore will pay attention to this root hub */
-	retval = register_root_hub(hcd);
-	if (retval != 0)
-		goto err_register_root_hub;
+	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);
+		if (hcd->uses_new_polling && HCD_POLL_RH(hcd))

drivers/usb/core/hcd.c int usb_add_hcd(struct usb_hcd *hcd, · line 2986

Quoted 10 lines

drivers/usb/core/hcd.c +5 -0

+			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 && HCD_DEFER_RH_REGISTER(shared_hcd))
+		usb_stop_hcd(shared_hcd);

This was added in the wrong place. It needs to come before the usb_stop_hcd(hcd) call, because usb_start_hcd(hcd) was run before we failed to register the shared hcd's root hub.

Furthermore, I don't think we need to stop the shared hcd at all. At this point, the usb_add_hcd() call for the shared hcd has already returned successfully. The usb_stop_hcd(shared_hcd) callback will be made automatically when the shared hcd is unregistered later on, after this function returns an error.

Come to think of it, don't we also need to modify the usb_disconnect(&rhdev) call in usb_remove_hcd() to handle the case where the root hub wasn't registered in the first place because it was deferred? At the very least, we should test the hcd's rh_registered flag before trying to unregister the root hub.

Alan Stern

Quoted 26 lines

drivers/usb/core/hcd.c +0 -0

 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);

include/linux/usb/hcd.h +2 -0

index 548a028f2dab..2c1fc9212cf2 100644
@@ -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_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_RH_REGISTER))
 
 	/*
 	 * Specifies if interfaces are authorized by default

Signature · 1 line

2.17.1

Kishon Vijay Abraham I

author

2021-08-26 11:14 UTC #5 lore raw

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

In reply to Kishon Vijay Abraham I

Set "HCD_FLAG_DEFER_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 <[email protected]>

Tested-by Chris Chiu <[email protected]>

---
 drivers/usb/host/xhci.c | 1 +
 1 file changed, 1 insertion(+)

drivers/usb/host/xhci.c +1 -0

index 3618070eba78..7176058c7f3e 100644
@@ -692,6 +692,7 @@ int xhci_run(struct usb_hcd *hcd)
 		if (ret)
 			xhci_free_command(xhci, command);
 	}
+	set_bit(HCD_FLAG_DEFER_RH_REGISTER, &hcd->flags);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"Finished xhci_run for USB2 roothub");

Signature · 1 line

2.17.1

Mathias Nyman

Acked-by on v2 3/3

2021-08-26 12:43 UTC #6 lore raw

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

Quoted 11 lines · Kishon Vijay Abraham I

Set "HCD_FLAG_DEFER_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 <[email protected]>
Suggested-by Mathias Nyman <[email protected]>
Tested-by Chris Chiu <[email protected]>
---

Acked-by Mathias Nyman <[email protected]>