On Sun, Mar 12, 2023 at 06:48:36PM -0400, 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. Note, please use all 72 columns in your changelog text if possible. > > To make endpoint allocation fair, pick up smallest > matching endpoints first, leaving bigger ones for > heavier applications. > > Keel old behavior when "wMaxPacketSize == 0" because "Keel"? > it's a special case. In this case a gadget driver wants > to use a whole available MaxPacketSize of claimed > endpoint. Since it doesn't know what MaxPacketSize > may be in a particular UDC endpoint, it just > relies on epautoconf core and gets what's available I don't see the wMaxPacketSize == 0 case in the code today, so why are you adding that? And this really isn't "smallest endpoints", it is "find the best fit" > > Signed-off-by: Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx> > --- > > v3: updated commit msg, rebased onto latest gregkh/usb-next > v2: rebased onto latest balbi/next branch > v1: https://lore.kernel.org/lkml/20200629200551.27040-1-ruslan.bilovol@xxxxxxxxx/ > > 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 ed5a92c474e5..086bb46e3f5a 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 What do you mean by "heavier"? This is a "find the smallest endpoint to fit the request" type of logic, right? If so, please say just that. > + */ > 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; > + 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; The fail comment should be on the return NULL line, or better yet, rewritten to say: /* If we found no endpoint that fits, then fail the request */ thanks, greg k-h
