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@xxxxxxxxx>
> ---
> 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
