Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johan Hovold <johan@xxxxxxxxxx> 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....

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



Bjørn

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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux