[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZCP34MKN3PyOQB6v@kroah.com>
Date: Wed, 29 Mar 2023 10:33:36 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Ruslan Bilovol <ruslan.bilovol@...il.com>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
peter.chen@...nel.org
Subject: Re: [PATCH v3] usb: gadget: epautoconf: claim smallest endpoints
first
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@...il.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
Powered by blists - more mailing lists