[PATCH 0/4] USB: serial: add support for multi-interface functions

[PATCH 0/4] USB: serial: add support for multi-interface functions

[Johan Hovold]

A single USB function can be implemented using a group of interfaces and
this is for example commonly used for Communication Class devices.
    
This series adds support for multi-interface functions to USB serial
core and exports an interface that allows drivers to claim a second
sibling interface. The interface could easily be extended to allow
claiming further interfaces if ever needed.

The final patch uses the new interface to properly claim both the
control and data interface of Maxlinear/Exar devices.

Johan


Johan Hovold (4):
  USB: serial: drop unused suspending flag
  USB: serial: refactor endpoint classification
  USB: serial: add support for multi-interface functions
  USB: serial: xr: claim both interfaces

 drivers/usb/serial/usb-serial.c | 135 ++++++++++++++++++++++++--------
 drivers/usb/serial/xr_serial.c  |  26 +++++-
 include/linux/usb/serial.h      |   8 +-
 3 files changed, 131 insertions(+), 38 deletions(-)

-- 
2.26.3

[PATCH 1/4] USB: serial: drop unused suspending flag

[Johan Hovold]

The suspending flag was added back in 2009 but no users ever followed.
Remove it.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb-serial.c | 8 +-------
 include/linux/usb/serial.h      | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 27e3bb58c872..2a38810a3979 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -1114,8 +1114,6 @@ int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 	struct usb_serial *serial = usb_get_intfdata(intf);
 	int i, r = 0;
 
-	serial->suspending = 1;
-
 	/*
 	 * serial->type->suspend() MUST return 0 in system sleep context,
 	 * otherwise, the resume callback has to recover device from
@@ -1123,10 +1121,8 @@ int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 	 */
 	if (serial->type->suspend) {
 		r = serial->type->suspend(serial, message);
-		if (r < 0) {
-			serial->suspending = 0;
+		if (r < 0)
 			goto err_out;
-		}
 	}
 
 	for (i = 0; i < serial->num_ports; ++i)
@@ -1151,7 +1147,6 @@ int usb_serial_resume(struct usb_interface *intf)
 
 	usb_serial_unpoison_port_urbs(serial);
 
-	serial->suspending = 0;
 	if (serial->type->resume)
 		rv = serial->type->resume(serial);
 	else
@@ -1168,7 +1163,6 @@ static int usb_serial_reset_resume(struct usb_interface *intf)
 
 	usb_serial_unpoison_port_urbs(serial);
 
