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

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

[Wlodzimierz Lipert]

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.

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

diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 1eb4fa2e623f..50a2e8a90447 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
 )
 {
 	struct usb_ep	*ep;
+	unsigned *epnum_map;
+	/* ep num 0 is reserved: not available for auto configuration */
+	u8 num = 1;
+	/* USB allows up to 16 IN and 16 OUT enpoints */
+	unsigned num_mask = 0xFFFFU;
 
 	if (gadget->ops->match_ep) {
 		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
@@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
 	/* report address */
 	desc->bEndpointAddress &= USB_DIR_IN;
 	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);
+		if (num > 15)
 			return NULL;
-		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
-	} else {
-		if (++gadget->out_epnum > 15)
-			return NULL;
-		desc->bEndpointAddress |= gadget->out_epnum;
+		num_mask = 1U << num;
 	}
 
+	epnum_map = desc->bEndpointAddress & USB_DIR_IN
+		? &gadget->in_epnum : &gadget->out_epnum;
+
+	/* check if requested ep number (if name encodes it) or any is available */
+	if (num_mask == (*epnum_map & num_mask))
+		return NULL;
+
+	/* find first available ep number (if not encoded in ep name) */
+	while (*epnum_map & (1U << num))
+		++num;
+
+	*epnum_map |= 1U << num;
+	desc->bEndpointAddress |= num;
 	ep->address = desc->bEndpointAddress;
 	ep->desc = NULL;
 	ep->comp_desc = NULL;
@@ -208,7 +220,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

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

[Greg KH]

On Wed, Apr 26, 2023 at 01:45:28PM +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.
> 
> Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> ---
>  drivers/usb/gadget/epautoconf.c | 35 ++++++++++++++++++++++-----------
>  include/linux/usb/gadget.h      |  4 ++--
>  2 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 1eb4fa2e623f..50a2e8a90447 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  )
>  {
>  	struct usb_ep	*ep;
> +	unsigned *epnum_map;
> +	/* ep num 0 is reserved: not available for auto configuration */
> +	u8 num = 1;
> +	/* USB allows up to 16 IN and 16 OUT enpoints */
> +	unsigned num_mask = 0xFFFFU;
>  
>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> @@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  	/* report address */
>  	desc->bEndpointAddress &= USB_DIR_IN;
>  	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);
> +		if (num > 15)
>  			return NULL;
> -		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
> -	} else {
> -		if (++gadget->out_epnum > 15)
> -			return NULL;
> -		desc->bEndpointAddress |= gadget->out_epnum;
> +		num_mask = 1U << num;
>  	}
>  
> +	epnum_map = desc->bEndpointAddress & USB_DIR_IN
> +		? &gadget->in_epnum : &gadget->out_epnum;
> +
> +	/* check if requested ep number (if name encodes it) or any is available */
> +	if (num_mask == (*epnum_map & num_mask))
> +		return NULL;
> +
> +	/* find first available ep number (if not encoded in ep name) */
> +	while (*epnum_map & (1U << num))
> +		++num;
> +
> +	*epnum_map |= 1U << num;
> +	desc->bEndpointAddress |= num;
>  	ep->address = desc->bEndpointAddress;
>  	ep->desc = NULL;
>  	ep->comp_desc = NULL;
> @@ -208,7 +220,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
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Alan Stern]

On Wed, Apr 26, 2023 at 01:45:28PM +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.
> 
> Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> ---
>  drivers/usb/gadget/epautoconf.c | 35 ++++++++++++++++++++++-----------
>  include/linux/usb/gadget.h      |  4 ++--
>  2 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 1eb4fa2e623f..50a2e8a90447 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  )
>  {
>  	struct usb_ep	*ep;
> +	unsigned *epnum_map;
> +	/* ep num 0 is reserved: not available for auto configuration */
> +	u8 num = 1;
> +	/* USB allows up to 16 IN and 16 OUT enpoints */
> +	unsigned num_mask = 0xFFFFU;

I think these initializers aren't needed.  They apply only in the case 
where the endpoint name doesn't encode a number.

>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> @@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
>  	/* report address */
>  	desc->bEndpointAddress &= USB_DIR_IN;
>  	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);
> +		if (num > 15)
>  			return NULL;

This check shouldn't be here, at least, not in this form.  If num > 15 
it's a bug in the UDC driver; we could report it that way.

