[PATCH] usb: core: improve handling of hubs with no ports

[PATCH] usb: core: improve handling of hubs with no ports

[Heiner Kallweit]

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;
 	}
 
 	/*
-- 
2.35.1

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Alan Stern]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Heiner Kallweit]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Alan Stern]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Heiner Kallweit]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Alan Stern]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Heiner Kallweit]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Alan Stern]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Jack Pham]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Heiner Kallweit]

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

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Alan Stern]

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
> 

Re: [PATCH] usb: core: improve handling of hubs with no ports

[Mathias Nyman]

On 24.2.2022 22.21, Alan Stern wrote:
> 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.

I think we need to start supporting xHC conreollers with just one roothub. 
Only call usb_add_hcd() once in those cases.

Even prepare for special cases where xHCI only has usb3 ports (usb2 pins routed
to a different host controller)

Currently driver reads port capabilities in:
usb_add_hcd()
  hcd->driver->reset   ...-> xhci_gen_setup()
    xhci_gen_setup()
      xhci_init(hcd)
        xhci_mem_init()
          xhci_setup_port_arrays()
  
Driver makes some assumptions based on if hcd is primary or not early in
xhci_gen_setup(), and initializes values like hcd->speed = HCD_USB2;

Should be doable, changes needed in at least xhci_run(), xhci_stop(),
xhci_resume(), xhci_suspend(), xhci_gen_setup() and probe.

-Mathias