Re: [PATCH v3 2/6] usb: xhci: Set up endpoints for the first 2 interfaces

[Janne Grunau <j@jannau.net>]

On Fri, Mar 22, 2024 at 09:17:08AM +0100, Heinrich Schuchardt wrote:
> On 3/22/24 08:47, Janne Grunau via B4 Relay wrote:
> > From: Janne Grunau <j@jannau.net>
> > 
> > The xhci driver currently only does the necessary initialization for
> > endpoints found in the first interface descriptor. Apple USB keyboards
> > (released 2021) use the second interface descriptor for the HID keyboard
> > boot protocol. To allow USB drivers to use endpoints from other
> > interface descriptors the xhci driver needs to ensure these endpoints
> > are initialized as well.
> > Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors
> > are considered during endpoint initialisation.
> > For now define it to 2 as that is sufficient for supporting the Apple
> > keyboards.
> > 
> > Reviewed-by: Marek Vasut <marex@denx.de>
> > Reviewed-by: Neal Gompa <neal@gompa.dev>
> > Signed-off-by: Janne Grunau <j@jannau.net>
> > ---
> >   drivers/usb/host/xhci.c | 31 +++++++++++++++++++------------
> >   include/usb.h           |  6 ++++++
> >   2 files changed, 25 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 534c4b973f..741e186ee0 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -606,24 +606,28 @@ static int xhci_set_configuration(struct usb_device *udev)
> >   	int slot_id = udev->slot_id;
> >   	struct xhci_virt_device *virt_dev = ctrl->devs[slot_id];
> >   	struct usb_interface *ifdesc;
> > +	unsigned int ifnum;
> > +	unsigned int max_ifnum = min((unsigned int)USB_MAX_ACTIVE_INTERFACES,
> 
> no_of_if being of type u8 limits the number of interfaces to 255. 
> Introducing USB_MAX_ACTIVE_INTERFACES limit us to the first two 
> interfaces. Is this really needed? Handling all interface would avoid 
> the introduction of artificial limitations which may hit us on the next 
> device.

It's not strictly necessary but assume that the code was only tested
with a single interface. Given the general state of u-boot's USB stack
I wouldn't be surprised if it would blew up with USB descriptors maxing
out the number of interfaces / endpoints. So limiting it to the minimum
of what's needed to support the device in front of me looks like the
safer option.

Janne