ALSA: usb-audio: Check sticky mixers precisely

[PATCH] ALSA: usb-audio: Check sticky mixers precisely

Posted by Rong Zhang 1 month, 2 weeks ago

Some mixers are asynchronous, and some have broken min/max. They are
mistakenly considered sticky due to how the check is implemented.

Check sticky mixers more precisely by checking approximately 16 values
and adding a msleep(10) between each check, so that asynchronous mixers
have enough time to change the value and mixers with broken min/max are
checked properly. Additionally, mark GET_CUR as broken when
get_cur_mix_raw() fails, instead of returning successfully.

Reported-by: Alexander Niemeyer <adventureFAN@gmx.de>
Closes: https://lore.kernel.org/r/6262cbbd-d1f2-4c9d-a1c7-9c5d12636f4b@gmx.de
Signed-off-by: Rong Zhang <i@rong.moe>
---
 sound/usb/mixer.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 703c118f9d4e..3d0f97730a06 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1256,22 +1256,59 @@ static void init_cur_mix_raw(struct usb_mixer_elem_info *cval, int ch, int idx)
 static int check_sticky_volume_control(struct usb_mixer_elem_info *cval,
 				       int channel, int saved)
 {
-	int sticky_test_values[] = { cval->min, cval->max };
-	int test, check, i;
+	int test, check, res;
+
+	/*
+	 * Check approximately 16 values (15 intervals).
+	 * If the resolution is not fine enough, check fewer values.
+	 */
+	res = DIV_ROUND_UP(cval->max - cval->min, 15);
+	res = res ? roundup(res, cval->res) : cval->res;
+
+	/*
+	 * If (cval->max - cval->min) is not a multiple of cval->res, we still
+	 * want to test cval->max anyway.
+	 */
+	for (test = cval->min; test < cval->max + res; test += res) {
+		if (test > cval->max)
+			test = cval->max;
 
-	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, channel, 0, test) ||
-		    get_cur_mix_raw(cval, channel, &check) ||
-		    check != saved) /* SET_CUR effective, non-sticky. */
+		if (snd_usb_set_cur_mix_value(cval, channel, 0, test))
+			return 0;
+
+		if (get_cur_mix_raw(cval, channel, &check))
+			goto get_cur_broken;
+		if (check != saved) /* SET_CUR effective, non-sticky. */
 			return 0;
+
+		/*
+		 * Leave some time for asynchronous mixers to change the value.
+		 *
+		 * Note that there is no need to wait between SET_CUR and
+		 * GET_CUR, as we don't care whether the GET_CUR value matches
+		 * the SET_CUR one. IOW, what we expect is just a GET_CUR value
+		 * differing from the saved one.
+		 *
+		 * Mixers of most devices are synchronous. The should have
+		 * returned early without extra sleep. Asynchronous mixers will
+		 * return once the accumulated time is enough for them to change
+		 * the value.
+		 */
+		msleep(10);
 	}
 
