From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:17
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
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:19
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 <redacted>
---
drivers/usb/core/hcd.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
@@ -2760,6 +2760,29 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)usb_put_dev(rhdev);}+/**+*usb_stop_hcd-HalttheHCD+*@hcd:theusb_hcdthathastobehalted+*+*Stopthetimerandinvoke->stop()callbackontheHCD+*/+staticvoidusb_stop_hcd(structusb_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-finishgenericHCDstructureinitializationandregister*@hcd:theusb_hcdstructuretoinitialize
@@ -2946,13 +2969,7 @@ int usb_add_hcd(struct usb_hcd *hcd,returnretval;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)*interruptoccurs),butusb_hcd_poll_rh_status()won'tinvoke*thehub_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)
On Tue, Aug 24, 2021 at 04:22:58PM +0530, Kishon Vijay Abraham I wrote:
quoted hunk
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 <redacted>
---
drivers/usb/core/hcd.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 15:19:02
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:
quoted
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 <redacted>
---
drivers/usb/core/hcd.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
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
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:
quoted
On Tue, Aug 24, 2021 at 04:22:58PM +0530, Kishon Vijay Abraham I wrote:
quoted
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 <redacted>
---
drivers/usb/core/hcd.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
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
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:21
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 <redacted>
---
drivers/usb/core/hcd.c | 20 +++++++++++---------
include/linux/usb/hcd.h | 8 ++++++--
2 files changed, 17 insertions(+), 11 deletions(-)
@@ -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)-gotoerr_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)+gotoerr_register_root_hub;+if(hcd->uses_new_polling&&HCD_POLL_RH(hcd))+usb_hcd_poll_rh_status(hcd);+}returnretval;
@@ -2988,7 +2990,7 @@ int usb_add_hcd(struct usb_hcd *hcd,returnretval;}-EXPORT_SYMBOL_GPL(usb_add_hcd);+EXPORT_SYMBOL_GPL(__usb_add_hcd);/***usb_remove_hcd-shutdownprocessingforgenericHCDs
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
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-24 14:56:20
On Tue, Aug 24, 2021 at 04:22:59PM +0530, Kishon Vijay Abraham I wrote:
quoted hunk
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 <redacted>
---
drivers/usb/core/hcd.c | 20 +++++++++++---------
include/linux/usb/hcd.h | 8 ++++++--
2 files changed, 17 insertions(+), 11 deletions(-)
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:25
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 <redacted>
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/core/hcd.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -2799,6 +2799,7 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla{intretval;structusb_device*rhdev;+structusb_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)+gotoerr_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)gotoerr_register_root_hub;
@@ -2972,6 +2982,8 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflaerr_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);
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 <redacted>
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/core/hcd.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -2799,6 +2799,7 @@ int __usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqfla{intretval;structusb_device*rhdev;+structusb_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
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:27
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 <redacted>
---
drivers/usb/core/hcd-pci.c | 11 ++++++-----
include/linux/usb/hcd.h | 8 +++++---
2 files changed, 11 insertions(+), 8 deletions(-)
@@ -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);returnretval;}-EXPORT_SYMBOL_GPL(usb_hcd_pci_probe);+EXPORT_SYMBOL_GPL(__usb_hcd_pci_probe);/* may be called without controller electrically present */
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
From: Kishon Vijay Abraham I <hidden> Date: 2021-08-24 10:53:49
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 <redacted>
---
drivers/usb/host/xhci-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)