Re: [PATCH v2] usb: gadget: epautoconf: claim smallest endpoints first

[Peter Chen <peter.chen@nxp.com>]

On 20-07-03 13:46:27, Ruslan Bilovol wrote:
> On Tue, Jun 30, 2020 at 4:58 AM Peter Chen <peter.chen@nxp.com> wrote:
> >
> > On 20-06-29 23:18:45, Ruslan Bilovol wrote:
> > > UDC hardware may have endpoints with different maxpacket
> > > size. Current endpoint matching code takes first matching
> > > endpoint from the list.
> > >
> > > It's always possible that gadget allocates endpoints for
> > > small transfers (maxpacket size) first, then larger ones.
> > > That works fine if all matching UDC endpoints have same
> > > maxpacket size or are big enough to serve that allocation.
> > >
> > > However, some UDCs have first endpoints in the list with
> > > bigger maxpacket size, whereas last endpoints are much
> > > smaller. In this case endpoint allocation will fail for
> > > the gadget (which allocates smaller endpoints first) on
> > > final endpoint allocations.
> > >
> > > To make endpoint allocation fair, pick up smallest
> > > matching endpoints first, leaving bigger ones for
> > > heavier applications.
> > >
> > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> > > ---
> > >
> > > v2: rebased onto latest balbi/next branch
> > >
> > >  drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> > > index 1eb4fa2e623f..6c453b5d87bb 100644
> > > --- a/drivers/usb/gadget/epautoconf.c
> > > +++ b/drivers/usb/gadget/epautoconf.c
> > > @@ -66,7 +66,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > >       struct usb_ss_ep_comp_descriptor *ep_comp
> > >  )
> > >  {
> > > -     struct usb_ep   *ep;
> > > +     struct usb_ep   *ep, *ep_min = NULL;
> > >
> > >       if (gadget->ops->match_ep) {
> > >               ep = gadget->ops->match_ep(gadget, desc, ep_comp);
> > > @@ -74,14 +74,27 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > >                       goto found_ep;
> > >       }
> > >
> > > -     /* Second, look at endpoints until an unclaimed one looks usable */
> > > +     /*
> > > +      * Second, look at endpoints until an unclaimed one looks usable.
> > > +      * Try to find one with smallest maxpacket limit, leaving larger
> > > +      * endpoints for heavier applications
> > > +      */
> > >       list_for_each_entry (ep, &gadget->ep_list, ep_list) {
> > > -             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
> > > -                     goto found_ep;
> > > +             if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp)) {
> > > +                     if (desc->wMaxPacketSize == 0)
> > > +                             goto found_ep;
> >
> > Why you do special handling for this? You still could give the smallest
> > maxpacket_limit EP for it, right?
> 
> Of course it's technically possible. However in case "wMaxPacketSize == 0"
> gadget driver wants to get maximum possible wMaxPacketSize from endpoint
> configuration and I was thinking about avoiding regressions if we always provide
> smaller endpoints.

You may only want to change the match logic, not but the special case.

Currently, it returns the first matched endpoint no matter
"wMaxPacketSize == 0" or not. And you changed the match logic
as returning the smallest maxPacketsize endpoint, you also don't need
to consider whether "wMaxPacketSize == 0" or not, otherwise, it may
introduce the complexity.

Peter

> 
> As I can see, providing smallest endpoint that matches requested wMaxPacketSize
> is OK, but if gadget driver just wants autoconf core to use it with
> maximum possible
> value, I'm thinking now if we can even change this part and if wMaxPacketSize
> is zero, find endpoint with maximum possible wMaxPacketSize
> 
> Does it make sense?
> 
> Thanks
> Ruslan
> 
> >
> > Peter
> >
> > > +                     else if (!ep_min)
> > > +                             ep_min = ep;
> > > +                     else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
> > > +                             ep_min = ep;
> > > +             }
> > >       }
> > >
> > >       /* Fail */
> > > -     return NULL;
> > > +     if (!ep_min)
> > > +             return NULL;
> > > +
> > > +     ep = ep_min;
> > >  found_ep:
> > >
> > >       /*
> > > --
> > > 2.17.1
> > >
> >
> > --
> >
> > Thanks,
> > Peter Chen

-- 

Thanks,
Peter Chen