| Message ID | 20240930052336.80589-1-olivierdautricourt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: xhci: xhci_setup_port_arrays: early -ENODEV if maxports is 0. | expand |
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
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)
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(+)