usb: core: prefer only full UAC3 config, not BADD

Commit Message

Will Mortensen Jan. 4, 2025, 7:45 a.m. UTC

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 <[e-mail]>
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 <[e-mail]>
Cc: Takashi Iwai <[e-mail]>
Cc: Tatsuyuki Ishi <[e-mail]>
Cc: Saranya Gopal <[e-mail]>
Cc: Felipe Balbi <[e-mail]>
Cc: Nikolay Yakimov <[e-mail]>
Signed-off-by: Will Mortensen <[e-mail]>
---
 drivers/usb/core/generic.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)


---
base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
change-id: 20250104-usb-choose-config-full-uac3-c5d9413fb650

Best regards,

Comments

Greg KH Jan. 4, 2025, 8:53 a.m. UTC | #1

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 <[e-mail]>
> 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 <[e-mail]>
> Cc: Takashi Iwai <[e-mail]>
> Cc: Tatsuyuki Ishi <[e-mail]>
> Cc: Saranya Gopal <[e-mail]>
> Cc: Felipe Balbi <[e-mail]>
> Cc: Nikolay Yakimov <[e-mail]>
> Signed-off-by: Will Mortensen <[e-mail]>
> ---
>  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

Will Mortensen Jan. 6, 2025, 9:08 a.m. UTC | #2

Hi Greg,

(For new CCs, see [1] for full context)

Thanks for the feedback! Replies below:

On Sat, Jan 4, 2025 at 12:53 AM Greg Kroah-Hartman
<[e-mail]> wrote:
> 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 <[e-mail]>
> > 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 <[e-mail]>
> > Cc: Takashi Iwai <[e-mail]>
> > Cc: Tatsuyuki Ishi <[e-mail]>
> > Cc: Saranya Gopal <[e-mail]>
> > Cc: Felipe Balbi <[e-mail]>
> > Cc: Nikolay Yakimov <[e-mail]>
> > Signed-off-by: Will Mortensen <[e-mail]>
> > ---
> >  drivers/usb/core/generic.c | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > @@ -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?

Sorry, I copied that style from the functions a few lines above. It
seems this file isn't consistent. :-) I'm happy to change it.

Answering your other points in reverse order:

> 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?

The only audio device I tested on was the Apple headphone adapter. :-)
I can try to test on a few more audio devices, and find some
descriptor dumps online.

I definitely take your point that we should avoid behavior changes
(presumably even at the cost of some code complexity) unless they're
strongly justified. This patch erred on the side of code simplicity at
the cost of unnecessary behavior changes. I'll strike a better balance
going forward.

> 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?

Hmm, do you mean this comment?

/*
 * […] (If the only UAC3
 * config is a BADD, we will instead select the first config,
 * which should be UAC1/2.)
 */

The UAC3 spec requires the first config to comply with UAC1/2. If the
device violates that then it's more complicated, but anyway, this
comment describes an unnecessary behavior change in the patch. I'll
avoid this going forward unless I can justify it better.

> 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"?

Good question. I think the UAC3 spec implies that full UAC3 (if
present) is preferred over UAC3 BADD and UAC1/2. But perhaps more to
the point, it says in section 4.1 "Standard Descriptors":

    Because any Device compliant with this specification is required to
    contain multiple Configuration descriptors, it is also required that any
    such device include a Configuration Summary Descriptor as part of
    the standard BOS descriptor.

And the USB 3.2 spec says in section 9.6.2.7 "Configuration Summary Descriptor":

    Configuration Summary Descriptors should be included in the BOS
    descriptor in order of descending preference.

And my Apple headphone adapters do have those descriptors (in the
opposite order of the configuration descriptors: full UAC3, UAC3 BADD,
UAC2). So those descriptors might be the best signal, but AFAICT the
kernel doesn't parse them. It seems the kernel just has
usb_choose_configuration(), which just looks at the first interface of
each configuration, and usb_device_driver.choose_configuration(),
which only one driver implements (r8152, to choose a vendor-specific
configuration sometimes).

As for userspace, at least when it comes to USB audio devices, it
seems like users generally have to write their own scripts or udev
rules that call usb_modeswitch or equivalent. At a quick glance, I
don't see any audio devices in the usb_modeswitch DB, nor any
automatic USB configuration selection in PipeWire/PulseAudio/JACK or
the various ALSA utilities (although I may not have looked in the
right places).

Anyway, if we really just want to delegate this whole issue to
userspace, it's a little funny that the kernel does have a policy of
preferring UAC3, albeit without distinguishing between BADD and full
UAC3. Are we just stuck with that now? :-) Would it ever make sense
for the kernel to try to respect the preference expressed by the
Configuration Summary Descriptors when they exist?


[1] https://lore.kernel.org/linux-usb/[e-mail]/T/

Patch

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;
 }
 
 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)) {
 				best = c;
 				break;
 			}
-
-			/* If there is no UAC3 config, prefer the first config */
+			/*
+			 * If there is no full UAC3 config, prefer the first
+			 * config.
+			 */
 			else if (i == 0)
 				best = c;