[PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[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/5f4ca4f8.1c69fb81.a4487.0f5f@mx.google.com
Reported-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
Tested-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---

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(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..316203bab0b8 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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
-- 
2.26.2

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Oliver Neukum]

Am Montag, den 21.09.2020, 10:10 +0200 schrieb 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.

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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Johan Hovold]

On Mon, Sep 21, 2020 at 10:43:12AM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 10:10 +0200 schrieb 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.
> 
> 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Oliver Neukum]

Am Montag, den 21.09.2020, 11:31 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 10:43:12AM +0200, Oliver Neukum wrote:
> > Am Montag, den 21.09.2020, 10:10 +0200 schrieb 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.
> > 
> > Hi,

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.

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

> 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Johan Hovold]

On Mon, Sep 21, 2020 at 12:29:16PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 11:31 +0200 schrieb Johan Hovold:
> > On Mon, Sep 21, 2020 at 10:43:12AM +0200, Oliver Neukum wrote:
> > > Am Montag, den 21.09.2020, 10:10 +0200 schrieb 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.
> > > 
> > > Hi,
> 
> 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.
> 
> 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.

> > 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Oliver Neukum]

Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 12:29:16PM +0200, Oliver Neukum wrote:

Hi,

> 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.

> 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Johan Hovold]

On Mon, Sep 21, 2020 at 01:49:14PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
> > On Mon, Sep 21, 2020 at 12:29:16PM +0200, Oliver Neukum wrote:
> 
> Hi,
> 
> > 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.

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.

> > 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Oliver Neukum]

Am Montag, den 21.09.2020, 14:03 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 01:49:14PM +0200, Oliver Neukum wrote:
> > Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
> > > On Mon, Sep 21, 2020 at 12:29:16PM +0200, Oliver Neukum wrote:
> > 
> > Hi,
> > 
> > > 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.
> 
> 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?

> > > 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Johan Hovold]

On Mon, Sep 21, 2020 at 02:17:07PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 14:03 +0200 schrieb Johan Hovold:
> > On Mon, Sep 21, 2020 at 01:49:14PM +0200, Oliver Neukum wrote:
> > > Am Montag, den 21.09.2020, 13:36 +0200 schrieb Johan Hovold:
> > > > On Mon, Sep 21, 2020 at 12:29:16PM +0200, Oliver Neukum wrote:
> > > 
> > > Hi,
> > > 
> > > > 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.
> > 
> > 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.

> > > > 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.

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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Greg Kroah-Hartman]

On Mon, Sep 21, 2020 at 10:10:22AM +0200, Johan Hovold wrote:
> 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/5f4ca4f8.1c69fb81.a4487.0f5f@mx.google.com
> Reported-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> Tested-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> 
> 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Johan Hovold]

On Fri, Sep 25, 2020 at 04:49:22PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 21, 2020 at 10:10:22AM +0200, Johan Hovold wrote:
> > 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/5f4ca4f8.1c69fb81.a4487.0f5f@mx.google.com
> > Reported-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> > Tested-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> > 
> > 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

Re: [PATCH v2] USB: cdc-acm: add Whistler radio scanners TRX series support

[Greg Kroah-Hartman]

On Fri, Sep 25, 2020 at 04:53:31PM +0200, Johan Hovold wrote:
> On Fri, Sep 25, 2020 at 04:49:22PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Sep 21, 2020 at 10:10:22AM +0200, Johan Hovold wrote:
> > > 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/5f4ca4f8.1c69fb81.a4487.0f5f@mx.google.com
> > > Reported-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> > > Tested-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > > 
> > > 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