On Tue, Jun 26, 2018 at 09:40:24AM +0200, Bjørn Mork wrote:
> 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....
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
