diff mbox series

[RFC,4/4] USB: cdc-acm: clean up handling of quirky devices

Message ID 20200921113525.32187-5-johan@kernel.org
State New
Headers show
Series USB: cdc-acm: handle broken union descriptors | expand

Commit Message

Johan Hovold Sept. 21, 2020, 11:35 a.m. UTC
Instead of falling back to "combined-interface" probing when detecting
broken union and call-management descriptors, assume all interfaces with
three endpoints are of "combined-interface" type.

This allows for the removal of the NO_DATA_INTERFACE quirk and makes the
probe algorithm somewhat easier to follow.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 44 ++++++++++---------------------------
 drivers/usb/class/cdc-acm.h | 11 +++++-----
 2 files changed, 16 insertions(+), 39 deletions(-)

Comments

Oliver Neukum Sept. 21, 2020, 11:53 a.m. UTC | #1
Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:
> Instead of falling back to "combined-interface" probing when detecting

> broken union and call-management descriptors, assume all interfaces with

> three endpoints are of "combined-interface" type.


Hi,

this just ignores a union header. I am afraid that is not correct.
Could you move it into the !union_header clause?

	Regards
		Oliver
Johan Hovold Sept. 21, 2020, 12:15 p.m. UTC | #2
On Mon, Sep 21, 2020 at 01:53:52PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:

> > Instead of falling back to "combined-interface" probing when detecting

> > broken union and call-management descriptors, assume all interfaces with

> > three endpoints are of "combined-interface" type.

> 

> Hi,

> 

> this just ignores a union header. I am afraid that is not correct.

> Could you move it into the !union_header clause?


And probe for a combined interface before falling back to the management
descriptor then? Along the lines of

	if (!union_desc) {
		if (bNumEndpoints == 3) {
			goto probe_combined_interface;
		} else if (call_intf_num > 0) {
			data_intf_num = call_intf_num;
			...
		} else {
			return -ENODEV;
		}
	} else {
		...
	}
			
Johan
Oliver Neukum Sept. 21, 2020, 12:45 p.m. UTC | #3
Am Montag, den 21.09.2020, 14:15 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 01:53:52PM +0200, Oliver Neukum wrote:

> > Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:

> > > Instead of falling back to "combined-interface" probing when detecting

> > > broken union and call-management descriptors, assume all interfaces with

> > > three endpoints are of "combined-interface" type.

> > 

> > Hi,

> > 

> > this just ignores a union header. I am afraid that is not correct.

> > Could you move it into the !union_header clause?

> 

> And probe for a combined interface before falling back to the management

> descriptor then? Along the lines of



Yes, exactly.

	Regards
		Oliver
diff mbox series

Patch

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 2758e295871e..eda883ce6d9d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1217,44 +1217,27 @@  static int acm_probe(struct usb_interface *intf,
 	if (cmgmd)
 		call_intf_num = cmgmd->bDataInterface;
 
+	if (intf->cur_altsetting->desc.bNumEndpoints == 3) {
+		dev_dbg(&intf->dev, "assuming single interface\n");
+		combined_interfaces = 1;
+		control_interface = data_interface = intf;
+		goto look_for_collapsed_interface;
+	}
+
 	if (!union_header) {
 		if (call_intf_num > 0) {
 			dev_dbg(&intf->dev, "No union descriptor, using call management descriptor\n");
-			/* quirks for Droids MuIn LCD */
-			if (quirks & NO_DATA_INTERFACE) {
-				data_interface = usb_ifnum_to_if(usb_dev, 0);
-			} else {
-				data_intf_num = call_intf_num;
-				data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
-			}
+			data_intf_num = call_intf_num;
+			data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
 			control_interface = intf;
 		} else {
-			if (intf->cur_altsetting->desc.bNumEndpoints != 3) {
-				dev_dbg(&intf->dev,"No union descriptor, giving up\n");
-				return -ENODEV;
-			} else {
-				dev_warn(&intf->dev,"No union descriptor, testing for castrated device\n");
-				combined_interfaces = 1;
-				control_interface = data_interface = intf;
-				goto look_for_collapsed_interface;
-			}
+			dev_dbg(&intf->dev, "No union descriptor, giving up\n");
+			return -ENODEV;
 		}
 	} else {
-		int class = -1;
-
 		data_intf_num = union_header->bSlaveInterface0;
 		control_interface = usb_ifnum_to_if(usb_dev, union_header->bMasterInterface0);
 		data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
-
-		if (control_interface)
-			class = control_interface->cur_altsetting->desc.bInterfaceClass;
-
-		if (class != USB_CLASS_COMM && class != USB_CLASS_CDC_DATA) {
-			dev_warn(&intf->dev, "Broken union descriptor, assuming single interface\n");
-			combined_interfaces = 1;
-			control_interface = data_interface = intf;
-			goto look_for_collapsed_interface;
-		}
 	}
 
 	if (!control_interface || !data_interface) {
@@ -1881,11 +1864,6 @@  static const struct usb_device_id acm_ids[] = {
 
 	/* NOTE: non-Nokia COMM/ACM/0xff is likely MSFT RNDIS... NOT a modem! */
 
-	/* Support for Droids MuIn LCD */
-	{ USB_DEVICE(0x04d8, 0x000b),
-	.driver_info = NO_DATA_INTERFACE,
-	},
-
 #if IS_ENABLED(CONFIG_INPUT_IMS_PCU)
 	{ USB_DEVICE(0x04d8, 0x0082),	/* Application mode */
 	.driver_info = IGNORE_DEVICE,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index b7174a0098a5..b2135095898f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -135,9 +135,8 @@  struct acm {
 #define NO_UNION_NORMAL			BIT(0)
 #define SINGLE_RX_URB			BIT(1)
 #define NO_CAP_LINE			BIT(2)
-#define NO_DATA_INTERFACE		BIT(4)
-#define IGNORE_DEVICE			BIT(5)
-#define QUIRK_CONTROL_LINE_STATE	BIT(6)
-#define CLEAR_HALT_CONDITIONS		BIT(7)
-#define SEND_ZERO_PACKET		BIT(8)
-#define DISABLE_ECHO			BIT(9)
+#define IGNORE_DEVICE			BIT(3)
+#define QUIRK_CONTROL_LINE_STATE	BIT(4)
+#define CLEAR_HALT_CONDITIONS		BIT(5)
+#define SEND_ZERO_PACKET		BIT(6)
+#define DISABLE_ECHO			BIT(7)