[PATCH 0/2] usb: gadget: Auto-cleanup usb_request in f_loopback, f_tcm

[PATCH 0/2] usb: gadget: Auto-cleanup usb_request in f_loopback, f_tcm

[Kuen-Han Tsai]

This series refactors f_loopback and f_tcm function drivers to use 
automatic, scope-based cleanup mechansim for usb_request objects.

Specific considerations in each driver:

  - In f_loopback.c, the shared buffer between in_req and out_req is
    handled by nullifying in_req->buf before returning on usb_ep_queue
    failure. This ensures the buffer is only freed once by the out_req
    cleanup.

  - In f_tcm.c, explicit nullification of request pointers in the f_uas
    structure on error is no longer necessary. The pointers are now only
    updated after all allocations within the respective functions 
    (bot_prepare_reqs, uasp_alloc_stream_res, uasp_alloc_cmd) succeed.
    Existing cleanup functions handle any stale pointers if an error
    occurs before the structure is updated.

Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
Kuen-Han Tsai (2):
      usb: gadget: f_tcm: Use auto-cleanup for usb_request
      usb: gadget: f_loopback: Use auto-cleanup for usb_request

 drivers/usb/gadget/function/f_loopback.c |  30 +++----
 drivers/usb/gadget/function/f_tcm.c      | 141 +++++++++++++++----------------
 2 files changed, 80 insertions(+), 91 deletions(-)
---
base-commit: dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa
change-id: 20251029-auto-cleanup-e0240f2d7f2a

Best regards,
-- 
Kuen-Han Tsai <khtsai@google.com>

[PATCH 1/2] usb: gadget: f_tcm: Use auto-cleanup for usb_request

[Kuen-Han Tsai]

Refactor f_tcm.c to use auto-cleanup mechanism for usb_request
allocations in bot_prepare_reqs(), uasp_alloc_stream_res(), and
uasp_alloc_cmd().

The explicit nullification of fu->..._req and stream->..._req pointers
on error is no longer needed. This is safe because these pointers are
only updated after all allocations within the function have succeeded.
If an error occurs, the fu structure members retain their previous
value, and the existing cleanup functions like bot_cleanup_old_alt() and
uasp_cleanup_old_alt() already handle stale pointers in the fu
structure.

Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/gadget/function/f_tcm.c | 141 +++++++++++++++++-------------------
 1 file changed, 67 insertions(+), 74 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6e8804f04baa774f5e6bed548b64769e93f6eb1c..782995040af3acdb42d380b4dbb012941592da06 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
+#include <linux/cleanup.h>
 #include <linux/configfs.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