-	serial->suspending = 0;
 	if (serial->type->reset_resume) {
 		rv = serial->type->reset_resume(serial);
 	} else {
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 952272002e48..7efba6caaadc 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -146,7 +146,6 @@ struct usb_serial {
 	struct usb_serial_driver	*type;
 	struct usb_interface		*interface;
 	unsigned char			disconnected:1;
-	unsigned char			suspending:1;
 	unsigned char			attached:1;
 	unsigned char			minors_reserved:1;
 	unsigned char			num_ports;
-- 
2.26.3

[PATCH 2/4] USB: serial: refactor endpoint classification

[Johan Hovold]

Refactor endpoint classification and replace the build-time
endpoint-array sanity checks with runtime checks in preparation for
handling endpoints from a sibling interface.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb-serial.c | 51 ++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 2a38810a3979..d981809c4ed3 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -711,36 +711,47 @@ static const struct tty_port_operations serial_port_ops = {
 	.shutdown		= serial_port_shutdown,
 };
 
+static void store_endpoint(struct usb_serial *serial,
+					struct usb_serial_endpoints *epds,
+					struct usb_endpoint_descriptor *epd)
+{
+	struct device *dev = &serial->interface->dev;
+	u8 addr = epd->bEndpointAddress;
+
+	if (usb_endpoint_is_bulk_in(epd)) {
+		if (epds->num_bulk_in == ARRAY_SIZE(epds->bulk_in))
+			return;
+		dev_dbg(dev, "found bulk in endpoint %02x\n", addr);
+		epds->bulk_in[epds->num_bulk_in++] = epd;
+	} else if (usb_endpoint_is_bulk_out(epd)) {
+		if (epds->num_bulk_out == ARRAY_SIZE(epds->bulk_out))
+			return;
+		dev_dbg(dev, "found bulk out endpoint %02x\n", addr);
+		epds->bulk_out[epds->num_bulk_out++] = epd;
+	} else if (usb_endpoint_is_int_in(epd)) {
+		if (epds->num_interrupt_in == ARRAY_SIZE(epds->interrupt_in))
+			return;
+		dev_dbg(dev, "found interrupt in endpoint %02x\n", addr);
+		epds->interrupt_in[epds->num_interrupt_in++] = epd;
+	} else if (usb_endpoint_is_int_out(epd)) {
+		if (epds->num_interrupt_out == ARRAY_SIZE(epds->interrupt_out))
+			return;
+		dev_dbg(dev, "found interrupt out endpoint %02x\n", addr);
+		epds->interrupt_out[epds->num_interrupt_out++] = epd;
+	}
+}
+
 static void find_endpoints(struct usb_serial *serial,
 					struct usb_serial_endpoints *epds)
 {
-	struct device *dev = &serial->interface->dev;
 	struct usb_host_interface *iface_desc;
 	struct usb_endpoint_descriptor *epd;
 	unsigned int i;
 
-	BUILD_BUG_ON(ARRAY_SIZE(epds->bulk_in) < USB_MAXENDPOINTS / 2);
-	BUILD_BUG_ON(ARRAY_SIZE(epds->bulk_out) < USB_MAXENDPOINTS / 2);
-	BUILD_BUG_ON(ARRAY_SIZE(epds->interrupt_in) < USB_MAXENDPOINTS / 2);
-	BUILD_BUG_ON(ARRAY_SIZE(epds->interrupt_out) < USB_MAXENDPOINTS / 2);
-
 	iface_desc = serial->interface->cur_altsetting;
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
 		epd = &iface_desc->endpoint[i].desc;
-
-		if (usb_endpoint_is_bulk_in(epd)) {
-			dev_dbg(dev, "found bulk in on endpoint %u\n", i);
-			epds->bulk_in[epds->num_bulk_in++] = epd;
-		} else if (usb_endpoint_is_bulk_out(epd)) {
-			dev_dbg(dev, "found bulk out on endpoint %u\n", i);
-			epds->bulk_out[epds->num_bulk_out++] = epd;
-		} else if (usb_endpoint_is_int_in(epd)) {
-			dev_dbg(dev, "found interrupt in on endpoint %u\n", i);
-			epds->interrupt_in[epds->num_interrupt_in++] = epd;
-		} else if (usb_endpoint_is_int_out(epd)) {
-			dev_dbg(dev, "found interrupt out on endpoint %u\n", i);
-			epds->interrupt_out[epds->num_interrupt_out++] = epd;
-		}
+		store_endpoint(serial, epds, epd);
 	}
 }
 
-- 
2.26.3

[PATCH 3/4] USB: serial: add support for multi-interface functions

[Johan Hovold]

A single USB function can be implemented using a group of interfaces and
this is for example commonly used for Communication Class devices.

Add support for multi-interface functions to USB serial core and export
an interface that allows drivers to claim a second sibling interface.
The interface could easily be extended to allow claiming further
interfaces if ever needed.

When a driver claims a sibling interface in probe(), core allocates
resources for any bulk in, bulk out, interrupt in and interrupt out
endpoints found also on the sibling interface.

Disconnect is implemented so that unbinding either interface will
release the other interface while disconnect() is called precisely once.

Similarly, suspend() is called when the first sibling interface is
suspended and resume() is called when the last sibling interface is
resumed by USB core.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb-serial.c | 84 ++++++++++++++++++++++++++++-----
 include/linux/usb/serial.h      |  7 +++
 2 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index d981809c4ed3..aaae71a0bbff 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -121,6 +121,44 @@ static void release_minors(struct usb_serial *serial)
 	serial->minors_reserved = 0;
 }
 
