Message-ID: <CAFCitJ8cLi1Tqk47fhr-F2PBHxxtu_9kr4sSHwdN6XWYmC=2cg@mail.gmail.com>
Date: Mon, 6 Jan 2025 01:08:43 -0800
From: Will Mortensen <willmo@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Nikolay Yakimov <root@...id.pp.ru>, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, AT <kernel@...nhelix.com>, 
	Ruslan Bilovol <ruslan.bilovol@...il.com>, Takashi Iwai <tiwai@...e.com>, 
	Tatsuyuki Ishi <ishitatsuyuki@...il.com>, Saranya Gopal <saranya.gopal@...el.com>, 
	Felipe Balbi <felipe.balbi@...ux.intel.com>, 
	Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Subject: Re: [PATCH] usb: core: prefer only full UAC3 config, not BADD

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
<gregkh@...uxfoundation.org> 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 <kernel@...nhelix.com>
> > 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@...il.com>
> > Cc: Takashi Iwai <tiwai@...e.com>
> > Cc: Tatsuyuki Ishi <ishitatsuyuki@...il.com>
> > Cc: Saranya Gopal <saranya.gopal@...el.com>
> > Cc: Felipe Balbi <felipe.balbi@...ux.intel.com>
> > Cc: Nikolay Yakimov <root@...id.pp.ru>
> > Signed-off-by: Will Mortensen <willmo@...il.com>
> > ---
> >  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/20250104-usb-choose-config-full-uac3-v1-1-f8bf8760ae90@gmail.com/T/

Powered by blists - more mailing lists