* Re: [PATCH v2] usb: gadget: Fix issue with config_ep_by_speed function.
@ 2020-05-19 0:17 Peter Chen
0 siblings, 0 replies; 2+ messages in thread
From: Peter Chen @ 2020-05-19 0:17 UTC (permalink / raw)
To: Pawel Laszczak
Cc: balbi, gregkh, linux-usb, linux-kernel, rogerq, jbergsagel,
nsekhar, nm, kurahul, jpawar, Thinh.Nguyen
Delete '.' at the end of subject.
Reply-To:
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.
>
> Problem has appeared during testing f_tcm (BOT/UAS) driver function.
>
> f_tcm function for SS use array of headers for both BOT/UAS alternate
> setting:
>
> static struct usb_descriptor_header *uasp_ss_function_desc[] = {
> (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,
>
> (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,
> };
>
> The first 5 descriptors are associated with BOT alternate setting,
> and others are associated with UAS.
>
> 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.
>
> Instead setting ep->comp_desc to uasp_bi_ep_comp_desc function in this
> case set ep->comp_desc to uasp_ss_bi_desc.
>
> This is due to the fact that it searches endpoint based on endpoint
> address:
>
> for_each_ep_desc(speed_desc, d_spd) {
> chosen_desc = (struct usb_endpoint_descriptor *)*d_spd;
> if (chosen_desc->bEndpoitAddress == _ep->address)
> goto ep_found;
> }
>
> And in result it uses the descriptor from BOT alternate setting
> instead UAS.
>
> Finally, it causes that controller driver during enabling endpoints
> detect that just enabled endpoint for bot.
>
> 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
>
> drivers/usb/gadget/composite.c | 78 ++++++++++++++++++++++++++--------
> include/linux/usb/composite.h | 3 ++
> 2 files changed, 64 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.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,
> }
>
> /**
> - * 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 == USB_DT_ENDPOINT)
> + if ((*t)->bDescriptorType == desc_type)
> return t;
> }
> return NULL;
> }
>
> /*
> - * 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 = next_ep_desc(start); \
> - ep_desc; ep_desc = next_ep_desc(ep_desc+1))
> +#define for_each_desc(start, iter_desc, desc_type) \
> + for (iter_desc = next_desc(start, desc_type); \
> + iter_desc; iter_desc = next_desc(iter_desc + 1, desc_type))
>
> /**
> - * 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 = NULL;
> + struct usb_interface_descriptor *int_desc = NULL;
> struct usb_descriptor_header **speed_desc = NULL;
>
> struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
> @@ -182,8 +187,21 @@ int config_ep_by_speed(struct usb_gadget *g,
> default:
> speed_desc = f->fs_descriptors;
> }
> +
> + /* find correct alternate setting descriptor */
> + for_each_desc(speed_desc, d_spd, USB_DT_INTERFACE) {
> + int_desc = (struct usb_interface_descriptor *)*d_spd;
> +
> + if (int_desc->bAlternateSetting == alt) {
> + speed_desc = 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 = (struct usb_endpoint_descriptor *)*d_spd;
> if (chosen_desc->bEndpointAddress == _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);
>
> /**
> 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 *);
>
> int usb_interface_id(struct usb_configuration *, struct usb_function *);
>
> +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);
>
> --
> 2.17.1
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH v2] usb: gadget: Fix issue with config_ep_by_speed function.
@ 2020-05-18 10:08 Pawel Laszczak
0 siblings, 0 replies; 2+ messages in thread
From: Pawel Laszczak @ 2020-05-18 10:08 UTC (permalink / raw)
To: balbi, gregkh
Cc: linux-usb, linux-kernel, rogerq, jbergsagel, nsekhar, nm,
kurahul, jpawar, Thinh.Nguyen, Pawel Laszczak
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.
Problem has appeared during testing f_tcm (BOT/UAS) driver function.
f_tcm function for SS use array of headers for both BOT/UAS alternate
setting:
static struct usb_descriptor_header *uasp_ss_function_desc[] = {
(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,
(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,
};
The first 5 descriptors are associated with BOT alternate setting,
and others are associated with UAS.
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.
Instead setting ep->comp_desc to uasp_bi_ep_comp_desc function in this
case set ep->comp_desc to uasp_ss_bi_desc.
This is due to the fact that it searches endpoint based on endpoint
address:
for_each_ep_desc(speed_desc, d_spd) {
chosen_desc = (struct usb_endpoint_descriptor *)*d_spd;
if (chosen_desc->bEndpoitAddress == _ep->address)
goto ep_found;
}
And in result it uses the descriptor from BOT alternate setting
instead UAS.
Finally, it causes that controller driver during enabling endpoints
detect that just enabled endpoint for bot.
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.
drivers/usb/gadget/composite.c | 78 ++++++++++++++++++++++++++--------
include/linux/usb/composite.h | 3 ++
2 files changed, 64 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.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,
}
/**
- * 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 == USB_DT_ENDPOINT)
+ if ((*t)->bDescriptorType == desc_type)
return t;
}
return NULL;
}
/*
- * 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 = next_ep_desc(start); \
- ep_desc; ep_desc = next_ep_desc(ep_desc+1))
+#define for_each_desc(start, iter_desc, desc_type) \
+ for (iter_desc = next_desc(start, desc_type); \
+ iter_desc; iter_desc = next_desc(iter_desc + 1, desc_type))
/**
- * 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 = NULL;
+ struct usb_interface_descriptor *int_desc = NULL;
struct usb_descriptor_header **speed_desc = NULL;
struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
@@ -182,8 +187,21 @@ int config_ep_by_speed(struct usb_gadget *g,
default:
speed_desc = f->fs_descriptors;
}
+
+ /* find correct alternate setting descriptor */
+ for_each_desc(speed_desc, d_spd, USB_DT_INTERFACE) {
+ int_desc = (struct usb_interface_descriptor *)*d_spd;
+
+ if (int_desc->bAlternateSetting == alt) {
+ speed_desc = 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 = (struct usb_endpoint_descriptor *)*d_spd;
if (chosen_desc->bEndpointAddress == _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);
/**
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 *);
int usb_interface_id(struct usb_configuration *, struct usb_function *);
+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);
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-19 0:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 0:17 [PATCH v2] usb: gadget: Fix issue with config_ep_by_speed function Peter Chen
-- strict thread matches above, loose matches on Subject: below --
2020-05-18 10:08 Pawel Laszczak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.