On Sat, Jan 04, 2025 at 07:45:29AM +0000, Will Mortensen wrote:
> Previously, usb_choose_configuration() chose the first config whose
> first interface was UAC3 (if there was such a config), which could mean
> choosing UAC3 BADD over full UAC3, potentially losing most of the
> device's functionality. With this patch, we check the config's first IAD
> and only prefer the config if it's full UAC3, not BADD.
>
> Note that if the device complies with the UAC3 spec, then the device's
> first config is UAC1/2. With this patch, if the device also has a UAC3
> BADD config but no full UAC3 config (which is allowed by the spec),
> then we'll select the first, UAC1/2 config, *not* the BADD config.
>
> That might be undesirable (?), so we could instead try to implement a
> priority scheme like: full UAC3 > UAC3 BADD > UAC1/2. But unless we also
> enhance this function to look at more than one IAD and interface per
> config, we could incorrectly select the BADD config over more fully-
> featured UAC1/2/3 configs if the full UAC3 IAD is not first in its
> config(s). I don't know enough about UAC3 devices to know what's
> preferable, and I'm not sure how simple vs. correct the heuristics in
> this function should be. :-) This patch errs on the side of simple.
>
> For some history, the preference for the first UAC3 config (instead of
> the first config, which should be UAC1/2) originated a bit before the
> Fixes commit, in commit f13912d3f014 ("usbcore: Select UAC3
> configuration for audio if present") and commit ff2a8c532c14 ("usbcore:
> Select only first configuration for non-UAC3 compliant devices"). Also,
> the Fixes commit's message is a bit wrong in one place since the UAC3
> spec prohibits a device's first configuration from being UAC3.
>
> I tested only with an Apple USB-C headphone adapter (as in the linked
> bug), which has three configs in the following order: UAC2, UAC3 BADD,
> full UAC3. Previously the UAC3 BADD config was selected; with this patch
> the full UAC3 config is selected.
>
> Reported-by: AT <kernel@xxxxxxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217501
> Fixes: 25b016145036 ("USB: Fix configuration selection issues introduced in v4.20.0")
> Cc: Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxxx>
> Cc: Tatsuyuki Ishi <ishitatsuyuki@xxxxxxxxx>
> Cc: Saranya Gopal <saranya.gopal@xxxxxxxxx>
> Cc: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> Cc: Nikolay Yakimov <root@xxxxxxxxxxx>
> Signed-off-by: Will Mortensen <willmo@xxxxxxxxx>
> ---
> drivers/usb/core/generic.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index b134bff5c3fe..ce9c86967922 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -20,6 +20,7 @@
> */
>
> #include <linux/usb.h>
> +#include <linux/usb/audio-v3.h>
> #include <linux/usb/hcd.h>
> #include <uapi/linux/usb/audio.h>
> #include "usb.h"
> @@ -48,9 +49,11 @@ static bool is_audio(struct usb_interface_descriptor *desc)
> return desc->bInterfaceClass == USB_CLASS_AUDIO;
> }
>
> -static bool is_uac3_config(struct usb_interface_descriptor *desc)
> +static bool is_full_uac3(struct usb_interface_assoc_descriptor *assoc)
> {
> - return desc->bInterfaceProtocol == UAC_VERSION_3;
> + return assoc->bFunctionClass == USB_CLASS_AUDIO
> + && assoc->bFunctionSubClass == UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0
> + && assoc->bFunctionProtocol == UAC_VERSION_3;
Nit, the "&&" should go on the previous lines, right?
> }
>
> int usb_choose_configuration(struct usb_device *udev)
> @@ -84,6 +87,8 @@ int usb_choose_configuration(struct usb_device *udev)
> num_configs = udev->descriptor.bNumConfigurations;
> for (i = 0; i < num_configs; (i++, c++)) {
> struct usb_interface_descriptor *desc = NULL;
> + /* first IAD if present, else NULL */
> + struct usb_interface_assoc_descriptor *assoc = c->intf_assoc[0];
>
> /* It's possible that a config has no interfaces! */
> if (c->desc.bNumInterfaces > 0)
> @@ -137,17 +142,21 @@ int usb_choose_configuration(struct usb_device *udev)
> /*
> * 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.
> + * But, still iterate through other configurations and select
> + * full UAC3 compliant config if present. (If the only UAC3
> + * config is a BADD, we will instead select the first config,
> + * which should be UAC1/2.)
> */
> if (desc && is_audio(desc)) {
> - /* Always prefer the first found UAC3 config */
> - if (is_uac3_config(desc)) {
> + /* Always prefer the first found full UAC3 config */
> + if (assoc != NULL && is_full_uac3(assoc)) {
I feel like this is making the kernel pick a specific policy, when
userspace might have wanted a different one, right? Is there anything
in the USB spec that mandates that this is the correct config to always
pick "first"?
And what about your comment above which says it "should" be the first
config, where is that required? What about devices that didn't have
that and now the functionality changes because that assumption isn't
true, and they weren't a "full UAC3 compliant" device?
In other words, I'm really worried about regressions here, what devices
has this change been tested on and how can we be assured nothing that
works today is suddenly going to break?
thanks,
greg k-h
