usb: serial: qcserial: add support for the Dell DW5821e module

Quoted 4 lines · Johan Hovold

Quoted 3 lines · Aleksander Morgado

This module exposes two USB configurations: a QMI+AT capable setup on USB config #1 and a MBIM capable setup on USB config #2.

Quoted 8 lines

Quoted 3 lines

By default the kernel will choose the MBIM capable configuration as long as the cdc_mbim driver is available. This patch adds support for the serial ports in the secondary configuration.

Could you please post the usb-devices output for this device?

Depending on another driver to select a specific configuration seems fragile (and that behaviour is even configurable).

I believe Aleksander might be referring to usb_choose_configuration() in drivers/usb/core/generic.c? It does some confusing things with multi-function/multi-configuration devices, explained by this comment:

/* From the remaining configs, choose the first one whose
* first interface is for a non-vendor-specific class.
* Reason: Linux is more likely to have a class driver
* than a vendor-specific driver. */

This code can make Linux default to a MBIM configuration if the MBIM function uses the first interface in that configuration, even if this configuration is not the first one. Availability of a driver is not considered. Except for RNDIS, just to make it the whole mess even more confusing....

The result is of course completely arbitrary on any multi-function device, where vendor-specific functions may be mixed with class functions in any order. The result will also change if you e.g. enable a vendor specific debug function in the MBIM configuration, and this function happens to use the first interface.

The logic in usb_choose_configuration() is obviously outdated. I assume it was created for single function devices, where that single function was exposed as either a class function or a vendor specific function using separate configurations. This sort of device is still quite common for a few device classes, like for example ethernet NICs. But things have changed a lot for these as well. Linux now has extensive support for vendor sepcific USB functions of all sorts. Not that I need to tell you that :-) The legacy code in usb_choose_configuration() is now often an obstacle making it more difficult for drivers providing the best possible support in Linux. And we end up with horrible hacks like this config-dependent blacklist entry in cdc_ether:

#if IS_ENABLED(CONFIG_USB_RTL8152)
/* Linksys USB3GIGV1 Ethernet Adapter */

{
	USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
			USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
	.driver_info = 0,
},

#endif

matched with code in the rtl8152 driver to switch the device back to its default configuration:

static int rtl8152_probe(struct usb_interface *intf,
const struct usb_device_id *id)

{
	struct usb_device *udev = interface_to_usbdev(intf);
	u8 version = rtl_get_version(intf);
	struct r8152 *tp;
	struct net_device *netdev;
	int ret;

	if (version == RTL_VER_UNKNOWN)
		return -ENODEV;

	if (udev->actconfig->desc.bConfigurationValue != 1) {
		usb_driver_set_configuration(udev, 1);
		return -ENODEV;
	}

..

Extremely ugly, and completely unnecessary if it weren't for that extra mess in usb_choose_configuration(). And the net effect is that the users end up losing the option of using the class driver for this device, since that will be black listed if they (or the distro) built the vendor specific driver.

Can we fix this? I've thought about it more than once for a few years, but so far I've rejected the thought. The Linux defaults coming out of usb_choose_configuration() are arbitrary, and often different from any other OS, confusing end users. But the Linux defaults are stable as long as the device configration is stable. Changing usb_choose_configuration() will break some existing setups. The breakages will of course be easy fixable from userspace, but still - proabably out question for Linux?

Sorry if I misunderstodd you, Aleksander. I guess you could be thinking about the usb_modeswitch logic too, which does consider cdc_mbim availability. The rant is still valid, though :-)