| Message ID | 20200921081022.6881-1-johan@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] USB: cdc-acm: add Whistler radio scanners TRX series support | expand |
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
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
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
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
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
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
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
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
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
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(-)