@@ -309,57 +310,54 @@ static int bot_prepare_reqs(struct f_uas *fu)
 {
 	int ret;
 
-	fu->bot_req_in = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
-	if (!fu->bot_req_in)
-		goto err;
-
-	fu->bot_req_out = usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
-	if (!fu->bot_req_out)
-		goto err_out;
+	struct usb_request *bot_req_in __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
+	if (!bot_req_in)
+		return -ENOMEM;
 
-	fu->cmd[0].req = usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
-	if (!fu->cmd[0].req)
-		goto err_cmd;
+	struct usb_request *bot_req_out __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
+	if (!bot_req_out)
+		return -ENOMEM;
 
-	fu->bot_status.req = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
-	if (!fu->bot_status.req)
-		goto err_sts;
+	struct usb_request *cmd_req __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
+	if (!cmd_req)
+		return -ENOMEM;
 
-	fu->bot_status.req->buf = &fu->bot_status.csw;
-	fu->bot_status.req->length = US_BULK_CS_WRAP_LEN;
-	fu->bot_status.req->complete = bot_status_complete;
-	fu->bot_status.csw.Signature = cpu_to_le32(US_BULK_CS_SIGN);
+	struct usb_request *status_req __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
+	if (!status_req)
+		return -ENOMEM;
 
-	fu->cmd[0].buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
-	if (!fu->cmd[0].buf)
-		goto err_buf;
+	cmd_req->buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
+	if (!cmd_req->buf)
+		return -ENOMEM;
 
-	fu->cmd[0].req->complete = bot_cmd_complete;
-	fu->cmd[0].req->buf = fu->cmd[0].buf;
-	fu->cmd[0].req->length = fu->ep_out->maxpacket;
-	fu->cmd[0].req->context = fu;
+	cmd_req->complete = bot_cmd_complete;
+	cmd_req->length = fu->ep_out->maxpacket;
+	cmd_req->context = fu;
 
 	ret = bot_enqueue_cmd_cbw(fu);
 	if (ret)
-		goto err_queue;
+		return ret;
+
+	fu->bot_req_in = no_free_ptr(bot_req_in);
+	fu->bot_req_out = no_free_ptr(bot_req_out);
+
+	/* This line is placed here because free_usb_request also frees its
+	 * buffer, which in this case points to the static fu->bot_status.csw.
+	 */
+	status_req->buf = &fu->bot_status.csw;
+	status_req->length = US_BULK_CS_WRAP_LEN;
+	status_req->complete = bot_status_complete;
+	fu->bot_status.csw.Signature = cpu_to_le32(US_BULK_CS_SIGN);
+	fu->bot_status.req = no_free_ptr(status_req);
+
+	fu->cmd[0].buf = cmd_req->buf;
+	fu->cmd[0].req = no_free_ptr(cmd_req);
+
 	return 0;
-err_queue:
-	kfree(fu->cmd[0].buf);
-	fu->cmd[0].buf = NULL;
-err_buf:
-	usb_ep_free_request(fu->ep_in, fu->bot_status.req);
-err_sts:
-	usb_ep_free_request(fu->ep_out, fu->cmd[0].req);
-	fu->cmd[0].req = NULL;
-err_cmd:
-	usb_ep_free_request(fu->ep_out, fu->bot_req_out);
-	fu->bot_req_out = NULL;
-err_out:
-	usb_ep_free_request(fu->ep_in, fu->bot_req_in);
-	fu->bot_req_in = NULL;
-err:
-	pr_err("BOT: endpoint setup failed\n");
-	return -ENOMEM;
 }
 
 static void bot_cleanup_old_alt(struct f_uas *fu)
@@ -878,50 +876,45 @@ static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
 {
 	init_completion(&stream->cmd_completion);
 
-	stream->req_in = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
-	if (!stream->req_in)
-		goto out;
+	struct usb_request *req_in __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
+	if (!req_in)
+		return -ENOMEM;
 
-	stream->req_out = usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
-	if (!stream->req_out)
-		goto err_out;
+	struct usb_request *req_out __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
+	if (!req_out)
+		return -ENOMEM;
 
-	stream->req_status = usb_ep_alloc_request(fu->ep_status, GFP_KERNEL);
+	struct usb_request *req_status __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_status, GFP_KERNEL);
 	if (!stream->req_status)
-		goto err_sts;
+		return -ENOMEM;
 
-	return 0;
+	stream->req_in = no_free_ptr(req_in);
+	stream->req_out = no_free_ptr(req_out);
+	stream->req_status = no_free_ptr(req_status);
 
