On 20-07-03 13:46:27, Ruslan Bilovol wrote:
> On Tue, Jun 30, 2020 at 4:58 AM Peter Chen <peter.chen@xxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >
> > > 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
