A portion of USB Headsets loses previous sound volume setting after a suspend resume

A portion of USB Headsets loses previous sound volume setting after a suspend resume

[En-Shuo Hsu]

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.

Re: A portion of USB Headsets loses previous sound volume setting after a suspend resume

[Takashi Iwai]

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

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;
 	}

Re: A portion of USB Headsets loses previous sound volume setting after a suspend resume

[Yu-Hsuan Hsu]

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

Re: A portion of USB Headsets loses previous sound volume setting after a suspend resume

[Takashi Iwai]

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