+int usb_serial_claim_interface(struct usb_serial *serial, struct usb_interface *intf)
+{
+	struct usb_driver *driver = serial->type->usb_driver;
+	int ret;
+
+	if (serial->sibling)
+		return -EBUSY;
+
+	ret = usb_driver_claim_interface(driver, intf, serial);
+	if (ret) {
+		dev_err(&serial->interface->dev,
+				"failed to claim sibling interface: %d\n", ret);
+		return ret;
+	}
+
+	serial->sibling = intf;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_serial_claim_interface);
+
+static void release_sibling(struct usb_serial *serial, struct usb_interface *intf)
+{
+	struct usb_driver *driver = serial->type->usb_driver;
+	struct usb_interface *sibling;
+
+	if (!serial->sibling)
+		return;
+
+	if (intf == serial->sibling)
+		sibling = serial->interface;
+	else
+		sibling = serial->sibling;
+
+	usb_set_intfdata(sibling, NULL);
+	usb_driver_release_interface(driver, sibling);
+}
+
 static void destroy_serial(struct kref *kref)
 {
 	struct usb_serial *serial;
@@ -742,13 +780,14 @@ static void store_endpoint(struct usb_serial *serial,
 }
 
 static void find_endpoints(struct usb_serial *serial,
-					struct usb_serial_endpoints *epds)
+					struct usb_serial_endpoints *epds,
+					struct usb_interface *intf)
 {
 	struct usb_host_interface *iface_desc;
 	struct usb_endpoint_descriptor *epd;
 	unsigned int i;
 
-	iface_desc = serial->interface->cur_altsetting;
+	iface_desc = intf->cur_altsetting;
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
 		epd = &iface_desc->endpoint[i].desc;
 		store_endpoint(serial, epds, epd);
@@ -917,7 +956,7 @@ static int usb_serial_probe(struct usb_interface *interface,
 
 		if (retval) {
 			dev_dbg(ddev, "sub driver rejected device\n");
-			goto err_put_serial;
+			goto err_release_sibling;
 		}
 	}
 
@@ -925,10 +964,12 @@ static int usb_serial_probe(struct usb_interface *interface,
 	epds = kzalloc(sizeof(*epds), GFP_KERNEL);
 	if (!epds) {
 		retval = -ENOMEM;
-		goto err_put_serial;
+		goto err_release_sibling;
 	}
 
-	find_endpoints(serial, epds);
+	find_endpoints(serial, epds, interface);
+	if (serial->sibling)
+		find_endpoints(serial, epds, serial->sibling);
 
 	if (epds->num_bulk_in < type->num_bulk_in ||
 			epds->num_bulk_out < type->num_bulk_out ||
@@ -1076,7 +1117,8 @@ static int usb_serial_probe(struct usb_interface *interface,
 
 err_free_epds:
 	kfree(epds);
-err_put_serial:
+err_release_sibling:
+	release_sibling(serial, interface);
 	usb_serial_put(serial);
 err_put_module:
 	module_put(type->driver.owner);
@@ -1092,6 +1134,10 @@ static void usb_serial_disconnect(struct usb_interface *interface)
 	struct usb_serial_port *port;
 	struct tty_struct *tty;
 
+	/* sibling interface is cleaning up */
+	if (!serial)
+		return;
+
 	usb_serial_console_disconnect(serial);
 
 	mutex_lock(&serial->disc_mutex);
@@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
 	if (serial->type->disconnect)
 		serial->type->disconnect(serial);
 
+	release_sibling(serial, interface);
+
 	/* let the last holder of this object cause it to be cleaned up */
 	usb_serial_put(serial);
 	dev_info(dev, "device disconnected\n");
@@ -1123,7 +1171,11 @@ static void usb_serial_disconnect(struct usb_interface *interface)
 int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct usb_serial *serial = usb_get_intfdata(intf);
-	int i, r = 0;
+	int i, r;
+
+	/* suspend when called for first sibling interface */
+	if (serial->suspend_count++)
+		return 0;
 
 	/*
 	 * serial->type->suspend() MUST return 0 in system sleep context,
@@ -1132,14 +1184,16 @@ int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 	 */
 	if (serial->type->suspend) {
 		r = serial->type->suspend(serial, message);
-		if (r < 0)
-			goto err_out;
+		if (r < 0) {
+			serial->suspend_count--;
+			return r;
+		}
 	}
 
 	for (i = 0; i < serial->num_ports; ++i)
 		usb_serial_port_poison_urbs(serial->port[i]);
-err_out:
-	return r;
+
+	return 0;
 }
 EXPORT_SYMBOL(usb_serial_suspend);
 
@@ -1156,6 +1210,10 @@ int usb_serial_resume(struct usb_interface *intf)
 	struct usb_serial *serial = usb_get_intfdata(intf);
 	int rv;
 
+	/* resume when called for last sibling interface */
+	if (--serial->suspend_count)
+		return 0;
+
 	usb_serial_unpoison_port_urbs(serial);
 
 	if (serial->type->resume)
@@ -1172,6 +1230,10 @@ static int usb_serial_reset_resume(struct usb_interface *intf)
 	struct usb_serial *serial = usb_get_intfdata(intf);
 	int rv;
 
+	/* resume when called for last sibling interface */
+	if (--serial->suspend_count)
+		return 0;
+
 	usb_serial_unpoison_port_urbs(serial);
 
 	if (serial->type->reset_resume) {
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 7efba6caaadc..e9b90577f50b 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -130,6 +130,8 @@ static inline void usb_set_serial_port_data(struct usb_serial_port *port,
  * @dev: pointer to the struct usb_device for this device
  * @type: pointer to the struct usb_serial_driver for this device
  * @interface: pointer to the struct usb_interface for this device
+ * @sibling: pointer to the struct usb_interface of any sibling interface
+ * @suspend_count: number of suspended (sibling) interfaces
  * @num_ports: the number of ports this device has
  * @num_interrupt_in: number of interrupt in endpoints we have
  * @num_interrupt_out: number of interrupt out endpoints we have
@@ -145,6 +147,8 @@ struct usb_serial {
 	struct usb_device		*dev;
 	struct usb_serial_driver	*type;
 	struct usb_interface		*interface;
+	struct usb_interface		*sibling;
+	unsigned int			suspend_count;
 	unsigned char			disconnected:1;
 	unsigned char			attached:1;
 	unsigned char			minors_reserved:1;
@@ -334,6 +338,9 @@ static inline void usb_serial_console_disconnect(struct usb_serial *serial) {}
 /* Functions needed by other parts of the usbserial core */
 struct usb_serial_port *usb_serial_port_get_by_minor(unsigned int minor);
 void usb_serial_put(struct usb_serial *serial);
+
+int usb_serial_claim_interface(struct usb_serial *serial, struct usb_interface *intf);
+
 int usb_serial_generic_open(struct tty_struct *tty, struct usb_serial_port *port);
 int usb_serial_generic_write_start(struct usb_serial_port *port, gfp_t mem_flags);
 int usb_serial_generic_write(struct tty_struct *tty, struct usb_serial_port *port,
-- 
2.26.3

[PATCH 4/4] USB: serial: xr: claim both interfaces

[Johan Hovold]

Use the new multi-interface support in USB serial core to properly claim
also the control interface during probe. This prevents having another
driver claim the control interface and makes core allocate resources
also for the interrupt endpoint (currently unused).

Switch to probing only Communication Class interfaces and use the Union
functional descriptor to determine the corresponding data interface.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/xr_serial.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index c59c8b47a120..88c73f88cb26 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/tty.h>
 #include <linux/usb.h>
+#include <linux/usb/cdc.h>
 #include <linux/usb/serial.h>
 
 struct xr_txrx_clk_mask {
@@ -550,15 +551,34 @@ static void xr_close(struct usb_serial_port *port)
 
 static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 {
-	/* Don't bind to control interface */
-	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
+	struct usb_interface *control = serial->interface;
+	struct usb_host_interface *alt = control->cur_altsetting;
+	struct usb_cdc_parsed_header hdrs;
+	struct usb_cdc_union_desc *desc;
+	struct usb_interface *data;
+	int ret;
+
+	ret = cdc_parse_cdc_header(&hdrs, control, alt->extra, alt->extralen);
+	if (ret < 0)
 		return -ENODEV;
 
+	desc = hdrs.usb_cdc_union_desc;
+	if (!desc)
+		return -ENODEV;
+
+	data = usb_ifnum_to_if(serial->dev, desc->bSlaveInterface0);
+	if (!data)
+		return -ENODEV;
+
+	ret = usb_serial_claim_interface(serial, data);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
 static const struct usb_device_id id_table[] = {
-	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
+	{ USB_DEVICE_INTERFACE_CLASS(0x04e2, 0x1410, USB_CLASS_COMM) }, /* XR21V141X */
 	{ }
 };
 MODULE_DEVICE_TABLE(usb, id_table);
-- 
2.26.3

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Oliver Neukum]

Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold:
> @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
>         if (serial->type->disconnect)
>                 serial->type->disconnect(serial);
>  
> +       release_sibling(serial, interface);
> +
>         /* let the last holder of this object cause it to be cleaned up */
>         usb_serial_put(serial);
>         dev_info(dev, "device disconnected\n");

Hi,

does this assume you are called for the original interface first?
I am afraid that is an assumption you cannot make. In fact, if somebody
is doing odd things with sysfs you cannot even assume both will see a
disconnect()

	Regards
		Oliver

Re: [PATCH 0/4] USB: serial: add support for multi-interface functions

[Greg KH]

On Tue, Mar 30, 2021 at 04:38:16PM +0200, Johan Hovold wrote:
> A single USB function can be implemented using a group of interfaces and
> this is for example commonly used for Communication Class devices.
>     
> This series adds support for multi-interface functions to USB serial
> core and exports an interface that allows drivers to claim a second
> sibling interface. The interface could easily be extended to allow
> claiming further interfaces if ever needed.
> 
> The final patch uses the new interface to properly claim both the
> control and data interface of Maxlinear/Exar devices.

Looks good, thanks for adding this:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Johan Hovold]

On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold:
> > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> >         if (serial->type->disconnect)
> >                 serial->type->disconnect(serial);
> >  
> > +       release_sibling(serial, interface);
> > +
> >         /* let the last holder of this object cause it to be cleaned up */
> >         usb_serial_put(serial);
> >         dev_info(dev, "device disconnected\n");
> 
> Hi,
> 
> does this assume you are called for the original interface first?

No, I handle either interface being unbound first (e.g. see
release_sibling()).

> I am afraid that is an assumption you cannot make. In fact, if somebody
> is doing odd things with sysfs you cannot even assume both will see a
> disconnect()

Right, but disconnect() will still be called also for the sibling
interface as part of release_sibling() above.

Johan

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Oliver Neukum]

Am Dienstag, den 30.03.2021, 17:22 +0200 schrieb Johan Hovold:
> On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote:
> > Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold:
> > > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> > >         if (serial->type->disconnect)
> > >                 serial->type->disconnect(serial);
> > >  
> > > +       release_sibling(serial, interface);
> > > +
> > >         /* let the last holder of this object cause it to be cleaned up */
> > >         usb_serial_put(serial);
> > >         dev_info(dev, "device disconnected\n");
> > 
> > Hi,
> > 
> > does this assume you are called for the original interface first?
> 
> No, I handle either interface being unbound first (e.g. see
> release_sibling()).
> 
> > I am afraid that is an assumption you cannot make. In fact, if somebody
> > is doing odd things with sysfs you cannot even assume both will see a
> > disconnect()
> 
> Right, but disconnect() will still be called also for the sibling
> interface as part of release_sibling() above.

OK, sorry I overlooked that.

	Regards
		Oliver

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Oliver Neukum]

Am Mittwoch, den 31.03.2021, 09:08 +0200 schrieb Oliver Neukum:
> Am Dienstag, den 30.03.2021, 17:22 +0200 schrieb Johan Hovold:
> > On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote:
> > > Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold:
> > > > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> > > >         if (serial->type->disconnect)
> > > >                 serial->type->disconnect(serial);
> > > >  
> > > > +       release_sibling(serial, interface);
> > > > +
> > > >         /* let the last holder of this object cause it to be cleaned up */
> > > >         usb_serial_put(serial);
> > > >         dev_info(dev, "device disconnected\n");
> > > 
> > > Hi,
> > > 
> > > does this assume you are called for the original interface first?
> > 
> > No, I handle either interface being unbound first (e.g. see
> > release_sibling()).
> > 
> > > I am afraid that is an assumption you cannot make. In fact, if somebody
> > > is doing odd things with sysfs you cannot even assume both will see a
> > > disconnect()
> > 
> > Right, but disconnect() will still be called also for the sibling
> > interface as part of release_sibling() above.
> 
> OK, sorry I overlooked that.

Hi,

on the third hand, the more I look at this, would you mind putting
sibling_release() with a modified name into usbcore? This functionality
is not limited to serial drivers. btusb needs it; cdc-acm needs it;
usbaudio neds it. We have code duplication.

	Regards
		Oliver

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Johan Hovold]

On Wed, Mar 31, 2021 at 01:21:15PM +0200, Oliver Neukum wrote:
> Am Mittwoch, den 31.03.2021, 09:08 +0200 schrieb Oliver Neukum:
> > Am Dienstag, den 30.03.2021, 17:22 +0200 schrieb Johan Hovold:
> > > On Tue, Mar 30, 2021 at 04:44:32PM +0200, Oliver Neukum wrote:
> > > > Am Dienstag, den 30.03.2021, 16:38 +0200 schrieb Johan Hovold:
> > > > > @@ -1115,6 +1161,8 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> > > > >         if (serial->type->disconnect)
> > > > >                 serial->type->disconnect(serial);
> > > > >  
> > > > > +       release_sibling(serial, interface);
> > > > > +
> > > > >         /* let the last holder of this object cause it to be cleaned up */
> > > > >         usb_serial_put(serial);
> > > > >         dev_info(dev, "device disconnected\n");
> > > > 
> > > > Hi,
> > > > 
> > > > does this assume you are called for the original interface first?
> > > 
> > > No, I handle either interface being unbound first (e.g. see
> > > release_sibling()).
> > > 
> > > > I am afraid that is an assumption you cannot make. In fact, if somebody
> > > > is doing odd things with sysfs you cannot even assume both will see a
> > > > disconnect()
> > > 
> > > Right, but disconnect() will still be called also for the sibling
> > > interface as part of release_sibling() above.
> > 
> > OK, sorry I overlooked that.
> 
> Hi,
> 
> on the third hand, the more I look at this, would you mind putting
> sibling_release() with a modified name into usbcore? This functionality
> is not limited to serial drivers. btusb needs it; cdc-acm needs it;
> usbaudio neds it. We have code duplication.

Tell me about it. ;) Unfortunately, drivers all tend to handle this
slightly different, for example, using a disconnected flag, some claim
more than one other interface, others look like they may be using their
interface data as a flag for other purposes, etc.

At some point we could unify all this but until then I don't think
putting only half of an interface into core makes much sense.

Johan

Re: [PATCH 0/4] USB: serial: add support for multi-interface functions

[Johan Hovold]

On Tue, Mar 30, 2021 at 04:50:02PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Mar 30, 2021 at 04:38:16PM +0200, Johan Hovold wrote:
> > A single USB function can be implemented using a group of interfaces and
> > this is for example commonly used for Communication Class devices.
> >     
> > This series adds support for multi-interface functions to USB serial
> > core and exports an interface that allows drivers to claim a second
> > sibling interface. The interface could easily be extended to allow
> > claiming further interfaces if ever needed.
> > 
> > The final patch uses the new interface to properly claim both the
> > control and data interface of Maxlinear/Exar devices.
> 
> Looks good, thanks for adding this:
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing. Now applied.

Johan

Re: [PATCH 3/4] USB: serial: add support for multi-interface functions

[Oliver Neukum]

Am Donnerstag, den 01.04.2021, 09:46 +0200 schrieb Johan Hovold:
> On Wed, Mar 31, 2021 at 01:21:15PM +0200, Oliver Neukum wrote:
> > Am Mittwoch, den 31.03.2021, 09:08 +0200 schrieb Oliver Neukum:

> > on the third hand, the more I look at this, would you mind putting
> > sibling_release() with a modified name into usbcore? This functionality
> > is not limited to serial drivers. btusb needs it; cdc-acm needs it;
> > usbaudio neds it. We have code duplication.
> 
> Tell me about it. ;) Unfortunately, drivers all tend to handle this
> slightly different, for example, using a disconnected flag, some claim
> more than one other interface, others look like they may be using their
> interface data as a flag for other purposes, etc.
> 
> At some point we could unify all this but until then I don't think
> putting only half of an interface into core makes much sense.

OK, very well, then let's look at this from a fundamental point
and design a bit. First, can we disregard the case of more than
two interfaces?

	Regards
		Oliver