Re: [PATCH] ALSA: usb-audio: Check sticky mixers precisely
Alexander Niemeyer <[email protected]>
| Newsgroups | gmane.linux.sound,gmane.linux.kernel |
|---|---|
| Message-ID | <[email protected]> |
Hi Rong, I think we found the reason for the different behavior. I reproduced the snd-usb-audio initialization sequence step by step with direct libusb UAC1 control transfers and isolated the problem to SET_RES on the *Mic Capture Volume control (Feature Unit 3)*. A fresh-device control test looks like this: Mic GET_RES = 256 no SET_RES Playback: GET_CUR = -3840 (-15 dB) SET_CUR = -2048 (-8 dB) GET_CUR changes to -2048 after 68.0 ms Result: PASS After another power cycle, I repeated the same test but issued just *one* SET_RES request to the Mic Feature Unit first: Mic GET_RES before = 256 Mic SET_RES(128) = success Mic GET_RES after = 256 Playback: GET_CUR = -3840 (-15 dB) SET_CUR = -2048 (-8 dB) GET_CUR remains -3840 for more than 1200 ms Result: FAIL So a single successful |SET_RES(128)| on Feature Unit 3 is sufficient to make subsequent |SET_CUR| requests to the Playback Volume control on Feature Unit 2 ineffective. I also tested the complete Mic SET_RES sequence used by snd-usb-audio: SET_RES 128 SET_RES 64 SET_RES 32 SET_RES 16 SET_RES 8 SET_RES 4 SET_RES 2 SET_RES 1 All requests return success, while GET_RES remains 256. After that sequence, Playback SET_CUR also remains ineffective for more than 1200 ms. Interestingly, the Mic control itself still works after this. In an ALSA-like Mic probe I could successfully change Mic Volume from 0 dB to -64 dB and then +1 dB, with GET_CUR reflecting those changes essentially immediately (~0.3 ms). Playback remained broken afterwards. I also checked whether SET_RES on the Playback Feature Unit itself causes the problem. It does not: Playback GET_RES = 256 SET_RES 128 -> 64 -> 32 -> 16 -> 8 -> 4 -> 2 -> 1 GET_RES still = 256 Playback SET_CUR(-8 dB) GET_CUR changes successfully after 87.7 ms So the problematic operation appears specifically to be *SET_RES on the Mic Feature Unit affecting the Playback Feature Unit*. I also clarified the separate advertised-minimum issue: Playback SET_CUR(-64 dB): no change after >1200 ms followed by SET_CUR(-8 dB): works normally after 54.9 ms Playback SET_CUR(-63 dB): works after 75.8 ms followed by SET_CUR(-8 dB): works after 43.6 ms Therefore the broken -64 dB endpoint does not leave the device in the broken state; it is a separate issue. -63 dB works normally. I also captured usbmon/pcapng traces for both a working direct-libusb SET_CUR sequence and the failing snd-usb-audio initialization, so I can send those as well if they are useful. This also seems to explain why the sticky-check changes did not help: by the time snd-usb-audio reaches the Playback Volume sticky check, the earlier Mic SET_RES sanity test has already put the device into the state where Playback SET_CUR no longer takes effect. Let me know if you would like me to test a patch or capture any additional traces. Thanks, Alexander Am 16.08.2026 um 17:08 schrieb Rong Zhang: > Hi Alexander, > > On Sun, 2026-08-16 at 16:09 +0200, Alexander Niemeyer wrote: >> Hi Rong, >> >> Sure. The libusb tests were direct USB Audio Class 1 control transfers >> to the headset using libusb/PyUSB, not ALSA mixer operations. >> >> I accessed Feature Unit 2 on AudioControl interface 0, master channel 0, >> with the UAC1 Volume control selector: >> >> wValue = 0x0200 /* Volume control, master channel */ >> wIndex = 0x0200 /* Feature Unit 2, interface 0 */ >> >> I used the standard class-specific requests directly, including GET_CUR, >> GET_MIN, GET_MAX, GET_RES and SET_CUR, with signed 16-bit little-endian >> volume values in 1/256 dB units. >> >> The device reported: >> >> GET_CUR: 0 ( 0 dB in that test) >> GET_MIN: -16384 (-64 dB) >> GET_MAX: 0 ( 0 dB) >> GET_RES: 256 ( 1 dB) >> >> For the timing tests I issued SET_CUR for a target value and then >> repeatedly queried GET_CUR until the value changed or the timeout expired. >> >> Valid values became visible after roughly: >> >> -1 dB ~81 ms >> -2 dB ~52 ms >> -4 dB ~47 ms >> -8 dB ~47 ms >> -16 dB ~52 ms >> -32 dB ~47 ms >> >> The advertised -64 dB minimum behaved differently: SET_CUR returned >> successfully, but GET_CUR did not change even after 1000 ms. >> >> To access the AudioControl interface with libusb, I unbound the >> AudioControl interface from snd-usb-audio for the duration of the test. >> >> I did not intentionally open a playback stream during those libusb >> tests. Because the AudioControl interface had been unbound from >> snd-usb-audio, I also do not believe there was an active ALSA playback >> stream at that point. > Thanks for the information. > > Unfortunately, I still don't exactly see why the device behaved > differently when GET_CUR/SET_CUR requests were sent from snd-usb-audio > compared to your libusb tests. > > snd-usb-audio also tries SET_RES to test the sanity of GET_RES. Could you > test if it breaks your device's GET_CUR? > > Maybe comparing them with usbmon can show some clues. You can use > Wireshark to sniff /dev/usbmon*. > > Hint: a Thunderbolt port usually corresponds to a dedicated USB root hub. > If you have one, plug the device to it to get pure usbmon trace results > with no noisy URBs from other devices. > > Thanks, > Rong > >> If the open-stream state is important, I can repeat the experiment >> specifically controlling for playback-stream-open versus >> playback-stream-closed. >> >> Thanks, >> Alexander >> >> Am 16.08.2026 um 15:50 schrieb Rong Zhang: >>> 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<[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 >>>>>