> -		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
> -	} else {
> -		if (++gadget->out_epnum > 15)
> -			return NULL;
> -		desc->bEndpointAddress |= gadget->out_epnum;
> +		num_mask = 1U << num;
>  	}
>  
> +	epnum_map = desc->bEndpointAddress & USB_DIR_IN
> +		? &gadget->in_epnum : &gadget->out_epnum;
> +
> +	/* check if requested ep number (if name encodes it) or any is available */
> +	if (num_mask == (*epnum_map & num_mask))
> +		return NULL;
> +
> +	/* find first available ep number (if not encoded in ep name) */
> +	while (*epnum_map & (1U << num))
> +		++num;

This rearrangement of the code is kind of awkward.  It would be better 
to put the availability check for the encoded-number case into the "if" 
clause, and put the search inside an "else" section, rather than trying 
to combine two rather different computations into a single piece of 
code.  That way you wouldn't need num_mask at all.

Also, your "while" loop doesn't enforce num <= 15.  For that matter, it 
might be more efficient to use a "find first bit" library routine rather 
than coding your own loop.

> +
> +	*epnum_map |= 1U << num;
> +	desc->bEndpointAddress |= num;
>  	ep->address = desc->bEndpointAddress;
>  	ep->desc = NULL;
>  	ep->comp_desc = NULL;
> @@ -208,7 +220,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;

This change doesn't really do anything, since the search doesn't allow 
num == 0 anyway.

>  }
>  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

Should these be changed to u16?  I guess it doesn't matter...

>   * @mA: last set mA value
>   * @otg_caps: OTG capabilities of this gadget.
>   * @sg_supported: true if we can handle scatter-gather

Alan Stern

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

[Wlodzimierz Lipert]

Hi Alan,

Thanks for the feedback, could you please see my comments below.
Mask logic is not clear straight away and I probably should be more
verbose in patch description.
Maybe you could reconsider the changes.

On Thu, Apr 27, 2023 at 3:08 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Apr 26, 2023 at 01:45:28PM +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.
> >
> > Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> > ---
> >  drivers/usb/gadget/epautoconf.c | 35 ++++++++++++++++++++++-----------
> >  include/linux/usb/gadget.h      |  4 ++--
> >  2 files changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> > index 1eb4fa2e623f..50a2e8a90447 100644
> > --- a/drivers/usb/gadget/epautoconf.c
> > +++ b/drivers/usb/gadget/epautoconf.c
> > @@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
> >  )
> >  {
> >       struct usb_ep   *ep;
> > +     unsigned *epnum_map;
> > +     /* ep num 0 is reserved: not available for auto configuration */
> > +     u8 num = 1;
> > +     /* USB allows up to 16 IN and 16 OUT enpoints */
> > +     unsigned num_mask = 0xFFFFU;
>
> I think these initializers aren't needed.  They apply only in the case
> where the endpoint name doesn't encode a number.

initialization is needed to ensure we stay within 16 bits and we don't
try to use bit 0.

>
> >       if (gadget->ops->match_ep) {
> >               ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> > @@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
> >       /* report address */
> >       desc->bEndpointAddress &= USB_DIR_IN;
> >       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);
> > +             if (num > 15)
> >                       return NULL;
>
> This check shouldn't be here, at least, not in this form.  If num > 15
> it's a bug in the UDC driver; we could report it that way.
>

the check is there to make logic below work,
its return value is consistent with the rest of the cases.

> > -             desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
> > -     } else {
> > -             if (++gadget->out_epnum > 15)
> > -                     return NULL;
> > -             desc->bEndpointAddress |= gadget->out_epnum;
> > +             num_mask = 1U << num;
> >       }
> >
> > +     epnum_map = desc->bEndpointAddress & USB_DIR_IN
> > +             ? &gadget->in_epnum : &gadget->out_epnum;
> > +
> > +     /* check if requested ep number (if name encodes it) or any is available */
> > +     if (num_mask == (*epnum_map & num_mask))
> > +             return NULL;
> > +
> > +     /* find first available ep number (if not encoded in ep name) */
> > +     while (*epnum_map & (1U << num))
> > +             ++num;
>
> This rearrangement of the code is kind of awkward.  It would be better
> to put the availability check for the encoded-number case into the "if"
> clause, and put the search inside an "else" section, rather than trying
> to combine two rather different computations into a single piece of
> code.  That way you wouldn't need num_mask at all.
>
> Also, your "while" loop doesn't enforce num <= 15.  For that matter, it
> might be more efficient to use a "find first bit" library routine rather
> than coding your own loop.

