gadget: Why do Microsoft OS descriptors need their own USB request?

gadget: Why do Microsoft OS descriptors need their own USB request?

[Chris Dickens]

Hi,

I've never understood it, so I figure I might as well just ask.  Why
does the Microsoft OS descriptors support require the allocation of a
separate USB request for the composite gadget device?  Both the
default control request buffer and the "special" OS descriptors buffer
are the same size (4KB) and use the same completion handler.  As far
as I can tell there is nothing distinct between them.  There's only
ever one outstanding USB request queued to ep0, so can the dedicated
USB request be removed and just share the default one?  I'm happy to
provide a patch, unless of course I've missed something.

Regards,
Chris

Re: gadget: Why do Microsoft OS descriptors need their own USB request?

[Greg Kroah-Hartman]

On Thu, Jul 02, 2020 at 05:11:11PM -0700, Chris Dickens wrote:
> Hi,
> 
> I've never understood it, so I figure I might as well just ask.  Why
> does the Microsoft OS descriptors support require the allocation of a
> separate USB request for the composite gadget device?  Both the
> default control request buffer and the "special" OS descriptors buffer
> are the same size (4KB) and use the same completion handler.  As far
> as I can tell there is nothing distinct between them.  There's only
> ever one outstanding USB request queued to ep0, so can the dedicated
> USB request be removed and just share the default one?  I'm happy to
> provide a patch, unless of course I've missed something.

Try it and see, I think it was needed for some reason, but look at git
history to be sure.

thanks,

greg k-h

Re: gadget: Why do Microsoft OS descriptors need their own USB request?

[Chris Dickens]

Well, the history shows it was there from the very beginning, so it is
unclear (at least to me) why it was needed.

Chris

On Thu, Jul 2, 2020 at 11:00 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 02, 2020 at 05:11:11PM -0700, Chris Dickens wrote:
> > Hi,
> >
> > I've never understood it, so I figure I might as well just ask.  Why
> > does the Microsoft OS descriptors support require the allocation of a
> > separate USB request for the composite gadget device?  Both the
> > default control request buffer and the "special" OS descriptors buffer
> > are the same size (4KB) and use the same completion handler.  As far
> > as I can tell there is nothing distinct between them.  There's only
> > ever one outstanding USB request queued to ep0, so can the dedicated
> > USB request be removed and just share the default one?  I'm happy to
> > provide a patch, unless of course I've missed something.
>
> Try it and see, I think it was needed for some reason, but look at git
> history to be sure.
>
> thanks,
>
> greg k-h

Re: gadget: Why do Microsoft OS descriptors need their own USB request?

[Greg Kroah-Hartman]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Jul 02, 2020 at 11:47:09PM -0700, Chris Dickens wrote:
> Well, the history shows it was there from the very beginning, so it is
> unclear (at least to me) why it was needed.

Look in the older history trees, perhaps it is in there?

And if it pre-dates git, odds are we don't remember anymore, some of us
deal with thousands of patches a week :)

thanks,

greg k-h

Re: gadget: Why do Microsoft OS descriptors need their own USB request?

[Chris Dickens]

Apologies, I just hit Reply in Gmail without editing...

On Thu, Jul 2, 2020 at 11:57 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> Look in the older history trees, perhaps it is in there?
>
> And if it pre-dates git, odds are we don't remember anymore, some of us
> deal with thousands of patches a week :)

It doesn't pre-date git--it was introduced in commit 19824d5eee ("usb:
gadget: OS String support") dated May 8, 2014.

Chris

Re: gadget: Why do Microsoft OS descriptors need their own USB request?

[Greg Kroah-Hartman]

On Fri, Jul 03, 2020 at 12:03:43AM -0700, Chris Dickens wrote:
> Apologies, I just hit Reply in Gmail without editing...
> 
> On Thu, Jul 2, 2020 at 11:57 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Look in the older history trees, perhaps it is in there?
> >
> > And if it pre-dates git, odds are we don't remember anymore, some of us
> > deal with thousands of patches a week :)
> 
> It doesn't pre-date git--it was introduced in commit 19824d5eee ("usb:
> gadget: OS String support") dated May 8, 2014.

