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:     Pawel Laszczak <pawell@cadence.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>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "jbergsagel@ti.com" <jbergsagel@ti.com>,
        "nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
        "kurahul@cadence.com" <kurahul@cadence.com>,
        "jpawar@cadence.com" <jpawar@cadence.com>,
        "Thinh.Nguyen@synopsys.com" <Thinh.Nguyen@synopsys.com>
Subject: Re: [PATCH v2] usb: gadget: Fix issue with config_ep_by_speed
 function.
Thread-Topic: [PATCH v2] usb: gadget: Fix issue with config_ep_by_speed
 function.
Thread-Index: AQHWLXLWiBgnUIlSFEeJ2Nea8YHp9g==
Date:   Tue, 19 May 2020 00:17:09 +0000
Message-ID: <20200519001734.GA2659@b29397-desktop>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6B0F89A0A4786C459368F444A6FD6A43@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b46cb641-87cc-4e3e-88be-08d7fb89f8cc
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 May 2020 00:17:09.8464
 (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: ufucYVG/UqwbF4Vgzj0+KS1JA0dTroxxoTSpq/YpGktVFCj0oyBX4pPyy3KAOqEfddZQYLxqUPn4nsSHMe0f/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6887
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Delete '.' at the end of subject.

Reply-To:=20
In-Reply-To: <20200518100844.28293-1-pawell@cadence.com>

On 20-05-18 12:08:45, Pawel Laszczak wrote:
> This patch adds new config_ep_by_speed_and_alt function which
> extends the config_ep_by_speed about alt parameter.
> This additional parameter allows to find proper usb_ss_ep_comp_descriptor=
.
>=20
> Problem has appeared during testing f_tcm (BOT/UAS) driver function.
>=20
> f_tcm function for SS use array of headers for both  BOT/UAS alternate
> setting:
>=20
> static struct usb_descriptor_header *uasp_ss_function_desc[] =3D {
>         (struct usb_descriptor_header *) &bot_intf_desc,
>         (struct usb_descriptor_header *) &uasp_ss_bi_desc,
>         (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>         (struct usb_descriptor_header *) &uasp_ss_bo_desc,
>         (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>=20
>         (struct usb_descriptor_header *) &uasp_intf_desc,
>         (struct usb_descriptor_header *) &uasp_ss_bi_desc,
>         (struct usb_descriptor_header *) &uasp_bi_ep_comp_desc,
>         (struct usb_descriptor_header *) &uasp_bi_pipe_desc,
>         (struct usb_descriptor_header *) &uasp_ss_bo_desc,
>         (struct usb_descriptor_header *) &uasp_bo_ep_comp_desc,
>         (struct usb_descriptor_header *) &uasp_bo_pipe_desc,
>         (struct usb_descriptor_header *) &uasp_ss_status_desc,
>         (struct usb_descriptor_header *) &uasp_status_in_ep_comp_desc,
>         (struct usb_descriptor_header *) &uasp_status_pipe_desc,
>         (struct usb_descriptor_header *) &uasp_ss_cmd_desc,
>         (struct usb_descriptor_header *) &uasp_cmd_comp_desc,
>         (struct usb_descriptor_header *) &uasp_cmd_pipe_desc,
>         NULL,
> };
>=20
> The first 5 descriptors are associated with BOT alternate setting,
> and others are associated with UAS.
>=20
> During handling UAS alternate setting f_tcm driver invokes
> config_ep_by_speed and this function sets incorrect companion endpoint
> descriptor in usb_ep object.
>=20
> Instead setting ep->comp_desc to uasp_bi_ep_comp_desc function in this
> case set ep->comp_desc to uasp_ss_bi_desc.
>=20
> This is due to the fact that it searches endpoint based on endpoint
> address:
>=20
>         for_each_ep_desc(speed_desc, d_spd) {
>                 chosen_desc =3D (struct usb_endpoint_descriptor *)*d_spd;
>                 if (chosen_desc->bEndpoitAddress =3D=3D _ep->address)
>                         goto ep_found;
>         }
>=20
> And in result it uses the descriptor from BOT alternate setting
> instead UAS.
>=20
> Finally, it causes that controller driver during enabling endpoints
> detect that just enabled endpoint for bot.
>=20
> Signed-off-by: Jayshri Pawar <jpawar@cadence.com>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
> Changelog:
> v2
> - redefine config_ep_by_speed() instead removing it.
> - remove changes from f_tcm.c file.

I think you may add f_tcm.c changes as the second patch,
since it is the user for this patch.

Peter
>=20
>  drivers/usb/gadget/composite.c | 78 ++++++++++++++++++++++++++--------
>  include/linux/usb/composite.h  |  3 ++
>  2 files changed, 64 insertions(+), 17 deletions(-)
>=20
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composit=
e.c
> index 3b4f67000315..858a71fbcd13 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -96,40 +96,43 @@ function_descriptors(struct usb_function *f,
>  }
> =20
>  /**
> - * next_ep_desc() - advance to the next EP descriptor
> + * next_desc() - advance to the next desc_type descriptor
>   * @t: currect pointer within descriptor array
> + * @desc_type: descriptor type
>   *
> - * Return: next EP descriptor or NULL
> + * Return: next desc_type descriptor or NULL
>   *
> - * Iterate over @t until either EP descriptor found or
> + * Iterate over @t until either desc_type descriptor found or
>   * NULL (that indicates end of list) encountered
>   */
>  static struct usb_descriptor_header**
> -next_ep_desc(struct usb_descriptor_header **t)
> +next_desc(struct usb_descriptor_header **t, u8 desc_type)
>  {
>  	for (; *t; t++) {
> -		if ((*t)->bDescriptorType =3D=3D USB_DT_ENDPOINT)
> +		if ((*t)->bDescriptorType =3D=3D desc_type)
>  			return t;
>  	}
>  	return NULL;
>  }
> =20
>  /*
> - * for_each_ep_desc()- iterate over endpoint descriptors in the
> - *		descriptors list
> - * @start:	pointer within descriptor array.
> - * @ep_desc:	endpoint descriptor to use as the loop cursor
> + * for_each_desc() - iterate over desc_type descriptors in the
> + * descriptors list
> + * @start: pointer within descriptor array.
> + * @iter_desc: desc_type descriptor to use as the loop cursor
> + * @desc_type: wanted descriptr type
>   */
> -#define for_each_ep_desc(start, ep_desc) \
> -	for (ep_desc =3D next_ep_desc(start); \
> -	      ep_desc; ep_desc =3D next_ep_desc(ep_desc+1))
> +#define for_each_desc(start, iter_desc, desc_type) \
> +	for (iter_desc =3D next_desc(start, desc_type); \
> +	     iter_desc; iter_desc =3D next_desc(iter_desc + 1, desc_type))
> =20
>  /**
> - * config_ep_by_speed() - configures the given endpoint
> + * config_ep_by_speed_and_alt() - configures the given endpoint
>   * according to gadget speed.
>   * @g: pointer to the gadget
>   * @f: usb function
>   * @_ep: the endpoint to configure
> + * @alt: alternate setting number
>   *
>   * Return: error code, 0 on success
>   *
> @@ -142,11 +145,13 @@ next_ep_desc(struct usb_descriptor_header **t)
>   * Note: the supplied function should hold all the descriptors
>   * for supported speeds
>   */
> -int config_ep_by_speed(struct usb_gadget *g,
> -			struct usb_function *f,
> -			struct usb_ep *_ep)
> +int config_ep_by_speed_and_alt(struct usb_gadget *g,
> +				struct usb_function *f,
> +				struct usb_ep *_ep,
> +				u8 alt)
>  {
>  	struct usb_endpoint_descriptor *chosen_desc =3D NULL;
> +	struct usb_interface_descriptor *int_desc =3D NULL;
>  	struct usb_descriptor_header **speed_desc =3D NULL;
> =20
>  	struct usb_ss_ep_comp_descriptor *comp_desc =3D NULL;
> @@ -182,8 +187,21 @@ int config_ep_by_speed(struct usb_gadget *g,
>  	default:
>  		speed_desc =3D f->fs_descriptors;
>  	}
> +
> +	/* find correct alternate setting descriptor */
> +	for_each_desc(speed_desc, d_spd, USB_DT_INTERFACE) {
> +		int_desc =3D (struct usb_interface_descriptor *)*d_spd;
> +
> +		if (int_desc->bAlternateSetting =3D=3D alt) {
> +			speed_desc =3D d_spd;
> +			goto intf_found;
> +		}
> +	}
> +	return -EIO;
> +
> +intf_found:
>  	/* find descriptors */
> -	for_each_ep_desc(speed_desc, d_spd) {
> +	for_each_desc(speed_desc, d_spd, USB_DT_ENDPOINT) {
>  		chosen_desc =3D (struct usb_endpoint_descriptor *)*d_spd;
>  		if (chosen_desc->bEndpointAddress =3D=3D _ep->address)
>  			goto ep_found;
> @@ -237,6 +255,32 @@ int config_ep_by_speed(struct usb_gadget *g,
>  	}
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(config_ep_by_speed_and_alt);
> +
> +/**
> + * config_ep_by_speed() - configures the given endpoint
> + * according to gadget speed.
> + * @g: pointer to the gadget
> + * @f: usb function
> + * @_ep: the endpoint to configure
> + *
> + * Return: error code, 0 on success
> + *
> + * This function chooses the right descriptors for a given
> + * endpoint according to gadget speed and saves it in the
> + * endpoint desc field. If the endpoint already has a descriptor
> + * assigned to it - overwrites it with currently corresponding
> + * descriptor. The endpoint maxpacket field is updated according
> + * to the chosen descriptor.
> + * Note: the supplied function should hold all the descriptors
> + * for supported speeds
> + */
> +int config_ep_by_speed(struct usb_gadget *g,
> +			struct usb_function *f,
> +			struct usb_ep *_ep)
> +{
> +	return config_ep_by_speed_and_alt(g, f, _ep, 0);
> +}
>  EXPORT_SYMBOL_GPL(config_ep_by_speed);
> =20
>  /**
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.=
h
> index 8675e145ea8b..2040696d75b6 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -249,6 +249,9 @@ int usb_function_activate(struct usb_function *);
> =20
>  int usb_interface_id(struct usb_configuration *, struct usb_function *);
> =20
> +int config_ep_by_speed_and_alt(struct usb_gadget *g, struct usb_function=
 *f,
> +				struct usb_ep *_ep, u8 alt);
> +
>  int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
>  			struct usb_ep *_ep);
> =20
> --=20
> 2.17.1
>=20

--=20

Thanks,
Peter Chen=