Re: [PATCH RFC] ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ALSA: hda/tas2781: partial reload in tas2781_hda_reload_if_lost() wipes config
- [Medium] ALSA: hda/tas2781: missing init_profile_id and double load in reload_if_lost()
- [Medium] ALSA: hda/tas2781: missing fw_state check in tas2781_hda_reload_if_lost()
--

commit 74dce121e5a4c4de7e8754632f2407d57f7750d6
Author: Roman Maidanovych <devuarpd@gmail.com>

ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

This patch checks bit 7 of B0_P0_R0x44 on TAS2781 amplifiers when a PCM stream
is opened and reloads the DSP program if the amplifier lost its program during
HDA codec runtime suspend.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221735

> diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
> index 44855f593daf8..177a2194aeb70 100644
> --- a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
> +++ b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c

[ ... ]

> @@ -162,6 +165,38 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
>  	return ret;
>  }
>  
> +/*
> + * The amplifiers can lose their DSP program while the HDA codec is runtime
> + * suspended, without the chip being reset. The cached program/configuration
> + * then makes tasdevice_tuning_switch() skip the download and the speakers
> + * stay silent. Bit 7 of B0_P0_R0x44 is set in that state: invalidate the
> + * cache and download the program again.
> + */
> +static void tas2781_hda_reload_if_lost(struct tasdevice_priv *tas_priv)
> +{
> +	bool lost = false;
> +	int i, ret, val;
> +
> +	if (tas_priv->chip_id != TAS2781)
> +		return;

[Severity: Medium]
Should tas2781_hda_reload_if_lost() also check that tas_priv->fw_state is
TASDEVICE_DSP_FW_ALL_OK before checking TAS2781_REG_DSP_LOST and calling
tasdevice_prmg_load()?

In tas2781_hda_bind(), tascodec_init() schedules asynchronous firmware loading
via request_firmware_nowait() and registers comp->playback_hook while
tas_priv->fw_state is still TASDEVICE_DSP_FW_PENDING and tas_priv->fmw is NULL.
Similarly, if tasdevice_dsp_parser() fails in tasdevice_dspfw_init(),
tas_priv->fw_state is set to TASDEVICE_DSP_FW_FAIL without freeing a partially
populated tas_priv->fmw:

sound/hda/codecs/side-codecs/tas2781_hda_i2c.c:tasdevice_dspfw_init() {
	...
	ret = tasdevice_dsp_parser(tas_priv);
	if (ret) {
		dev_err(tas_priv->dev, "dspfw load %s error\n",
			tas_priv->coef_binaryname);
		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
		return;
	}
}

Previously, tas2781_hda_playback_hook() only called tasdevice_tuning_switch(),
which returns early unless fw_state is TASDEVICE_RCA_FW_OK or
TASDEVICE_DSP_FW_ALL_OK. Without checking fw_state == TASDEVICE_DSP_FW_ALL_OK
here, opening a PCM stream before firmware loading finishes, in RCA-only mode,
or after a DSP firmware parse error can read TAS2781_DSP_LOST as set on the
unprogrammed amplifier and call tasdevice_prmg_load() with NULL or partially
parsed firmware.

> +
> +	for (i = 0; i < tas_priv->ndev; i++) {
> +		ret = tasdevice_dev_read(tas_priv, i, TAS2781_REG_DSP_LOST, &val);
> +		if (ret || !(val & TAS2781_DSP_LOST))
> +			continue;
> +
> +		tas_priv->tasdevice[i].cur_book = -1;
> +		tas_priv->tasdevice[i].cur_prog = -1;
> +		tas_priv->tasdevice[i].cur_conf = -1;
> +		lost = true;
> +	}

[Severity: Medium]
What happens if only a subset of channels have TAS2781_DSP_LOST set (or if
tasdevice_dev_read() fails transiently on one channel while succeeding on
another)?

Earlier calls to tasdevice_select_cfg_blk() leave
tas_priv->tasdevice[chn].is_loading set to true on all channels:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_select_cfg_blk() {
	...
			for (; chn < chnend; chn++)
				tas_priv->tasdevice[chn].is_loading = true;
	...
}

When tas2781_hda_reload_if_lost() then calls tasdevice_prmg_load(), the loop
only updates channels where cur_prog != prm_no and does not clear is_loading
to false on the remaining channels:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_prmg_load() {
	...
	for (i = 0, prog_status = 0; i < tas_priv->ndev; i++) {
		if (prm_no >= 0 && tas_priv->tasdevice[i].cur_prog != prm_no) {
			tas_priv->tasdevice[i].cur_conf = -1;
			tas_priv->tasdevice[i].is_loading = true;
			prog_status++;
		}
	}
}

Because is_loading is still true on the non-lost channel,
tasdevice_load_data(tas_priv, &(program->dev_data)) downloads the DSP program
(including its software reset commands) to that channel as well, wiping its
hardware state. However, because cur_conf was never reset to -1 on that
channel, the subsequent tasdevice_tuning_switch() ->
tasdevice_select_tuningprm_cfg() call sees cur_conf == cfg_no and skips
reloading the configuration and calibration data on that channel, leaving it
unconfigured.

> +
> +	if (lost) {
> +		dev_dbg(tas_priv->dev, "DSP program lost, reloading\n");
> +		tasdevice_prmg_load(tas_priv, tas_priv->cur_prog);
> +	}
> +}

[Severity: Medium]
Does reloading the program here miss applying tas_priv->rcabin.init_profile_id,
and can it cause a double program download when force_fwload_status is enabled?

In both tasdevice_dspfw_init() and tas2781_system_resume(), calling
tasdevice_prmg_load() is immediately followed by applying init_profile_id
before tasdevice_tuning_switch():

sound/hda/codecs/side-codecs/tas2781_hda_i2c.c:tas2781_system_resume() {
	...
	tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog);

	/* Init common setting for different audio profiles */
	if (tas_hda->priv->rcabin.init_profile_id >= 0)
		tasdevice_select_cfg_blk(tas_hda->priv,
			tas_hda->priv->rcabin.init_profile_id,
			TASDEVICE_BIN_BLK_PRE_POWER_UP);
	...
}

Without that step in tas2781_hda_reload_if_lost(), common profile settings
from init_profile_id are not restored after tasdevice_prmg_load() reloads the
DSP program.

In addition, tasdevice_tuning_switch(tas_hda->priv, 0, false) runs immediately
after tas2781_hda_reload_if_lost() in tas2781_hda_playback_hook() and calls
tasdevice_select_tuningprm_cfg(), which reloads the DSP program whenever
cur_prog != prm_no || tas_priv->force_fwload_status:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_select_tuningprm_cfg() {
	...
			if (prm_no >= 0
				&& (tas_priv->tasdevice[i].cur_prog != prm_no
				|| tas_priv->force_fwload_status)) {
				tas_priv->tasdevice[i].cur_conf = -1;
				tas_priv->tasdevice[i].is_loading = true;
				prog_status++;
			}
	...
}

If the "Speaker Force Firmware Load" control (force_fwload_status) is enabled,
calling tasdevice_prmg_load() in tas2781_hda_reload_if_lost() and then calling
tasdevice_tuning_switch() will download the full DSP program twice in a row on
PCM open.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-tas2781-reload-if-lost-v1-1-9d336120f1ba@gmail.com?part=1