USB: cdc-acm: add Whistler radio scanners TRX series support

PATCH In review Johan Hovold 3 participants 12 messages Sep 21, 2020 – Sep 25, 2020

Subsystems (3)

Stable branch USB USB ACM driver

lore

Share & tools

lore

Johan Hovold

author

2020-09-21 08:10 UTC #0 lore raw

PATCH v2 1/1 USB: cdc-acm: add Whistler radio scanners TRX series support

Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.

Note that the NO_DATA_INTERFACE quirk was added by commit fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") to handle a combined-interface-type device with a broken call-management descriptor by hardcoding the "data" interface number.

Link https://lore.kernel.org/r/[email protected] open here

Reported-by Daniel Caujolle-Bert <[email protected]>

Tested-by Daniel Caujolle-Bert <[email protected]>

Cc [email protected]

Signed-off-by Johan Hovold <[email protected]>

---

v2
- use the right class define in the device-id table (not subclass with same value)

 drivers/usb/class/cdc-acm.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

drivers/usb/class/cdc-acm.c +24 ~~-12~~

index 7f6f3ab5b8a6..316203bab0b8 100644
@@ -1220,27 +1220,26 @@ static int acm_probe(struct usb_interface *intf,
 	if (cmgmd)
 		call_intf_num = cmgmd->bDataInterface;
 
-	if (!union_header) {
-		if (call_intf_num > 0) {
+	combined_interfaces = (quirks & NO_DATA_INTERFACE) != 0;
+
+	if (!union_header || combined_interfaces) {
+		if (call_intf_num > 0 && !combined_interfaces) {
 			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 {
+			}
+
+			if (!combined_interfaces) {
 				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;
 			}
+			control_interface = data_interface = intf;
+			goto look_for_collapsed_interface;
 		}
 	} else {
 		data_intf_num = union_header->bSlaveInterface0;

@@ -1807,6 +1806,19 @@ static const struct usb_device_id acm_ids[] = {
 	.driver_info = CLEAR_HALT_CONDITIONS,
 	},
 
+	{ USB_DEVICE_INTERFACE_CLASS(0x2a59, 0x0010, USB_CLASS_COMM),	/* Whistler TRX-1 */
+	  .driver_info = NO_DATA_INTERFACE,
+	},
+	{ USB_DEVICE_INTERFACE_CLASS(0x2a59, 0x0011, USB_CLASS_COMM),	/* Whistler TRX-2 */
+	  .driver_info = NO_DATA_INTERFACE,
+	},
+	{ USB_DEVICE_INTERFACE_CLASS(0x2a59, 0x0012, USB_CLASS_COMM),	/* Whistler TRX-1e */
+	  .driver_info = NO_DATA_INTERFACE,
+	},
+	{ USB_DEVICE_INTERFACE_CLASS(0x2a59, 0x0013, USB_CLASS_COMM),	/* Whistler TRX-2e */
+	  .driver_info = NO_DATA_INTERFACE,
+	},
+
 	/* Nokia S60 phones expose two ACM channels. The first is
 	 * a modem and is picked up by the standard AT-command
 	 * information below. The second is 'vendor-specific' but

Signature · 1 line

2.26.2

Oliver Neukum

maintainer

2020-09-21 08:43 UTC #1 lore raw

Am Montag, den 21.09.2020, 10:10 +0200 schrieb Johan Hovold:

Quoted 4 lines

Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.

Hi,

it amazes me what solutions people can come up with. Yet in this case using a quirk looks like an inferior solution. If your master is a storage interface, you will have a condition on the device you can test for without the need for a quirk.

Regards
Oliver

Johan Hovold

author

2020-09-21 09:31 UTC #2 lore raw

Quoted 6 lines · Oliver Neukum

Am Montag, den 21.09.2020, 10:10 +0200 schrieb Johan Hovold:
Quoted 4 lines
Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.

Quoted 6 lines

Hi,
it amazes me what solutions people can come up with. Yet in this case using a quirk looks like an inferior solution. If your master is a storage interface, you will have a condition on the device you can test for without the need for a quirk.

Sure, and I mentioned that as an alternative, another would be checking for a control interface with three endpoints directly.

My fear is that any change in this direction risk introducing regression if there are devices out there with broken descriptors that we currently happen to support by chance. Then again, probably better to try to handle any such breakage if/when reported.

I'll respin.

Johan

Oliver Neukum

maintainer

2020-09-21 10:29 UTC #3 lore raw

Am Montag, den 21.09.2020, 11:31 +0200 schrieb Johan Hovold:

Quoted 8 lines · Oliver Neukum

Quoted 7 lines
Am Montag, den 21.09.2020, 10:10 +0200 schrieb Johan Hovold:
Quoted 4 lines
Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.
Hi,

Hi,

Quoted 8 lines

Quoted 5 lines
it amazes me what solutions people can come up with. Yet in this case using a quirk looks like an inferior solution. If your master is a storage interface, you will have a condition on the device you can test for without the need for a quirk.
Sure, and I mentioned that as an alternative, another would be checking for a control interface with three endpoints directly.

These tests are not mutually exclusive. You can check for both conditions being met. In fact you have to, it seems to me.

Quoted 4 lines

My fear is that any change in this direction risk introducing regression if there are devices out there with broken descriptors that we currently happen to support by chance. Then again, probably better to try to handle any such breakage if/when reported.

Well, I guess the chance that we break devices which claim to be storage devices we will simply have to take. Those devices are quite broken in any case.

Thank you for redoing this.

Regards
Oliver

Johan Hovold

author

2020-09-21 11:36 UTC #4 lore raw

Quoted 18 lines · Oliver Neukum

Am Montag, den 21.09.2020, 11:31 +0200 schrieb Johan Hovold:
Quoted 8 lines · Oliver Neukum
Quoted 7 lines
Am Montag, den 21.09.2020, 10:10 +0200 schrieb Johan Hovold:
Quoted 4 lines
Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.
Hi,
Hi,
Quoted 6 lines
Quoted 5 lines
it amazes me what solutions people can come up with. Yet in this case using a quirk looks like an inferior solution. If your master is a storage interface, you will have a condition on the device you can test for without the need for a quirk.

Quoted 5 lines

Quoted 2 lines
Sure, and I mentioned that as an alternative, another would be checking for a control interface with three endpoints directly.
These tests are not mutually exclusive. You can check for both conditions being met. In fact you have to, it seems to me.

I meant that instead of falling back to "combined-interface" probing we could assume that all interfaces with three endpoints are "combined" and simply ignore the union and call management descriptors and all the ways that devices may have gotten those wrong.

I'll include that as an RFC.

Quoted 8 lines

Quoted 4 lines
My fear is that any change in this direction risk introducing regression if there are devices out there with broken descriptors that we currently happen to support by chance. Then again, probably better to try to handle any such breakage if/when reported.
Well, I guess the chance that we break devices which claim to be storage devices we will simply have to take. Those devices are quite broken in any case.

I was thinking more of the individual entries in the device-id table whose control interfaces may not even be of the Communication class. But hopefully that was verified when adding them.

Johan

Oliver Neukum

maintainer

2020-09-21 11:49 UTC #5 lore raw

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

Quoted 1 line · Oliver Neukum

Hi,

Quoted 4 lines

I meant that instead of falling back to "combined-interface" probing we could assume that all interfaces with three endpoints are "combined" and simply ignore the union and call managementy. descriptors and all the ways that devices may have gotten those wrong.

I am afraid we would break the spec. I cannot recall a prohibition on having more endpoints than necessary. Heuristics and ignoring invalid descriptors is one things. Ignoring valid descriptors is something else.

Quoted 3 lines

I was thinking more of the individual entries in the device-id table whose control interfaces may not even be of the Communication class. But hopefully that was verified when adding them.

Now you are confusing me. In case of a quirky device, why change the current logic?

Regards
Oliver

Johan Hovold

author

2020-09-21 12:03 UTC #6 lore raw

Quoted 10 lines · Oliver Neukum

Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
Quoted 1 line · Oliver Neukum
Hi,
Quoted 4 lines
I meant that instead of falling back to "combined-interface" probing we could assume that all interfaces with three endpoints are "combined" and simply ignore the union and call managementy. descriptors and all the ways that devices may have gotten those wrong.

Quoted 4 lines

I am afraid we would break the spec. I cannot recall a prohibition on having more endpoints than necessary. Heuristics and ignoring invalid descriptors is one things. Ignoring valid descriptors is something else.

That depends on how you read the spec (see "3.3.1 Communication Class Interface"). But sure, it's probably be better to err on the safe-side.

Quoted 6 lines

Quoted 3 lines
I was thinking more of the individual entries in the device-id table whose control interfaces may not even be of the Communication class. But hopefully that was verified when adding them.
Now you are confusing me. In case of a quirky device, why change the current logic?

Just because they have a quirk defined, doesn't mean they don't rely on the generic probe algorithm (e.g. a USB_DEVICE entry which matches all interface classes and only specifies SEND_ZERO_PACKET).

Johan

Oliver Neukum

maintainer

2020-09-21 12:17 UTC #7 lore raw

Am Montag, den 21.09.2020, 14:03 +0200 schrieb Johan Hovold:

Quoted 11 lines · Oliver Neukum

Quoted 10 lines
Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
Quoted 1 line · Oliver Neukum
Hi,
Quoted 4 lines
I meant that instead of falling back to "combined-interface" probing we could assume that all interfaces with three endpoints are "combined" and simply ignore the union and call managementy. descriptors and all the ways that devices may have gotten those wrong.

Quoted 7 lines

Quoted 4 lines
I am afraid we would break the spec. I cannot recall a prohibition on having more endpoints than necessary. Heuristics and ignoring invalid descriptors is one things. Ignoring valid descriptors is something else.
That depends on how you read the spec (see "3.3.1 Communication Class Interface"). But sure, it's probably be better to err on the safe-side.

You mean 3.4.1?

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
I was thinking more of the individual entries in the device-id table whose control interfaces may not even be of the Communication class. But hopefully that was verified when adding them.
Now you are confusing me. In case of a quirky device, why change the current logic?
Just because they have a quirk defined, doesn't mean they don't rely on the generic probe algorithm (e.g. a USB_DEVICE entry which matches all interface classes and only specifies SEND_ZERO_PACKET).

Right, so let me be more specific. It would probably be unwise to change the decision tree in probe() as far as devices whose quirks affect decisions in that already are concerned.

Regards
Oliver

Johan Hovold

author

2020-09-21 13:44 UTC #8 lore raw

Quoted 17 lines · Oliver Neukum

Am Montag, den 21.09.2020, 14:03 +0200 schrieb Johan Hovold:
Quoted 16 lines · Oliver Neukum
Quoted 14 lines
Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
Quoted 1 line · Oliver Neukum
Hi,
Quoted 4 lines
I meant that instead of falling back to "combined-interface" probing we could assume that all interfaces with three endpoints are "combined" and simply ignore the union and call managementy. descriptors and all the ways that devices may have gotten those wrong.
I am afraid we would break the spec. I cannot recall a prohibition on having more endpoints than necessary. Heuristics and ignoring invalid descriptors is one things. Ignoring valid descriptors is something else.

Quoted 4 lines

Quoted 2 lines
That depends on how you read the spec (see "3.3.1 Communication Class Interface"). But sure, it's probably be better to err on the safe-side.
You mean 3.4.1?

It's 3.3.1 in Version 1.1 at least.

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I was thinking more of the individual entries in the device-id table whose control interfaces may not even be of the Communication class. But hopefully that was verified when adding them.
Now you are confusing me. In case of a quirky device, why change the current logic?

Quoted 7 lines

Quoted 3 lines
Just because they have a quirk defined, doesn't mean they don't rely on the generic probe algorithm (e.g. a USB_DEVICE entry which matches all interface classes and only specifies SEND_ZERO_PACKET).
Right, so let me be more specific. It would probably be unwise to change the decision tree in probe() as far as devices whose quirks affect decisions in that already are concerned.

But we need to draw line somewhere to keep the code maintainable and ourselves sane, especially since a lot of these devices where added without any record of their descriptors.

I guess we could add another test for the device-id fields, but I'm reluctant to add more special casing before we know it's needed.

Johan

Greg Kroah-Hartman

maintainer

2020-09-25 14:49 UTC #9 lore raw

In reply to Johan Hovold

Quoted 15 lines · Johan Hovold

Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.
Note that the NO_DATA_INTERFACE quirk was added by commit fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") to handle a combined-interface-type device with a broken call-management descriptor by hardcoding the "data" interface number.
Link https://lore.kernel.org/r/[email protected] open here
Reported-by Daniel Caujolle-Bert <[email protected]>
Tested-by Daniel Caujolle-Bert <[email protected]>
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>

Quoted 5 lines

---
v2
- use the right class define in the device-id table (not subclass with same value)

Is this independant of your other patch series for cdc-acm?

thanks,

greg k-h

Johan Hovold

author

2020-09-25 14:53 UTC #10 lore raw

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Johan Hovold
Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.
Note that the NO_DATA_INTERFACE quirk was added by commit fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") to handle a combined-interface-type device with a broken call-management descriptor by hardcoding the "data" interface number.
Link https://lore.kernel.org/r/[email protected] open here
Reported-by Daniel Caujolle-Bert <[email protected]>
Tested-by Daniel Caujolle-Bert <[email protected]>
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---

Quoted 5 lines

Quoted 3 lines
v2
- use the right class define in the device-id table (not subclass with same value)
Is this independant of your other patch series for cdc-acm?

This one is superseded by the series, so please drop this patch. Sorry for not making that clear.

Johan

Greg Kroah-Hartman

maintainer

2020-09-25 15:00 UTC #11 lore raw

Quoted 19 lines · Johan Hovold

Quoted 18 lines · Greg Kroah-Hartman
Quoted 17 lines · Johan Hovold
Add support for Whistler radio scanners TRX series, which have a union descriptor that designates a mass-storage interface as master. Handle that by generalising the NO_DATA_INTERFACE quirk to allow us to fall back to using the combined-interface detection.
Note that the NO_DATA_INTERFACE quirk was added by commit fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") to handle a combined-interface-type device with a broken call-management descriptor by hardcoding the "data" interface number.
Link https://lore.kernel.org/r/[email protected] open here
Reported-by Daniel Caujolle-Bert <[email protected]>
Tested-by Daniel Caujolle-Bert <[email protected]>
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
v2
- use the right class define in the device-id table (not subclass with same value)
Is this independant of your other patch series for cdc-acm?
This one is superseded by the series, so please drop this patch. Sorry for not making that clear.

No worries, thanks for the quick response. Now dropped.

greg k-h