Re: [PATCH 3/7] soundwire: intel: Split PDIs and DAIs initialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] soundwire: intel: sdw_intel_init_pdis() breaks PDI init on ACE2.x
- [Medium] soundwire: intel: missing link power down on error in intel_link_startup()
--

commit 8870f97b0ab1ec823a91e4c81168130f5f4ee09d
Author: Cezary Rojewski <cezary.rojewski@intel.com>

soundwire: intel: Split PDIs and DAIs initialization

This commit splits PDI initialization out of intel_register_dai() in
drivers/soundwire/intel.c into a new sdw_intel_init_pdis() helper and calls
it from intel_link_startup() so that the register_dai() callback can be
optional.

> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index d53fa3d299bc7..708503373183f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1027,22 +1027,28 @@ static int intel_create_dai(struct sdw_cdns *cdns,
>  	return 0;
>  }
>  
> -static int intel_register_dai(struct sdw_intel *sdw)
> +int sdw_intel_init_pdis(struct sdw_intel *sdw)
>  {
> -	struct sdw_cdns_dai_runtime **dai_runtime_array;
>  	struct sdw_cdns_stream_config config;
> -	struct sdw_cdns *cdns = &sdw->cdns;
> -	struct sdw_cdns_streams *stream;
> -	struct snd_soc_dai_driver *dais;
> -	int num_dai, ret, off = 0;
> +	int ret;
>  
>  	/* Read the PDI config and initialize cadence PDI */
>  	intel_pdi_init(sdw, &config);
> -	ret = sdw_cdns_pdi_init(cdns, config);
> +	ret = sdw_cdns_pdi_init(&sdw->cdns, config);
>  	if (ret)
>  		return ret;
>  
>  	intel_pdi_stream_ch_update(sdw, &sdw->cdns.pcm);
> +	return 0;
> +}

[Severity: High]
Does calling sdw_intel_init_pdis() unconditionally from intel_link_startup()
break PDI initialization on ACE2.x+ platforms?

intel_link_startup() in drivers/soundwire/intel_auxdevice.c is shared between
cAVS/ACE1.x (sdw_intel_cnl_hw_ops in drivers/soundwire/intel.c) and ACE2.x+
(sdw_intel_lnl_hw_ops in drivers/soundwire/intel_ace2x.c).

However, sdw_intel_init_pdis() is implemented in drivers/soundwire/intel.c
and calls its file-local intel_pdi_init() and intel_pdi_stream_ch_update()
helpers, which access the cAVS/ACE1.x SHIM layout at SDW_SHIM_PCMSCAP(link_id)
and SDW_SHIM_PCMSYCHC(link_id, pdi_num). On ACE2.x+, sdw->link_res->shim
points to the per-link SDW_SHIM2_GENERIC_BASE(link_id), where the PDI
registers are instead at SDW_SHIM2_PCMSCAP and SDW_SHIM2_PCMSYCHC(pdi_num).

Meanwhile, intel_register_dai() in drivers/soundwire/intel_ace2x.c still has
its own PDI initialization:

drivers/soundwire/intel_ace2x.c:intel_register_dai() {
	...
	/* Read the PDI config and initialize cadence PDI */
	intel_pdi_init(sdw, &config);
	ret = sdw_cdns_pdi_init(cdns, config);
	if (ret)
		return ret;

	intel_pdi_stream_ch_update(sdw, &sdw->cdns.pcm);
	...
}

On ACE2.x+, intel_link_startup() will first run the cAVS/ACE1.x
sdw_intel_init_pdis() against the SHIM2 MMIO space, reading bogus PDI counts
and allocating cdns->pcm.{bd,in,out}, and then sdw_intel_register_dai() will
call sdw_cdns_pdi_init() a second time (leaking the first devm_kcalloc() PDI
allocations and leaving stale pointers in cdns->pcm.in or cdns->pcm.out if a
stream count was non-zero in the bogus read and zero in the SHIM2 read, since
cdns_allocate_pdi() returns early when num == 0). Furthermore, if the now
optional register_dai callback is omitted on ACE2.x+, the SHIM2 PDI
initialization will never run at all.

Should init_pdis be dispatched through struct sdw_intel_hw_ops and split out
of intel_register_dai() in drivers/soundwire/intel_ace2x.c as well?

[ ... ]

> diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
> index 5983fbc5725a2..114dadedfa14f 100644
> --- a/drivers/soundwire/intel_auxdevice.c
> +++ b/drivers/soundwire/intel_auxdevice.c
> @@ -423,6 +423,10 @@ int intel_link_startup(struct auxiliary_device *auxdev)
>  	if (ret)
>  		goto err_init;
>  
> +	ret = sdw_intel_init_pdis(sdw);
> +	if (ret)
> +		goto err_init;

[Severity: Medium]
If sdw_intel_init_pdis() fails here in intel_link_startup(), should this jump
to err_power_up instead of err_init?

At this point sdw_intel_link_power_up(sdw) has already succeeded:

drivers/soundwire/intel_auxdevice.c:intel_link_startup() {
	...
	/* Initialize shim, controller */
	ret = sdw_intel_link_power_up(sdw);
	if (ret)
		goto err_init;

	ret = sdw_intel_init_pdis(sdw);
	if (ret)
		goto err_init;
	...
err_power_up:
	sdw_intel_link_power_down(sdw);
err_init:
	return ret;
}

Jumping to err_init skips sdw_intel_link_power_down(sdw), leaving the link
powered up, sdw->cdns.link_up set to true, and BIT(link_id) set in
*sdw->link_res->shim_mask while sdw->startup_done remains false.

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