Re: [PATCH 2/2] ALSA: usb-audio: Do not expose sticky volume control mixers

[Rong Zhang <i@rong.moe>]

Hi Takashi,

Thanks for your review.

On Thu, 2026-04-09 at 09:08 +0200, Takashi Iwai wrote:
> On Wed, 08 Apr 2026 20:33:06 +0200,
> Rong Zhang wrote:
> > 
> > Some devices expose sticky mixers that accept SET_CUR but do absolutely
> > nothing. Registering mixers for them confuses userspace and results in
> > ineffective volume control.
> > 
> > Check if the volume control is sticky by setting the volume to the
> > maximum or minimum value, and prevent the mixer from being registered
> > accordingly.
> > 
> > Quirky device sample:
> > 
> >   usb 7-1: New USB device found, idVendor=0e0b, idProduct=fa01, bcdDevice= 1.00
> >   usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> >   usb 7-1: Product: Feaulle Rainbow
> >   usb 7-1: Manufacturer: Generic
> >   usb 7-1: SerialNumber: 20210726905926
> >   (Mic Capture Volume)
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >  sound/usb/mixer.c | 37 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index a25e8145af67..9f0aed36e27d 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -27,6 +27,7 @@
> >   *  	- parse available sample rates again when clock sources changed
> >   */
> >  
> > +#include <linux/array_size.h>
> 
> It's only for ARRAY_SIZE()?  Then no need for extra inclusion.  It's
> already included by others.
> 
> >  #include <linux/bitops.h>
> >  #include <linux/init.h>
> >  #include <linux/list.h>
> > @@ -1287,17 +1288,49 @@ static int get_min_max_with_quirks(struct usb_mixer_elem_info *cval,
> >  		if (cval->res == 0)
> >  			cval->res = 1;
> >  
> > -		/* Additional checks for the proper resolution
> > +		/* Additional checks
> > +		 *
> > +		 * Some devices expose sticky mixers that accept SET_CUR
> > +		 * but do absolutely nothing.
> >  		 *
> >  		 * Some devices report smaller resolutions than actually
> >  		 * reacting.  They don't return errors but simply clip
> >  		 * to the lower aligned value.
> >  		 */
> > -		if (cval->min + cval->res < cval->max) {
> > +		if (cval->min < cval->max) {
> > +			int sticky_test_values[] = { cval->min, cval->max };
> >  			int last_valid_res = cval->res;
> >  			int saved, test, check;
> > +			bool effective = false;
> > +
> >  			if (get_cur_mix_raw(cval, minchn, &saved) < 0)
> >  				goto no_res_check;
> > +
> > +			for (i = 0; i < ARRAY_SIZE(sticky_test_values); i++) {
> > +				test = sticky_test_values[i];
> > +				if (test == saved)
> > +					continue;
> > +				/* Assume non-sticky on failure. */
> > +				if (snd_usb_set_cur_mix_value(cval, minchn, 0, test) ||
> > +				    get_cur_mix_raw(cval, minchn, &check) ||
> > +				    check != saved) { /* SET_CUR effective, non-sticky. */
> > +					effective = true;
> > +					break;
> > +				}
> > +			}
> > +			if (!effective) {
> > +				usb_audio_warn(cval->head.mixer->chip,
> > +					"%d:%d: sticky mixer values (%d/%d/%d => %d), disabling\n",
> > +					cval->head.id, mixer_ctrl_intf(cval->head.mixer),
> > +					cval->min, cval->max, cval->res, saved);
> > +
> > +				cval->initialized = 1;
> > +				cval->min = cval->max = cval->res = 0;
> > +				return -ENODEV;
> > +			}
> > +
> > +			if (cval->min + cval->res >= cval->max)
> > +				goto no_res_check;
> 
> Hm, it's quite lots of changes, and I'd like this to be factored out
> as a function instead.  Maybe the resolution check code could be moved
> into a function, too; get_min_max_with_quirks() is too lengthy.

Sure, I will refactor the current code path beforehand when I resubmit
this.

Hmm, does patch 1 make sense to you? If so, I would be grateful if you
could you apply it separately so that I can send a new series solely
focusing on refactoring and adding the sticky mixer check.

> 
> Also, note that currently there is no error check for get_min_max*().
> Your code works just because you set cval->min = cval->max, and there
> is an additional check at a later point of this.
> 
> Ideally speaking, we should have an error check to explicitly handle a
> case like this.  But the current code allows the error at init, at
> least, we tolerate the errors at reading UAC_GET_MIN and _MAX.  So, if
> any, the error check would need to evaluate the error number and
> ignore -EINVAL or such...

Makes sense, I will address it beforehand when I resubmit this.

BTW, how about checking against -ENODEV instead? It can be considered to
be an indication that the caller should not register the mixer and is
much more readable. I don't have a strong preference for this though,
and ignoring -EINVAL also makes sense to me.

Thanks,
Rong

> 
> 
> thanks,
> 
> Takashi