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

[RFC PATCH 0/5] 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. This series performs some cleanups and implements
the suggested solution.

Kishon Vijay Abraham I (5):
  usb: core: hcd: Modularize HCD stop configuration in usb_stop_hcd()
  usb: core: hcd: Let usb_add_hcd() indicate if roothub has to be
    registered
  usb: core: hcd: Add support for registering secondary RH along with
    primary HCD
  usb: core: hcd-pci: Let usb_hcd_pci_probe() indicate if RH has to be
    registered
  xhci-pci: Use flag to not register roothub while adding primary HCD

 drivers/usb/core/hcd-pci.c  | 11 +++---
 drivers/usb/core/hcd.c      | 72 ++++++++++++++++++++++++-------------
 drivers/usb/host/xhci-pci.c |  2 +-
 include/linux/usb/hcd.h     | 16 ++++++---
 4 files changed, 65 insertions(+), 36 deletions(-)

-- 
2.17.1

[RFC PATCH 1/5] 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 | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 0f8b7c93310e..c036ba5311b3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2760,6 +2760,29 @@ 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)
+{
+	if (!hcd)
+		return;
+
+	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 +2969,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 +3039,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

[RFC PATCH 2/5] usb: core: hcd: Let usb_add_hcd() indicate if roothub has to be registered

[Kishon Vijay Abraham I]

No functional change. Add __usb_add_hcd() which takes "register_hub"
flag that indicates if roothub has to be registered or not. This is in
preparation for allowing xhci to register roothub after the shared hcd
is created. The interface for usb_add_hcd() is not modified to make sure
there is no USB subsystem wide changes.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/core/hcd.c  | 20 +++++++++++---------
 include/linux/usb/hcd.h |  8 ++++++--
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index c036ba5311b3..4d7a9f0e2caa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2788,13 +2788,14 @@ static void usb_stop_hcd(struct usb_hcd *hcd)
  * @hcd: the usb_hcd structure to initialize
  * @irqnum: Interrupt line to allocate
  * @irqflags: Interrupt type flags
+ * @register_hub: Flag to indicate if roothub has to be registered.
  *
  * Finish the remaining parts of generic HCD initialization: allocate the
  * buffers of consistent memory, register the bus, request the IRQ line,
  * and call the driver's reset() and start() routines.
  */
-int usb_add_hcd(struct usb_hcd *hcd,
-		unsigned int irqnum, unsigned long irqflags)
+int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflags,
+		  bool register_hub)
 {
 	int retval;
 	struct usb_device *rhdev;
@@ -2959,12 +2960,13 @@ 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;
-
-	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
-		usb_hcd_poll_rh_status(hcd);
+	if (register_hub) {
+		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;
 
@@ -2988,7 +2990,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 	return retval;
 }
-EXPORT_SYMBOL_GPL(usb_add_hcd);
+EXPORT_SYMBOL_GPL(__usb_add_hcd);
 
 /**
  * usb_remove_hcd - shutdown processing for generic HCDs
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 548a028f2dab..2c99cfe20531 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -468,8 +468,8 @@ extern struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
 extern struct usb_hcd *usb_get_hcd(struct usb_hcd *hcd);
 extern void usb_put_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
-extern int usb_add_hcd(struct usb_hcd *hcd,
-		unsigned int irqnum, unsigned long irqflags);
+extern int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflags,
+			 bool register_hub);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
 int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
@@ -477,6 +477,10 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 
 struct platform_device;
 extern void usb_hcd_platform_shutdown(struct platform_device *dev);
+
+#define usb_add_hcd(hcd, irqnum, irqflags) \
+	__usb_add_hcd(hcd, irqnum, irqflags, true)
+
 #ifdef CONFIG_USB_HCD_TEST_MODE
 extern int ehset_single_step_set_feature(struct usb_hcd *hcd, int port);
 #else
-- 
2.17.1

[RFC PATCH 3/5] usb: core: hcd: Add support for registering secondary RH along with primary HCD

[Kishon Vijay Abraham I]

Add support for registering secondary roothub (RH) along with primary HCD.
It has been observed with certain PCIe USB cards that as soon as the
primary HCD is registered, port status change is handled leading to cold
plug devices getting not detected. For such cases, registering both the
root hubs along with the second HCD is useful.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/core/hcd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 4d7a9f0e2caa..9c8df22a7d9a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2799,6 +2799,7 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla
 {
 	int retval;
 	struct usb_device *rhdev;
+	struct usb_hcd *shared_hcd = NULL;
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
 		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
@@ -2961,6 +2962,15 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla
 
 	/* starting here, usbcore will pay attention to this root hub */
 	if (register_hub) {
+		shared_hcd = hcd->shared_hcd;
+		if (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);
+		}
+
 		retval = register_root_hub(hcd);
 		if (retval != 0)
 			goto err_register_root_hub;
@@ -2972,6 +2982,8 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla
 
 err_register_root_hub:
 	usb_stop_hcd(hcd);
+err_register_shared_root_hub:
+	usb_stop_hcd(shared_hcd);
 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
-- 
2.17.1

[RFC PATCH 4/5] usb: core: hcd-pci: Let usb_hcd_pci_probe() indicate if RH has to be registered

[Kishon Vijay Abraham I]

No functional change. Add __usb_hcd_pci_probe() which takes "register_hub"
flag that indicates if roothub (RH) has to be registered or not. This is in
preparation for allowing xhci-pci to register roothub after the shared hcd
is created. The interface for usb_hcd_pci_probe() is not modified to make
sure there are minimal code changes.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/core/hcd-pci.c | 11 ++++++-----
 include/linux/usb/hcd.h    |  8 +++++---
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index d630cccd2e6e..65c1f9aee4d1 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -160,6 +160,7 @@ static void ehci_wait_for_companions(struct pci_dev *pdev, struct usb_hcd *hcd,
  * @dev: USB Host Controller being probed
  * @id: pci hotplug id connecting controller to HCD framework
  * @driver: USB HC driver handle
+ * @register_hub: Flag to indicate of roothub has to be registered or not
  *
  * Context: task context, might sleep
  *
@@ -171,8 +172,8 @@ static void ehci_wait_for_companions(struct pci_dev *pdev, struct usb_hcd *hcd,
  *
  * Return: 0 if successful.
  */
-int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
-		      const struct hc_driver *driver)
+int __usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
+			const struct hc_driver *driver, bool register_hub)
 {
 	struct usb_hcd		*hcd;
 	int			retval;
@@ -262,7 +263,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
 		down_write(&companions_rwsem);
 		dev_set_drvdata(&dev->dev, hcd);
 		for_each_companion(dev, hcd, ehci_pre_add);
-		retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
+		retval = __usb_add_hcd(hcd, hcd_irq, IRQF_SHARED, register_hub);
 		if (retval != 0)
 			dev_set_drvdata(&dev->dev, NULL);
 		for_each_companion(dev, hcd, ehci_post_add);
@@ -270,7 +271,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
 	} else {
 		down_read(&companions_rwsem);
 		dev_set_drvdata(&dev->dev, hcd);
-		retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
+		retval = __usb_add_hcd(hcd, hcd_irq, IRQF_SHARED, register_hub);
 		if (retval != 0)
 			dev_set_drvdata(&dev->dev, NULL);
 		else
@@ -296,7 +297,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
 	dev_err(&dev->dev, "init %s fail, %d\n", pci_name(dev), retval);
 	return retval;
 }
-EXPORT_SYMBOL_GPL(usb_hcd_pci_probe);
+EXPORT_SYMBOL_GPL(__usb_hcd_pci_probe);
 
 
 /* may be called without controller electrically present */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 2c99cfe20531..49c1a8d56b2c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -493,14 +493,16 @@ static inline int ehset_single_step_set_feature(struct usb_hcd *hcd, int port)
 #ifdef CONFIG_USB_PCI
 struct pci_dev;
 struct pci_device_id;
-extern int usb_hcd_pci_probe(struct pci_dev *dev,
-			     const struct pci_device_id *id,
-			     const struct hc_driver *driver);
+extern int __usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id,
+			       const struct hc_driver *driver, bool register_hub);
 extern void usb_hcd_pci_remove(struct pci_dev *dev);
 extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
 
 extern int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *dev);
 
