| Message ID | 994d8963-ca4d-d4cb-a3f6-988d6aa9bcd7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: core: improve handling of hubs with no ports | expand |
On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: > I get the "hub doesn't have any ports" error message on a system with > Amlogic S905W SoC. Seems the SoC has internal USB 3.0 supports but > is crippled with regard to USB 3.0 ports. > Maybe we shouldn't consider this scenario an error. So let's change > the message to info level, but otherwise keep the handling of the > scenario as it is today. With the patch it looks like this on my > system. > > dwc2 c9100000.usb: supply vusb_d not found, using dummy regulator > dwc2 c9100000.usb: supply vusb_a not found, using dummy regulator > dwc2 c9100000.usb: EPs: 7, dedicated fifos, 712 entries in SPRAM > xhci-hcd xhci-hcd.0.auto: xHCI Host Controller > xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1 > xhci-hcd xhci-hcd.0.auto: hcc params 0x0228f664 hci version 0x100 quirks 0x0000000002010010 > xhci-hcd xhci-hcd.0.auto: irq 49, io mem 0xc9000000 > hub 1-0:1.0: USB hub found > hub 1-0:1.0: 2 ports detected > xhci-hcd xhci-hcd.0.auto: xHCI Host Controller > xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2 > xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed > usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > hub 2-0:1.0: USB hub found > hub 2-0:1.0: hub has no ports, exiting > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/usb/core/hub.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 83b5aff25..e3f40d1f4 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, > ret = -ENODEV; > goto fail; > } else if (hub->descriptor->bNbrPorts == 0) { > - message = "hub doesn't have any ports!"; > - ret = -ENODEV; > - goto fail; > + dev_info(hub_dev, "hub has no ports, exiting\n"); > + return -ENODEV; > } > > /* How about instead changing xhci-hcd so that it doesn't try to register a USB-3 root hub if the controller doesn't have any USB-3 ports? I think that would make more sense. Alan Stern
On 23.02.2022 03:10, Alan Stern wrote: > On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: >> I get the "hub doesn't have any ports" error message on a system with >> Amlogic S905W SoC. Seems the SoC has internal USB 3.0 supports but >> is crippled with regard to USB 3.0 ports. >> Maybe we shouldn't consider this scenario an error. So let's change >> the message to info level, but otherwise keep the handling of the >> scenario as it is today. With the patch it looks like this on my >> system. >> >> dwc2 c9100000.usb: supply vusb_d not found, using dummy regulator >> dwc2 c9100000.usb: supply vusb_a not found, using dummy regulator >> dwc2 c9100000.usb: EPs: 7, dedicated fifos, 712 entries in SPRAM >> xhci-hcd xhci-hcd.0.auto: xHCI Host Controller >> xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1 >> xhci-hcd xhci-hcd.0.auto: hcc params 0x0228f664 hci version 0x100 quirks 0x0000000002010010 >> xhci-hcd xhci-hcd.0.auto: irq 49, io mem 0xc9000000 >> hub 1-0:1.0: USB hub found >> hub 1-0:1.0: 2 ports detected >> xhci-hcd xhci-hcd.0.auto: xHCI Host Controller >> xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2 >> xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed >> usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. >> hub 2-0:1.0: USB hub found >> hub 2-0:1.0: hub has no ports, exiting >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/usb/core/hub.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index 83b5aff25..e3f40d1f4 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, >> ret = -ENODEV; >> goto fail; >> } else if (hub->descriptor->bNbrPorts == 0) { >> - message = "hub doesn't have any ports!"; >> - ret = -ENODEV; >> - goto fail; >> + dev_info(hub_dev, "hub has no ports, exiting\n"); >> + return -ENODEV; >> } >> >> /* > > How about instead changing xhci-hcd so that it doesn't try to register > a USB-3 root hub if the controller doesn't have any USB-3 ports? I > think that would make more sense. > Right, this would be better. I checked and it seems to be a little bit bigger endeavor. If I let register_root_hub() fail, then this removes the USB3 bus/host (shared hcd), but also the USB2 bus/host. It took an additional change to xhci_plat_probe() to make it work on my system. Not sure what the impact could be on systems not using xhci_plat_probe(). Users may face the same issue like me, and having a USB3 hub with no ports may remove also the USB2 bus/host. What I can do: submit my patches as RFC, then there's a better basis for a discussion. > Alan Stern Heiner
On Wed, Feb 23, 2022 at 01:26:23PM +0100, Heiner Kallweit wrote: > On 23.02.2022 03:10, Alan Stern wrote: > > On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: > >> > >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > >> index 83b5aff25..e3f40d1f4 100644 > >> --- a/drivers/usb/core/hub.c > >> +++ b/drivers/usb/core/hub.c > >> @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, > >> ret = -ENODEV; > >> goto fail; > >> } else if (hub->descriptor->bNbrPorts == 0) { > >> - message = "hub doesn't have any ports!"; > >> - ret = -ENODEV; > >> - goto fail; > >> + dev_info(hub_dev, "hub has no ports, exiting\n"); > >> + return -ENODEV; > >> } > >> > >> /* > > > > How about instead changing xhci-hcd so that it doesn't try to register > > a USB-3 root hub if the controller doesn't have any USB-3 ports? I > > think that would make more sense. > > > Right, this would be better. I checked and it seems to be a little bit > bigger endeavor. If I let register_root_hub() fail, then this removes > the USB3 bus/host (shared hcd), but also the USB2 bus/host. > It took an additional change to xhci_plat_probe() to make it work on my > system. Not sure what the impact could be on systems not using > xhci_plat_probe(). Users may face the same issue like me, and having > a USB3 hub with no ports may remove also the USB2 bus/host. Don't change register_root_hub(). Just change xhci_plat_probe(); make it skip the second call to usb_add_hcd() if there are no USB-3 ports. Alan Stern > What I can do: submit my patches as RFC, then there's a better basis > for a discussion. > > > Alan Stern > > Heiner
On 23.02.2022 15:17, Alan Stern wrote: > On Wed, Feb 23, 2022 at 01:26:23PM +0100, Heiner Kallweit wrote: >> On 23.02.2022 03:10, Alan Stern wrote: >>> On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: >>>> >>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >>>> index 83b5aff25..e3f40d1f4 100644 >>>> --- a/drivers/usb/core/hub.c >>>> +++ b/drivers/usb/core/hub.c >>>> @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, >>>> ret = -ENODEV; >>>> goto fail; >>>> } else if (hub->descriptor->bNbrPorts == 0) { >>>> - message = "hub doesn't have any ports!"; >>>> - ret = -ENODEV; >>>> - goto fail; >>>> + dev_info(hub_dev, "hub has no ports, exiting\n"); >>>> + return -ENODEV; >>>> } >>>> >>>> /* >>> >>> How about instead changing xhci-hcd so that it doesn't try to register >>> a USB-3 root hub if the controller doesn't have any USB-3 ports? I >>> think that would make more sense. >>> >> Right, this would be better. I checked and it seems to be a little bit >> bigger endeavor. If I let register_root_hub() fail, then this removes >> the USB3 bus/host (shared hcd), but also the USB2 bus/host. >> It took an additional change to xhci_plat_probe() to make it work on my >> system. Not sure what the impact could be on systems not using >> xhci_plat_probe(). Users may face the same issue like me, and having >> a USB3 hub with no ports may remove also the USB2 bus/host. > > Don't change register_root_hub(). Just change xhci_plat_probe(); make > it skip the second call to usb_add_hcd() if there are no USB-3 ports. > How would I know the number of USB-3 ports before calling usb_add_hcd()? get_hub_descriptor() can be called only later in usb_add_hcd(). > Alan Stern > >> What I can do: submit my patches as RFC, then there's a better basis >> for a discussion. >> >>> Alan Stern >> >> Heiner
On Wed, Feb 23, 2022 at 03:58:35PM +0100, Heiner Kallweit wrote: > On 23.02.2022 15:17, Alan Stern wrote: > > On Wed, Feb 23, 2022 at 01:26:23PM +0100, Heiner Kallweit wrote: > >> On 23.02.2022 03:10, Alan Stern wrote: > >>> On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: > >>>> > >>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > >>>> index 83b5aff25..e3f40d1f4 100644 > >>>> --- a/drivers/usb/core/hub.c > >>>> +++ b/drivers/usb/core/hub.c > >>>> @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, > >>>> ret = -ENODEV; > >>>> goto fail; > >>>> } else if (hub->descriptor->bNbrPorts == 0) { > >>>> - message = "hub doesn't have any ports!"; > >>>> - ret = -ENODEV; > >>>> - goto fail; > >>>> + dev_info(hub_dev, "hub has no ports, exiting\n"); > >>>> + return -ENODEV; > >>>> } > >>>> > >>>> /* > >>> > >>> How about instead changing xhci-hcd so that it doesn't try to register > >>> a USB-3 root hub if the controller doesn't have any USB-3 ports? I > >>> think that would make more sense. > >>> > >> Right, this would be better. I checked and it seems to be a little bit > >> bigger endeavor. If I let register_root_hub() fail, then this removes > >> the USB3 bus/host (shared hcd), but also the USB2 bus/host. > >> It took an additional change to xhci_plat_probe() to make it work on my > >> system. Not sure what the impact could be on systems not using > >> xhci_plat_probe(). Users may face the same issue like me, and having > >> a USB3 hub with no ports may remove also the USB2 bus/host. > > > > Don't change register_root_hub(). Just change xhci_plat_probe(); make > > it skip the second call to usb_add_hcd() if there are no USB-3 ports. > > > How would I know the number of USB-3 ports before calling usb_add_hcd()? > get_hub_descriptor() can be called only later in usb_add_hcd(). Where do you think get_hub_descriptor() gets that information from? It's stored in xhci->usb3_rhub.num_ports. (Now, because I'm not very familiar with the xhci-hcd driver, I can't tell you whether xhci->usb3_rhub.num_ports gets initialized before or after the usb_add_hcd() call for the shared_hcd. You'll have to figure that out yourself.) Alan Stern
On 23.02.2022 15:17, Alan Stern wrote: > On Wed, Feb 23, 2022 at 01:26:23PM +0100, Heiner Kallweit wrote: >> On 23.02.2022 03:10, Alan Stern wrote: >>> On Tue, Feb 22, 2022 at 10:13:09PM +0100, Heiner Kallweit wrote: >>>> >>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >>>> index 83b5aff25..e3f40d1f4 100644 >>>> --- a/drivers/usb/core/hub.c >>>> +++ b/drivers/usb/core/hub.c >>>> @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, >>>> ret = -ENODEV; >>>> goto fail; >>>> } else if (hub->descriptor->bNbrPorts == 0) { >>>> - message = "hub doesn't have any ports!"; >>>> - ret = -ENODEV; >>>> - goto fail; >>>> + dev_info(hub_dev, "hub has no ports, exiting\n"); >>>> + return -ENODEV; >>>> } >>>> >>>> /* >>> >>> How about instead changing xhci-hcd so that it doesn't try to register >>> a USB-3 root hub if the controller doesn't have any USB-3 ports? I >>> think that would make more sense. >>> >> Right, this would be better. I checked and it seems to be a little bit >> bigger endeavor. If I let register_root_hub() fail, then this removes >> the USB3 bus/host (shared hcd), but also the USB2 bus/host. >> It took an additional change to xhci_plat_probe() to make it work on my >> system. Not sure what the impact could be on systems not using >> xhci_plat_probe(). Users may face the same issue like me, and having >> a USB3 hub with no ports may remove also the USB2 bus/host. > > Don't change register_root_hub(). Just change xhci_plat_probe(); make > it skip the second call to usb_add_hcd() if there are no USB-3 ports. > This works on my system. However a consequence is that xhci->shared_hcd is NULL. There are a few places like the following in xhci.c where this may result in a NPE. Not knowing the USB subsystem in detail I can't say whether these places are in any relevant path. static int xhci_run_finished(struct xhci_hcd *xhci) { if (xhci_start(xhci)) { xhci_halt(xhci); return -ENODEV; } xhci->shared_hcd->state = HC_STATE_RUNNING; > Alan Stern > >> What I can do: submit my patches as RFC, then there's a better basis >> for a discussion. >> >>> Alan Stern >> >> Heiner Heiner
On Wed, Feb 23, 2022 at 09:58:56PM +0100, Heiner Kallweit wrote: > On 23.02.2022 15:17, Alan Stern wrote: > > Don't change register_root_hub(). Just change xhci_plat_probe(); make > > it skip the second call to usb_add_hcd() if there are no USB-3 ports. > > > This works on my system. However a consequence is that xhci->shared_hcd > is NULL. Why is that? xhci->shared_hcd doesn't get set in usb_add_hcd(), so skipping that call shouldn't cause it to be NULL. Note: If you skip calling usb_add_hcd(), you will also have to skip the corresponding call to usb_remove_hcd(). There may be a few more subtleties involved as well; like I said before, I'm not an expert on this driver. You should ask the xhci-hcd maintainer for advice. Alan Stern > There are a few places like the following in xhci.c where > this may result in a NPE. Not knowing the USB subsystem in detail > I can't say whether these places are in any relevant path. > > static int xhci_run_finished(struct xhci_hcd *xhci) > { > if (xhci_start(xhci)) { > xhci_halt(xhci); > return -ENODEV; > } > xhci->shared_hcd->state = HC_STATE_RUNNING; > > > > > Alan Stern > > > >> What I can do: submit my patches as RFC, then there's a better basis > >> for a discussion. > >> > >>> Alan Stern > >> > >> Heiner > > Heiner
On Wed, Feb 23, 2022 at 05:13:03PM -0500, Alan Stern wrote: > On Wed, Feb 23, 2022 at 09:58:56PM +0100, Heiner Kallweit wrote: > > On 23.02.2022 15:17, Alan Stern wrote: > > > Don't change register_root_hub(). Just change xhci_plat_probe(); make > > > it skip the second call to usb_add_hcd() if there are no USB-3 ports. I believe this had been attempted in the past, but it does not appear that patch was ever accepted: https://lore.kernel.org/linux-usb/1517221474-19627-1-git-send-email-tqnguyen@apm.com/ Jack > > This works on my system. However a consequence is that xhci->shared_hcd > > is NULL. > > Why is that? xhci->shared_hcd doesn't get set in usb_add_hcd(), so > skipping that call shouldn't cause it to be NULL. > > Note: If you skip calling usb_add_hcd(), you will also have to skip the > corresponding call to usb_remove_hcd(). There may be a few more > subtleties involved as well; like I said before, I'm not an expert on > this driver. You should ask the xhci-hcd maintainer for advice. > > Alan Stern > > > There are a few places like the following in xhci.c where > > this may result in a NPE. Not knowing the USB subsystem in detail > > I can't say whether these places are in any relevant path. > > > > static int xhci_run_finished(struct xhci_hcd *xhci) > > { > > if (xhci_start(xhci)) { > > xhci_halt(xhci); > > return -ENODEV; > > } > > xhci->shared_hcd->state = HC_STATE_RUNNING; > > > > > > > > > Alan Stern > > > > > >> What I can do: submit my patches as RFC, then there's a better basis > > >> for a discussion. > > >> > > >>> Alan Stern > > >> > > >> Heiner > > > > Heiner
On 24.02.2022 21:06, Jack Pham wrote: > On Wed, Feb 23, 2022 at 05:13:03PM -0500, Alan Stern wrote: >> On Wed, Feb 23, 2022 at 09:58:56PM +0100, Heiner Kallweit wrote: >>> On 23.02.2022 15:17, Alan Stern wrote: >>>> Don't change register_root_hub(). Just change xhci_plat_probe(); make >>>> it skip the second call to usb_add_hcd() if there are no USB-3 ports. > > I believe this had been attempted in the past, but it does not appear > that patch was ever accepted: > > https://lore.kernel.org/linux-usb/1517221474-19627-1-git-send-email-tqnguyen@apm.com/ > I also found that xhci at several places relies on a proper shared_hcd, even if there are no USB3 ports. Therefore maybe go with the less invasive original version of my patch? https://www.spinics.net/lists/linux-usb/msg222998.html > Jack > >>> This works on my system. However a consequence is that xhci->shared_hcd >>> is NULL. >> >> Why is that? xhci->shared_hcd doesn't get set in usb_add_hcd(), so >> skipping that call shouldn't cause it to be NULL. >> >> Note: If you skip calling usb_add_hcd(), you will also have to skip the >> corresponding call to usb_remove_hcd(). There may be a few more >> subtleties involved as well; like I said before, I'm not an expert on >> this driver. You should ask the xhci-hcd maintainer for advice. >> >> Alan Stern >> >>> There are a few places like the following in xhci.c where >>> this may result in a NPE. Not knowing the USB subsystem in detail >>> I can't say whether these places are in any relevant path. >>> >>> static int xhci_run_finished(struct xhci_hcd *xhci) >>> { >>> if (xhci_start(xhci)) { >>> xhci_halt(xhci); >>> return -ENODEV; >>> } >>> xhci->shared_hcd->state = HC_STATE_RUNNING; >>> >>> >>> >>>> Alan Stern >>>> >>>>> What I can do: submit my patches as RFC, then there's a better basis >>>>> for a discussion. >>>>> >>>>>> Alan Stern >>>>> >>>>> Heiner >>> >>> Heiner
On Thu, Feb 24, 2022 at 09:16:05PM +0100, Heiner Kallweit wrote: > On 24.02.2022 21:06, Jack Pham wrote: > > On Wed, Feb 23, 2022 at 05:13:03PM -0500, Alan Stern wrote: > >> On Wed, Feb 23, 2022 at 09:58:56PM +0100, Heiner Kallweit wrote: > >>> On 23.02.2022 15:17, Alan Stern wrote: > >>>> Don't change register_root_hub(). Just change xhci_plat_probe(); make > >>>> it skip the second call to usb_add_hcd() if there are no USB-3 ports. > > > > I believe this had been attempted in the past, but it does not appear > > that patch was ever accepted: > > > > https://lore.kernel.org/linux-usb/1517221474-19627-1-git-send-email-tqnguyen@apm.com/ > > > I also found that xhci at several places relies on a proper shared_hcd, > even if there are no USB3 ports. Therefore maybe go with the less invasive > original version of my patch? > > https://www.spinics.net/lists/linux-usb/msg222998.html The patch that Jack refers to, written by Tung Nguyen, does always create the shared_hcd. It simply avoids registering the shared_hcd when there are no USB-3 ports. You should try that patch and see if it works on your system. Alan Stern > > Jack > > > >>> This works on my system. However a consequence is that xhci->shared_hcd > >>> is NULL. > >> > >> Why is that? xhci->shared_hcd doesn't get set in usb_add_hcd(), so > >> skipping that call shouldn't cause it to be NULL. > >> > >> Note: If you skip calling usb_add_hcd(), you will also have to skip the > >> corresponding call to usb_remove_hcd(). There may be a few more > >> subtleties involved as well; like I said before, I'm not an expert on > >> this driver. You should ask the xhci-hcd maintainer for advice. > >> > >> Alan Stern > >> > >>> There are a few places like the following in xhci.c where > >>> this may result in a NPE. Not knowing the USB subsystem in detail > >>> I can't say whether these places are in any relevant path. > >>> > >>> static int xhci_run_finished(struct xhci_hcd *xhci) > >>> { > >>> if (xhci_start(xhci)) { > >>> xhci_halt(xhci); > >>> return -ENODEV; > >>> } > >>> xhci->shared_hcd->state = HC_STATE_RUNNING; > >>> > >>> > >>> > >>>> Alan Stern > >>>> > >>>>> What I can do: submit my patches as RFC, then there's a better basis > >>>>> for a discussion. > >>>>> > >>>>>> Alan Stern > >>>>> > >>>>> Heiner > >>> > >>> Heiner >
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 83b5aff25..e3f40d1f4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1423,9 +1423,8 @@ static int hub_configure(struct usb_hub *hub, ret = -ENODEV; goto fail; } else if (hub->descriptor->bNbrPorts == 0) { - message = "hub doesn't have any ports!"; - ret = -ENODEV; - goto fail; + dev_info(hub_dev, "hub has no ports, exiting\n"); + return -ENODEV; } /*
I get the "hub doesn't have any ports" error message on a system with Amlogic S905W SoC. Seems the SoC has internal USB 3.0 supports but is crippled with regard to USB 3.0 ports. Maybe we shouldn't consider this scenario an error. So let's change the message to info level, but otherwise keep the handling of the scenario as it is today. With the patch it looks like this on my system. dwc2 c9100000.usb: supply vusb_d not found, using dummy regulator dwc2 c9100000.usb: supply vusb_a not found, using dummy regulator dwc2 c9100000.usb: EPs: 7, dedicated fifos, 712 entries in SPRAM xhci-hcd xhci-hcd.0.auto: xHCI Host Controller xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1 xhci-hcd xhci-hcd.0.auto: hcc params 0x0228f664 hci version 0x100 quirks 0x0000000002010010 xhci-hcd xhci-hcd.0.auto: irq 49, io mem 0xc9000000 hub 1-0:1.0: USB hub found hub 1-0:1.0: 2 ports detected xhci-hcd xhci-hcd.0.auto: xHCI Host Controller xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2 xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. hub 2-0:1.0: USB hub found hub 2-0:1.0: hub has no ports, exiting Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/usb/core/hub.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)