[PATCH 1/2] usb:gadget:composite: add public functions for managing setup requests

[PATCH 1/2] usb:gadget:composite: add public functions for managing setup requests

[Enrico Scholz]

From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

The composite driver does some bookkeeping about pending requests and
decides in its cleanup function whether requests must be dequeued.

There are some function drivers (dfu, acm) which queue the requests
directly which causes e.g.

| :/ dfu /tmp/img(img)c
| ...
| g_multi gadget0: high-speed config #1: Multifunction Composite Gadget
| fsl_free_request: Freeing queued request
| [<2fd8d8e5>] (unwind_backtrace+0x1/0x78) from [<2fd34b1f>] (fsl_free_request+0x1f/0x34)
| [<2fd34b1f>] (fsl_free_request+0x1f/0x34) from [<2fd337cf>] (composite_dev_cleanup+0x77/0xc0)
| [<2fd337cf>] (composite_dev_cleanup+0x77/0xc0) from [<2fd33867>] (__composite_unbind+0x4f/0x94)
| [<2fd33867>] (__composite_unbind+0x4f/0x94) from [<2fd3432b>] (gadget_unbind_driver+0x37/0x70)
| [<2fd3432b>] (gadget_unbind_driver+0x37/0x70) from [<2fd1275f>] (device_remove+0xf/0x20)
| [<2fd1275f>] (device_remove+0xf/0x20) from [<2fd1289b>] (unregister_driver+0x47/0x60)
| [<2fd1289b>] (unregister_driver+0x47/0x60) from [<2fd34663>] (usb_gadget_unregister_driver+0xf/0x18)
| [<2fd34663>] (usb_gadget_unregister_driver+0xf/0x18) from [<2fd37c5b>] (usb_multi_unregister+0x13/0x30)
| [<2fd37c5b>] (usb_multi_unregister+0x13/0x30) from [<2fd59f67>] (do_dfu+0x47/0x68)
| [<2fd59f67>] (do_dfu+0x47/0x68) from [<2fd04fdf>] (execute_command+0x23/0x4c)
| [<2fd04fdf>] (execute_command+0x23/0x4c) from [<2fd0a737>] (run_list_real+0x5ef/0x690)
| [<2fd0a737>] (run_list_real+0x5ef/0x690) from [<2fd0a00b>] (parse_stream_outer+0xc7/0x154)
| [<2fd0a00b>] (parse_stream_outer+0xc7/0x154) from [<2fd0a927>] (run_shell+0x3f/0x6c)
| [<2fd0a927>] (run_shell+0x3f/0x6c) from [<2fd01103>] (run_init+0xeb/0x210)
| [<2fd01103>] (run_init+0xeb/0x210) from [<2fd01253>] (start_barebox+0x2b/0x6c)
| [<2fd01253>] (start_barebox+0x2b/0x6c) from [<2fd89b37>] (barebox_non_pbl_start+0xc3/0x108)
| [<2fd89b37>] (barebox_non_pbl_start+0xc3/0x108) from [<2fd00005>] (__bare_init_start+0x1/0xc)

and related NULL pointer dereferences after 'dfu-util -e'.

Add a helper function which can be called by function drivers and
export the complete method.

*NOTE*: kernel uses the same code and probably suffers from the same
problem.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 drivers/usb/gadget/composite.c | 7 ++++++-
 include/linux/usb/composite.h  | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f55ae5698e08..98f7b5bf7fb4 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1509,7 +1509,7 @@ EXPORT_SYMBOL_GPL(usb_string_ids_n);
 
 /*-------------------------------------------------------------------------*/
 
-static void composite_setup_complete(struct usb_ep *ep, struct usb_request *req)
+void composite_setup_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct usb_composite_dev *cdev;
 
@@ -1556,6 +1556,11 @@ static int composite_ep0_queue(struct usb_composite_dev *cdev,
 	return ret;
 }
 
