Re: [PATCH v3 7/7] ASoC: Intel: avs: SoundWire PCM BE operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ASoC: Intel: avs: broken sdw_suspended state and m_rt lifecycle
  in pcm_sdw.c
- [High] ASoC: Intel: avs: stream left in SDW_STREAM_DEPREPARED on
  resume in pcm_sdw.c
- [High] ASoC: Intel: avs: bulk PDIs 0 and 1 assigned to PCM streams in
  avs_sdw_dai_startup()
--

commit 18beef33b6f436db2bab29aa861b96311ce4d2f9
Author: Cezary Rojewski <cezary.rojewski@intel.com>

ASoC: Intel: avs: SoundWire PCM BE operations

This commit adds back-end DAI operations and component registration for
SoundWire PCM streams in the Intel AVS driver.

> diff --git a/sound/soc/intel/avs/pcm_sdw.c b/sound/soc/intel/avs/pcm_sdw.c
> new file mode 100644
> index 0000000000000..578162e9fc4dc
> --- /dev/null
> +++ b/sound/soc/intel/avs/pcm_sdw.c
> @@ -0,0 +1,291 @@

[ ... ]

> +static int avs_sdw_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct sdw_stream_runtime *sdw_stream;
> +	struct snd_soc_acpi_mach *mach;
> +	struct snd_soc_dai *codec_dai;
> +	struct avs_dma_data *data;
> +	struct sdw_cdns_pdi *pdi;
> +	struct sdw_slave *codec;
> +	int dir, ret;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		dir = SDW_DATA_DIR_RX;
> +	else
> +		dir = SDW_DATA_DIR_TX;
> +
> +	ret = avs_dai_startup(substream, dai);
> +	if (ret)
> +		return ret;
> +
> +	sdw_stream = sdw_alloc_stream(substream->name, SDW_STREAM_PCM);
> +	if (!sdw_stream) {
> +		ret = -ENOMEM;
> +		goto err_alloc_stream;
> +	}
> +
> +	pdi = sdw_cdns_assign_pdi(codec->bus, dir);
> +	if (!pdi) {
> +		ret = -EBUSY;
> +		goto err_assign_pdi;
> +	}

[Severity: High]
Does sdw_cdns_assign_pdi() assign bulk PDIs 0 and 1 to the first two PCM
substreams on a link?

On Intel cAVS SoundWire controllers, SDW_SHIM_PCMSCAP reports bidirectional
streams in pcm_bd with pcm_in and pcm_out set to 0, where PDI 0 and PDI 1
are reserved for 1-channel bulk transfers and PCM PDIs start at index 2.
Because sdw_cdns_assign_pdi() searches stream->bd starting from index 0,
passing the resulting pdi to sdw_intel_pdi_config() and
sdw_stream_add_master() in avs_sdw_dai_hw_params() enables
CDNS_PORTCTRL_BULK_ENABLE on TX, maps PDI 1 to DataPort 0 in
CDNS_PDI_CONFIG(1) while configuring DataPort 1 in pconfig.num, and
computes bulk ALH stream IDs instead of PCM ALH stream IDs.

> +	/* Fulfill the codec needs by setting the sdw_stream. */
> +	codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	snd_soc_dai_set_stream(codec_dai, sdw_stream, substream->stream);
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	data->sdw_stream = sdw_stream;
> +	data->pdi = pdi;
> +	return 0;
> +
> +err_assign_pdi:
> +	sdw_release_stream(sdw_stream);
> +err_alloc_stream:
> +	avs_dai_shutdown(substream, dai);
> +	return ret;
> +}
> +
> +static void avs_sdw_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_acpi_mach *mach;
> +	struct snd_soc_dai *codec_dai;
> +	struct avs_dma_data *data;
> +	struct sdw_slave *codec;
> +
> +	codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +
> +	snd_soc_dai_set_stream(codec_dai, NULL, substream->stream);
> +	sdw_cdns_release_pdi(codec->bus, data->pdi);
> +	sdw_release_stream(data->sdw_stream);
> +	avs_dai_shutdown(substream, dai);
> +}
> +
> +static int avs_sdw_dai_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *hw_params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct sdw_stream_config sconfig = {0};
> +	struct sdw_port_config pconfig = {0};
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_dma_data *data;
> +	struct sdw_cdns_pdi *pdi;
> +	struct sdw_slave *codec;
> +	int ret;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	if (data->path)
> +		return 0;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +	pdi = data->pdi;
> +	pdi->ch_count = params_channels(hw_params);
> +	pdi->h_ch_num = pdi->ch_count - 1;
> +	pdi->l_ch_num = 0;
> +	sdw_intel_pdi_config(codec->bus, pdi);
> +
> +	if (data->sdw_suspended)
> +		return avs_dai_be_hw_params(substream, hw_params, dai, pdi->intel_alh_id);
> +
> +	sconfig.direction = pdi->dir;
> +	sconfig.ch_count = pdi->ch_count;
> +	sconfig.frame_rate = params_rate(hw_params);
> +	sconfig.type = SDW_STREAM_PCM;
> +	sconfig.bps = snd_pcm_format_width(params_format(hw_params));
> +	pconfig.num = pdi->num;
> +	pconfig.ch_mask = (1 << pdi->ch_count) - 1;
> +
> +	ret = sdw_stream_add_master(codec->bus, &sconfig, &pconfig, 1, data->sdw_stream);
> +	if (ret) {
> +		sdw_stream_remove_master(codec->bus, data->sdw_stream);
> +		return ret;
> +	}
> +
> +	ret = avs_dai_be_hw_params(substream, hw_params, dai, pdi->intel_alh_id);
> +	if (ret)
> +		sdw_stream_remove_master(codec->bus, data->sdw_stream);
> +	return ret;
> +}
> +
> +static int avs_sdw_dai_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_dma_data *data;
> +	struct sdw_slave *codec;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	if (!data->path)
> +		return 0;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +
> +	avs_path_reset(data->path);
> +	avs_path_free(data->path);
> +	data->path = NULL;
> +	sdw_deprepare_stream(data->sdw_stream);
> +	if (!data->sdw_suspended)
> +		sdw_stream_remove_master(codec->bus, data->sdw_stream);
> +
> +	return 0;
> +}

