Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200629201845.28138-1-ruslan.bilovol@gmail.com> <20200630015843.GB12443@b29397-desktop>
In-Reply-To: <20200630015843.GB12443@b29397-desktop>
From:   Ruslan Bilovol <ruslan.bilovol@gmail.com>
Date:   Fri, 3 Jul 2020 13:46:27 +0300
Message-ID: <CAB=otbSnP7ecO9W5osxqSHSa4JRCUU4KR-g2BjBweDBUwjWobA@mail.gmail.com>
Subject: Re: [PATCH v2] usb: gadget: epautoconf: claim smallest endpoints first
To:     Peter Chen <peter.chen@nxp.com>
Cc:     "balbi@kernel.org" <balbi@kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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