[PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[Michael Walle]

The bInterfaceProtocol is hardcoded to USB_CDC_ACM_PROTO_AT_V25TER. This
will lead to problems with ModemManger which will gladly try to probe
that port as a modem if the gadget also has a network function.
ModemManager will try to send AT commands to the ACM port. Make the
bInterfaceProtocol configurable.

This will also set bFunctionProtocol to the same value, see commit
5c8db070b448 ("USB: Change acm_iad_descriptor bFunctionProtocol to
USB_CDC_ACM_PROTO_AT_V25TER") for more details.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
See the following link for the filter logic:
https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/blob/main/src/mm-filter.c?ref_type=heads#L303
---
 .../ABI/testing/configfs-usb-gadget-acm       |  7 +++
 drivers/usb/gadget/function/f_acm.c           | 50 ++++++++++++++++++-
 drivers/usb/gadget/function/u_serial.h        |  4 ++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-acm b/Documentation/ABI/testing/configfs-usb-gadget-acm
index d21092d75a05..25e68be9eb66 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-acm
+++ b/Documentation/ABI/testing/configfs-usb-gadget-acm
@@ -6,3 +6,10 @@ Description:
 		This item contains just one readonly attribute: port_num.
 		It contains the port number of the /dev/ttyGS<n> device
 		associated with acm function's instance "name".
+
+What:		/config/usb-gadget/gadget/functions/acm.name/protocol
+Date:		Aug 2024
+KernelVersion:	6.13
+Description:
+		Reported bInterfaceProtocol for the ACM device. For legacy
+		reasons, this defaults to 1 (USB_CDC_ACM_PROTO_AT_V25TER).
diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 724b2631f249..b78f46586a0f 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -41,6 +41,7 @@ struct f_acm {
 	struct gserial			port;
 	u8				ctrl_id, data_id;
 	u8				port_num;
+	u8				bInterfaceProtocol;
 
 	u8				pending;
 
@@ -89,7 +90,7 @@ acm_iad_descriptor = {
 	.bInterfaceCount = 	2,	// control + data
 	.bFunctionClass =	USB_CLASS_COMM,
 	.bFunctionSubClass =	USB_CDC_SUBCLASS_ACM,
-	.bFunctionProtocol =	USB_CDC_ACM_PROTO_AT_V25TER,
+	/* .bFunctionProtocol = DYNAMIC */
 	/* .iFunction =		DYNAMIC */
 };
 
@@ -101,7 +102,7 @@ static struct usb_interface_descriptor acm_control_interface_desc = {
 	.bNumEndpoints =	1,
 	.bInterfaceClass =	USB_CLASS_COMM,
 	.bInterfaceSubClass =	USB_CDC_SUBCLASS_ACM,
-	.bInterfaceProtocol =	USB_CDC_ACM_PROTO_AT_V25TER,
+	/* .bInterfaceProtocol = DYNAMIC */
 	/* .iInterface = DYNAMIC */
 };
 
@@ -663,6 +664,9 @@ acm_bind(struct usb_configuration *c, struct usb_function *f)
 		goto fail;
 	acm->notify = ep;
 
+	acm_iad_descriptor.bFunctionProtocol = acm->bInterfaceProtocol;
+	acm_control_interface_desc.bInterfaceProtocol = acm->bInterfaceProtocol;
+
 	/* allocate notification */
 	acm->notify_req = gs_alloc_req(ep,
 			sizeof(struct usb_cdc_notification) + 2,
@@ -719,8 +723,14 @@ static void acm_unbind(struct usb_configuration *c, struct usb_function *f)
 static void acm_free_func(struct usb_function *f)
 {
 	struct f_acm		*acm = func_to_acm(f);
+	struct f_serial_opts	*opts;
+
+	opts = container_of(f->fi, struct f_serial_opts, func_inst);
 
 	kfree(acm);
+	mutex_lock(&opts->lock);
+	opts->refcnt--;
+	mutex_unlock(&opts->lock);
 }
 
 static void acm_resume(struct usb_function *f)
@@ -761,7 +771,11 @@ static struct usb_function *acm_alloc_func(struct usb_function_instance *fi)
 	acm->port.func.disable = acm_disable;
 
 	opts = container_of(fi, struct f_serial_opts, func_inst);
+	mutex_lock(&opts->lock);
 	acm->port_num = opts->port_num;
+	acm->bInterfaceProtocol = opts->protocol;
+	opts->refcnt++;
+	mutex_unlock(&opts->lock);
 	acm->port.func.unbind = acm_unbind;
 	acm->port.func.free_func = acm_free_func;
 	acm->port.func.resume = acm_resume;
@@ -812,11 +826,42 @@ static ssize_t f_acm_port_num_show(struct config_item *item, char *page)
 
 CONFIGFS_ATTR_RO(f_acm_, port_num);
 
+static ssize_t f_acm_protocol_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%u\n", to_f_serial_opts(item)->protocol);
+}
+
+static ssize_t f_acm_protocol_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct f_serial_opts *opts = to_f_serial_opts(item);
+	int ret;
+
+	mutex_lock(&opts->lock);
+
+	if (opts->refcnt) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = kstrtou8(page, 0, &opts->protocol);
+	if (ret)
+		goto out;
+	ret = count;
+
+out:
+	mutex_unlock(&opts->lock);
+	return ret;
+}
+
+CONFIGFS_ATTR(f_acm_, protocol);
+
 static struct configfs_attribute *acm_attrs[] = {
 #ifdef CONFIG_U_SERIAL_CONSOLE
 	&f_acm_attr_console,
 #endif
 	&f_acm_attr_port_num,
+	&f_acm_attr_protocol,
 	NULL,
 };
 
@@ -843,6 +888,7 @@ static struct usb_function_instance *acm_alloc_instance(void)
 	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
 	if (!opts)
 		return ERR_PTR(-ENOMEM);
+	opts->protocol = USB_CDC_ACM_PROTO_AT_V25TER;
 	opts->func_inst.free_func_inst = acm_free_instance;
 	ret = gserial_alloc_line(&opts->port_num);
 	if (ret) {
diff --git a/drivers/usb/gadget/function/u_serial.h b/drivers/usb/gadget/function/u_serial.h
index 901d99310bc4..1782d7d7f7be 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -17,6 +17,10 @@
 struct f_serial_opts {
 	struct usb_function_instance func_inst;
 	u8 port_num;
+	u8 protocol;
+
+	struct mutex lock;
+	int refcnt;
 };
 
 /*
-- 
2.39.2

Re: [PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[Greg Kroah-Hartman]

On Tue, Jul 30, 2024 at 09:43:37PM +0200, Michael Walle wrote:
>  struct f_serial_opts {
>  	struct usb_function_instance func_inst;
>  	u8 port_num;
> +	u8 protocol;
> +
> +	struct mutex lock;
> +	int refcnt;

Attempting to "roll your own" reference count is almost never a good
idea.  If you really need one, please use the proper in-kernel apis for
this.  But you need to justify it as well, I didn't see why this was
needed at all.

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[Michael Walle]

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

Hi Greg,

On Wed Jul 31, 2024 at 10:32 AM CEST, Greg Kroah-Hartman wrote:
> On Tue, Jul 30, 2024 at 09:43:37PM +0200, Michael Walle wrote:
> >  struct f_serial_opts {
> >  	struct usb_function_instance func_inst;
> >  	u8 port_num;
> > +	u8 protocol;
> > +
> > +	struct mutex lock;
> > +	int refcnt;
>
> Attempting to "roll your own" reference count is almost never a good
> idea.  If you really need one, please use the proper in-kernel apis for
> this.  But you need to justify it as well, I didn't see why this was
> needed at all.

Honestly, I couldn't grok all that usb gadget magic, so I've looked
at similar gadgets and took that from there:
  grep refcnt drivers/usb/gadget/function/ -r

They are all doing the same, so maybe that code is old or didn't use
the proper APIs back then.

The refcnt will prevent changing the options (the protocol here)
while a gadget is still in use/bound.

-michael

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

Re: [PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[Greg Kroah-Hartman]

On Wed, Jul 31, 2024 at 10:57:04AM +0200, Michael Walle wrote:
> Hi Greg,
> 
> On Wed Jul 31, 2024 at 10:32 AM CEST, Greg Kroah-Hartman wrote:
> > On Tue, Jul 30, 2024 at 09:43:37PM +0200, Michael Walle wrote:
> > >  struct f_serial_opts {
> > >  	struct usb_function_instance func_inst;
> > >  	u8 port_num;
> > > +	u8 protocol;
> > > +
> > > +	struct mutex lock;
> > > +	int refcnt;
> >
> > Attempting to "roll your own" reference count is almost never a good
> > idea.  If you really need one, please use the proper in-kernel apis for
> > this.  But you need to justify it as well, I didn't see why this was
> > needed at all.
> 
> Honestly, I couldn't grok all that usb gadget magic, so I've looked
> at similar gadgets and took that from there:
>   grep refcnt drivers/usb/gadget/function/ -r
> 
> They are all doing the same, so maybe that code is old or didn't use
> the proper APIs back then.

It's old code, please do things properly.

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_acm: make bInterfaceProtocol configurable

[Michael Walle]

Hi Greg,

On Wed Jul 31, 2024 at 11:16 AM CEST, Greg Kroah-Hartman wrote:
> On Wed, Jul 31, 2024 at 10:57:04AM +0200, Michael Walle wrote:
> > Hi Greg,
> > 
> > On Wed Jul 31, 2024 at 10:32 AM CEST, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 30, 2024 at 09:43:37PM +0200, Michael Walle wrote:
> > > >  struct f_serial_opts {
> > > >  	struct usb_function_instance func_inst;
> > > >  	u8 port_num;
> > > > +	u8 protocol;
> > > > +
> > > > +	struct mutex lock;
> > > > +	int refcnt;
> > >
> > > Attempting to "roll your own" reference count is almost never a good
> > > idea.  If you really need one, please use the proper in-kernel apis for
> > > this.  But you need to justify it as well, I didn't see why this was
> > > needed at all.
> > 
> > Honestly, I couldn't grok all that usb gadget magic, so I've looked
> > at similar gadgets and took that from there:
> >   grep refcnt drivers/usb/gadget/function/ -r
> > 
> > They are all doing the same, so maybe that code is old or didn't use
> > the proper APIs back then.
>
> It's old code, please do things properly.

Sorry, I have to come back to this. What do you have in mind?

The mutex is needed in any case to protect the members of
f_serial_ops from concurrent access between the .bind() and the
configfs write.

Which leaves us with refcnt. I don't think refcount_t is suitable
here. For example, refcount_inc() will warn if you increment from
0; which makes sense for resource reference counting. But doesn't
make sense in this case; there is no resource handling or freeing if
the refcnt is 0. It just prohibits the write to the configfs
attribute if refcnt != 0, that is, there is at least one instance of
the gadget function.

Maybe I should just rename "refcnt" to "users" and add a comment to
the mutex what it will protect.

What do you think?

-michael