Re: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0

[Nikolay Yakimov <root@livid.pp.ru>]

On Wed, 16 Jan 2019 at 07:24, Gopal, Saranya <saranya.gopal@intel.com> wrote:
>
> Hi Yakimov,
>
> As per UAC3 configuration, the first configuration will always be backward-compatible.
> So, there cannot be any UAC3-compatible device which has first configuration as UAC3.

Thanks for the clarification. I would argue though that not all
devices might adhere to the specification, so it's still probably a
good idea to check all configurations "just in case". I will not press
this point though.

> And secondly, the commit ff2a8c532c14 does not break the pre-existing logic.
> I also thought so much about moving the code above Ethernet check while preparing ff2a8c532c14.
> But, then, it doesn't break the existing logic and so decided against moving it.

It does seem to change the logic for RNDIS devices with multiple
configurations when RNDIS kernel module is enabled.

Consider an RNDIS device with multiple configurations (num_configs > 1)

Before f13912d3f014a, the following sequence would be executed:

1. i == 0.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    best = c;
}
else ...

The condition is true, so this branch is executed. The first
configuration is selected. Since it's the last condition (all the
remaining code in the loop body is in the else branch), the loop will
continue onto the next iteration.

2. i == 1.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    ...
}
else if (udev->descriptor.bDeviceClass !=
                    USB_CLASS_VENDOR_SPEC &&
            (desc && desc->bInterfaceClass !=
                    USB_CLASS_VENDOR_SPEC)) {
    best = c;
    break;
} else ...

If the second configuration is not vendor-specific, the first "else
if" branch would be executed, and the second configuration is
selected. The loop is exited on break.

After ff2a8c532c14, the following sequence would be executed:

1. i == 0.

if (i == 0 && num_configs > 1 && desc &&
        (is_rndis(desc) || is_activesync(desc))) {
    best = c;
}

The condition is true, so this branch is executed. The first
configuration is selected, but the loop body continues execution.

2. Still i == 0.

if (i == 0 && num_configs > 1 && desc && is_audio(desc)) {
    ...
}

A redundant check is preformed to check if the device is an audio
device. We already know it isn't.

3. Still i == 0.

if (i > 0 && desc && is_audio(desc)) {
    ...
}
else ...

A redundant check is preformed to check if (i > 0). We already know it isn't.

4. Still i == 0.

else if (udev->descriptor.bDeviceClass !=
                    USB_CLASS_VENDOR_SPEC &&
            (desc && desc->bInterfaceClass !=
                    USB_CLASS_VENDOR_SPEC)) {
    best = c;
    break;
}
else ...

The first "else if" condition is checked. Unless bDeviceClass is
USB_CLASS_VENDOR_SPEC (correct me if I'm wrong, but in most cases I
think it wouldn't be), that condition evaluates as true. The
configuration is selected (redundantly), and the loop is exited on
break.

The result is this:
Before f13912d3f014a, if an RNDIS device has non-vendor-specific
configurations after the first one, that one would be selected.
After ff2a8c532c14, the first configuration would always be selected
for RNDIS devices. Besides, there are several redundant checks in this
case, which is, if nothing else, confusing.

Hopefully I've explained my point clearly enough.

>
> Thanks,
> Saranya