Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
@ 2020-02-12 10:19 Jayshri Pawar
  2020-02-14  3:40 ` Thinh Nguyen
  0 siblings, 1 reply; 6+ messages in thread
From: Jayshri Pawar @ 2020-02-12 10:19 UTC (permalink / raw)
  To: linux-usb
  Cc: gregkh, balbi, heikki.krogerus, rogerq, linux-kernel, jbergsagel,
	nsekhar, nm, peter.chen, kurahul, pawell, sparmar, jpawar

This patch adds additional parameter alt to config_ep_by_speed function.
This additional parameter allows to improve this function and
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 *) &bot_uasp_ss_bi_desc,
        (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
        (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
        (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,

        (struct usb_descriptor_header *) &uasp_intf_desc,
        (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.

This is due to the fact that it search 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>
---
 drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
 drivers/usb/gadget/function/f_acm.c          |  7 +--
 drivers/usb/gadget/function/f_ecm.c          |  7 +--
 drivers/usb/gadget/function/f_eem.c          |  4 +-
 drivers/usb/gadget/function/f_fs.c           |  3 +-
 drivers/usb/gadget/function/f_hid.c          |  4 +-
 drivers/usb/gadget/function/f_loopback.c     |  2 +-
 drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
 drivers/usb/gadget/function/f_midi.c         |  2 +-
 drivers/usb/gadget/function/f_ncm.c          |  7 +--
 drivers/usb/gadget/function/f_obex.c         |  4 +-
 drivers/usb/gadget/function/f_phonet.c       |  4 +-
 drivers/usb/gadget/function/f_rndis.c        |  7 +--
 drivers/usb/gadget/function/f_serial.c       |  4 +-
 drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
 drivers/usb/gadget/function/f_subset.c       |  4 +-
 drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
 drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
 drivers/usb/gadget/function/f_uvc.c          |  5 ++-
 drivers/usb/gadget/function/u_audio.c        |  4 +-
 include/linux/usb/composite.h                |  2 +-
 21 files changed, 99 insertions(+), 71 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 3b4f67000315..a9df15ad2fcf 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -96,33 +96,35 @@ 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
@@ -130,6 +132,7 @@ next_ep_desc(struct usb_descriptor_header **t)
  * @g: pointer to the gadget
  * @f: usb function
  * @_ep: the endpoint to configure
+ * @alt: alternate setting number
  *
  * Return: error code, 0 on success
  *
@@ -144,9 +147,11 @@ next_ep_desc(struct usb_descriptor_header **t)
  */
 int config_ep_by_speed(struct usb_gadget *g,
 			struct usb_function *f,
-			struct usb_ep *_ep)
+			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;
diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 7c152c28b26c..0be4a07f1624 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -430,7 +430,8 @@ static int acm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		usb_ep_disable(acm->notify);
 
 		if (!acm->notify->desc)
-			if (config_ep_by_speed(cdev->gadget, f, acm->notify))
+			if (config_ep_by_speed(cdev->gadget, f,
+					       acm->notify, alt))
 				return -EINVAL;
 
 		usb_ep_enable(acm->notify);
@@ -445,9 +446,9 @@ static int acm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			dev_dbg(&cdev->gadget->dev,
 				"activate acm ttyGS%d\n", acm->port_num);
 			if (config_ep_by_speed(cdev->gadget, f,
-					       acm->port.in) ||
+					       acm->port.in, alt) ||
 			    config_ep_by_speed(cdev->gadget, f,
-					       acm->port.out)) {
+					       acm->port.out, alt)) {
 				acm->port.in->desc = NULL;
 				acm->port.out->desc = NULL;
 				return -EINVAL;
diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index 7f5cf488b2b1..e5a122cdaa98 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -544,7 +544,8 @@ static int ecm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		usb_ep_disable(ecm->notify);
 		if (!(ecm->notify->desc)) {
 			VDBG(cdev, "init ecm ctrl %d\n", intf);
-			if (config_ep_by_speed(cdev->gadget, f, ecm->notify))
+			if (config_ep_by_speed(cdev->gadget, f,
+					       ecm->notify, alt))
 				goto fail;
 		}
 		usb_ep_enable(ecm->notify);
@@ -563,9 +564,9 @@ static int ecm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		    !ecm->port.out_ep->desc) {
 			DBG(cdev, "init ecm\n");
 			if (config_ep_by_speed(cdev->gadget, f,
-					       ecm->port.in_ep) ||
+					       ecm->port.in_ep, alt) ||
 			    config_ep_by_speed(cdev->gadget, f,
-					       ecm->port.out_ep)) {
+					       ecm->port.out_ep, alt)) {
 				ecm->port.in_ep->desc = NULL;
 				ecm->port.out_ep->desc = NULL;
 				goto fail;
diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
index b81a91d504bd..2cfb1ceb45f1 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -196,9 +196,9 @@ static int eem_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (!eem->port.in_ep->desc || !eem->port.out_ep->desc) {
 			DBG(cdev, "init eem\n");
 			if (config_ep_by_speed(cdev->gadget, f,
-					       eem->port.in_ep) ||
+					       eem->port.in_ep, alt) ||
 			    config_ep_by_speed(cdev->gadget, f,
-					       eem->port.out_ep)) {
+					       eem->port.out_ep, alt)) {
 				eem->port.in_ep->desc = NULL;
 				eem->port.out_ep->desc = NULL;
 				goto fail;
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 6171d28331e6..374b3b23cd02 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1950,7 +1950,8 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 	while(count--) {
 		ep->ep->driver_data = ep;
 
-		ret = config_ep_by_speed(func->gadget, &func->function, ep->ep);
+		ret = config_ep_by_speed(func->gadget, &func->function,
+					 ep->ep, 0);
 		if (ret) {
 			pr_err("%s: config_ep_by_speed(%s) returned %d\n",
 					__func__, ep->ep->name, ret);
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index f3816a5c861e..9cbf622dc33c 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -647,7 +647,7 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		usb_ep_disable(hidg->in_ep);
 
 		status = config_ep_by_speed(f->config->cdev->gadget, f,
-					    hidg->in_ep);
+					    hidg->in_ep, alt);
 		if (status) {
 			ERROR(cdev, "config_ep_by_speed FAILED!\n");
 			goto fail;
@@ -672,7 +672,7 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		usb_ep_disable(hidg->out_ep);
 
 		status = config_ep_by_speed(f->config->cdev->gadget, f,
-					    hidg->out_ep);
+					    hidg->out_ep, alt);
 		if (status) {
 			ERROR(cdev, "config_ep_by_speed FAILED!\n");
 			goto free_req_in;
diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c
index 1803646b3678..6ff45212bf8e 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -362,7 +362,7 @@ static int enable_endpoint(struct usb_composite_dev *cdev,
 {
 	int					result;
 
-	result = config_ep_by_speed(cdev->gadget, &(loop->function), ep);
+	result = config_ep_by_speed(cdev->gadget, &loop->function, ep, 0);
 	if (result)
 		goto out;
 
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 7c96c4665178..9aa9fd4e4785 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2241,7 +2241,8 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg)
 	fsg = common->fsg;
 
 	/* Enable the endpoints */
-	rc = config_ep_by_speed(common->gadget, &(fsg->function), fsg->bulk_in);
+	rc = config_ep_by_speed(common->gadget, &fsg->function,
+				fsg->bulk_in, 0);
 	if (rc)
 		goto reset;
 	rc = usb_ep_enable(fsg->bulk_in);
@@ -2251,7 +2252,7 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg)
 	fsg->bulk_in_enabled = 1;
 
 	rc = config_ep_by_speed(common->gadget, &(fsg->function),
-				fsg->bulk_out);
+				fsg->bulk_out, 0);
 	if (rc)
 		goto reset;
 	rc = usb_ep_enable(fsg->bulk_out);
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 46af0aa07e2e..f371ef38a251 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -343,7 +343,7 @@ static int f_midi_start_ep(struct f_midi *midi,
 
 	usb_ep_disable(ep);
 
-	err = config_ep_by_speed(midi->gadget, f, ep);
+	err = config_ep_by_speed(midi->gadget, f, ep, 0);
 	if (err) {
 		ERROR(cdev, "can't configure %s: %d\n", ep->name, err);
 		return err;
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 1d900081b1f0..f5c1ad67aa58 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -876,7 +876,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 
 		if (!(ncm->notify->desc)) {
 			DBG(cdev, "init ncm ctrl %d\n", intf);
-			if (config_ep_by_speed(cdev->gadget, f, ncm->notify))
+			if (config_ep_by_speed(cdev->gadget, f,
+					       ncm->notify, alt))
 				goto fail;
 		}
 		usb_ep_enable(ncm->notify);
@@ -905,9 +906,9 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			    !ncm->port.out_ep->desc) {
 				DBG(cdev, "init ncm\n");
 				if (config_ep_by_speed(cdev->gadget, f,
-						       ncm->port.in_ep) ||
+						       ncm->port.in_ep, alt) ||
 				    config_ep_by_speed(cdev->gadget, f,
-						       ncm->port.out_ep)) {
+						       ncm->port.out_ep, alt)) {
 					ncm->port.in_ep->desc = NULL;
 					ncm->port.out_ep->desc = NULL;
 					goto fail;
diff --git a/drivers/usb/gadget/function/f_obex.c b/drivers/usb/gadget/function/f_obex.c
index ab26d84ed95e..bacceced3bba 100644
--- a/drivers/usb/gadget/function/f_obex.c
+++ b/drivers/usb/gadget/function/f_obex.c
@@ -212,9 +212,9 @@ static int obex_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			dev_dbg(&cdev->gadget->dev,
 				"init obex ttyGS%d\n", obex->port_num);
 			if (config_ep_by_speed(cdev->gadget, f,
-					       obex->port.in) ||
+					       obex->port.in, alt) ||
 			    config_ep_by_speed(cdev->gadget, f,
-					       obex->port.out)) {
+					       obex->port.out, alt)) {
 				obex->port.out->desc = NULL;
 				obex->port.in->desc = NULL;
 				goto fail;
diff --git a/drivers/usb/gadget/function/f_phonet.c b/drivers/usb/gadget/function/f_phonet.c
index 8b72b192c747..e1f04447c6da 100644
--- a/drivers/usb/gadget/function/f_phonet.c
+++ b/drivers/usb/gadget/function/f_phonet.c
@@ -416,8 +416,8 @@ static int pn_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (alt == 1) {
 			int i;
 
-			if (config_ep_by_speed(gadget, f, fp->in_ep) ||
-			    config_ep_by_speed(gadget, f, fp->out_ep)) {
+			if (config_ep_by_speed(gadget, f, fp->in_ep, alt) ||
+			    config_ep_by_speed(gadget, f, fp->out_ep, alt)) {
 				fp->in_ep->desc = NULL;
 				fp->out_ep->desc = NULL;
 				spin_unlock(&port->lock);
diff --git a/drivers/usb/gadget/function/f_rndis.c b/drivers/usb/gadget/function/f_rndis.c
index 0d8e4a364ca6..d5dde0e2f2ef 100644
--- a/drivers/usb/gadget/function/f_rndis.c
+++ b/drivers/usb/gadget/function/f_rndis.c
@@ -547,7 +547,8 @@ static int rndis_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 
 		if (!rndis->notify->desc) {
 			VDBG(cdev, "init rndis ctrl %d\n", intf);
-			if (config_ep_by_speed(cdev->gadget, f, rndis->notify))
+			if (config_ep_by_speed(cdev->gadget, f,
+					       rndis->notify, alt))
 				goto fail;
 		}
 		usb_ep_enable(rndis->notify);
@@ -563,9 +564,9 @@ static int rndis_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (!rndis->port.in_ep->desc || !rndis->port.out_ep->desc) {
 			DBG(cdev, "init rndis\n");
 			if (config_ep_by_speed(cdev->gadget, f,
-					       rndis->port.in_ep) ||
+					       rndis->port.in_ep, alt) ||
 			    config_ep_by_speed(cdev->gadget, f,
-					       rndis->port.out_ep)) {
+					       rndis->port.out_ep, alt)) {
 				rndis->port.in_ep->desc = NULL;
 				rndis->port.out_ep->desc = NULL;
 				goto fail;
diff --git a/drivers/usb/gadget/function/f_serial.c b/drivers/usb/gadget/function/f_serial.c
index 1406255d0865..784455c57dd5 100644
--- a/drivers/usb/gadget/function/f_serial.c
+++ b/drivers/usb/gadget/function/f_serial.c
@@ -158,8 +158,8 @@ static int gser_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	if (!gser->port.in->desc || !gser->port.out->desc) {
 		dev_dbg(&cdev->gadget->dev,
 			"activate generic ttyGS%d\n", gser->port_num);
-		if (config_ep_by_speed(cdev->gadget, f, gser->port.in) ||
-		    config_ep_by_speed(cdev->gadget, f, gser->port.out)) {
+		if (config_ep_by_speed(cdev->gadget, f, gser->port.in, alt) ||
+		    config_ep_by_speed(cdev->gadget, f, gser->port.out, alt)) {
 			gser->port.in->desc = NULL;
 			gser->port.out->desc = NULL;
 			return -EINVAL;
diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index ed68a4860b7d..86124149de7d 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -650,7 +650,8 @@ enable_source_sink(struct usb_composite_dev *cdev, struct f_sourcesink *ss,
 
 	/* one bulk endpoint writes (sources) zeroes IN (to the host) */
 	ep = ss->in_ep;
-	result = config_ep_by_speed(cdev->gadget, &(ss->function), ep);
+	result = config_ep_by_speed(cdev->gadget, &ss->function,
+				    ep, alt);
 	if (result)
 		return result;
 	result = usb_ep_enable(ep);
@@ -668,7 +669,7 @@ enable_source_sink(struct usb_composite_dev *cdev, struct f_sourcesink *ss,
 
 	/* one bulk endpoint reads (sinks) anything OUT (from the host) */
 	ep = ss->out_ep;
-	result = config_ep_by_speed(cdev->gadget, &(ss->function), ep);
+	result = config_ep_by_speed(cdev->gadget, &ss->function, ep, alt);
 	if (result)
 		goto fail;
 	result = usb_ep_enable(ep);
@@ -690,7 +691,8 @@ enable_source_sink(struct usb_composite_dev *cdev, struct f_sourcesink *ss,
 	/* one iso endpoint writes (sources) zeroes IN (to the host) */
 	ep = ss->iso_in_ep;
 	if (ep) {
-		result = config_ep_by_speed(cdev->gadget, &(ss->function), ep);
+		result = config_ep_by_speed(cdev->gadget, &ss->function,
+					    ep, alt);
 		if (result)
 			goto fail2;
 		result = usb_ep_enable(ep);
@@ -711,7 +713,8 @@ enable_source_sink(struct usb_composite_dev *cdev, struct f_sourcesink *ss,
 	/* one iso endpoint reads (sinks) anything OUT (from the host) */
 	ep = ss->iso_out_ep;
 	if (ep) {
-		result = config_ep_by_speed(cdev->gadget, &(ss->function), ep);
+		result = config_ep_by_speed(cdev->gadget, &ss->function,
+					    ep, alt);
 		if (result)
 			goto fail3;
 		result = usb_ep_enable(ep);
diff --git a/drivers/usb/gadget/function/f_subset.c b/drivers/usb/gadget/function/f_subset.c
index 4d945254905d..5362ae567f6c 100644
--- a/drivers/usb/gadget/function/f_subset.c
+++ b/drivers/usb/gadget/function/f_subset.c
@@ -264,8 +264,8 @@ static int geth_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	}
 
 	DBG(cdev, "init + activate cdc subset\n");
-	if (config_ep_by_speed(cdev->gadget, f, geth->port.in_ep) ||
-	    config_ep_by_speed(cdev->gadget, f, geth->port.out_ep)) {
+	if (config_ep_by_speed(cdev->gadget, f, geth->port.in_ep, alt) ||
+	    config_ep_by_speed(cdev->gadget, f, geth->port.out_ep, alt)) {
 		geth->port.in_ep->desc = NULL;
 		geth->port.out_ep->desc = NULL;
 		return -EINVAL;
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 36504931b2d1..3c564629d268 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -392,12 +392,12 @@ static void bot_set_alt(struct f_uas *fu)
 
 	fu->flags = USBG_IS_BOT;
 
-	config_ep_by_speed(gadget, f, fu->ep_in);
+	config_ep_by_speed(gadget, f, fu->ep_in, USB_G_ALT_INT_BBB);
 	ret = usb_ep_enable(fu->ep_in);
 	if (ret)
 		goto err_b_in;
 
-	config_ep_by_speed(gadget, f, fu->ep_out);
+	config_ep_by_speed(gadget, f, fu->ep_out, USB_G_ALT_INT_BBB);
 	ret = usb_ep_enable(fu->ep_out);
 	if (ret)
 		goto err_b_out;
@@ -849,21 +849,21 @@ static void uasp_set_alt(struct f_uas *fu)
 	if (gadget->speed >= USB_SPEED_SUPER)
 		fu->flags |= USBG_USE_STREAMS;
 
-	config_ep_by_speed(gadget, f, fu->ep_in);
+	config_ep_by_speed(gadget, f, fu->ep_in, USB_G_ALT_INT_UAS);
 	ret = usb_ep_enable(fu->ep_in);
 	if (ret)
 		goto err_b_in;
 
-	config_ep_by_speed(gadget, f, fu->ep_out);
+	config_ep_by_speed(gadget, f, fu->ep_out, USB_G_ALT_INT_UAS);
 	ret = usb_ep_enable(fu->ep_out);
 	if (ret)
 		goto err_b_out;
 
-	config_ep_by_speed(gadget, f, fu->ep_cmd);
+	config_ep_by_speed(gadget, f, fu->ep_cmd, USB_G_ALT_INT_UAS);
 	ret = usb_ep_enable(fu->ep_cmd);
 	if (ret)
 		goto err_cmd;
-	config_ep_by_speed(gadget, f, fu->ep_status);
+	config_ep_by_speed(gadget, f, fu->ep_status, USB_G_ALT_INT_UAS);
 	ret = usb_ep_enable(fu->ep_status);
 	if (ret)
 		goto err_status;
@@ -1780,7 +1780,7 @@ static struct usb_pipe_usage_descriptor uasp_bi_pipe_desc = {
 	.bPipeID =		DATA_IN_PIPE_ID,
 };
 
-static struct usb_endpoint_descriptor uasp_ss_bi_desc = {
+static struct usb_endpoint_descriptor bot_uasp_ss_bi_desc = {
 	.bLength =		USB_DT_ENDPOINT_SIZE,
 	.bDescriptorType =	USB_DT_ENDPOINT,
 	.bEndpointAddress =	USB_DIR_IN,
@@ -1823,7 +1823,7 @@ static struct usb_pipe_usage_descriptor uasp_bo_pipe_desc = {
 	.bPipeID =		DATA_OUT_PIPE_ID,
 };
 
-static struct usb_endpoint_descriptor uasp_ss_bo_desc = {
+static struct usb_endpoint_descriptor bot_uasp_ss_bo_desc = {
 	.bLength =		USB_DT_ENDPOINT_SIZE,
 	.bDescriptorType =	USB_DT_ENDPOINT,
 	.bEndpointAddress =	USB_DIR_OUT,
@@ -1947,16 +1947,16 @@ static struct usb_descriptor_header *uasp_hs_function_desc[] = {
 
 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_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_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 *) &bot_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 *) &bot_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,
@@ -2016,14 +2016,14 @@ static int tcm_bind(struct usb_configuration *c, struct usb_function *f)
 	bot_intf_desc.bInterfaceNumber = iface;
 	uasp_intf_desc.bInterfaceNumber = iface;
 	fu->iface = iface;
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_bi_desc,
+	ep = usb_ep_autoconfig_ss(gadget, &bot_uasp_ss_bi_desc,
 			&uasp_bi_ep_comp_desc);
 	if (!ep)
 		goto ep_fail;
 
 	fu->ep_in = ep;
 
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_bo_desc,
+	ep = usb_ep_autoconfig_ss(gadget, &bot_uasp_ss_bo_desc,
 			&uasp_bo_ep_comp_desc);
 	if (!ep)
 		goto ep_fail;
@@ -2042,14 +2042,14 @@ static int tcm_bind(struct usb_configuration *c, struct usb_function *f)
 	fu->ep_cmd = ep;
 
 	/* Assume endpoint addresses are the same for both speeds */
-	uasp_bi_desc.bEndpointAddress =	uasp_ss_bi_desc.bEndpointAddress;
-	uasp_bo_desc.bEndpointAddress = uasp_ss_bo_desc.bEndpointAddress;
+	uasp_bi_desc.bEndpointAddress =	bot_uasp_ss_bi_desc.bEndpointAddress;
+	uasp_bo_desc.bEndpointAddress = bot_uasp_ss_bo_desc.bEndpointAddress;
 	uasp_status_desc.bEndpointAddress =
 		uasp_ss_status_desc.bEndpointAddress;
 	uasp_cmd_desc.bEndpointAddress = uasp_ss_cmd_desc.bEndpointAddress;
 
-	uasp_fs_bi_desc.bEndpointAddress = uasp_ss_bi_desc.bEndpointAddress;
-	uasp_fs_bo_desc.bEndpointAddress = uasp_ss_bo_desc.bEndpointAddress;
+	uasp_fs_bi_desc.bEndpointAddress = bot_uasp_ss_bi_desc.bEndpointAddress;
+	uasp_fs_bo_desc.bEndpointAddress = bot_uasp_ss_bo_desc.bEndpointAddress;
 	uasp_fs_status_desc.bEndpointAddress =
 		uasp_ss_status_desc.bEndpointAddress;
 	uasp_fs_cmd_desc.bEndpointAddress = uasp_ss_cmd_desc.bEndpointAddress;
diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index 349deae7cabd..0bcebe6e74c3 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -601,7 +601,7 @@ static int f_audio_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		return 0;
 	} else if (intf == audio->as_intf) {
 		if (alt == 1) {
-			err = config_ep_by_speed(cdev->gadget, f, out_ep);
+			err = config_ep_by_speed(cdev->gadget, f, out_ep, alt);
 			if (err)
 				return err;
 
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index fb0a892687c0..7d4a479baebb 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -290,7 +290,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 		usb_ep_disable(uvc->control_ep);
 
 		if (!uvc->control_ep->desc)
-			if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep))
+			if (config_ep_by_speed(cdev->gadget, f,
+					       uvc->control_ep, alt))
 				return -EINVAL;
 
 		usb_ep_enable(uvc->control_ep);
@@ -341,7 +342,7 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 		usb_ep_disable(uvc->video.ep);
 
 		ret = config_ep_by_speed(f->config->cdev->gadget,
-				&(uvc->func), uvc->video.ep);
+					&uvc->func, uvc->video.ep, alt);
 		if (ret)
 			return ret;
 		usb_ep_enable(uvc->video.ep);
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 6d956f190f5a..acaa2288cf35 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -360,7 +360,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 
 	ep = audio_dev->out_ep;
 	prm = &uac->c_prm;
-	config_ep_by_speed(gadget, &audio_dev->func, ep);
+	config_ep_by_speed(gadget, &audio_dev->func, ep, 0);
 	req_len = prm->max_psize;
 
 	prm->ep_enabled = true;
@@ -413,7 +413,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 
 	ep = audio_dev->in_ep;
 	prm = &uac->p_prm;
-	config_ep_by_speed(gadget, &audio_dev->func, ep);
+	config_ep_by_speed(gadget, &audio_dev->func, ep, 0);
 
 	ep_desc = ep->desc;
 
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 8675e145ea8b..73ffc05d69e7 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -250,7 +250,7 @@ int usb_function_activate(struct usb_function *);
 int usb_interface_id(struct usb_configuration *, struct usb_function *);
 
 int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
-			struct usb_ep *_ep);
+			struct usb_ep *_ep, u8 alt);
 
 #define	MAX_CONFIG_INTERFACES		16	/* arbitrary; max 255 */
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
  2020-02-12 10:19 [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function Jayshri Pawar
@ 2020-02-14  3:40 ` Thinh Nguyen
  2020-02-14  4:26   ` Pawel Laszczak
  0 siblings, 1 reply; 6+ messages in thread
From: Thinh Nguyen @ 2020-02-14  3:40 UTC (permalink / raw)
  To: Jayshri Pawar, linux-usb
  Cc: gregkh, balbi, heikki.krogerus, rogerq, linux-kernel, jbergsagel,
	nsekhar, nm, peter.chen, kurahul, pawell, sparmar

Hi,

Jayshri Pawar wrote:
> This patch adds additional parameter alt to config_ep_by_speed function.
> This additional parameter allows to improve this function and
> 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 *) &bot_uasp_ss_bi_desc,
>          (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>          (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
>          (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>
>          (struct usb_descriptor_header *) &uasp_intf_desc,
>          (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.
>
> This is due to the fact that it search 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>
> ---
>   drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
>   drivers/usb/gadget/function/f_acm.c          |  7 +--
>   drivers/usb/gadget/function/f_ecm.c          |  7 +--
>   drivers/usb/gadget/function/f_eem.c          |  4 +-
>   drivers/usb/gadget/function/f_fs.c           |  3 +-
>   drivers/usb/gadget/function/f_hid.c          |  4 +-
>   drivers/usb/gadget/function/f_loopback.c     |  2 +-
>   drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
>   drivers/usb/gadget/function/f_midi.c         |  2 +-
>   drivers/usb/gadget/function/f_ncm.c          |  7 +--
>   drivers/usb/gadget/function/f_obex.c         |  4 +-
>   drivers/usb/gadget/function/f_phonet.c       |  4 +-
>   drivers/usb/gadget/function/f_rndis.c        |  7 +--
>   drivers/usb/gadget/function/f_serial.c       |  4 +-
>   drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
>   drivers/usb/gadget/function/f_subset.c       |  4 +-
>   drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
>   drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
>   drivers/usb/gadget/function/f_uvc.c          |  5 ++-
>   drivers/usb/gadget/function/u_audio.c        |  4 +-
>   include/linux/usb/composite.h                |  2 +-
>   21 files changed, 99 insertions(+), 71 deletions(-)
>

I think we should create a new function and keep the old 
config_ep_by_speed() for default alt-setting (e.i. have 
config_ep_by_speed calls the new function with default alt-setting 0). 
This way, we can keep compatibility with old function drivers and 
minimize changes. At least that's what we did.

BR,
Thinh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
  2020-02-14  3:40 ` Thinh Nguyen
