A portion of USB Headsets loses previous sound volume setting after a suspend resume
Hi
We recently found that some USB headsets may fall back to their full volume after a suspend and resume. We think the issue is caused by the logic of mixer_ctl_feature_put https://github.com/torvalds/linux/blob/a3fa7a101dcff93791d1b1bdb3affcad1410c8c1/sound/usb/mixer.c#L1396 in sound/usb/mixer.c:
err = snd_usb_get_cur_mix_value(cval, c + 1, cnt, &oval); if (err < 0) return filter_error(cval, err); val = ucontrol->value.integer.value[cnt]; val = get_abs_value(cval, val); if (oval != val) { snd_usb_set_cur_mix_value(cval, c + 1, cnt, val); changed = 1; }
The existing codes get the existing mixer control value and ignore the set if the val doesn't change. However, in the suspend and resume case, the USB headset's control value is actually changed.
Removing the cache logic is a potential fix, but a better solution may be to properly handle the suspend resume scenario in snd_usb_get_cur_mix_value. We may need to mark the cache in usb_mixer_elem_info to be dirty.
The issue is verified to be reproduced with Dell WH3022 and Logitech USB Headset H340 by: 1. Boot to OS. 2. Plug in the headset and check sound output. 3. Play an audio/video and keep with low volume. 4. Suspend. 5. Resume. 6. When audio/video is played, the headset's sound output can't keep the original volume. --> issue "
Would like to know your thoughts on this issue.
On Thu, 09 Sep 2021 12:21:46 +0200, En-Shuo Hsu wrote:
Hi
We recently found that some USB headsets may fall back to their full volume after a suspend and resume. We think the issue is caused by the logic of mixer_ctl_feature_put in sound/usb/mixer.c:
err = snd_usb_get_cur_mix_value(cval, c + 1, cnt, &oval); if (err < 0) return filter_error(cval, err); val = ucontrol->value.integer.value[cnt]; val = get_abs_value(cval, val); if (oval != val) { snd_usb_set_cur_mix_value(cval, c + 1, cnt, val); changed = 1; }
The existing codes get the existing mixer control value and ignore the set if the val doesn't change. However, in the suspend and resume case, the USB headset's control value is actually changed.
Removing the cache logic is a potential fix, but a better solution may be to properly handle the suspend resume scenario in snd_usb_get_cur_mix_value. We may need to mark the cache in usb_mixer_elem_info to be dirty.
The issue is verified to be reproduced with Dell WH3022 and Logitech USB Headset H340 by:
- Boot to OS.
- Plug in the headset and check sound output.
- Play an audio/video and keep with low volume.
- Suspend.
- Resume.
- When audio/video is played, the headset's sound output can't keep the
original volume. --> issue "
Would like to know your thoughts on this issue.
USB-audio driver has an assumption that the normal resume code presumes the configuration while reset_resume needs the full initialization, but this looks too naive, then.
Could you check the following works?
Takashi
--- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -1080,7 +1080,7 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume) * we just notify and restart the mixers */ list_for_each_entry(mixer, &chip->mixer_list, list) { - err = snd_usb_mixer_resume(mixer, reset_resume); + err = snd_usb_mixer_resume(mixer, true /*reset_resume*/); if (err < 0) goto err_out; }
Takashi Iwai tiwai@suse.de 於 2021年9月9日 週四 下午9:30寫道：
On Thu, 09 Sep 2021 12:21:46 +0200, En-Shuo Hsu wrote:
Hi
We recently found that some USB headsets may fall back to their full volume after a suspend and resume. We think the issue is caused by the logic of mixer_ctl_feature_put in sound/usb/mixer.c:
err = snd_usb_get_cur_mix_value(cval, c + 1, cnt, &oval); if (err < 0) return filter_error(cval, err); val = ucontrol->value.integer.value[cnt]; val = get_abs_value(cval, val); if (oval != val) { snd_usb_set_cur_mix_value(cval, c + 1, cnt, val); changed = 1; }
The existing codes get the existing mixer control value and ignore the set if the val doesn't change. However, in the suspend and resume case, the USB headset's control value is actually changed.
Removing the cache logic is a potential fix, but a better solution may be to properly handle the suspend resume scenario in snd_usb_get_cur_mix_value. We may need to mark the cache in usb_mixer_elem_info to be dirty.
The issue is verified to be reproduced with Dell WH3022 and Logitech USB Headset H340 by:
- Boot to OS.
- Plug in the headset and check sound output.
- Play an audio/video and keep with low volume.
- Suspend.
- Resume.
- When audio/video is played, the headset's sound output can't keep the
original volume. --> issue "
Would like to know your thoughts on this issue.
USB-audio driver has an assumption that the normal resume code presumes the configuration while reset_resume needs the full initialization, but this looks too naive, then.
Could you check the following works?
Takashi
--- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -1080,7 +1080,7 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume) * we just notify and restart the mixers */ list_for_each_entry(mixer, &chip->mixer_list, list) {
err = snd_usb_mixer_resume(mixer, reset_resume);
err = snd_usb_mixer_resume(mixer, true /*reset_resume*/); if (err < 0) goto err_out; }
Thanks! It works. Is it an appropriate fix to set the reset_resume to true in usb_audio_resume? If it's acceptable, we can send the patch.
Yu-Hsuan
On Fri, 10 Sep 2021 11:25:37 +0200, Yu-Hsuan Hsu wrote:
Takashi Iwai tiwai@suse.de 於 2021年9月9日 週四 下午9:30寫道：
On Thu, 09 Sep 2021 12:21:46 +0200, En-Shuo Hsu wrote:
Hi
We recently found that some USB headsets may fall back to their full volume after a suspend and resume. We think the issue is caused by the logic of mixer_ctl_feature_put in sound/usb/mixer.c:
err = snd_usb_get_cur_mix_value(cval, c + 1, cnt, &oval); if (err < 0) return filter_error(cval, err); val = ucontrol->value.integer.value[cnt]; val = get_abs_value(cval, val); if (oval != val) { snd_usb_set_cur_mix_value(cval, c + 1, cnt, val); changed = 1; }
The existing codes get the existing mixer control value and ignore the set if the val doesn't change. However, in the suspend and resume case, the USB headset's control value is actually changed.
Removing the cache logic is a potential fix, but a better solution may be to properly handle the suspend resume scenario in snd_usb_get_cur_mix_value. We may need to mark the cache in usb_mixer_elem_info to be dirty.
The issue is verified to be reproduced with Dell WH3022 and Logitech USB Headset H340 by:
- Boot to OS.
- Plug in the headset and check sound output.
- Play an audio/video and keep with low volume.
- Suspend.
- Resume.
- When audio/video is played, the headset's sound output can't keep the
original volume. --> issue "
Would like to know your thoughts on this issue.
USB-audio driver has an assumption that the normal resume code presumes the configuration while reset_resume needs the full initialization, but this looks too naive, then.
Could you check the following works?
Takashi
--- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -1080,7 +1080,7 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume) * we just notify and restart the mixers */ list_for_each_entry(mixer, &chip->mixer_list, list) {
err = snd_usb_mixer_resume(mixer, reset_resume);
err = snd_usb_mixer_resume(mixer, true /*reset_resume*/); if (err < 0) goto err_out; }Thanks! It works. Is it an appropriate fix to set the reset_resume to true in usb_audio_resume? If it's acceptable, we can send the patch.
Well, if the above works, basically we can get rid of the whole difference of both resume procedures. It'd be a good cleanup, too. I already have a patch, and will submit soon later.
thanks,
Takashi
participants (3)
-
En-Shuo Hsu -
Takashi Iwai -
Yu-Hsuan Hsu