+int composite_queue_setup_request(struct usb_composite_dev *cdev)
+{
+	return composite_ep0_queue(cdev, cdev->req);
+}
+
 static int count_ext_compat(struct usb_configuration *c)
 {
 	int i, res;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index c3ee403abfe9..cc570657e55f 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -521,6 +521,10 @@ extern struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
 
 extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
 
+extern void composite_setup_complete(struct usb_ep *ep,
+		struct usb_request *req);
+extern int composite_queue_setup_request(struct usb_composite_dev *cdev);
+
 extern void composite_disconnect(struct usb_gadget *gadget);
 extern void composite_reset(struct usb_gadget *gadget);
 
-- 
2.45.0

[PATCH 2/2] usb:gadget: use helper function to queue composite requests

[Enrico Scholz]

From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

Fixes bookkeeping in composite driver.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 drivers/usb/gadget/function/dfu.c   | 2 +-
 drivers/usb/gadget/function/f_acm.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/dfu.c b/drivers/usb/gadget/function/dfu.c
index 3a6d2cf385b1..a41ff22dcebc 100644
--- a/drivers/usb/gadget/function/dfu.c
+++ b/drivers/usb/gadget/function/dfu.c
@@ -807,7 +807,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	if (value >= 0) {
 		req->zero = 0;
 		req->length = value;
-		value = usb_ep_queue(cdev->gadget->ep0, req);
+		value = composite_queue_setup_request(cdev);
 		if (value < 0)
 			ERROR(cdev, "dfu response on ttyGS%d, err %d\n",
 					dfu->port_num, value);
diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 3532fd589274..7fa0a4358fe6 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -309,6 +309,8 @@ static void acm_complete_set_line_coding(struct usb_ep *ep,
 	struct f_acm	*acm = ep->driver_data;
 	struct usb_composite_dev *cdev = acm->port.func.config->cdev;
 
+	composite_setup_complete(ep, req);
+
 	if (req->status != 0) {
 		DBG(cdev, "acm ttyGS%d completion, err %d\n",
 				acm->port_num, req->status);
@@ -406,7 +408,7 @@ static int acm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 			w_value, w_index, w_length);
 		req->zero = 0;
 		req->length = value;
-		value = usb_ep_queue(cdev->gadget->ep0, req);
+		value = composite_queue_setup_request(cdev);
 		if (value < 0)
 			ERROR(cdev, "acm response on ttyGS%d, err %d\n",
 					acm->port_num, value);
-- 
2.45.0

Re: [PATCH 1/2] usb:gadget:composite: add public functions for managing setup requests

[Sascha Hauer]

On Tue, 14 May 2024 15:03:26 +0200, Enrico Scholz wrote:
> The composite driver does some bookkeeping about pending requests and
> decides in its cleanup function whether requests must be dequeued.
> 
> There are some function drivers (dfu, acm) which queue the requests
> directly which causes e.g.
> 
> | :/ dfu /tmp/img(img)c
> | ...
> | g_multi gadget0: high-speed config #1: Multifunction Composite Gadget
> | fsl_free_request: Freeing queued request
> | [<2fd8d8e5>] (unwind_backtrace+0x1/0x78) from [<2fd34b1f>] (fsl_free_request+0x1f/0x34)
> | [<2fd34b1f>] (fsl_free_request+0x1f/0x34) from [<2fd337cf>] (composite_dev_cleanup+0x77/0xc0)
> | [<2fd337cf>] (composite_dev_cleanup+0x77/0xc0) from [<2fd33867>] (__composite_unbind+0x4f/0x94)
> | [<2fd33867>] (__composite_unbind+0x4f/0x94) from [<2fd3432b>] (gadget_unbind_driver+0x37/0x70)
> | [<2fd3432b>] (gadget_unbind_driver+0x37/0x70) from [<2fd1275f>] (device_remove+0xf/0x20)
> | [<2fd1275f>] (device_remove+0xf/0x20) from [<2fd1289b>] (unregister_driver+0x47/0x60)
> | [<2fd1289b>] (unregister_driver+0x47/0x60) from [<2fd34663>] (usb_gadget_unregister_driver+0xf/0x18)
> | [<2fd34663>] (usb_gadget_unregister_driver+0xf/0x18) from [<2fd37c5b>] (usb_multi_unregister+0x13/0x30)
> | [<2fd37c5b>] (usb_multi_unregister+0x13/0x30) from [<2fd59f67>] (do_dfu+0x47/0x68)
> | [<2fd59f67>] (do_dfu+0x47/0x68) from [<2fd04fdf>] (execute_command+0x23/0x4c)
> | [<2fd04fdf>] (execute_command+0x23/0x4c) from [<2fd0a737>] (run_list_real+0x5ef/0x690)
> | [<2fd0a737>] (run_list_real+0x5ef/0x690) from [<2fd0a00b>] (parse_stream_outer+0xc7/0x154)
> | [<2fd0a00b>] (parse_stream_outer+0xc7/0x154) from [<2fd0a927>] (run_shell+0x3f/0x6c)
> | [<2fd0a927>] (run_shell+0x3f/0x6c) from [<2fd01103>] (run_init+0xeb/0x210)
> | [<2fd01103>] (run_init+0xeb/0x210) from [<2fd01253>] (start_barebox+0x2b/0x6c)
> | [<2fd01253>] (start_barebox+0x2b/0x6c) from [<2fd89b37>] (barebox_non_pbl_start+0xc3/0x108)
> | [<2fd89b37>] (barebox_non_pbl_start+0xc3/0x108) from [<2fd00005>] (__bare_init_start+0x1/0xc)
> 
> [...]

Applied, thanks!

[1/2] usb:gadget:composite: add public functions for managing setup requests
      https://git.pengutronix.de/cgit/barebox/commit/?id=bf91067ef12c (link may not be stable)
[2/2] usb:gadget: use helper function to queue composite requests
      https://git.pengutronix.de/cgit/barebox/commit/?id=593248cde35d (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>