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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 09 Apr 2026 16:20:30 +0200,
Rong Zhang wrote:
> 
> 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.

OK, I'm going to apply only the first patch now.

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

The -EINVAL could be a temporary error at the probe time, and there
were cases where the later read works.  I meant the code:

		if (get_ctl_value(cval, UAC_GET_MAX, (cval->control << 8) | minchn, &cval->max) < 0 ||
		    get_ctl_value(cval, UAC_GET_MIN, (cval->control << 8) | minchn, &cval->min) < 0) {
			usb_audio_err(cval->head.mixer->chip,
				      "%d:%d: cannot get min/max values for control %d (id %d)\n",
				   cval->head.id, mixer_ctrl_intf(cval->head.mixer),
							       cval->control, cval->head.id);
			return -EINVAL;
		}

... and maybe -EINVAL isn't really a good sign, but better to replace
with -EAGAIN.  Then continuing after -EAGAIN in the caller side would
become more logical.


thanks,

Takashi