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+15YcajwZ1GA
Date:   Tue, 30 Jun 2020 01:58:19 +0000
Message-ID: <20200630015843.GB12443@b29397-desktop>
References: <20200629201845.28138-1-ruslan.bilovol@gmail.com>
In-Reply-To: <20200629201845.28138-1-ruslan.bilovol@gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D32088B65AECB64C9F8E0F709B056022@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 39b77814-5200-46ac-3fc9-08d81c991003
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Jun 2020 01:58:19.6112
 (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: CRCwtQrEv7w7K2EaXw126Xv5dVG8/5sATlxZvDhPBpmJziGB3/Mh0p0xCwH/joSYftaRbyl/2vchbRmI0jGnfw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB6551
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-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.
>=20
> 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.
>=20
> 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.
>=20
> To make endpoint allocation fair, pick up smallest
> matching endpoints first, leaving bigger ones for
> heavier applications.
>=20
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> ---
>=20
> v2: rebased onto latest balbi/next branch
>=20
>  drivers/usb/gadget/epautoconf.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>=20
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoc=
onf.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;
> =20
>  	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;
>  	}
> =20
> -	/* 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 =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?

Peter

> +			else if (!ep_min)
> +				ep_min =3D ep;
> +			else if (ep->maxpacket_limit < ep_min->maxpacket_limit)
> +				ep_min =3D ep;
> +		}
>  	}
> =20
>  	/* Fail */
> -	return NULL;
> +	if (!ep_min)
> +		return NULL;
> +
> +	ep =3D ep_min;
>  found_ep:
> =20
>  	/*
> --=20
> 2.17.1
>=20

--=20

Thanks,
Peter Chen=