On Sat, Apr 29, 2023 at 04:45:16PM +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.
> Refined autoconf logic.
>
> Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@xxxxxxxxx>
> ---
You didn't describe here the difference from the previous version.
> drivers/usb/gadget/epautoconf.c | 25 ++++++++++++++++---------
> include/linux/usb/gadget.h | 4 ++--
> 2 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 1eb4fa2e623f..74430807b803 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;
> + u8 num;
Please copy the indentation style of the surrounding code.
>
> 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;
A continuation line should be indented by something other than one tab
stop, to make it look different from a nested statement.
Other than these minor stylistic issues, the patch looks fine.
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Alan Stern
> +
> 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);
> + 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;
> + 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
> * @mA: last set mA value
> * @otg_caps: OTG capabilities of this gadget.
> * @sg_supported: true if we can handle scatter-gather
> --
> 2.39.2
>