I don't understand what you mean by "duplicate" here, can you submit a
patch to show what you are wanting to change?

thanks,

greg k-h

[PATCH] usb: gadget: composite: Remove dedicated OS Feature Descriptors request

[Chris Dickens]

Currently Microsoft OS Feature Descriptors are handled using a
separately allocated USB request, however everything about this USB
request is identical to the USB request used for all other control
responses. Simplify the code by removing this separate USB request and
using the same USB request as all other control responses.

While at it, simplify the composite_ep0_queue() function by removing the
req and gfp_flags arguments. The former is no longer necessary with a
single USB request and the latter is always GFP_ATOMIC.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>
---
 drivers/usb/gadget/composite.c | 69 +++++-----------------------------
 drivers/usb/gadget/configfs.c  | 11 ------
 include/linux/usb/composite.h  |  9 -----
 3 files changed, 9 insertions(+), 80 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5c1eb96a5c57..020801e70caf 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1443,26 +1443,17 @@ static void composite_setup_complete(struct usb_ep *ep, struct usb_request *req)
 
 	if (cdev->req == req)
 		cdev->setup_pending = false;
-	else if (cdev->os_desc_req == req)
-		cdev->os_desc_pending = false;
 	else
 		WARN(1, "unknown request %p\n", req);
 }
 
-static int composite_ep0_queue(struct usb_composite_dev *cdev,
-		struct usb_request *req, gfp_t gfp_flags)
+static int composite_ep0_queue(struct usb_composite_dev *cdev)
 {
 	int ret;
 
-	ret = usb_ep_queue(cdev->gadget->ep0, req, gfp_flags);
-	if (ret == 0) {
-		if (cdev->req == req)
-			cdev->setup_pending = true;
-		else if (cdev->os_desc_req == req)
-			cdev->os_desc_pending = true;
-		else
-			WARN(1, "unknown request %p\n", req);
-	}
+	ret = usb_ep_queue(cdev->gadget->ep0, cdev->req, GFP_ATOMIC);
+	if (ret == 0)
+		cdev->setup_pending = true;
 
 	return ret;
 }
@@ -1519,7 +1510,7 @@ static int fill_ext_compat(struct usb_configuration *c, u8 *buf)
 				buf += 23;
 			}
 			count += 24;
-			if (count + 24 >= USB_COMP_EP0_OS_DESC_BUFSIZ)
+			if (count + 24 >= USB_COMP_EP0_BUFSIZ)
 				return count;
 		}
 	}
