Re: [PATCH] ASoC: qcom: audioreach: apply volume changes to a running graph

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: audioreach: unchecked error from
  audioreach_gain_set_vol_ctrl()
- [Medium] ASoC: qcom: audioreach: unlocked dw->power and gain update in
  mixer put
--

commit dc9672b8329eabe445c390fc81928bbbadffdb8a
Author: Marc Herrero <marcht@proton.me>

ASoC: qcom: audioreach: apply volume changes to a running graph

This commit sends mixer volume changes to the DSP immediately when the
DAPM widget is powered and returns 0 when the requested gain matches the
stored value.

> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index 612d8f03e6e98..5b3378fd29fce 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1323,8 +1323,19 @@ static int audioreach_put_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,
>  {
>  	struct snd_soc_dapm_widget *dw = snd_soc_dapm_kcontrol_to_widget(kcontrol);
>  	struct audioreach_module *mod = dw->dobj.private;
> +	long gain = ucontrol->value.integer.value[0];
>  
> -	mod->gain = ucontrol->value.integer.value[0];
> +	if (mod->gain == gain)
> +		return 0;
> +
> +	mod->gain = gain;
> +
> +	/* The PGA event only applies the gain when the widget powers up */
> +	if (dw->power) {

[Severity: Medium]
Can a volume update race with DAPM widget power-up without
snd_soc_dapm_mutex_lock(dw->dapm) and leave the DSP programmed with a
stale gain?

snd_ctl_elem_write() calls audioreach_put_vol_ctrl_audio_mixer() under
card->controls_rwsem, whereas DAPM power sequencing runs under
card->dapm_mutex.  audioreach_gain_set_vol_ctrl() writes the passed vol
argument into the packet before q6apm_send_cmd_sync() takes apm->lock:

CPU0                                  CPU1
dapm_seq_run_coalesced()
  dw->power = 1
  ...
  audioreach_pga_event()
    vol = mod->gain (old)
                                      audioreach_put_vol_ctrl_audio_mixer()
                                        mod->gain = gain (new)
                                        if (dw->power)
                                          audioreach_gain_set_vol_ctrl()
                                            q6apm_send_cmd_sync(new)
    audioreach_gain_set_vol_ctrl()
      q6apm_send_cmd_sync(old)  <-- overwrites DSP gain with old value

After this, the DSP holds the old gain while mod->gain holds the new
value, and retrying the same target volume returns 0 without updating the
DSP.  Taking snd_soc_dapm_mutex_lock(dw->dapm) around the mod->gain
update, dw->power check, and audioreach_gain_set_vol_ctrl() call would
serialize this with DAPM power sequencing.

> +		struct snd_soc_component *c = snd_soc_dapm_to_component(dw->dapm);
> +
> +		audioreach_gain_set_vol_ctrl(dev_get_drvdata(c->dev), mod, gain);
> +	}
>  
>  	return 1;

[Severity: Medium]
Should audioreach_put_vol_ctrl_audio_mixer() check and propagate errors
from audioreach_gain_set_vol_ctrl()?

audioreach_gain_set_vol_ctrl() can fail with -ENOMEM when allocating the
GPR command packet or with a negative errno from q6apm_send_cmd_sync().
Ignoring the failure reports success to userspace and leaves mod->gain set
to the new value while the DSP keeps the old gain, so later writes of the
same volume hit the mod->gain == gain check and return 0 without retrying.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-q6apm-vol-live-v1-1-be85eef649c1@proton.me?part=1