The code structure tries to make the amount of branches minimal,
 this why I introduced mask and epnum_map ptr.
num <= 15 is enforced by mask itself:
 " if (num_mask == (*epnum_map & num_mask))" will return NULL
in case "encoded" ep is already unavailable or all the ep are unavailable so
we wont go any further (avoiding inc. num above 15).

>
> > +
> > +     *epnum_map |= 1U << num;
> > +     desc->bEndpointAddress |= num;
> >       ep->address = desc->bEndpointAddress;
> >       ep->desc = NULL;
> >       ep->comp_desc = NULL;
> > @@ -208,7 +220,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;
>
> This change doesn't really do anything, since the search doesn't allow
> num == 0 anyway.

yes this change is there for consistency - if anyone tries to use it.

>
> >  }
> >  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
>
> Should these be changed to u16?  I guess it doesn't matter...

tried to avoid struct padding changes

>
> >   * @mA: last set mA value
> >   * @otg_caps: OTG capabilities of this gadget.
> >   * @sg_supported: true if we can handle scatter-gather
>
> Alan Stern



-- 
BR/Pozdrawiam. Wlodzimierz Lipert

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

[Alan Stern]

On Thu, Apr 27, 2023 at 07:55:13AM +0200, Wlodzimierz Lipert wrote:
> Hi Alan,
> 
> Thanks for the feedback, could you please see my comments below.
> Mask logic is not clear straight away and I probably should be more
> verbose in patch description.
> Maybe you could reconsider the changes.
> 
> On Thu, Apr 27, 2023 at 3:08 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, Apr 26, 2023 at 01:45:28PM +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.
> > >
> > > Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> > > ---
> > >  drivers/usb/gadget/epautoconf.c | 35 ++++++++++++++++++++++-----------
> > >  include/linux/usb/gadget.h      |  4 ++--
> > >  2 files changed, 26 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> > > index 1eb4fa2e623f..50a2e8a90447 100644
> > > --- a/drivers/usb/gadget/epautoconf.c
> > > +++ b/drivers/usb/gadget/epautoconf.c
> > > @@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > >  )
> > >  {
> > >       struct usb_ep   *ep;
> > > +     unsigned *epnum_map;
> > > +     /* ep num 0 is reserved: not available for auto configuration */
> > > +     u8 num = 1;
> > > +     /* USB allows up to 16 IN and 16 OUT enpoints */
> > > +     unsigned num_mask = 0xFFFFU;
> >
> > I think these initializers aren't needed.  They apply only in the case
> > where the endpoint name doesn't encode a number.
> 
> initialization is needed to ensure we stay within 16 bits and we don't
> try to use bit 0.
> 
> >
> > >       if (gadget->ops->match_ep) {
> > >               ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> > > @@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > >       /* report address */
> > >       desc->bEndpointAddress &= USB_DIR_IN;
> > >       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);
> > > +             if (num > 15)
> > >                       return NULL;
> >
> > This check shouldn't be here, at least, not in this form.  If num > 15
> > it's a bug in the UDC driver; we could report it that way.
> >
> 
> the check is there to make logic below work,
> its return value is consistent with the rest of the cases.
> 
> > > -             desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
> > > -     } else {
> > > -             if (++gadget->out_epnum > 15)
> > > -                     return NULL;
> > > -             desc->bEndpointAddress |= gadget->out_epnum;
> > > +             num_mask = 1U << num;
> > >       }
> > >
> > > +     epnum_map = desc->bEndpointAddress & USB_DIR_IN
> > > +             ? &gadget->in_epnum : &gadget->out_epnum;
> > > +
> > > +     /* check if requested ep number (if name encodes it) or any is available */
> > > +     if (num_mask == (*epnum_map & num_mask))
> > > +             return NULL;
> > > +
> > > +     /* find first available ep number (if not encoded in ep name) */
> > > +     while (*epnum_map & (1U << num))
> > > +             ++num;
> >
> > This rearrangement of the code is kind of awkward.  It would be better
> > to put the availability check for the encoded-number case into the "if"
> > clause, and put the search inside an "else" section, rather than trying
> > to combine two rather different computations into a single piece of
> > code.  That way you wouldn't need num_mask at all.
> >
> > Also, your "while" loop doesn't enforce num <= 15.  For that matter, it
> > might be more efficient to use a "find first bit" library routine rather
> > than coding your own loop.
> 
> The code structure tries to make the amount of branches minimal,
>  this why I introduced mask and epnum_map ptr.
> num <= 15 is enforced by mask itself:
>  " if (num_mask == (*epnum_map & num_mask))" will return NULL
> in case "encoded" ep is already unavailable or all the ep are unavailable so
> we wont go any further (avoiding inc. num above 15).
> 
> >
> > > +
> > > +     *epnum_map |= 1U << num;
> > > +     desc->bEndpointAddress |= num;
> > >       ep->address = desc->bEndpointAddress;
> > >       ep->desc = NULL;
> > >       ep->comp_desc = NULL;

