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