| Message ID | 20190115161354.6806-1-root@livid.pp.ru (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 25b0161450363ed7fe9fe618cda202e15817311a |
| Headers | show |
| Series | USB: Fix configuration selection issues introduced in v4.20.0 | expand |
On Tue, Jan 15, 2019 at 07:13:54PM +0300, Nikolay Yakimov wrote: > Commit f13912d3f014a introduced changes to the usb_choose_configuration function > to better support USB Audio UAC3-compatible devices. However, there are a few > problems with this patch. First of all, it adds new "if" clauses in the middle > of an existing "if"/"else if" tree, which obviously breaks pre-existing logic. > Secondly, since it continues iterating over configurations in one of the branches, > other code in the loop can choose an unintended configuration. Finally, > if an audio device's first configuration is UAC3-compatible, and there > are multiple UAC3 configurations, the second one would be chosen, due to > the first configuration never being checked for UAC3-compatibility. > > Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a > somewhat unnecessarily convoluted way, in my opinion, and does nothing > to fix the first or the last one. > > This patch tries to rectify problems described by essentially rewriting > code introduced in f13912d3f014a. Notice the code was moved to *before* > the "if"/"else if" tree. > > Signed-off-by: Nikolay Yakimov <root@livid.pp.ru> > --- > drivers/usb/core/generic.c | 44 ++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 19 deletions(-) Were you able to test this on one of the devices that ff2a8c532c14 ("usbcore: Select only first configuration for non-UAC3 compliant devices") was created to fix? thanks, greg k-h
tue, 15 jan. 2019 at 19:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > Were you able to test this on one of the devices that ff2a8c532c14 > ("usbcore: Select only first configuration for non-UAC3 compliant > devices") was created to fix? Yes, owning such a device (non-UAC3 with multiple configurations) was the motivation for investigating in the first place (since it was broken on v4.20.0 and v4.20.1). So the proposed patch is tested against it. In fact, I only discovered ff2a8c532c14 ("usbcore: Select only first configuration for non-UAC3 compliant devices") when preparing to send in the patch (it's not in any released kernel yet) > > thanks, > > greg k-h
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. 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. Thanks, Saranya > -----Original Message----- > From: Nikolay Yakimov [mailto:root@livid.pp.ru] > Sent: Tuesday, January 15, 2019 9:44 PM > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Nikolay Yakimov <root@livid.pp.ru>; Gopal, Saranya > <saranya.gopal@intel.com>; linux-usb@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: [PATCH] USB: Fix configuration selection issues introduced in v4.20.0 > > Commit f13912d3f014a introduced changes to the usb_choose_configuration > function > to better support USB Audio UAC3-compatible devices. However, there are a > few > problems with this patch. First of all, it adds new "if" clauses in the middle > of an existing "if"/"else if" tree, which obviously breaks pre-existing logic. > Secondly, since it continues iterating over configurations in one of the branches, > other code in the loop can choose an unintended configuration. Finally, > if an audio device's first configuration is UAC3-compatible, and there > are multiple UAC3 configurations, the second one would be chosen, due to > the first configuration never being checked for UAC3-compatibility. > > Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a > somewhat unnecessarily convoluted way, in my opinion, and does nothing > to fix the first or the last one. > > This patch tries to rectify problems described by essentially rewriting > code introduced in f13912d3f014a. Notice the code was moved to *before* > the "if"/"else if" tree. > > Signed-off-by: Nikolay Yakimov <root@livid.pp.ru> > --- > drivers/usb/core/generic.c | 44 ++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 19 deletions(-) > > diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c > index f713cecc1f41..1ac9c1e5f773 100644 > --- a/drivers/usb/core/generic.c > +++ b/drivers/usb/core/generic.c > @@ -118,6 +118,31 @@ int usb_choose_configuration(struct usb_device > *udev) > continue; > } > > + /* > + * Select first configuration as default for audio so that > + * devices that don't comply with UAC3 protocol are supported. > + * But, still iterate through other configurations and > + * select UAC3 compliant config if present. > + */ > + if (desc && is_audio(desc)) { > + /* Always prefer the first found UAC3 config */ > + if (is_uac3_config(desc)) { > + best = c; > + break; > + } > + > + /* If there is no UAC3 config, prefer the first config */ > + else if (i == 0) > + best = c; > + > + /* Unconditional continue, because the rest of the code > + * in the loop is irrelevant for audio devices, and > + * because it can reassign best, which for audio devices > + * we don't want. > + */ > + continue; > + } > + > /* When the first config's first interface is one of Microsoft's > * pet nonstandard Ethernet-over-USB protocols, ignore it > unless > * this kernel has enabled the necessary host side driver. > @@ -132,25 +157,6 @@ int usb_choose_configuration(struct usb_device > *udev) > #endif > } > > - /* > - * Select first configuration as default for audio so that > - * devices that don't comply with UAC3 protocol are supported. > - * But, still iterate through other configurations and > - * select UAC3 compliant config if present. > - */ > - if (i == 0 && num_configs > 1 && desc && is_audio(desc)) { > - best = c; > - continue; > - } > - > - if (i > 0 && desc && is_audio(desc)) { > - if (is_uac3_config(desc)) { > - best = c; > - break; > - } > - continue; > - } > - > /* 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 > -- > 2.20.1
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
> 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. Agreed. Thanks for pointing it out. I somehow missed this :( Thanks, Saranya
Hey there. Sorry to nag, but it seems we came to the conclusion that there's indeed a problem with the current code, and then collectively decided that we're done. Can I do something to expedite this? Are there any issues with my proposed patch? If there are, what can I do to fix those? Alternatively, perhaps it would be easier/faster if a bona fide kernel developer could fix problems described in this thread (well, actually, I believe we're down to one somewhat pressing problem, which could be fixed just by moving some code around) On Thu, 17 Jan 2019 at 06:53, Gopal, Saranya <saranya.gopal@intel.com> wrote: > > > 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. > > Agreed. Thanks for pointing it out. I somehow missed this :( > > Thanks, > Saranya
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c index f713cecc1f41..1ac9c1e5f773 100644 --- a/drivers/usb/core/generic.c +++ b/drivers/usb/core/generic.c @@ -118,6 +118,31 @@ int usb_choose_configuration(struct usb_device *udev) continue; } + /* + * Select first configuration as default for audio so that + * devices that don't comply with UAC3 protocol are supported. + * But, still iterate through other configurations and + * select UAC3 compliant config if present. + */ + if (desc && is_audio(desc)) { + /* Always prefer the first found UAC3 config */ + if (is_uac3_config(desc)) { + best = c; + break; + } + + /* If there is no UAC3 config, prefer the first config */ + else if (i == 0) + best = c; + + /* Unconditional continue, because the rest of the code + * in the loop is irrelevant for audio devices, and + * because it can reassign best, which for audio devices + * we don't want. + */ + continue; + } + /* When the first config's first interface is one of Microsoft's * pet nonstandard Ethernet-over-USB protocols, ignore it unless * this kernel has enabled the necessary host side driver. @@ -132,25 +157,6 @@ int usb_choose_configuration(struct usb_device *udev) #endif } - /* - * Select first configuration as default for audio so that - * devices that don't comply with UAC3 protocol are supported. - * But, still iterate through other configurations and - * select UAC3 compliant config if present. - */ - if (i == 0 && num_configs > 1 && desc && is_audio(desc)) { - best = c; - continue; - } - - if (i > 0 && desc && is_audio(desc)) { - if (is_uac3_config(desc)) { - best = c; - break; - } - continue; - } - /* 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
Commit f13912d3f014a introduced changes to the usb_choose_configuration function to better support USB Audio UAC3-compatible devices. However, there are a few problems with this patch. First of all, it adds new "if" clauses in the middle of an existing "if"/"else if" tree, which obviously breaks pre-existing logic. Secondly, since it continues iterating over configurations in one of the branches, other code in the loop can choose an unintended configuration. Finally, if an audio device's first configuration is UAC3-compatible, and there are multiple UAC3 configurations, the second one would be chosen, due to the first configuration never being checked for UAC3-compatibility. Commit ff2a8c532c14 tries to fix the second issue, but it goes about it in a somewhat unnecessarily convoluted way, in my opinion, and does nothing to fix the first or the last one. This patch tries to rectify problems described by essentially rewriting code introduced in f13912d3f014a. Notice the code was moved to *before* the "if"/"else if" tree. Signed-off-by: Nikolay Yakimov <root@livid.pp.ru> --- drivers/usb/core/generic.c | 44 ++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 19 deletions(-)