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