If the controller reports HCSPARAMS1.maxports==0 then we can skip the
whole function: it would fail later after doing a bunch of unnecessary
stuff. It can occur on a buggy hardware (the value is driven by external
signals).

Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
---
 drivers/usb/host/xhci-mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index d2900197a49e..e8406db78782 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2160,6 +2160,11 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 
 	num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
+	if (num_ports == 0) {
+		xhci_warn(xhci, "Host controller has no port enabled\n");
+		return -ENODEV;
+	}
+
 	xhci->hw_ports = kcalloc_node(num_ports, sizeof(*xhci->hw_ports),
 				flags, dev_to_node(dev));
 	if (!xhci->hw_ports)
-- 
2.43.0

Hi,

> If the controller reports HCSPARAMS1.maxports==0 then we can skip the
> whole function: it would fail later after doing a bunch of unnecessary
> stuff. It can occur on a buggy hardware (the value is driven by external
> signals).

This function runs once during HC initialization, so what's the benefit
of bypassing it? Does it take unusually long time? Does it panic?

It seems to alreday be written to handle such abnormal cases gracefully.

Regards,
Michal

Hello,

On Fri, Oct 04, 2024 at 12:57:16PM +0200, Michał Pecio wrote:
> Hi,
> 
> > If the controller reports HCSPARAMS1.maxports==0 then we can skip the
> > whole function: it would fail later after doing a bunch of unnecessary
> > stuff. It can occur on a buggy hardware (the value is driven by external
> > signals).
> 
> This function runs once during HC initialization, so what's the benefit
> of bypassing it? Does it take unusually long time? Does it panic?
> 
> It seems to alreday be written to handle such abnormal cases gracefully.

That is correct, the case is handled without panic, but the 0 value gets
silently propagated until it eventually fails on line 2220:
	if (xhci->usb2_rhub.num_ports == 0 && xhci->usb3_rhub.num_ports == 0) {
		xhci_warn(xhci, "No ports on the roothubs?\n");
		return -ENODEV;
	}
The benefits are only:
  - Reporting a more precise issue
  - Avoids iterating through the capability structures of the controller
  - failsafe if future changes

This is totally a nitpick as the case is unusual, if you think it is not
worth taking it upstream i'll understand.


Kr,
Olivier

On 4.10.2024 22.14, Olivier Dautricourt wrote:
> Hello,
> 
> On Fri, Oct 04, 2024 at 12:57:16PM +0200, Michał Pecio wrote:
>> Hi,
>>
>>> If the controller reports HCSPARAMS1.maxports==0 then we can skip the
>>> whole function: it would fail later after doing a bunch of unnecessary
>>> stuff. It can occur on a buggy hardware (the value is driven by external
>>> signals).
>>
>> This function runs once during HC initialization, so what's the benefit
>> of bypassing it? Does it take unusually long time? Does it panic?
>>
>> It seems to alreday be written to handle such abnormal cases gracefully.
> 
> That is correct, the case is handled without panic, but the 0 value gets
> silently propagated until it eventually fails on line 2220:
> 	if (xhci->usb2_rhub.num_ports == 0 && xhci->usb3_rhub.num_ports == 0) {
> 		xhci_warn(xhci, "No ports on the roothubs?\n");
> 		return -ENODEV;
> 	}
> The benefits are only:
>    - Reporting a more precise issue
>    - Avoids iterating through the capability structures of the controller
>    - failsafe if future changes
> 
> This is totally a nitpick as the case is unusual, if you think it is not
> worth taking it upstream i'll understand.
> 

I think we'll skip this. An abnormal case like this where the host would be
useless anyway is already handled reasonably enough by driver.

Thanks
Mathias

  

> That is correct, the case is handled without panic, but the 0 value
> gets silently propagated until it eventually fails on line 2220:
> 	if (xhci->usb2_rhub.num_ports == 0 && xhci->usb3_rhub.num_ports == 0) {
>              xhci_warn(xhci, "No ports on the roothubs?\n");
>              return -ENODEV;
> 	}
> The benefits are only:
>   - Reporting a more precise issue
>   - Avoids iterating through the capability structures of the
>     controller
>   - failsafe if future changes

Well, simplifying things is not bad in principle, but in this case it
looks like this patch adds a branch and some 50 bytes of code/data for
the sake of optimizing something with no practical relevance (any such
hardware is useless, rejected by this driver, and violates the spec).

So, maybe just not worth the cost, no matter how small ;)

Regards,
Michal

On Mon, Sep 30, 2024 at 07:23:29AM +0200, Olivier Dautricourt wrote:
> If the controller reports HCSPARAMS1.maxports==0 then we can skip the
> whole function: it would fail later after doing a bunch of unnecessary
> stuff. It can occur on a buggy hardware (the value is driven by external
> signals).

What "buggy hardware" is this that can not pass the USB testing for this
type of issue?

> 
> Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
> ---
>  drivers/usb/host/xhci-mem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index d2900197a49e..e8406db78782 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2160,6 +2160,11 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
>  	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>  
>  	num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> +	if (num_ports == 0) {
> +		xhci_warn(xhci, "Host controller has no port enabled\n");
> +		return -ENODEV;
> +	}

Should this be backported to older kernels, if so, how far back if this
is common hardware?

thanks,

greg k-h

Hello,

On Fri, Oct 04, 2024 at 10:07:01AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 30, 2024 at 07:23:29AM +0200, Olivier Dautricourt wrote:
> > If the controller reports HCSPARAMS1.maxports==0 then we can skip the
> > whole function: it would fail later after doing a bunch of unnecessary
> > stuff. It can occur on a buggy hardware (the value is driven by external
> > signals).
> 
> What "buggy hardware" is this that can not pass the USB testing for this
> type of issue?

This is a behaviour found while debugging a custom firmware where this
value happen to be controlled here, i don't know any hardware out there
with such issue, this change should be seen as a software nitpick and is
not trying to fix a specific hardware.

> 
> > 
> > Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
> > ---
> >  drivers/usb/host/xhci-mem.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index d2900197a49e..e8406db78782 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -2160,6 +2160,11 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
> >  	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
> >  
> >  	num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> > +	if (num_ports == 0) {
> > +		xhci_warn(xhci, "Host controller has no port enabled\n");
> > +		return -ENODEV;
> > +	}
> 
> Should this be backported to older kernels, if so, how far back if this
> is common hardware?

I don't think this would have to be ported to stable trees: The function
handles the case without failure: the 0 value is propagated until line
2220 and fails on condition:
	if (xhci->usb2_rhub.num_ports == 0 && xhci->usb3_rhub.num_ports == 0) {
		xhci_warn(xhci, "No ports on the roothubs?\n");
		return -ENODEV;
	}

The change merely avoids passing 0 value through kcalloc_node calls and
unnecessary accesses to the capability structures of the controller.

Kr,
Olivier