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

[Johan Hovold <johan@kernel.org>]

On Tue, Jun 26, 2018 at 09:40:24AM +0200, Bjørn Mork wrote:
> Johan Hovold <johan@kernel.org> writes:
> > On Sat, Jun 23, 2018 at 11:24:08PM +0200, Aleksander Morgado wrote:
> >> This module exposes two USB configurations: a QMI+AT capable setup on
> >> USB config #1 and a MBIM capable setup on USB config #2.
> >>
> >> 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....

Ah, that may be the case. Aleksander, would you mind updating the commit
message and drop the cdc_mbim driver bit?

Please also include the usb-devices output in the commit message for
future reference.

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

Wow. But from a quick glance at the corresponding commit

	10c3271712f5 ("r8152: disable the ECM mode")

it looks like this may at least partly have been due to firmware issues
when switching configurations.

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

Yeah, not nice.

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

Yeah, perhaps it's too late to change, but whatever default we pick, we
should at least allow user space to override it.

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

Yup. And the commit message would still need to be amended.

Thanks,
Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html