Here's what I was thinking of.  Maybe when you see it written out, 
you'll agree that this approach is simpler and easier to follow.

	...
	unsigned int	*epmap;
	unsigned int	num;

	...
	epmap = (usb_endpoint_dir_in(desc) ?
			&gadget->in_epnum : &gadget->out_epnum);
	if (isdigit(ep->name[2])) {		/* Number encoded in name */
		num = simple_strtoul(&ep->name[2], NULL, 10);
		if (*epmap & (1 << num))
			return NULL;		/* Endpoint is unavailable */

	} else {				/* Find first available */
		num = ffz(*epnum_map) - 1;
		if (num > 15)
			return NULL;		/* No endpoints available */
	}

	*epmap |= 1 << num;
	desc->bEndpointAddress |= num;
	...

And then of course the usb_ep_autoconfig_reset() routine would set 
both gadget->in_epnum and gadget->out_epnum to 1, like in your patch.
You could even change their names to in_epmap and out_epmap, which 
better describes their new meaning.

If you want, you could add

		WARN_ON(num > 15);

immediately after the simple_strtoul() line.  But since it's not there 
now, I don't think it is needed.

Alan Stern

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

[Wlodzimierz Lipert]

Indeed, easier to follow. What you think about adding the check (below),
IMHO we should not test bits outside of valid range, and it will cover
bugs in ep name.

