[PATCH 0/3] usb: gadget: Fix issues when using f_acm with ci_udc

[PATCH 0/3] usb: gadget: Fix issues when using f_acm with ci_udc

[Stephan Gerhold]

The ACM console does not work properly with ci_udc, it crashes immediately
when setting it up, because usb_ep_disable() is called on an USB endpoint
that was never enabled before. There are also some other issues in f_acm
where state is not correctly claimed or cleaned up.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
Stephan Gerhold (3):
      usb: gadget: f_acm: Claim requested USB endpoints
      usb: gadget: f_acm: Allow restarting ACM console after stopping it
      usb: gadget: introduce 'enabled' flag in struct usb_ep

 drivers/usb/gadget/f_acm.c |  4 ++++
 include/linux/usb/gadget.h | 27 +++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)
---
base-commit: 0efe8ea57fc7a1a6fc5f64fb3cf6bc4a1a4fc219
change-id: 20250407-acm-fixes-935a27c7a2b5

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

[PATCH 1/3] usb: gadget: f_acm: Claim requested USB endpoints

[Stephan Gerhold]

U-Boot has an older version of the Linux gadget API, where USB endpoints
returned by usb_ep_autoconfig() are not automatically claimed. As written
in the documentation comment:

 "To prevent the endpoint from being returned by a later autoconfig call,
  claim it by assigning ep->driver_data to some non-null value."

Right now f_acm doesn't do that, which means that e.g. ep_in and ep_notify
may end up being assigned the same endpoint. Surprisingly, the ACM console
is still somehow working, but this is not the expected behavior. It will
break with a later commit that disallows calling usb_ep_enable() multiple
times.

Fix this by assigning some data to ep->driver_data, similar to the other
gadget drivers.

Fixes: fc2b399ac03b ("usb: gadget: Add CDC ACM function")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/usb/gadget/f_acm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index f18c6a0a76110df62e925de1882316eed463b343..2665fa4168f99b35a8c595aa24cb3fc4e8ab8529 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -238,18 +238,21 @@ static int acm_bind(struct usb_configuration *c, struct usb_function *f)
 		return -ENODEV;
 
 	f_acm->ep_in = ep;
+	ep->driver_data = c->cdev; /* claim */
 
 	ep = usb_ep_autoconfig(gadget, &acm_fs_out_desc);
 	if (!ep)
 		return -ENODEV;
 
 	f_acm->ep_out = ep;
+	ep->driver_data = c->cdev; /* claim */
 
 	ep = usb_ep_autoconfig(gadget, &acm_fs_notify_desc);
 	if (!ep)
 		return -ENODEV;
 
 	f_acm->ep_notify = ep;
