Re: [PATCH] ALSA: usb-audio: Check sticky mixers precisely
Alexander Niemeyer <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.linux.sound |
|---|---|
| Message-ID | <[email protected]> |
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 <[email protected]> > Closes: https://lore.kernel.org/r/[email protected] > Signed-off-by: Rong Zhang <[email protected]> > --- > 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 >