-err_sts:
-	usb_ep_free_request(fu->ep_out, stream->req_out);
-	stream->req_out = NULL;
-err_out:
-	usb_ep_free_request(fu->ep_in, stream->req_in);
-	stream->req_in = NULL;
-out:
-	return -ENOMEM;
+	return 0;
 }
 
 static int uasp_alloc_cmd(struct f_uas *fu, int i)
 {
-	fu->cmd[i].req = usb_ep_alloc_request(fu->ep_cmd, GFP_KERNEL);
-	if (!fu->cmd[i].req)
-		goto err;
+	struct usb_request *req __free(free_usb_request) =
+		usb_ep_alloc_request(fu->ep_cmd, GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
 
-	fu->cmd[i].buf = kmalloc(fu->ep_cmd->maxpacket, GFP_KERNEL);
-	if (!fu->cmd[i].buf)
-		goto err_buf;
+	req->buf = kmalloc(fu->ep_cmd->maxpacket, GFP_KERNEL);
+	if (!req->buf)
+		return -ENOMEM;
 
-	fu->cmd[i].req->complete = uasp_cmd_complete;
-	fu->cmd[i].req->buf = fu->cmd[i].buf;
-	fu->cmd[i].req->length = fu->ep_cmd->maxpacket;
-	fu->cmd[i].req->context = fu;
+	req->complete = uasp_cmd_complete;
+	req->length = fu->ep_cmd->maxpacket;
+	req->context = fu;
+	fu->cmd[i].buf = req->buf;
+	fu->cmd[i].req = no_free_ptr(req);
 	return 0;
-
-err_buf:
-	usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
-err:
-	return -ENOMEM;
 }
 
 static int uasp_prepare_reqs(struct f_uas *fu)

-- 
2.51.1.851.g4ebd6896fd-goog

[PATCH 2/2] usb: gadget: f_loopback: Use auto-cleanup for usb_request

[Kuen-Han Tsai]

Refactor f_loopback.c to use auto-cleanup mechanism for usb_request
allocations in alloc_requests().

The shared buffer between in_req and out_req is handled by nullifying
in_req->buf before returning on usb_ep_queue failure, ensuring the
buffer is only freed once by the out_req cleanup.

Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/gadget/function/f_loopback.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c
index 49b009a7d5d79285c2397c7aebb8c8fcd3b7dafb..cdf42d8b3d774e5324e83dc308aa5caa265eac3b 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -8,6 +8,7 @@
 
 /* #define VERBOSE_DEBUG */
 
+#include <linux/cleanup.h>
 #include <linux/slab.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
@@ -308,9 +309,8 @@ static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
 static int alloc_requests(struct usb_composite_dev *cdev,
 			  struct f_loopback *loop)
 {
-	struct usb_request *in_req, *out_req;
 	int i;
-	int result = 0;
+	int result;
 
 	/*
 	 * allocate a bunch of read buffers and queue them all at once.
@@ -318,16 +318,16 @@ static int alloc_requests(struct usb_composite_dev *cdev,
 	 * for out transfer and reuse them in IN transfers to implement
 	 * our loopback functionality
 	 */
-	for (i = 0; i < loop->qlen && result == 0; i++) {
-		result = -ENOMEM;
-
-		in_req = usb_ep_alloc_request(loop->in_ep, GFP_ATOMIC);
+	for (i = 0; i < loop->qlen; i++) {
+		struct usb_request *in_req __free(free_usb_request) =
+			usb_ep_alloc_request(loop->in_ep, GFP_ATOMIC);
 		if (!in_req)
-			goto fail;
+			return -ENOMEM;
 
-		out_req = lb_alloc_ep_req(loop->out_ep, loop->buflen);
+		struct usb_request *out_req __free(free_usb_request) =
+			lb_alloc_ep_req(loop->out_ep, loop->buflen);
 		if (!out_req)
-			goto fail_in;
+			return -ENOMEM;
 
 		in_req->complete = loopback_complete;
 		out_req->complete = loopback_complete;
@@ -339,20 +339,16 @@ static int alloc_requests(struct usb_composite_dev *cdev,
 
 		result = usb_ep_queue(loop->out_ep, out_req, GFP_ATOMIC);
 		if (result) {
+			in_req->buf = NULL;
 			ERROR(cdev, "%s queue req --> %d\n",
 					loop->out_ep->name, result);
-			goto fail_out;
+			return result;
 		}
+		retain_and_null_ptr(in_req);
+		retain_and_null_ptr(out_req);
 	}
 
 	return 0;
-
-fail_out:
-	free_ep_req(loop->out_ep, out_req);
-fail_in:
-	usb_ep_free_request(loop->in_ep, in_req);
-fail:
-	return result;
 }
 
 static int enable_endpoint(struct usb_composite_dev *cdev,

-- 
2.51.1.851.g4ebd6896fd-goog

Re: [PATCH 1/2] usb: gadget: f_tcm: Use auto-cleanup for usb_request

[Greg Kroah-Hartman]

On Thu, Oct 30, 2025 at 11:14:19PM +0800, Kuen-Han Tsai wrote:
> Refactor f_tcm.c to use auto-cleanup mechanism for usb_request
> allocations in bot_prepare_reqs(), uasp_alloc_stream_res(), and
> uasp_alloc_cmd().

Using guards are great for new code, or for bug fixes, but please don't
just refactor code to use them for the sake of using them.  It makes it
hard to review and justify the churn, especially when there is almost no
code savings here at all.

> The explicit nullification of fu->..._req and stream->..._req pointers
> on error is no longer needed. This is safe because these pointers are
> only updated after all allocations within the function have succeeded.
> If an error occurs, the fu structure members retain their previous
> value, and the existing cleanup functions like bot_cleanup_old_alt() and
> uasp_cleanup_old_alt() already handle stale pointers in the fu
> structure.

This seems to imply this is really fragile, and tricky, and maybe not
worth it?

The comment you added kind of enforces that feeling:

> +	fu->bot_req_in = no_free_ptr(bot_req_in);
> +	fu->bot_req_out = no_free_ptr(bot_req_out);
> +
> +	/* This line is placed here because free_usb_request also frees its
> +	 * buffer, which in this case points to the static fu->bot_status.csw.
> +	 */

Which is "this line"?

> +	status_req->buf = &fu->bot_status.csw;

This one?

> +	status_req->length = US_BULK_CS_WRAP_LEN;

Or that one?

Using guards for buffers for other structures is rough, as you have seen
here, I don't really see the benefit at all, do you?

thanks,

greg k-h

Re: [PATCH 2/2] usb: gadget: f_loopback: Use auto-cleanup for usb_request

[Greg Kroah-Hartman]

On Thu, Oct 30, 2025 at 11:14:20PM +0800, Kuen-Han Tsai wrote:
> Refactor f_loopback.c to use auto-cleanup mechanism for usb_request
> allocations in alloc_requests().
> 
> The shared buffer between in_req and out_req is handled by nullifying
> in_req->buf before returning on usb_ep_queue failure, ensuring the
> buffer is only freed once by the out_req cleanup.

Same here, it's tricky stuff, and you are only saving 4 lines of code?

Not really worth it I would think.

thanks,

greg k-h

Re: [PATCH 1/2] usb: gadget: f_tcm: Use auto-cleanup for usb_request

[Kuen-Han Tsai]

Hi Greg,

Thanks for the detailed review.

On Thu, Oct 30, 2025 at 11:33 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 30, 2025 at 11:14:19PM +0800, Kuen-Han Tsai wrote:
> > Refactor f_tcm.c to use auto-cleanup mechanism for usb_request
> > allocations in bot_prepare_reqs(), uasp_alloc_stream_res(), and
> > uasp_alloc_cmd().
>
> Using guards are great for new code, or for bug fixes, but please don't
> just refactor code to use them for the sake of using them.  It makes it
> hard to review and justify the churn, especially when there is almost no
> code savings here at all.

You're absolutely right. The benefit doesn't justify the churn.

>
> > The explicit nullification of fu->..._req and stream->..._req pointers
> > on error is no longer needed. This is safe because these pointers are
> > only updated after all allocations within the function have succeeded.
> > If an error occurs, the fu structure members retain their previous
> > value, and the existing cleanup functions like bot_cleanup_old_alt() and
> > uasp_cleanup_old_alt() already handle stale pointers in the fu
> > structure.
>
> This seems to imply this is really fragile, and tricky, and maybe not
> worth it?
>
> The comment you added kind of enforces that feeling:
>
> > +     fu->bot_req_in = no_free_ptr(bot_req_in);
> > +     fu->bot_req_out = no_free_ptr(bot_req_out);
> > +
> > +     /* This line is placed here because free_usb_request also frees its
> > +      * buffer, which in this case points to the static fu->bot_status.csw.
> > +      */
>
> Which is "this line"?
>
> > +     status_req->buf = &fu->bot_status.csw;
>
> This one?
>
> > +     status_req->length = US_BULK_CS_WRAP_LEN;
>
> Or that one?
>
> Using guards for buffers for other structures is rough, as you have seen
> here, I don't really see the benefit at all, do you?
>

My apologies about the noise. After introducing the auto-cleanup
helpers recently, I had misunderstood that they were a preferred
pattern to proactively refactor existing goto logic with.

I see your point clearly now that these types of guards are best for
new codes or bug fixes, not for refactoring-only changes where the
benefit is minimal.

I will drop this patch series.

Regards,
Kuen-Han