Re: [PATCH v3 2/7] soundwire: intel: Allow for dynamic assignment of PDIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] soundwire: intel: NULL deref in intel_hw_params() on
  dai_runtime->pdi
- [High] soundwire: cadence: sdw_cdns_assign_pdi() assigns Bulk PDIs 0
  and 1
- [Medium] soundwire: intel: sdw_intel_pdi_config() adds separate layer
  over DAI ops
- [Low] soundwire: intel: unused variables ch and dir in intel_prepare()
--

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

soundwire: intel: Allow for dynamic assignment of PDIs

This commit adds sdw_cdns_assign_pdi() and sdw_cdns_release_pdi() for
dynamic PDI allocation and exports sdw_intel_pdi_config() to configure
SHIM, ALH, and stream registers for a PDI.

> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index a0a63b76793e2..6c1ad11c6583a 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
[ ... ]
> +struct sdw_cdns_pdi *sdw_cdns_assign_pdi(struct sdw_bus *bus, u32 dir)
> +{
> +	struct sdw_cdns *cdns = bus_to_cdns(bus);
> +	struct sdw_cdns_streams *stream = &cdns->pcm;
> +	struct sdw_cdns_pdi *pdis, *pdi;
> +	size_t num_pdis;
> +	int i;
> +
> +	if (dir == SDW_DATA_DIR_RX) {
> +		num_pdis = stream->num_in;
> +		pdis = stream->in;
> +	} else {
> +		num_pdis = stream->num_out;
> +		pdis = stream->out;
> +	}
> +
> +	spin_lock(&cdns->pdi_lock);
> +	for (i = 0; i < num_pdis; i++) {
> +		if (!pdis[i].assigned) {
> +			pdi = &pdis[i];
> +			goto found;
> +		}
> +	}
> +
> +	for (i = 0; i < stream->num_bd; i++) {
> +		if (!stream->bd[i].assigned) {
> +			pdi = &stream->bd[i];
> +			goto found;
> +		}
> +	}

[Severity: High]
Does searching stream->bd from index 0 hand out PDI 0 and PDI 1 to PCM
streams?

In sdw_cdns_pdi_init(), stream->bd is allocated starting at PDI 0, and
the first two entries (pdi->num 0 and 1) are reserved for Bulk transfers
on DataPort 0.  On Intel controllers where all PDIs are bidirectional
(num_in == 0 and num_out == 0), the first two sdw_cdns_assign_pdi() calls
return PDI 0 and PDI 1.  When sdw_intel_pdi_config() then calls
sdw_cdns_config_stream(), TX streams on PDI 0 or 1 set
CDNS_PORTCTRL_BULK_ENABLE, PDI 1 is mapped onto DataPort 0 alongside
PDI 0, and intel_pdi_shim_configure() / intel_pdi_alh_configure() omit
the +2 PCM ALH stream offset applied when pdi->num >= 2.

Should the stream->bd loop start at i = 2 to skip the two Bulk PDIs?

[ ... ]

> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index af65214836b4d..0baa032fb4e8e 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -674,6 +674,22 @@ intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
>  	intel_writel(alh, SDW_ALH_STRMZCFG(pdi->intel_alh_id), conf);
>  }
>  
> +/**
> + * sdw_intel_pdi_config - Configure specified PDI
> + * @bus: SoundWire bus instance
> + * @pdi: PDI instance to configure
> + */
> +void sdw_intel_pdi_config(struct sdw_bus *bus, struct sdw_cdns_pdi *pdi)
> +{
> +	struct sdw_cdns *cdns = bus_to_cdns(bus);
> +	struct sdw_intel *sdw = cdns_to_intel(cdns);
> +
> +	intel_pdi_shim_configure(sdw, pdi);
> +	intel_pdi_alh_configure(sdw, pdi);
> +	sdw_cdns_config_stream(cdns, pdi->ch_count, pdi->dir, pdi);
> +}
> +EXPORT_SYMBOL(sdw_intel_pdi_config);

[Severity: Medium]
As reported by Vinod Koul on v1 [1], why add a separate layer exporting
sdw_intel_pdi_config() and sdw_cdns_assign_pdi() / sdw_cdns_release_pdi()
for driver-specific DAI ops instead of updating the SoundWire Intel DAI
driver to handle both startup and hw_params uniformly across Intel
drivers?

> @@ -745,9 +761,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  
>  	/* do run-time configurations for SHIM, ALH and PDI/PORT */
> -	intel_pdi_shim_configure(sdw, pdi);
> -	intel_pdi_alh_configure(sdw, pdi);
> -	sdw_cdns_config_stream(cdns, ch, dir, pdi);
> +	sdw_intel_pdi_config(&cdns->bus, dai_runtime->pdi);

[Severity: High]
Does passing dai_runtime->pdi here cause a NULL pointer dereference?

dai_runtime->pdi is NULL until "dai_runtime->pdi = pdi;" a few lines
below, and sdw_intel_pdi_config() -> intel_pdi_shim_configure()
immediately dereferences pdi.  Passing pdi (or assigning
dai_runtime->pdi before calling sdw_intel_pdi_config()) avoids the crash.

> @@ -823,9 +837,7 @@ static int intel_prepare(struct snd_pcm_substream *substream,
>  		else
>  			dir = SDW_DATA_DIR_TX;
>  
> -		intel_pdi_shim_configure(sdw, dai_runtime->pdi);
> -		intel_pdi_alh_configure(sdw, dai_runtime->pdi);
> -		sdw_cdns_config_stream(cdns, ch, dir, dai_runtime->pdi);
> +		sdw_intel_pdi_config(&cdns->bus, dai_runtime->pdi);

[Severity: Low]
After replacing sdw_cdns_config_stream() with sdw_intel_pdi_config(), the
local variables ch and dir in intel_prepare() are set but never used,
triggering -Wunused-but-set-variable warnings.

[1] https://lore.kernel.org/r/aulzN9MCl5CJB1Al@parshuram

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