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;
From:   Peter Chen <peter.chen@nxp.com>
To:     Ruslan Bilovol <ruslan.bilovol@gmail.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>
Subject: Re: [PATCH v2] usb: gadget: epautoconf: claim smallest endpoints
 first
Thread-Topic: [PATCH v2] usb: gadget: epautoconf: claim smallest endpoints
 first
Thread-Index: AQHWTlKTg5pjRWaycU+d+M1I+15YcajwZ1GAgAVKcYCAAZFcAA==
Date:   Sat, 4 Jul 2020 10:42:49 +0000
Message-ID: <20200704104258.GB5695@b29397-desktop>
References: <20200629201845.28138-1-ruslan.bilovol@gmail.com>
 <20200630015843.GB12443@b29397-desktop>
 <CAB=otbSnP7ecO9W5osxqSHSa4JRCUU4KR-g2BjBweDBUwjWobA@mail.gmail.com>
In-Reply-To: <CAB=otbSnP7ecO9W5osxqSHSa4JRCUU4KR-g2BjBweDBUwjWobA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3309747428F6324B90A630E7351E5EE3@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9fcc686f-8752-46b8-bd9a-08d82006ff49
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Jul 2020 10:42:49.6426
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CPUVYNPvUCXesKmEg/03dMf3pEQAG6ziNDjFheCO6VotMOvfB7w7tB7koawYOd65sNkl3J9UF9kWVo5tWUJhOA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR04MB3026
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 20-07-03 13:46:27, Ruslan Bilovol wrote:
> 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/epa=
utoconf.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 =3D NULL;
> > >
> > >       if (gadget->ops->match_ep) {
> > >               ep =3D 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 usabl=
e */
> > > +     /*
> > > +      * Second, look at endpoints until an unclaimed one looks usabl=
e.
> > > +      * Try to find one with smallest maxpacket limit, leaving large=
r
> > > +      * 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 =3D=3D 0)
> > > +                             goto found_ep;
> >
> > Why you do special handling for this? You still could give the smallest
> > maxpacket_limit EP for it, right?
>=20
> Of course it's technically possible. However in case "wMaxPacketSize =3D=
=3D 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 =3D=3D 0" or not. And you changed the match logic
as returning the smallest maxPacketsize endpoint, you also don't need
to consider whether "wMaxPacketSize =3D=3D 0" or not, otherwise, it may
introduce the complexity.

Peter

>=20
> As I can see, providing smallest endpoint that matches requested wMaxPack=
etSize
> 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 wMaxPacket=
Size
> is zero, find endpoint with maximum possible wMaxPacketSize
>=20
> Does it make sense?
>=20
> Thanks
> Ruslan
>=20
> >
> > Peter
> >
> > > +                     else if (!ep_min)
> > > +                             ep_min =3D ep;
> > > +                     else if (ep->maxpacket_limit < ep_min->maxpacke=
t_limit)
> > > +                             ep_min =3D ep;
> > > +             }
> > >       }
> > >
> > >       /* Fail */
> > > -     return NULL;
> > > +     if (!ep_min)
> > > +             return NULL;
> > > +
> > > +     ep =3D ep_min;
> > >  found_ep:
> > >
> > >       /*
> > > --
> > > 2.17.1
> > >
> >
> > --
> >
> > Thanks,
> > Peter Chen

--=20

Thanks,
Peter Chen=