+	ep->driver_data = c->cdev; /* claim */
 
 	if (gadget_is_dualspeed(gadget)) {
 		/* Assume endpoint addresses are the same for both speeds */

-- 
2.47.2

Re: [PATCH 1/3] usb: gadget: f_acm: Claim requested USB endpoints

[Mattijs Korpershoek]

Hi Stephan,

Thank you for the patch.

On lun., avril 07, 2025 at 16:59, Stephan Gerhold <stephan.gerhold@linaro.org> wrote:

> U-Boot has an older version of the Linux gadget API, where USB endpoints
> returned by usb_ep_autoconfig() are not automatically claimed. As written
> in the documentation comment:
>
>  "To prevent the endpoint from being returned by a later autoconfig call,
>   claim it by assigning ep->driver_data to some non-null value."
>
> Right now f_acm doesn't do that, which means that e.g. ep_in and ep_notify
> may end up being assigned the same endpoint. Surprisingly, the ACM console
> is still somehow working, but this is not the expected behavior. It will
> break with a later commit that disallows calling usb_ep_enable() multiple
> times.
>
> Fix this by assigning some data to ep->driver_data, similar to the other
> gadget drivers.
>
> Fixes: fc2b399ac03b ("usb: gadget: Add CDC ACM function")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  drivers/usb/gadget/f_acm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
> index f18c6a0a76110df62e925de1882316eed463b343..2665fa4168f99b35a8c595aa24cb3fc4e8ab8529 100644
> --- a/drivers/usb/gadget/f_acm.c
> +++ b/drivers/usb/gadget/f_acm.c
> @@ -238,18 +238,21 @@ static int acm_bind(struct usb_configuration *c, struct usb_function *f)
>  		return -ENODEV;
>  
>  	f_acm->ep_in = ep;
> +	ep->driver_data = c->cdev; /* claim */
>  
>  	ep = usb_ep_autoconfig(gadget, &acm_fs_out_desc);
>  	if (!ep)
>  		return -ENODEV;
>  
>  	f_acm->ep_out = ep;
> +	ep->driver_data = c->cdev; /* claim */
>  
>  	ep = usb_ep_autoconfig(gadget, &acm_fs_notify_desc);
>  	if (!ep)
>  		return -ENODEV;
>  
>  	f_acm->ep_notify = ep;
> +	ep->driver_data = c->cdev; /* claim */
>  
>  	if (gadget_is_dualspeed(gadget)) {
>  		/* Assume endpoint addresses are the same for both speeds */
>
> -- 
> 2.47.2

[PATCH 2/3] usb: gadget: f_acm: Allow restarting ACM console after stopping it

[Stephan Gerhold]

When using IOMUX, the "usbacm" console can be added/removed dynamically
from the stdout/stderr/stdin environment variables to allow temporarily
starting other USB gadgets (e.g. Fastboot).

However, right now acm_stdio_stop() does not completely undo
acm_stdio_start(): The USB gadget is unregistered, but as long as dev->priv
stays set acm_stdio_start() will never register the USB gadget again.

Clear dev->priv after we detach to make sure a start operation after a stop
operation registers the gadget again.

Fixes: fc2b399ac03b ("usb: gadget: Add CDC ACM function")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/usb/gadget/f_acm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index 2665fa4168f99b35a8c595aa24cb3fc4e8ab8529..8f7256069f58527b2e43e5add725a0b5f06baa6d 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -663,6 +663,7 @@ static int acm_stdio_stop(struct stdio_dev *dev)
 {
 	g_dnl_unregister();
 	g_dnl_clear_detach();
+	dev->priv = NULL;
 
 	return 0;
 }

-- 
2.47.2

Re: [PATCH 2/3] usb: gadget: f_acm: Allow restarting ACM console after stopping it

[Mattijs Korpershoek]

Hi Stephan,

Thank you for the patch.

On lun., avril 07, 2025 at 16:59, Stephan Gerhold <stephan.gerhold@linaro.org> wrote:

> When using IOMUX, the "usbacm" console can be added/removed dynamically
> from the stdout/stderr/stdin environment variables to allow temporarily
> starting other USB gadgets (e.g. Fastboot).
>
> However, right now acm_stdio_stop() does not completely undo
> acm_stdio_start(): The USB gadget is unregistered, but as long as dev->priv
> stays set acm_stdio_start() will never register the USB gadget again.
>
> Clear dev->priv after we detach to make sure a start operation after a stop
> operation registers the gadget again.
>
> Fixes: fc2b399ac03b ("usb: gadget: Add CDC ACM function")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  drivers/usb/gadget/f_acm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
> index 2665fa4168f99b35a8c595aa24cb3fc4e8ab8529..8f7256069f58527b2e43e5add725a0b5f06baa6d 100644
> --- a/drivers/usb/gadget/f_acm.c
> +++ b/drivers/usb/gadget/f_acm.c
> @@ -663,6 +663,7 @@ static int acm_stdio_stop(struct stdio_dev *dev)
>  {
>  	g_dnl_unregister();
>  	g_dnl_clear_detach();
> +	dev->priv = NULL;
>  
>  	return 0;
>  }
>
> -- 
> 2.47.2

[PATCH 3/3] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Stephan Gerhold]

f_acm calls usb_ep_disable(f_acm->ep_notify) unconditionally in
acm_start_ctrl(), even if the USB endpoint was never enabled before. This
causes crashes for some UDC drivers (e.g. ci_udc), because they dereference
data structures that are assigned only after having called usb_ep_enable().

The f_acm driver in U-Boot is similar to the Linux driver, where this issue
does not occur because usb_ep_disable() and usb_ep_enable() internally
track the enabled state. In Linux this change was made in commit
b0bac2581c19 ("usb: gadget: introduce 'enabled' flag in struct usb_ep") by
Robert Baldyga.

Fix the crashes for f_acm by making the same change in U-Boot. This makes
the API less bug-prone and avoids introducing crashes when adapting new
gadget drivers from Linux.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 include/linux/usb/gadget.h | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c7927df15aa386f33eb3b3889adee854d42386a8..fe79bf64a0e1c037e69e694fe58cbe5343e18a70 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -179,6 +179,7 @@ struct usb_ep {
 	const struct usb_ep_ops	*ops;
 	struct list_head	ep_list;
 	struct usb_ep_caps	caps;
+	bool			enabled;
 	unsigned		maxpacket:16;
 	unsigned		maxpacket_limit:16;
 	unsigned		max_streams:16;
@@ -230,7 +231,18 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
 static inline int usb_ep_enable(struct usb_ep *ep,
 				const struct usb_endpoint_descriptor *desc)
 {
-	return ep->ops->enable(ep, desc);
+	int ret;
+
+	if (ep->enabled)
+		return 0;
+
+	ret = ep->ops->enable(ep, desc);
+	if (ret)
+		return ret;
+
+	ep->enabled = true;
+
+	return 0;
 }
 
 /**
@@ -247,7 +259,18 @@ static inline int usb_ep_enable(struct usb_ep *ep,
  */
 static inline int usb_ep_disable(struct usb_ep *ep)
 {
-	return ep->ops->disable(ep);
+	int ret;
+
+	if (!ep->enabled)
+		return 0;
+
+	ret = ep->ops->disable(ep);
+	if (ret)
+		return ret;
+
+	ep->enabled = false;
+
+	return 0;
 }
 
 /**

-- 
2.47.2

Re: [PATCH 3/3] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Mattijs Korpershoek]

Hi Stephan,

Thank you for the patch.

On lun., avril 07, 2025 at 16:59, Stephan Gerhold <stephan.gerhold@linaro.org> wrote:

> f_acm calls usb_ep_disable(f_acm->ep_notify) unconditionally in
> acm_start_ctrl(), even if the USB endpoint was never enabled before. This
> causes crashes for some UDC drivers (e.g. ci_udc), because they dereference
> data structures that are assigned only after having called usb_ep_enable().
>
> The f_acm driver in U-Boot is similar to the Linux driver, where this issue
> does not occur because usb_ep_disable() and usb_ep_enable() internally
> track the enabled state. In Linux this change was made in commit
> b0bac2581c19 ("usb: gadget: introduce 'enabled' flag in struct usb_ep") by
> Robert Baldyga.
>
> Fix the crashes for f_acm by making the same change in U-Boot. This makes
> the API less bug-prone and avoids introducing crashes when adapting new
> gadget drivers from Linux.
>
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  include/linux/usb/gadget.h | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index c7927df15aa386f33eb3b3889adee854d42386a8..fe79bf64a0e1c037e69e694fe58cbe5343e18a70 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -179,6 +179,7 @@ struct usb_ep {
>  	const struct usb_ep_ops	*ops;
>  	struct list_head	ep_list;
>  	struct usb_ep_caps	caps;
> +	bool			enabled;
>  	unsigned		maxpacket:16;
>  	unsigned		maxpacket_limit:16;
>  	unsigned		max_streams:16;
> @@ -230,7 +231,18 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
>  static inline int usb_ep_enable(struct usb_ep *ep,
>  				const struct usb_endpoint_descriptor *desc)
>  {
> -	return ep->ops->enable(ep, desc);
> +	int ret;
> +
> +	if (ep->enabled)
> +		return 0;
> +
> +	ret = ep->ops->enable(ep, desc);
> +	if (ret)
> +		return ret;
> +
> +	ep->enabled = true;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -247,7 +259,18 @@ static inline int usb_ep_enable(struct usb_ep *ep,
>   */
>  static inline int usb_ep_disable(struct usb_ep *ep)
>  {
> -	return ep->ops->disable(ep);
> +	int ret;
> +
> +	if (!ep->enabled)
> +		return 0;
> +
> +	ret = ep->ops->disable(ep);
> +	if (ret)
> +		return ret;
> +
> +	ep->enabled = false;
> +
> +	return 0;
>  }
>  
>  /**
>
> -- 
> 2.47.2

Re: [PATCH 0/3] usb: gadget: Fix issues when using f_acm with ci_udc

[Mattijs Korpershoek]

Hi,

On Mon, 07 Apr 2025 16:59:34 +0200, Stephan Gerhold wrote:
> The ACM console does not work properly with ci_udc, it crashes immediately
> when setting it up, because usb_ep_disable() is called on an USB endpoint
> that was never enabled before. There are also some other issues in f_acm
> where state is not correctly claimed or cleaned up.
> 
> 

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/3] usb: gadget: f_acm: Claim requested USB endpoints
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/eeef1223025c68bd546a1f9411bdcdf518f8cb97
[2/3] usb: gadget: f_acm: Allow restarting ACM console after stopping it
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/22b5aad20ae0f17bd9617fb9c10ddb38d2b91920
[3/3] usb: gadget: introduce 'enabled' flag in struct usb_ep
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/59310d1ecb9f56a1bac405a5edfa9774f2d90220

--
Mattijs