@@ -1581,7 +1572,7 @@ static int fill_ext_prop(struct usb_configuration *c, int interface, u8 *buf)
 			list_for_each_entry(ext_prop, &d->ext_prop, entry) {
 				n = ext_prop->data_len +
 					ext_prop->name_len + 14;
-				if (count + n >= USB_COMP_EP0_OS_DESC_BUFSIZ)
+				if (count + n >= USB_COMP_EP0_BUFSIZ)
 					return count;
 				usb_ext_prop_put_size(buf, n);
 				usb_ext_prop_put_type(buf, ext_prop->type);
@@ -1893,12 +1884,9 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 			int				interface;
 			int				count = 0;
 
-			req = cdev->os_desc_req;
-			req->context = cdev;
-			req->complete = composite_setup_complete;
 			buf = req->buf;
 			os_desc_cfg = cdev->os_desc_config;
-			w_length = min_t(u16, w_length, USB_COMP_EP0_OS_DESC_BUFSIZ);
+			w_length = min_t(u16, w_length, USB_COMP_EP0_BUFSIZ);
 			memset(buf, 0, w_length);
 			buf[5] = 0x01;
 			switch (ctrl->bRequestType & USB_RECIP_MASK) {
@@ -2019,7 +2007,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 		req->length = value;
 		req->context = cdev;
 		req->zero = value < w_length;
-		value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
+		value = composite_ep0_queue(cdev);
 		if (value < 0) {
 			DBG(cdev, "ep_queue --> %d\n", value);
 			req->status = 0;
@@ -2187,30 +2175,6 @@ int composite_dev_prepare(struct usb_composite_driver *composite,
 	return ret;
 }
 
-int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
-				  struct usb_ep *ep0)
-{
-	int ret = 0;
-
-	cdev->os_desc_req = usb_ep_alloc_request(ep0, GFP_KERNEL);
-	if (!cdev->os_desc_req) {
-		ret = -ENOMEM;
-		goto end;
-	}
-
-	cdev->os_desc_req->buf = kmalloc(USB_COMP_EP0_OS_DESC_BUFSIZ,
-					 GFP_KERNEL);
-	if (!cdev->os_desc_req->buf) {
-		ret = -ENOMEM;
-		usb_ep_free_request(ep0, cdev->os_desc_req);
-		goto end;
-	}
-	cdev->os_desc_req->context = cdev;
-	cdev->os_desc_req->complete = composite_setup_complete;
-end:
-	return ret;
-}
-
 void composite_dev_cleanup(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget_string_container *uc, *tmp;
@@ -2220,15 +2184,6 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
 		list_del(&uc->list);
 		kfree(uc);
 	}
-	if (cdev->os_desc_req) {
-		if (cdev->os_desc_pending)
-			usb_ep_dequeue(cdev->gadget->ep0, cdev->os_desc_req);
-
-		kfree(cdev->os_desc_req->buf);
-		cdev->os_desc_req->buf = NULL;
-		usb_ep_free_request(cdev->gadget->ep0, cdev->os_desc_req);
-		cdev->os_desc_req = NULL;
-	}
 	if (cdev->req) {
 		if (cdev->setup_pending)
 			usb_ep_dequeue(cdev->gadget->ep0, cdev->req);
@@ -2286,12 +2241,6 @@ static int composite_bind(struct usb_gadget *gadget,
 	if (status < 0)
 		goto fail;
 
-	if (cdev->use_os_string) {
-		status = composite_os_desc_req_prepare(cdev, gadget->ep0);
-		if (status)
-			goto fail;
-	}
-
 	update_unchanged_dev_desc(&cdev->desc, composite->dev);
 
 	/* has userspace failed to provide a serial number? */
@@ -2460,7 +2409,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
 		DBG(cdev, "%s: Completing delayed status\n", __func__);
 		req->length = 0;
 		req->context = cdev;
-		value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
+		value = composite_ep0_queue(cdev);
 		if (value < 0) {
 			DBG(cdev, "ep_queue --> %d\n", value);
 			req->status = 0;
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 9dc06a4e1b30..24a86de6293c 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1232,12 +1232,6 @@ static int configfs_do_nothing(struct usb_composite_dev *cdev)
 	return -EINVAL;
 }
 
-int composite_dev_prepare(struct usb_composite_driver *composite,
-		struct usb_composite_dev *dev);
-
-int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
-				  struct usb_ep *ep0);
-
 static void purge_configs_funcs(struct gadget_info *gi)
 {
 	struct usb_configuration	*c;
@@ -1393,11 +1387,6 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		}
 		usb_ep_autoconfig_reset(cdev->gadget);
 	}
-	if (cdev->use_os_string) {
-		ret = composite_os_desc_req_prepare(cdev, gadget->ep0);
-		if (ret)
-			goto err_purge_funcs;
-	}
 
 	usb_ep_autoconfig_reset(cdev->gadget);
 	return 0;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2040696d75b6..505d3509a8bb 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -54,9 +54,6 @@
 /* big enough to hold our biggest descriptor */
 #define USB_COMP_EP0_BUFSIZ	4096
 
-/* OS feature descriptor length <= 4kB */
-#define USB_COMP_EP0_OS_DESC_BUFSIZ	4096
-
 #define USB_MS_TO_HS_INTERVAL(x)	(ilog2((x * 1000 / 125)) + 1)
 struct usb_configuration;
 
@@ -423,8 +420,6 @@ extern void usb_composite_unregister(struct usb_composite_driver *driver);
 extern void usb_composite_setup_continue(struct usb_composite_dev *cdev);
 extern int composite_dev_prepare(struct usb_composite_driver *composite,
 		struct usb_composite_dev *cdev);
-extern int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
-					 struct usb_ep *ep0);
 void composite_dev_cleanup(struct usb_composite_dev *cdev);
 
 static inline struct usb_composite_driver *to_cdriver(
@@ -440,14 +435,12 @@ static inline struct usb_composite_driver *to_cdriver(
  * struct usb_composite_device - represents one composite usb gadget
  * @gadget: read-only, abstracts the gadget's usb peripheral controller
  * @req: used for control responses; buffer is pre-allocated
- * @os_desc_req: used for OS descriptors responses; buffer is pre-allocated
  * @config: the currently active configuration
  * @qw_sign: qwSignature part of the OS string
  * @b_vendor_code: bMS_VendorCode part of the OS string
  * @use_os_string: false by default, interested gadgets set it
  * @os_desc_config: the configuration to be used with OS descriptors
  * @setup_pending: true when setup request is queued but not completed
- * @os_desc_pending: true when os_desc request is queued but not completed
  *
  * One of these devices is allocated and initialized before the
  * associated device driver's bind() is called.
@@ -478,7 +471,6 @@ static inline struct usb_composite_driver *to_cdriver(
 struct usb_composite_dev {
 	struct usb_gadget		*gadget;
 	struct usb_request		*req;
-	struct usb_request		*os_desc_req;
 
 	struct usb_configuration	*config;
 
@@ -513,7 +505,6 @@ struct usb_composite_dev {
 
 	/* public: */
 	unsigned int			setup_pending:1;
-	unsigned int			os_desc_pending:1;
 };
 
 extern int usb_string_id(struct usb_composite_dev *c);
-- 
2.27.0

Re: [PATCH] usb: gadget: composite: Remove dedicated OS Feature Descriptors request

[Greg Kroah-Hartman]

On Fri, Jul 03, 2020 at 01:35:34AM -0700, Chris Dickens wrote:
> Currently Microsoft OS Feature Descriptors are handled using a
> separately allocated USB request, however everything about this USB
> request is identical to the USB request used for all other control
> responses. Simplify the code by removing this separate USB request and
> using the same USB request as all other control responses.
> 
> While at it, simplify the composite_ep0_queue() function by removing the
> req and gfp_flags arguments. The former is no longer necessary with a
> single USB request and the latter is always GFP_ATOMIC.
> 
> Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>
> ---
>  drivers/usb/gadget/composite.c | 69 +++++-----------------------------
>  drivers/usb/gadget/configfs.c  | 11 ------
>  include/linux/usb/composite.h  |  9 -----
>  3 files changed, 9 insertions(+), 80 deletions(-)

Did you confirm by testing that this actually works with a real device
that wants to talk to Windows with those feature descriptors?

thanks,

greg k-h

Re: [PATCH] usb: gadget: composite: Remove dedicated OS Feature Descriptors request

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]


Hi,

Chris Dickens <christopher.a.dickens@gmail.com> writes:

> Currently Microsoft OS Feature Descriptors are handled using a
> separately allocated USB request, however everything about this USB
> request is identical to the USB request used for all other control
> responses. Simplify the code by removing this separate USB request and
> using the same USB request as all other control responses.
>
> While at it, simplify the composite_ep0_queue() function by removing the
> req and gfp_flags arguments. The former is no longer necessary with a
> single USB request and the latter is always GFP_ATOMIC.

I would rather move the removal of the extra arguments to a separate
patch.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]