[PATCH 0/2] ASoC: Intel: Skylake: Fixes for skl_get_ssp_clks()

[PATCH 0/2] ASoC: Intel: Skylake: Fixes for skl_get_ssp_clks()

[Peter Ujfalusi]

Hi,

while looking at long standing sparse reports regarding to arrays of flexible
structures (arrays of flexible arrays of flexible structures, really).

I came across these which did not looked right.
The skl_get_ssp_clks() as some issues and can work under only strict single
condition.

Regards,
Peter
---
Peter Ujfalusi (2):
  ASoC: Intel: Skylake: Correct the ssp rate discovery in
    skl_get_ssp_clks()
  ASoC: Intel: Skylake: Correct the handling of fmt_config flexible
    array

 sound/soc/intel/skylake/skl-nhlt.c | 40 ++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 13 deletions(-)

-- 
2.37.0

[PATCH 1/2] ASoC: Intel: Skylake: Correct the ssp rate discovery in skl_get_ssp_clks()

[Peter Ujfalusi]

The present flag is only set once when one rate has been found to be saved.
This will effectively going to ignore any rate discovered at later time and
based on the code, this is not the intention.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
index 2439a574ac2f..366f7bd9bc02 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -99,7 +99,6 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
 	struct nhlt_fmt_cfg *fmt_cfg;
 	struct wav_fmt_ext *wav_fmt;
 	unsigned long rate;
-	bool present = false;
 	int rate_index = 0;
 	u16 channels, bps;
 	u8 clk_src;
@@ -113,6 +112,8 @@ static void skl_get_ssp_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks,
 		return;
 
 	for (i = 0; i < fmt->fmt_count; i++) {
+		bool present = false;
+
 		fmt_cfg = &fmt->fmt_config[i];
 		wav_fmt = &fmt_cfg->fmt_ext;
 
-- 
2.37.0

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

[Peter Ujfalusi]

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.

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;
 }
 
-- 
2.37.0

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

[Péter Ujfalusi]

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