Re: [PATCH 2/2] ASoC: Intel: Skylake: Correct the handling of fmt_config flexible array

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

On 30/06/2022 09:33, Peter Ujfalusi wrote:
> The struct nhlt_format's fmt_config is a flexible array, it must not be
> used as normal array.
> When moving to the next nhlt_fmt_cfg we need to take into account the data
> behind the ->config.caps (indicated by ->config.size).
> 
> The logic of the code also changed: it is no longer saves the _last_
> fmt_cfg for all found rates.
> 
> At the same time prepare the code for converting the fmt_config to a u8
> flexible array to prevent further abuse.

I should have dropped this sentence, I don't know why it is here.
The hinted change is the only way to please sparse, but I'm not sure if
I will ever send it as a patch.

> Fixes: bc2bd45b1f7f3 ("ASoC: Intel: Skylake: Parse nhlt and register clock device")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-nhlt.c | 37 ++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
> index 366f7bd9bc02..deb7b820325e 100644
> --- a/sound/soc/intel/skylake/skl-nhlt.c
> +++ b/sound/soc/intel/skylake/skl-nhlt.c
> @@ -111,11 +111,12 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
>  	if (fmt->fmt_count == 0)
>  		return;
>  
> +	fmt_cfg = (struct nhlt_fmt_cfg *)fmt->fmt_config;
>  	for (i = 0; i < fmt->fmt_count; i++) {
> +		struct nhlt_fmt_cfg *saved_fmt_cfg = fmt_cfg;
>  		bool present = false;
>  
> -		fmt_cfg = &fmt->fmt_config[i];
> -		wav_fmt = &fmt_cfg->fmt_ext;
> +		wav_fmt = &saved_fmt_cfg->fmt_ext;
>  
>  		channels = wav_fmt->fmt.channels;
>  		bps = wav_fmt->fmt.bits_per_sample;
> @@ -133,12 +134,18 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
>  		 * derive the rate.
>  		 */
>  		for (j = i; j < fmt->fmt_count; j++) {
> -			fmt_cfg = &fmt->fmt_config[j];
> -			wav_fmt = &fmt_cfg->fmt_ext;
> +			struct nhlt_fmt_cfg *tmp_fmt_cfg = fmt_cfg;
> +
> +			wav_fmt = &tmp_fmt_cfg->fmt_ext;
>  			if ((fs == wav_fmt->fmt.samples_per_sec) &&
> -			   (bps == wav_fmt->fmt.bits_per_sample))
> +			   (bps == wav_fmt->fmt.bits_per_sample)) {
>  				channels = max_t(u16, channels,
>  						wav_fmt->fmt.channels);
> +				saved_fmt_cfg = tmp_fmt_cfg;
> +			}
> +			/* Move to the next nhlt_fmt_cfg */
> +			tmp_fmt_cfg = (struct nhlt_fmt_cfg *)(tmp_fmt_cfg->config.caps +
> +							      tmp_fmt_cfg->config.size);
>  		}
>  
>  		rate = channels * bps * fs;
> @@ -154,8 +161,11 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
>  
>  		/* Fill rate and parent for sclk/sclkfs */
>  		if (!present) {
> +			struct nhlt_fmt_cfg *first_fmt_cfg;
> +
> +			first_fmt_cfg = (struct nhlt_fmt_cfg *)fmt->fmt_config;
>  			i2s_config_ext = (struct skl_i2s_config_blob_ext *)
> -						fmt->fmt_config[0].config.caps;
> +						first_fmt_cfg->config.caps;
>  
>  			/* MCLK Divider Source Select */
>  			if (is_legacy_blob(i2s_config_ext->hdr.sig)) {
> @@ -169,6 +179,9 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
>  
>  			parent = skl_get_parent_clk(clk_src);
>  
> +			/* Move to the next nhlt_fmt_cfg */
> +			fmt_cfg = (struct nhlt_fmt_cfg *)(fmt_cfg->config.caps +
> +							  fmt_cfg->config.size);
>  			/*
>  			 * Do not copy the config data if there is no parent
>  			 * clock available for this clock source select
> @@ -177,9 +190,9 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
>  				continue;
>  
>  			sclk[id].rate_cfg[rate_index].rate = rate;
> -			sclk[id].rate_cfg[rate_index].config = fmt_cfg;
> +			sclk[id].rate_cfg[rate_index].config = saved_fmt_cfg;
>  			sclkfs[id].rate_cfg[rate_index].rate = rate;
> -			sclkfs[id].rate_cfg[rate_index].config = fmt_cfg;
> +			sclkfs[id].rate_cfg[rate_index].config = saved_fmt_cfg;
>  			sclk[id].parent_name = parent->name;
>  			sclkfs[id].parent_name = parent->name;
>  
> @@ -193,13 +206,13 @@ static void skl_get_mclk(struct skl_dev *skl, struct skl_ssp_clk *mclk,
>  {
>  	struct skl_i2s_config_blob_ext *i2s_config_ext;
>  	struct skl_i2s_config_blob_legacy *i2s_config;
> -	struct nhlt_specific_cfg *fmt_cfg;
> +	struct nhlt_fmt_cfg *fmt_cfg;
>  	struct skl_clk_parent_src *parent;
>  	u32 clkdiv, div_ratio;
>  	u8 clk_src;
>  
> -	fmt_cfg = &fmt->fmt_config[0].config;
> -	i2s_config_ext = (struct skl_i2s_config_blob_ext *)fmt_cfg->caps;
> +	fmt_cfg = (struct nhlt_fmt_cfg *)fmt->fmt_config;
> +	i2s_config_ext = (struct skl_i2s_config_blob_ext *)fmt_cfg->config.caps;
>  
>  	/* MCLK Divider Source Select and divider */
>  	if (is_legacy_blob(i2s_config_ext->hdr.sig)) {
> @@ -228,7 +241,7 @@ static void skl_get_mclk(struct skl_dev *skl, struct skl_ssp_clk *mclk,
>  		return;
>  
>  	mclk[id].rate_cfg[0].rate = parent->rate/div_ratio;
> -	mclk[id].rate_cfg[0].config = &fmt->fmt_config[0];
> +	mclk[id].rate_cfg[0].config = fmt_cfg;
>  	mclk[id].parent_name = parent->name;
>  }
>  

-- 
Péter