| Message ID | 20200921135951.24045-5-johan@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | USB: cdc-acm: handle broken union descriptors | expand |
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold: > For interfaces that lack a union descriptor, probe for a > "combined-interface" before falling back to the call-management > descriptor instead of the other way round. Hi, the more I look at this the more it seems to me like the device that has the quirk does NOT have a collapsed interface but two interfaces and just a lack of a union descriptor. I am taking the original author into CC (hoping it still workd) Johan, would just taking the first three patches of the series work for now? Regards Oliver
On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote: > Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold: > > For interfaces that lack a union descriptor, probe for a > > "combined-interface" before falling back to the call-management > > descriptor instead of the other way round. > > Hi, > > the more I look at this the more it seems to me like the > device that has the quirk does NOT have a collapsed interface > but two interfaces and just a lack of a union descriptor. But then why name the quirk NO_DATA_INTERFACE if it has a data interface? By hardcoding the data-interface number to be the one and only interface, you'd end up probing for a "combined" interface also with a broken call-management descriptor. Side note: I really think we should start mandating lsusb output to go along with any patch for quirky devices. > I am taking the original author into CC (hoping it still workd) > Johan, would just taking the first three patches of the series work for > now? Of course, no problem holding back the last one for a bit. Johan
Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold: > On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote: > > Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold: > > > For interfaces that lack a union descriptor, probe for a > > > "combined-interface" before falling back to the call-management > > > descriptor instead of the other way round. > > > > Hi, Hi, > > > > the more I look at this the more it seems to me like the > > device that has the quirk does NOT have a collapsed interface > > but two interfaces and just a lack of a union descriptor. > > But then why name the quirk NO_DATA_INTERFACE if it has a data In hindsight that seems not the best name. > interface? By hardcoding the data-interface number to be the one and > only interface, you'd end up probing for a "combined" interface also > with a broken call-management descriptor. Well, by the changelog assuming a combined interface caused an oops. Thence I am forced to conclude that the davices _has_ a separate data interface, but no union descriptor. > Side note: I really think we should start mandating lsusb output to go > along with any patch for quirky devices. Good idea. Convince Greg. Regards Oliver
On Mon, Sep 21, 2020 at 05:04:34PM +0200, Oliver Neukum wrote: > Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold: > > On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote: > > > Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold: > > > > For interfaces that lack a union descriptor, probe for a > > > > "combined-interface" before falling back to the call-management > > > > descriptor instead of the other way round. > > > > > > Hi, > > Hi, > > > > > > > the more I look at this the more it seems to me like the > > > device that has the quirk does NOT have a collapsed interface > > > but two interfaces and just a lack of a union descriptor. > > > > But then why name the quirk NO_DATA_INTERFACE if it has a data > > In hindsight that seems not the best name. > > > interface? By hardcoding the data-interface number to be the one and > > only interface, you'd end up probing for a "combined" interface also > > with a broken call-management descriptor. > > Well, by the changelog assuming a combined interface caused an oops. > Thence I am forced to conclude that the davices _has_ a separate > data interface, but no union descriptor. No, the oops was probably due to the missing sanity check later added by 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces"). With a broken call-management descriptor pointing to a non-existent interface we'd oops before that commit. > > Side note: I really think we should start mandating lsusb output to go > > along with any patch for quirky devices. > > Good idea. Convince Greg. Heh. I'm sure he can be convinced. :) Johan
Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold: > > > interface? By hardcoding the data-interface number to be the one and > > > only interface, you'd end up probing for a "combined" interface also > > > with a broken call-management descriptor. > > > > Well, by the changelog assuming a combined interface caused an oops. > > Thence I am forced to conclude that the davices _has_ a separate > > data interface, but no union descriptor. > > No, the oops was probably due to the missing sanity check later added by > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces"). > > With a broken call-management descriptor pointing to a non-existent > interface we'd oops before that commit. Hi, maybe I am dense, but a patch that comes after a patch that is said to fix something? Furthermore that patch would not come it work, it would merely make probe() fail cleanly. If I read the changelog correctly, the change makes the device work. Regards Oliver
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index a361b937684a..357e896a4fc0 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -1218,26 +1218,19 @@ static int acm_probe(struct usb_interface *intf, call_intf_num = cmgmd->bDataInterface; if (!union_header) { - if (call_intf_num > 0) { + if (intf->cur_altsetting->desc.bNumEndpoints == 3) { + dev_dbg(&intf->dev, "No union descriptor, assuming single interface\n"); + combined_interfaces = 1; + control_interface = data_interface = intf; + goto look_for_collapsed_interface; + } else 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; @@ -1881,11 +1874,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)
For interfaces that lack a union descriptor, probe for a "combined-interface" before falling back to the call-management descriptor instead of the other way round. 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 | 32 ++++++++++---------------------- drivers/usb/class/cdc-acm.h | 11 +++++------ 2 files changed, 15 insertions(+), 28 deletions(-)