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 at 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