Re: [PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Apr 29, 2023 at 05:47:33PM +0200, Wlodzimierz Lipert wrote:
> usb_ep_autoconfig_ss tries to use endpoint name or internal counters to generate
> bEndpointAddress - this leads to duplicate addresses. Fix changes the
> way in/out_epnum is used, now as bitmap which represents unavailable ep numbers.

Please properly wrap your changelog lines at 72 columns like your editor
asked you to.

> v2: formatting fixes only (errors).
> v3: refined autoconf logic (removed num_mask, switched to use ffz).
> v4: formatting only changes (style).

As per the documentation, this information about changes needs to go
below the --- line.



> 
> Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> ---
>  drivers/usb/gadget/epautoconf.c | 25 ++++++++++++++++---------
>  include/linux/usb/gadget.h      |  4 ++--
>  2 files changed, 18 insertions(+), 11 deletions(-)

What commit id does this fix?  Does it need to go to older stable
kernels?  If so, how far back?

> 
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 1eb4fa2e623f..f73a7fe3d7d7 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -67,6 +67,8 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  )
>  {
>  	struct usb_ep	*ep;
> +	unsigned	*epmap;

Why "unsigned"?  Didn't checkpatch complain about this?

> +	u8		num;
>  
>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> @@ -93,19 +95,23 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  
>  	/* report address */
>  	desc->bEndpointAddress &= USB_DIR_IN;
> +	epmap = usb_endpoint_dir_in(desc) ?
> +			&gadget->in_epnum : &gadget->out_epnum;

I hate ?: lines with a passion.  Please spell it out with a real if
statement as the generated code is the same and it's easier to actually
read by humans.

> +
>  	if (isdigit(ep->name[2])) {
> -		u8 num = simple_strtoul(&ep->name[2], NULL, 10);
> -		desc->bEndpointAddress |= num;
> -	} else if (desc->bEndpointAddress & USB_DIR_IN) {
> -		if (++gadget->in_epnum > 15)
> +		num = simple_strtoul(&ep->name[2], NULL, 10);
> +		WARN_ON(num == 0 || num > 15);

You just crashed the system if this happened if panic-on-warn is enabled :(

Please never do this, if there is an issue, fix it up and handle it
properly.

> +		if (*epmap & (1U << num))
>  			return NULL;
> -		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
>  	} else {
> -		if (++gadget->out_epnum > 15)
> +		/* find first available ep number (not encoded in ep name) */
> +		num = ffz(*epmap);
> +		if (num > 15)
>  			return NULL;
> -		desc->bEndpointAddress |= gadget->out_epnum;
>  	}
>  
> +	*epmap |= 1U << num;

BIT()?

> +	desc->bEndpointAddress |= num;
>  	ep->address = desc->bEndpointAddress;
>  	ep->desc = NULL;
>  	ep->comp_desc = NULL;
> @@ -208,7 +214,8 @@ void usb_ep_autoconfig_reset (struct usb_gadget *gadget)
>  		ep->claimed = false;
>  		ep->driver_data = NULL;
>  	}
> -	gadget->in_epnum = 0;
> -	gadget->out_epnum = 0;
> +	/* ep num 0 is reserved: not available for auto configuration */
> +	gadget->in_epnum = 1U;
> +	gadget->out_epnum = 1U;
>  }
>  EXPORT_SYMBOL_GPL(usb_ep_autoconfig_reset);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 6a178177e4c9..1e00e22202bc 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -343,8 +343,8 @@ struct usb_gadget_ops {
>   *	and sometimes configuration.
>   * @dev: Driver model state for this abstract device.
>   * @isoch_delay: value from Set Isoch Delay request. Only valid on SS/SSP
> - * @out_epnum: last used out ep number
> - * @in_epnum: last used in ep number
> + * @out_epnum: bitmap of allocated out ep numbers
> + * @in_epnum: bitmap of allocated in ep numbers

As you changed the functionality of these variables, why not rename them
at the same time to make it obvious?  'in_epnum_bitmap'?

thanks,

greg k-h