| Message ID | 20230312224836.297793-1-ruslan.bilovol@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: gadget: epautoconf: claim smallest endpoints first | expand |
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@gmail.com> > --- > > 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@gmail.com/ > > 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
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 + */ 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; + + ep = ep_min; found_ep: /*
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. Keel old behavior when "wMaxPacketSize == 0" because 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 Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com> --- 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@gmail.com/ drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)