[Severity: High]
When a suspended stream resumes, does leaving data->sdw_stream in
SDW_STREAM_DEPREPARED cause sdw_prepare_stream() to skip computing its port
and transport parameters?

During suspend, avs_sdw_dai_hw_free() calls sdw_deprepare_stream(), which
sets data->sdw_stream->state to SDW_STREAM_DEPREPARED and subtracts the
stream bandwidth from bus->params.bandwidth.  On resume,
avs_sdw_dai_hw_params() returns early when data->sdw_suspended is set
without restoring the state to SDW_STREAM_CONFIGURED, so
sdw_prepare_stream() runs with update_params set to true while
sdw_compute_port_params() ignores streams in SDW_STREAM_DEPREPARED:

drivers/soundwire/generic_bandwidth_allocation.c:sdw_compute_group_params() {
    ...
		if (m_rt->stream == stream) {
			/* Only runtime during prepare should be added */
			if (stream->state != SDW_STREAM_CONFIGURED)
				continue;
		} else {
			/* Include streams withPrepared or Enabled seamlessly */
			if (m_rt->stream->state != SDW_STREAM_PREPARED &&
			    m_rt->stream->state != SDW_STREAM_ENABLED)
				continue;
		}
    ...
}

> +
> +static int avs_sdw_dai_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct avs_dma_data *data;
> +	int ret;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +
> +	/*
> +	 * There is no guarantee trigger(RESUME) fires, so reset the flag here.
> +	 * Ensures there is no sdw_master leak should hw_free() occur without resuming.
> +	 */
> +	data->sdw_suspended = false;
> +
> +	ret = sdw_prepare_stream(data->sdw_stream);
> +	if (ret) {
> +		dev_err(dai->dev, "sdw stream prepare failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = avs_dai_prepare(substream, dai);
> +	if (ret)
> +		sdw_deprepare_stream(data->sdw_stream);
> +	return ret;
> +}
> +
> +static int avs_sdw_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct avs_dma_data *data;
> +	int ret = 0;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		if (rtd->dai_link->ignore_suspend)
> +			break;
> +		fallthrough;
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = sdw_enable_stream(data->sdw_stream);
> +		if (ret)
> +			break;
> +
> +		ret = avs_path_run(data->path, AVS_TPLG_TRIGGER_AUTO);
> +		if (ret)
> +			sdw_disable_stream(data->sdw_stream);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +		if (rtd->dai_link->ignore_suspend)
> +			break;
> +		data->sdw_suspended = true;
> +		fallthrough;

[Severity: High]
Can tracking suspend via data->sdw_suspended in avs_sdw_dai_trigger() break
resume for non-running streams and leave a freed data->sdw_stream linked on
codec->bus->m_rt_list if resume fails?

snd_pcm_do_suspend() only issues SNDRV_PCM_TRIGGER_SUSPEND when the
substream is running, so a configured substream in PAUSED, PREPARED, SETUP,
or XRUN state enters avs_component_suspend() with data->sdw_suspended still
false.  avs_sdw_dai_hw_free() then calls sdw_stream_remove_master(), which
frees the master and codec slave runtimes and sets data->sdw_stream->state
to SDW_STREAM_RELEASED, but avs_component_resume() only re-runs CPU DAI
hw_params and never calls the codec DAI's hw_params (sdw_stream_add_slave()),
causing sdw_prepare_stream() in avs_sdw_dai_prepare() to fail with -EINVAL
and disconnect the substream.

Conversely, when a running stream suspends (setting data->sdw_suspended =
true) and avs_component_resume_hw_params() fails before
avs_sdw_dai_prepare() clears data->sdw_suspended, closing the substream
skips sdw_stream_remove_master() in avs_sdw_dai_hw_free() (because
!data->path or data->sdw_suspended is true) and frees data->sdw_stream in
avs_sdw_dai_shutdown(), leaving m_rt on codec->bus->m_rt_list with a
dangling m_rt->stream pointer (use-after-free on subsequent SoundWire bus
operations).  The same dangling m_rt occurs if codec DAI hw_params fails
after sdw_stream_add_slave() allocates m_rt on codec->bus before
avs_sdw_dai_hw_params() sets data->path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106121557.2338556-1-cezary.rojewski@intel.com?part=7