@ 2020-02-14  4:26   ` Pawel Laszczak
  2020-02-14 19:26     ` Thinh Nguyen
  0 siblings, 1 reply; 6+ messages in thread
From: Pawel Laszczak @ 2020-02-14  4:26 UTC (permalink / raw)
  To: Thinh Nguyen, Jayshri Dajiram Pawar, linux-usb
  Cc: gregkh, balbi, heikki.krogerus, rogerq, linux-kernel, jbergsagel,
	nsekhar, nm, peter.chen, Rahul Kumar, Sanket Parmar

Hi,

>Hi,
>
>Jayshri Pawar wrote:
>> This patch adds additional parameter alt to config_ep_by_speed function.
>> This additional parameter allows to improve this function and
>> 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 *) &bot_uasp_ss_bi_desc,
>>          (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>>          (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
>>          (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>>
>>          (struct usb_descriptor_header *) &uasp_intf_desc,
>>          (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.
>>
>> This is due to the fact that it search 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>
>> ---
>>   drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
>>   drivers/usb/gadget/function/f_acm.c          |  7 +--
>>   drivers/usb/gadget/function/f_ecm.c          |  7 +--
>>   drivers/usb/gadget/function/f_eem.c          |  4 +-
>>   drivers/usb/gadget/function/f_fs.c           |  3 +-
>>   drivers/usb/gadget/function/f_hid.c          |  4 +-
>>   drivers/usb/gadget/function/f_loopback.c     |  2 +-
>>   drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
>>   drivers/usb/gadget/function/f_midi.c         |  2 +-
>>   drivers/usb/gadget/function/f_ncm.c          |  7 +--
>>   drivers/usb/gadget/function/f_obex.c         |  4 +-
>>   drivers/usb/gadget/function/f_phonet.c       |  4 +-
>>   drivers/usb/gadget/function/f_rndis.c        |  7 +--
>>   drivers/usb/gadget/function/f_serial.c       |  4 +-
>>   drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
>>   drivers/usb/gadget/function/f_subset.c       |  4 +-
>>   drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
>>   drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
>>   drivers/usb/gadget/function/f_uvc.c          |  5 ++-
>>   drivers/usb/gadget/function/u_audio.c        |  4 +-
>>   include/linux/usb/composite.h                |  2 +-
>>   21 files changed, 99 insertions(+), 71 deletions(-)
>>
>
>I think we should create a new function and keep the old
>config_ep_by_speed() for default alt-setting (e.i. have
>config_ep_by_speed calls the new function with default alt-setting 0).
>This way, we can keep compatibility with old function drivers and
>minimize changes. At least that's what we did.
>

I don't think we need the separate function. 
If we set last parameter alt=0 then this function will work in the same way as old one.

Regards,
Pawel

>BR,
>Thinh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
  2020-02-14  4:26   ` Pawel Laszczak
@ 2020-02-14 19:26     ` Thinh Nguyen
  2020-02-14 19:55       ` Pawel Laszczak
  0 siblings, 1 reply; 6+ messages in thread
From: Thinh Nguyen @ 2020-02-14 19:26 UTC (permalink / raw)
  To: Pawel Laszczak, Thinh Nguyen, Jayshri Dajiram Pawar, linux-usb
  Cc: gregkh, balbi, heikki.krogerus, rogerq, linux-kernel, jbergsagel,
	nsekhar, nm, peter.chen, Rahul Kumar, Sanket Parmar

Pawel Laszczak wrote:
> Hi,
>
>> Hi,
>>
>> Jayshri Pawar wrote:
>>> This patch adds additional parameter alt to config_ep_by_speed function.
>>> This additional parameter allows to improve this function and
>>> 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 *) &bot_uasp_ss_bi_desc,
>>>           (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>>>           (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
>>>           (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>>>
>>>           (struct usb_descriptor_header *) &uasp_intf_desc,
>>>           (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.
>>>
>>> This is due to the fact that it search 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>
>>> ---
>>>    drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
>>>    drivers/usb/gadget/function/f_acm.c          |  7 +--
>>>    drivers/usb/gadget/function/f_ecm.c          |  7 +--
>>>    drivers/usb/gadget/function/f_eem.c          |  4 +-
>>>    drivers/usb/gadget/function/f_fs.c           |  3 +-
>>>    drivers/usb/gadget/function/f_hid.c          |  4 +-
>>>    drivers/usb/gadget/function/f_loopback.c     |  2 +-
>>>    drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
>>>    drivers/usb/gadget/function/f_midi.c         |  2 +-
>>>    drivers/usb/gadget/function/f_ncm.c          |  7 +--
>>>    drivers/usb/gadget/function/f_obex.c         |  4 +-
>>>    drivers/usb/gadget/function/f_phonet.c       |  4 +-
>>>    drivers/usb/gadget/function/f_rndis.c        |  7 +--
>>>    drivers/usb/gadget/function/f_serial.c       |  4 +-
>>>    drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
>>>    drivers/usb/gadget/function/f_subset.c       |  4 +-
>>>    drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
>>>    drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
>>>    drivers/usb/gadget/function/f_uvc.c          |  5 ++-
>>>    drivers/usb/gadget/function/u_audio.c        |  4 +-
>>>    include/linux/usb/composite.h                |  2 +-
>>>    21 files changed, 99 insertions(+), 71 deletions(-)
>>>
>> I think we should create a new function and keep the old
>> config_ep_by_speed() for default alt-setting (e.i. have
>> config_ep_by_speed calls the new function with default alt-setting 0).
>> This way, we can keep compatibility with old function drivers and
>> minimize changes. At least that's what we did.
>>
> I don't think we need the separate function.
> If we set last parameter alt=0 then this function will work in the same way as old one.
>

I wasn't talking about that. This patch modifies the 
config_ep_by_speed() parameters, which requires to make a change to 
every function driver of the kernel, and all in a single patch. This 
makes it difficult to backport this fix. The only function driver that 
you really need this fix for is f_tcm because of the stream eps right?

You could just add a function like

    int config_ep_by_speed_and_alt(struct usb_gadget *g, struct
    usb_function *f, unsigned int alt, struct usb_ep *_ep);


Then redefine config_ep_by_speed() to use it

    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, 0, _ep);
    }

This way, 1) you can split this patch, 2) you only need to make a fix to 
f_tcm, 3) this fix can be backported much easier.

Anyways, this is just a suggestion. It's up to the maintainers to decide.

BR,
Thinh


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
  2020-02-14 19:26     ` Thinh Nguyen
@ 2020-02-14 19:55       ` Pawel Laszczak
  2020-03-04  9:29         ` Roger Quadros
  0 siblings, 1 reply; 6+ messages in thread
From: Pawel Laszczak @ 2020-02-14 19:55 UTC (permalink / raw)
  To: Thinh Nguyen, Jayshri Dajiram Pawar, linux-usb, balbi
  Cc: gregkh, balbi, heikki.krogerus, rogerq, linux-kernel, jbergsagel,
	nsekhar, nm, peter.chen, Rahul Kumar, Sanket Parmar

>
>Pawel Laszczak wrote:
>> Hi,
>>
>>> Hi,
>>>
>>> Jayshri Pawar wrote:
>>>> This patch adds additional parameter alt to config_ep_by_speed function.
>>>> This additional parameter allows to improve this function and
>>>> 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 *) &bot_uasp_ss_bi_desc,
>>>>           (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>>>>           (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
>>>>           (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>>>>
>>>>           (struct usb_descriptor_header *) &uasp_intf_desc,
>>>>           (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.
>>>>
>>>> This is due to the fact that it search 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>
>>>> ---
>>>>    drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
>>>>    drivers/usb/gadget/function/f_acm.c          |  7 +--
>>>>    drivers/usb/gadget/function/f_ecm.c          |  7 +--
>>>>    drivers/usb/gadget/function/f_eem.c          |  4 +-
>>>>    drivers/usb/gadget/function/f_fs.c           |  3 +-
>>>>    drivers/usb/gadget/function/f_hid.c          |  4 +-
>>>>    drivers/usb/gadget/function/f_loopback.c     |  2 +-
>>>>    drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
>>>>    drivers/usb/gadget/function/f_midi.c         |  2 +-
>>>>    drivers/usb/gadget/function/f_ncm.c          |  7 +--
>>>>    drivers/usb/gadget/function/f_obex.c         |  4 +-
>>>>    drivers/usb/gadget/function/f_phonet.c       |  4 +-
>>>>    drivers/usb/gadget/function/f_rndis.c        |  7 +--
>>>>    drivers/usb/gadget/function/f_serial.c       |  4 +-
>>>>    drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
>>>>    drivers/usb/gadget/function/f_subset.c       |  4 +-
>>>>    drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
>>>>    drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
>>>>    drivers/usb/gadget/function/f_uvc.c          |  5 ++-
>>>>    drivers/usb/gadget/function/u_audio.c        |  4 +-
>>>>    include/linux/usb/composite.h                |  2 +-
>>>>    21 files changed, 99 insertions(+), 71 deletions(-)
>>>>
>>> I think we should create a new function and keep the old
>>> config_ep_by_speed() for default alt-setting (e.i. have
>>> config_ep_by_speed calls the new function with default alt-setting 0).
>>> This way, we can keep compatibility with old function drivers and
>>> minimize changes. At least that's what we did.
>>>
>> I don't think we need the separate function.
>> If we set last parameter alt=0 then this function will work in the same way as old one.
>>
>
>I wasn't talking about that. This patch modifies the
>config_ep_by_speed() parameters, which requires to make a change to
>every function driver of the kernel, and all in a single patch. This
>makes it difficult to backport this fix. The only function driver that
>you really need this fix for is f_tcm because of the stream eps right?
>
>You could just add a function like
>
>    int config_ep_by_speed_and_alt(struct usb_gadget *g, struct
>    usb_function *f, unsigned int alt, struct usb_ep *_ep);
>
>
>Then redefine config_ep_by_speed() to use it
>
>    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, 0, _ep);
>    }
>
>This way, 1) you can split this patch, 2) you only need to make a fix to
>f_tcm, 3) this fix can be backported much easier.
>
>Anyways, this is just a suggestion. It's up to the maintainers to decide.

Thanks for clarification, now I got it. In my opinion, both solution has pros and cons
1. Original patch. 
cons: introduce many change in many files
pros:  we have only single API function - simpler Gadget Subsystem API 

2. using config_ep_by_speed_and_alt
cons: minimal impact to other files. 
pros: introduce the new API function which in fact could be  omitted. 

It's only my personal opinion :) . 

Felipe and Greg what is you opinion ?

Best Regards,
Pawel

>
>BR,
>Thinh


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function.
  2020-02-14 19:55       ` Pawel Laszczak
@ 2020-03-04  9:29         ` Roger Quadros
  0 siblings, 0 replies; 6+ messages in thread
From: Roger Quadros @ 2020-03-04  9:29 UTC (permalink / raw)
  To: Pawel Laszczak, Thinh Nguyen, Jayshri Dajiram Pawar, linux-usb, balbi
  Cc: gregkh, heikki.krogerus, linux-kernel, jbergsagel, nsekhar, nm,
	peter.chen, Rahul Kumar, Sanket Parmar

Hi Pawel,

On 14/02/2020 21:55, Pawel Laszczak wrote:
>>
>> Pawel Laszczak wrote:
>>> Hi,
>>>
>>>> Hi,
>>>>
>>>> Jayshri Pawar wrote:
>>>>> This patch adds additional parameter alt to config_ep_by_speed function.
>>>>> This additional parameter allows to improve this function and
>>>>> 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 *) &bot_uasp_ss_bi_desc,
>>>>>            (struct usb_descriptor_header *) &bot_bi_ep_comp_desc,
>>>>>            (struct usb_descriptor_header *) &bot_uasp_ss_bo_desc,
>>>>>            (struct usb_descriptor_header *) &bot_bo_ep_comp_desc,
>>>>>
>>>>>            (struct usb_descriptor_header *) &uasp_intf_desc,
>>>>>            (struct usb_descriptor_header *) &bot_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 *) &bot_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 drivr 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 bot_uasp_ss_bi_desc.
>>>>>
>>>>> This is due to the fact that it search 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>
>>>>> ---
>>>>>     drivers/usb/gadget/composite.c               | 46 ++++++++++++++------
>>>>>     drivers/usb/gadget/function/f_acm.c          |  7 +--
>>>>>     drivers/usb/gadget/function/f_ecm.c          |  7 +--
>>>>>     drivers/usb/gadget/function/f_eem.c          |  4 +-
>>>>>     drivers/usb/gadget/function/f_fs.c           |  3 +-
>>>>>     drivers/usb/gadget/function/f_hid.c          |  4 +-
>>>>>     drivers/usb/gadget/function/f_loopback.c     |  2 +-
>>>>>     drivers/usb/gadget/function/f_mass_storage.c |  5 ++-
>>>>>     drivers/usb/gadget/function/f_midi.c         |  2 +-
>>>>>     drivers/usb/gadget/function/f_ncm.c          |  7 +--
>>>>>     drivers/usb/gadget/function/f_obex.c         |  4 +-
>>>>>     drivers/usb/gadget/function/f_phonet.c       |  4 +-
>>>>>     drivers/usb/gadget/function/f_rndis.c        |  7 +--
>>>>>     drivers/usb/gadget/function/f_serial.c       |  4 +-
>>>>>     drivers/usb/gadget/function/f_sourcesink.c   | 11 +++--
>>>>>     drivers/usb/gadget/function/f_subset.c       |  4 +-
>>>>>     drivers/usb/gadget/function/f_tcm.c          | 36 +++++++--------
>>>>>     drivers/usb/gadget/function/f_uac1_legacy.c  |  2 +-
>>>>>     drivers/usb/gadget/function/f_uvc.c          |  5 ++-
>>>>>     drivers/usb/gadget/function/u_audio.c        |  4 +-
>>>>>     include/linux/usb/composite.h                |  2 +-
>>>>>     21 files changed, 99 insertions(+), 71 deletions(-)
>>>>>
>>>> I think we should create a new function and keep the old
>>>> config_ep_by_speed() for default alt-setting (e.i. have
>>>> config_ep_by_speed calls the new function with default alt-setting 0).
>>>> This way, we can keep compatibility with old function drivers and
>>>> minimize changes. At least that's what we did.
>>>>
>>> I don't think we need the separate function.
>>> If we set last parameter alt=0 then this function will work in the same way as old one.
>>>
>>
>> I wasn't talking about that. This patch modifies the
>> config_ep_by_speed() parameters, which requires to make a change to
>> every function driver of the kernel, and all in a single patch. This
>> makes it difficult to backport this fix. The only function driver that
>> you really need this fix for is f_tcm because of the stream eps right?
>>
>> You could just add a function like
>>
>>     int config_ep_by_speed_and_alt(struct usb_gadget *g, struct
>>     usb_function *f, unsigned int alt, struct usb_ep *_ep);
>>
>>
>> Then redefine config_ep_by_speed() to use it
>>
>>     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, 0, _ep);
>>     }
>>
>> This way, 1) you can split this patch, 2) you only need to make a fix to
>> f_tcm, 3) this fix can be backported much easier.
>>
>> Anyways, this is just a suggestion. It's up to the maintainers to decide.
> 
> Thanks for clarification, now I got it. In my opinion, both solution has pros and cons
> 1. Original patch.
> cons: introduce many change in many files
> pros:  we have only single API function - simpler Gadget Subsystem API
> 
> 2. using config_ep_by_speed_and_alt
> cons: minimal impact to other files.
> pros: introduce the new API function which in fact could be  omitted.
> 

I would vote for 2.

> It's only my personal opinion :) .
> 
> Felipe and Greg what is you opinion ?
> 
> Best Regards,
> Pawel
> 
>>
>> BR,
>> Thinh
> 

-- 

cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 10:19 [PATCH v1] usb:gadget: Fix issue with config_ep_by_speed function Jayshri Pawar
2020-02-14  3:40 ` Thinh Nguyen
2020-02-14  4:26   ` Pawel Laszczak
2020-02-14 19:26     ` Thinh Nguyen
2020-02-14 19:55       ` Pawel Laszczak
2020-03-04  9:29         ` Roger Quadros

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git