+	/* Check again after the last msleep(). */
+	if (get_cur_mix_raw(cval, channel, &check))
+		goto get_cur_broken;
+	if (check != saved)
+		return 0;
+
 	if (cval->head.mixer->chip->quirk_flags & QUIRK_FLAG_MIXER_GET_CUR_BROKEN) {
+get_cur_broken:
 		usb_audio_info(cval->head.mixer->chip,
 			       "%d:%d: broken mixer GET_CUR (%d/%d/%d => %d)\n",
 			       cval->head.id, mixer_ctrl_intf(cval->head.mixer),

---
base-commit: 3eb40771c00a8488fa6ed2cc1fe203477908bf38
change-id: 74676fce-uac-precise-sticky-check-94474a22b57d

Thanks,
Rong

Re: [PATCH] ALSA: usb-audio: Check sticky mixers precisely

Posted by Alexander Niemeyer 1 month, 1 week ago

Hi Rong,

I tested the sticky-check part of your patch on the Logitech PRO X 
Wireless (046d:0aba) on Fedora 44, kernel 7.1.8-200.fc44.x86_64.

Since your patch is based on a newer tree, I used a minimal backport of 
the new ~16-value / 10 ms sticky-check logic to the 7.1.8 code. The 
GET_CUR-broken handling from the newer tree was not included; GET_CUR 
itself succeeds on this device.

Unfortunately, the playback control is still classified as sticky:

2:0: sticky mixer values (-16384/0/256 => -3840), disabling

I then instrumented the check and tried an additional diagnostic: after 
every successful SET_CUR, wait 100 ms and perform another GET_CUR before 
issuing the next SET_CUR.

For the playback volume, the saved value was -3840 and GET_CUR remained 
at -3840 for every tested value, even after 100 ms, for example:

test=-15104 immediate=-3840 after100ms=-3840
test=-13824 immediate=-3840 after100ms=-3840
test=-3584 immediate=-3840 after100ms=-3840
test=-2304 immediate=-3840 after100ms=-3840
test=-1024 immediate=-3840 after100ms=-3840
test=0 immediate=-3840 after100ms=-3840

So in this case the issue does not appear to be simply that the 
accumulated 10 ms sleeps are too short. During the probe-time sticky 
check, SET_CUR succeeds but GET_CUR for the playback control remains 
unchanged even when each SET_CUR is given 100 ms before the next one.

This differs from my previous direct libusb tests with the AudioControl 
interface unbound, where valid SET_CUR values became visible through 
GET_CUR after roughly 47–81 ms.

The first debug line I saw with |saved=0| was from the Mic Capture 
Volume control; that control changed immediately and returned as 
non-sticky. The sequence above with |saved=-3840| is the problematic PCM 
Playback Volume control.

I'd be happy to test another version or run additional diagnostics if 
useful.

Best regards,
Alexander


Am 15.08.2026 um 23:47 schrieb Rong Zhang:
> Some mixers are asynchronous, and some have broken min/max. They are
> mistakenly considered sticky due to how the check is implemented.
>
> Check sticky mixers more precisely by checking approximately 16 values
> and adding a msleep(10) between each check, so that asynchronous mixers
> have enough time to change the value and mixers with broken min/max are
> checked properly. Additionally, mark GET_CUR as broken when
> get_cur_mix_raw() fails, instead of returning successfully.
>
> Reported-by: Alexander Niemeyer <adventureFAN@gmx.de>
> Closes: https://lore.kernel.org/r/6262cbbd-d1f2-4c9d-a1c7-9c5d12636f4b@gmx.de
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>   sound/usb/mixer.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 703c118f9d4e..3d0f97730a06 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -1256,22 +1256,59 @@ static void init_cur_mix_raw(struct usb_mixer_elem_info *cval, int ch, int idx)
>   static int check_sticky_volume_control(struct usb_mixer_elem_info *cval,
>   				       int channel, int saved)
>   {
> -	int sticky_test_values[] = { cval->min, cval->max };
> -	int test, check, i;
> +	int test, check, res;
> +
> +	/*
> +	 * Check approximately 16 values (15 intervals).
> +	 * If the resolution is not fine enough, check fewer values.
> +	 */
> +	res = DIV_ROUND_UP(cval->max - cval->min, 15);
> +	res = res ? roundup(res, cval->res) : cval->res;
> +
> +	/*
> +	 * If (cval->max - cval->min) is not a multiple of cval->res, we still
> +	 * want to test cval->max anyway.
> +	 */
> +	for (test = cval->min; test < cval->max + res; test += res) {
> +		if (test > cval->max)
> +			test = cval->max;
>   
> -	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, channel, 0, test) ||
> -		    get_cur_mix_raw(cval, channel, &check) ||
> -		    check != saved) /* SET_CUR effective, non-sticky. */
> +		if (snd_usb_set_cur_mix_value(cval, channel, 0, test))
> +			return 0;
> +
> +		if (get_cur_mix_raw(cval, channel, &check))
> +			goto get_cur_broken;
> +		if (check != saved) /* SET_CUR effective, non-sticky. */
>   			return 0;
> +
> +		/*
> +		 * Leave some time for asynchronous mixers to change the value.
> +		 *
> +		 * Note that there is no need to wait between SET_CUR and
> +		 * GET_CUR, as we don't care whether the GET_CUR value matches
> +		 * the SET_CUR one. IOW, what we expect is just a GET_CUR value
> +		 * differing from the saved one.
> +		 *
> +		 * Mixers of most devices are synchronous. The should have
> +		 * returned early without extra sleep. Asynchronous mixers will
> +		 * return once the accumulated time is enough for them to change
> +		 * the value.
> +		 */
> +		msleep(10);
>   	}
>   
> +	/* Check again after the last msleep(). */
> +	if (get_cur_mix_raw(cval, channel, &check))
> +		goto get_cur_broken;
> +	if (check != saved)
> +		return 0;
> +
>   	if (cval->head.mixer->chip->quirk_flags & QUIRK_FLAG_MIXER_GET_CUR_BROKEN) {
> +get_cur_broken:
>   		usb_audio_info(cval->head.mixer->chip,
>   			       "%d:%d: broken mixer GET_CUR (%d/%d/%d => %d)\n",
>   			       cval->head.id, mixer_ctrl_intf(cval->head.mixer),
>
> ---
> base-commit: 3eb40771c00a8488fa6ed2cc1fe203477908bf38
> change-id: 74676fce-uac-precise-sticky-check-94474a22b57d
>
> Thanks,
> Rong
>

Re: [PATCH] ALSA: usb-audio: Check sticky mixers precisely

Posted by Rong Zhang 1 month, 1 week ago

Hi Alexander,

On Sun, 2026-08-16 at 07:14 +0200, Alexander Niemeyer wrote:
> Hi Rong,
> 
> I tested the sticky-check part of your patch on the Logitech PRO X 
> Wireless (046d:0aba) on Fedora 44, kernel 7.1.8-200.fc44.x86_64.
> 
> Since your patch is based on a newer tree, I used a minimal backport of 
> the new ~16-value / 10 ms sticky-check logic to the 7.1.8 code. The 
> GET_CUR-broken handling from the newer tree was not included; GET_CUR 
> itself succeeds on this device.
> 
> Unfortunately, the playback control is still classified as sticky:
> 
> 2:0: sticky mixer values (-16384/0/256 => -3840), disabling
> 
> I then instrumented the check and tried an additional diagnostic: after 
> every successful SET_CUR, wait 100 ms and perform another GET_CUR before 
> issuing the next SET_CUR.
> 
> For the playback volume, the saved value was -3840 and GET_CUR remained 
> at -3840 for every tested value, even after 100 ms, for example:
> 
> test=-15104 immediate=-3840 after100ms=-3840
> test=-13824 immediate=-3840 after100ms=-3840
> test=-3584 immediate=-3840 after100ms=-3840
> test=-2304 immediate=-3840 after100ms=-3840
> test=-1024 immediate=-3840 after100ms=-3840
> test=0 immediate=-3840 after100ms=-3840
> 
> So in this case the issue does not appear to be simply that the 
> accumulated 10 ms sleeps are too short. During the probe-time sticky 
> check, SET_CUR succeeds but GET_CUR for the playback control remains 
> unchanged even when each SET_CUR is given 100 ms before the next one.
> 
> This differs from my previous direct libusb tests with the AudioControl 
> interface unbound, where valid SET_CUR values became visible through 
> GET_CUR after roughly 47–81 ms.

Really interesting. Maybe the mixer changes its value only when there is
an opened playback stream.

Could you clarify your "libusb tests"?

Thanks,
Rong

> 
> The first debug line I saw with |saved=0| was from the Mic Capture 
> Volume control; that control changed immediately and returned as 
> non-sticky. The sequence above with |saved=-3840| is the problematic PCM 
> Playback Volume control.
> 
> I'd be happy to test another version or run additional diagnostics if 
> useful.
> 
> Best regards,
> Alexander
> 
> 
> Am 15.08.2026 um 23:47 schrieb Rong Zhang:
> > Some mixers are asynchronous, and some have broken min/max. They are
> > mistakenly considered sticky due to how the check is implemented.
> > 
> > Check sticky mixers more precisely by checking approximately 16 values
> > and adding a msleep(10) between each check, so that asynchronous mixers
> > have enough time to change the value and mixers with broken min/max are
> > checked properly. Additionally, mark GET_CUR as broken when
> > get_cur_mix_raw() fails, instead of returning successfully.
> > 
> > Reported-by: Alexander Niemeyer <adventureFAN@gmx.de>
> > Closes: https://lore.kernel.org/r/6262cbbd-d1f2-4c9d-a1c7-9c5d12636f4b@gmx.de
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >   sound/usb/mixer.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
> >   1 file changed, 44 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 703c118f9d4e..3d0f97730a06 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -1256,22 +1256,59 @@ static void init_cur_mix_raw(struct usb_mixer_elem_info *cval, int ch, int idx)
> >   static int check_sticky_volume_control(struct usb_mixer_elem_info *cval,
> >   				       int channel, int saved)
> >   {
> > -	int sticky_test_values[] = { cval->min, cval->max };
> > -	int test, check, i;
> > +	int test, check, res;
> > +
> > +	/*
> > +	 * Check approximately 16 values (15 intervals).
> > +	 * If the resolution is not fine enough, check fewer values.
> > +	 */
> > +	res = DIV_ROUND_UP(cval->max - cval->min, 15);
> > +	res = res ? roundup(res, cval->res) : cval->res;
> > +
> > +	/*
> > +	 * If (cval->max - cval->min) is not a multiple of cval->res, we still
> > +	 * want to test cval->max anyway.
> > +	 */
> > +	for (test = cval->min; test < cval->max + res; test += res) {
> > +		if (test > cval->max)
> > +			test = cval->max;
> >   
> > -	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, channel, 0, test) ||
> > -		    get_cur_mix_raw(cval, channel, &check) ||
> > -		    check != saved) /* SET_CUR effective, non-sticky. */
> > +		if (snd_usb_set_cur_mix_value(cval, channel, 0, test))
> > +			return 0;
> > +
> > +		if (get_cur_mix_raw(cval, channel, &check))
> > +			goto get_cur_broken;
> > +		if (check != saved) /* SET_CUR effective, non-sticky. */
> >   			return 0;
> > +
> > +		/*
> > +		 * Leave some time for asynchronous mixers to change the value.
> > +		 *
> > +		 * Note that there is no need to wait between SET_CUR and
> > +		 * GET_CUR, as we don't care whether the GET_CUR value matches
> > +		 * the SET_CUR one. IOW, what we expect is just a GET_CUR value
> > +		 * differing from the saved one.
> > +		 *
> > +		 * Mixers of most devices are synchronous. The should have
> > +		 * returned early without extra sleep. Asynchronous mixers will
> > +		 * return once the accumulated time is enough for them to change
> > +		 * the value.
> > +		 */
> > +		msleep(10);
> >   	}
> >   
> > +	/* Check again after the last msleep(). */
> > +	if (get_cur_mix_raw(cval, channel, &check))
> > +		goto get_cur_broken;
> > +	if (check != saved)
> > +		return 0;
> > +
> >   	if (cval->head.mixer->chip->quirk_flags & QUIRK_FLAG_MIXER_GET_CUR_BROKEN) {
> > +get_cur_broken:
> >   		usb_audio_info(cval->head.mixer->chip,
> >   			       "%d:%d: broken mixer GET_CUR (%d/%d/%d => %d)\n",
> >   			       cval->head.id, mixer_ctrl_intf(cval->head.mixer),
> > 
> > ---
> > base-commit: 3eb40771c00a8488fa6ed2cc1fe203477908bf38
> > change-id: 74676fce-uac-precise-sticky-check-94474a22b57d
> > 
> > Thanks,
> > Rong
> >