+#define usb_hcd_pci_probe(dev, id, driver) \
+	__usb_hcd_pci_probe(dev, id, driver, true)
+
 #ifdef CONFIG_PM
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
-- 
2.17.1

[RFC PATCH 5/5] xhci-pci: Use flag to not register roothub while adding primary HCD

[Kishon Vijay Abraham I]

Invoke __usb_hcd_pci_probe() without setting "register_hub" so that
primary roothub is not registered here. Instead it will be registered
along with secondary roothub. This is required for cold plugged USB
devices to be detected in certain PCIe USB cards (like Inateck USB
card).

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

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1c9a7957c45c..7734ff13aea9 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -397,7 +397,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	 * to say USB 2.0, but I'm not sure what the implications would be in
 	 * the other parts of the HCD code.
 	 */
-	retval = usb_hcd_pci_probe(dev, id, &xhci_pci_hc_driver);
+	retval = __usb_hcd_pci_probe(dev, id, &xhci_pci_hc_driver, 0);
 
 	if (retval)
 		goto put_runtime_pm;
-- 
2.17.1

Re: [RFC PATCH 3/5] usb: core: hcd: Add support for registering secondary RH along with primary HCD

[Mathias Nyman]

On 24.8.2021 13.53, Kishon Vijay Abraham I wrote:
> Add support for registering secondary roothub (RH) along with primary HCD.
> It has been observed with certain PCIe USB cards that as soon as the
> primary HCD is registered, port status change is handled leading to cold
> plug devices getting not detected. For such cases, registering both the
> root hubs along with the second HCD is useful.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> ---
>  drivers/usb/core/hcd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 4d7a9f0e2caa..9c8df22a7d9a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2799,6 +2799,7 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla
>  {
>  	int retval;
>  	struct usb_device *rhdev;
> +	struct usb_hcd *shared_hcd = NULL;
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
>  		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> @@ -2961,6 +2962,15 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla
>  
>  	/* starting here, usbcore will pay attention to this root hub */
>  	if (register_hub) {
> +		shared_hcd = hcd->shared_hcd;
> +		if (shared_hcd) {
> +			retval = register_root_hub(shared_hcd);

There is a possibility we try yo register the shared roothub before it is properly set up here.

For example the mediatek driver (xhci-mtk.c) creates both hcds before adding them,
so hcd->shared_hcd exists when usb_add_hcd() is called for the primary hcd,
causing this code to register the hcd->shared_hcd roothub which is not properly added yet.

How about skipping the new __usb_hcd_pci_probe() and __usb_add_hcd() and instead add a new
flag to hcd->flags, something like HCD_FLAG_DEFER_PRI_RH_REGISTER?

The host controller driver can set this flag in the hcd->driver->start(hcd) callback called
before roothub registration here from usb_add_hcd(). If flag is set we skip the roothub registration.

So something like:
shared_hcd = hcd->share_hcd;

if (!usb_hcd_is_primary_hcd(hcd) && shared_hcd && shared_hcd->flags & HCD_FLAG_DEFER_PRI_RH_REGISTER)
        register_root_hub(shared_hcd)
if (!(hcd->flags & HCD_FLAG_DEFER_PRI_RH_REGISTER))
        register_root_hub(hcd)

-Mathias 

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

[Greg Kroah-Hartman]

On Tue, Aug 24, 2021 at 04:22:58PM +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 | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 0f8b7c93310e..c036ba5311b3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2760,6 +2760,29 @@ 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)
> +{
> +	if (!hcd)
> +		return;

That's impossible to hit, so no need to check for it, right?

thanks,

greg k-h

Re: [RFC PATCH 2/5] usb: core: hcd: Let usb_add_hcd() indicate if roothub has to be registered

[Greg Kroah-Hartman]

On Tue, Aug 24, 2021 at 04:22:59PM +0530, Kishon Vijay Abraham I wrote:
> No functional change. Add __usb_add_hcd() which takes "register_hub"
> flag that indicates if roothub has to be registered or not. This is in

"flags" like this are horrid, never do that, except at a local static
function level that might make code easier.

For this, make a usb_hcd_add_and_register() function, so that you know
instantly when seeing it be called, as to what it does.  Otherwise you
have to go look up a random boolean value and that's impossible to
remember over time.

thanks,

greg k-h

Re: [RFC PATCH 4/5] usb: core: hcd-pci: Let usb_hcd_pci_probe() indicate if RH has to be registered

[Greg Kroah-Hartman]

On Tue, Aug 24, 2021 at 04:23:01PM +0530, Kishon Vijay Abraham I wrote:
> No functional change. Add __usb_hcd_pci_probe() which takes "register_hub"
> flag that indicates if roothub (RH) has to be registered or not. This is in
> preparation for allowing xhci-pci to register roothub after the shared hcd
> is created. The interface for usb_hcd_pci_probe() is not modified to make
> sure there are minimal code changes.

Same "add a flag" comment here, don't do that, make the function name
obvious please.

thanks,

greg k-h

Re: [RFC PATCH 2/5] usb: core: hcd: Let usb_add_hcd() indicate if roothub has to be registered

[Alan Stern]

On Tue, Aug 24, 2021 at 04:22:59PM +0530, Kishon Vijay Abraham I wrote:
> No functional change. Add __usb_add_hcd() which takes "register_hub"
> flag that indicates if roothub has to be registered or not. This is in
> preparation for allowing xhci to register roothub after the shared hcd
> is created. The interface for usb_add_hcd() is not modified to make sure
> there is no USB subsystem wide changes.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/usb/core/hcd.c  | 20 +++++++++++---------
>  include/linux/usb/hcd.h |  8 ++++++--
>  2 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index c036ba5311b3..4d7a9f0e2caa 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2788,13 +2788,14 @@ static void usb_stop_hcd(struct usb_hcd *hcd)
>   * @hcd: the usb_hcd structure to initialize
>   * @irqnum: Interrupt line to allocate
>   * @irqflags: Interrupt type flags
> + * @register_hub: Flag to indicate if roothub has to be registered.
>   *
>   * Finish the remaining parts of generic HCD initialization: allocate the
>   * buffers of consistent memory, register the bus, request the IRQ line,
>   * and call the driver's reset() and start() routines.
>   */
> -int usb_add_hcd(struct usb_hcd *hcd,
> -		unsigned int irqnum, unsigned long irqflags)
> +int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflags,
> +		  bool register_hub)

For future reference: When you change the name of a function, be 
sure to update the name in the kerneldoc comment as well.

Alan Stern

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

[Kishon Vijay Abraham I]

Hi Greg,

On 24/08/21 6:36 pm, Greg Kroah-Hartman wrote:
> On Tue, Aug 24, 2021 at 04:22:58PM +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 | 42 +++++++++++++++++++++++++-----------------
>>  1 file changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 0f8b7c93310e..c036ba5311b3 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2760,6 +2760,29 @@ 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)
>> +{
>> +	if (!hcd)
>> +		return;
> 
> That's impossible to hit, so no need to check for it, right?

Patch 3 of this series adds support for registering roothub of shared
HCD. So after that patch there can be a case where shared_hcd is NULL.
The other option would be to check for non-null value in hcd and then
invoke usb_stop_hcd().

Thanks
Kishon

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

[Greg Kroah-Hartman]

On Tue, Aug 24, 2021 at 08:48:45PM +0530, Kishon Vijay Abraham I wrote:
> Hi Greg,
> 
> On 24/08/21 6:36 pm, Greg Kroah-Hartman wrote:
> > On Tue, Aug 24, 2021 at 04:22:58PM +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 | 42 +++++++++++++++++++++++++-----------------
> >>  1 file changed, 25 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >> index 0f8b7c93310e..c036ba5311b3 100644
> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -2760,6 +2760,29 @@ 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)
> >> +{
> >> +	if (!hcd)
> >> +		return;
> > 
> > That's impossible to hit, so no need to check for it, right?
> 
> Patch 3 of this series adds support for registering roothub of shared
> HCD. So after that patch there can be a case where shared_hcd is NULL.
> The other option would be to check for non-null value in hcd and then
> invoke usb_stop_hcd().

Then add the check when you need it please.

thanks,

greg k-h