Message-ID: <87v7uqx5ry.wl-tiwai@suse.de>
Date: Tue, 07 Jan 2025 11:44:17 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Will Mortensen <willmo@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	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

On Mon, 06 Jan 2025 10:08:43 +0100,
Will Mortensen wrote:
> 
> 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?

It sounds like a good suggestion.  We should check the actual UAC3
devices, but judging (only) from the description, this will lead to a
best choice.

Speaking of regression: the current pickup of UAC3 BADD caused already
a regression, and basically this patch can be seen as a sort of
long-standing regression, too, as stated in the bugzilla entry.


thanks,

Takashi

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