On Thu, Apr 27, 2023 at 5:18 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Apr 27, 2023 at 07:55:13AM +0200, Wlodzimierz Lipert wrote:
> > Hi Alan,
> >
> > Thanks for the feedback, could you please see my comments below.
> > Mask logic is not clear straight away and I probably should be more
> > verbose in patch description.
> > Maybe you could reconsider the changes.
> >
> > On Thu, Apr 27, 2023 at 3:08 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, Apr 26, 2023 at 01:45:28PM +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.
> > > >
> > > > Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com>
> > > > ---
> > > >  drivers/usb/gadget/epautoconf.c | 35 ++++++++++++++++++++++-----------
> > > >  include/linux/usb/gadget.h      |  4 ++--
> > > >  2 files changed, 26 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> > > > index 1eb4fa2e623f..50a2e8a90447 100644
> > > > --- a/drivers/usb/gadget/epautoconf.c
> > > > +++ b/drivers/usb/gadget/epautoconf.c
> > > > @@ -67,6 +67,11 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > > >  )
> > > >  {
> > > >       struct usb_ep   *ep;
> > > > +     unsigned *epnum_map;
> > > > +     /* ep num 0 is reserved: not available for auto configuration */
> > > > +     u8 num = 1;
> > > > +     /* USB allows up to 16 IN and 16 OUT enpoints */
> > > > +     unsigned num_mask = 0xFFFFU;
> > >
> > > I think these initializers aren't needed.  They apply only in the case
> > > where the endpoint name doesn't encode a number.
> >
> > initialization is needed to ensure we stay within 16 bits and we don't
> > try to use bit 0.
> >
> > >
> > > >       if (gadget->ops->match_ep) {
> > > >               ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> > > > @@ -94,18 +99,25 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > > >       /* report address */
> > > >       desc->bEndpointAddress &= USB_DIR_IN;
> > > >       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);
> > > > +             if (num > 15)
> > > >                       return NULL;
> > >
> > > This check shouldn't be here, at least, not in this form.  If num > 15
> > > it's a bug in the UDC driver; we could report it that way.
> > >
> >
> > the check is there to make logic below work,
> > its return value is consistent with the rest of the cases.
> >
> > > > -             desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
> > > > -     } else {
> > > > -             if (++gadget->out_epnum > 15)
> > > > -                     return NULL;
> > > > -             desc->bEndpointAddress |= gadget->out_epnum;
> > > > +             num_mask = 1U << num;
> > > >       }
> > > >
> > > > +     epnum_map = desc->bEndpointAddress & USB_DIR_IN
> > > > +             ? &gadget->in_epnum : &gadget->out_epnum;
> > > > +
> > > > +     /* check if requested ep number (if name encodes it) or any is available */
> > > > +     if (num_mask == (*epnum_map & num_mask))
> > > > +             return NULL;
> > > > +
> > > > +     /* find first available ep number (if not encoded in ep name) */
> > > > +     while (*epnum_map & (1U << num))
> > > > +             ++num;
> > >
> > > This rearrangement of the code is kind of awkward.  It would be better
> > > to put the availability check for the encoded-number case into the "if"
> > > clause, and put the search inside an "else" section, rather than trying
> > > to combine two rather different computations into a single piece of
> > > code.  That way you wouldn't need num_mask at all.
> > >
> > > Also, your "while" loop doesn't enforce num <= 15.  For that matter, it
> > > might be more efficient to use a "find first bit" library routine rather
> > > than coding your own loop.
> >
> > The code structure tries to make the amount of branches minimal,
> >  this why I introduced mask and epnum_map ptr.
> > num <= 15 is enforced by mask itself:
> >  " if (num_mask == (*epnum_map & num_mask))" will return NULL
> > in case "encoded" ep is already unavailable or all the ep are unavailable so
> > we wont go any further (avoiding inc. num above 15).
> >
> > >
> > > > +
> > > > +     *epnum_map |= 1U << num;
> > > > +     desc->bEndpointAddress |= num;
> > > >       ep->address = desc->bEndpointAddress;
> > > >       ep->desc = NULL;
> > > >       ep->comp_desc = NULL;
>
> Here's what I was thinking of.  Maybe when you see it written out,
> you'll agree that this approach is simpler and easier to follow.
>
>         ...
>         unsigned int    *epmap;
>         unsigned int    num;
>
>         ...
>         epmap = (usb_endpoint_dir_in(desc) ?
>                         &gadget->in_epnum : &gadget->out_epnum);
>         if (isdigit(ep->name[2])) {             /* Number encoded in name */
>                 num = simple_strtoul(&ep->name[2], NULL, 10);
                 if (num > 15 || *epmap & (1 << num))
>                         return NULL;            /* Endpoint is unavailable */
>
>         } else {                                /* Find first available */
>                 num = ffz(*epnum_map) - 1;
>                 if (num > 15)
>                         return NULL;            /* No endpoints available */
>         }
>
>         *epmap |= 1 << num;
>         desc->bEndpointAddress |= num;
>         ...
>
> And then of course the usb_ep_autoconfig_reset() routine would set
> both gadget->in_epnum and gadget->out_epnum to 1, like in your patch.
> You could even change their names to in_epmap and out_epmap, which
> better describes their new meaning.
>
> If you want, you could add
>
>                 WARN_ON(num > 15);
>
> immediately after the simple_strtoul() line.  But since it's not there
> now, I don't think it is needed.
>
> Alan Stern



-- 
BR/Pozdrawiam. Wlodzimierz Lipert

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

[Alan Stern]

On Thu, Apr 27, 2023 at 05:36:34PM +0200, Wlodzimierz Lipert wrote:
> Indeed, easier to follow. What you think about adding the check (below),
> IMHO we should not test bits outside of valid range, and it will cover
> bugs in ep name.

> >         if (isdigit(ep->name[2])) {             /* Number encoded in name */
> >                 num = simple_strtoul(&ep->name[2], NULL, 10);
>                  if (num > 15 || *epmap & (1 << num))
> >                         return NULL;            /* Endpoint is unavailable */

We don't want to cover up bugs; we want to _fix_ them.  That's why I 
suggested adding a WARN_ON(), so that people will be aware there's a bug 
that needs fixing.  If you prefer, you could do it like this:

		if (num > 15) {
			WARN_ON(1);	/* Invalid endpoint number */
			return NULL;
		}
		if (*epmap & (1 << num))
			return NULL;	/* Endpoint is unavailable */

That would be okay.

Alan Stern