[PATCH] ASoC: SOF: topology: Move a variable assignment behind condition checks in sof_dai_load()

[PATCH] ASoC: SOF: topology: Move a variable assignment behind condition checks in sof_dai_load()

[Markus Elfring]

Date: Thu, 13 Apr 2023 13:56:44 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “sof_dai_load”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the local variable “private” behind some condition checks.

This issue was detected by using the Coccinelle software.

Fixes: c5232c0171428f005a3204e1c264231fb5999b28 ("ASoC: SOF: topology: parse and store d0i3_compatible flag")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/sof/topology.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index d3d536b0a8f5..3fffe3826160 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -1680,7 +1680,7 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
 	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
 	const struct sof_ipc_pcm_ops *ipc_pcm_ops = sof_ipc_get_ops(sdev, pcm);
 	struct snd_soc_tplg_stream_caps *caps;
-	struct snd_soc_tplg_private *private = &pcm->priv;
+	struct snd_soc_tplg_private *private;
 	struct snd_sof_pcm *spcm;
 	int stream;
 	int ret;
@@ -1716,6 +1716,7 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
 	dai_drv->dobj.private = spcm;
 	list_add(&spcm->list, &sdev->pcm_list);

+	private = &pcm->priv;
 	ret = sof_parse_tokens(scomp, spcm, stream_tokens,
 			       ARRAY_SIZE(stream_tokens), private->array,
 			       le32_to_cpu(private->size));
--
2.40.0

[PATCH] ASoC: SOF: Intel: hda-stream: Move three variable assignments behind condition checks in hda_dsp_iccmax_stream_hw_params()

[Markus Elfring]

Date: Wed, 19 Apr 2023 20:42:19 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “hda_dsp_iccmax_stream_hw_params”.

Thus avoid the risk for undefined behaviour by moving the assignment
for three local variables behind some condition checks.

This issue was detected by using the Coccinelle software.

Fixes: 7d88b9608142f95ccdd3dfb190da4a5faddb1cc7 ("ASoC: SOF: Intel: hdac_ext_stream: consistent prefixes for variables/members")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/sof/intel/hda-stream.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 79d818e6a0fa..9c44127014fc 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -407,10 +407,10 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
 				    struct snd_dma_buffer *dmab,
 				    struct snd_pcm_hw_params *params)
 {
-	struct hdac_stream *hstream = &hext_stream->hstream;
-	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
+	struct hdac_stream *hstream;
+	int sd_offset;
 	int ret;
-	u32 mask = 0x1 << hstream->index;
+	u32 mask;

 	if (!hext_stream) {
 		dev_err(sdev->dev, "error: no stream available\n");
@@ -422,9 +422,12 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
 		return -ENODEV;
 	}

+	hstream = &hext_stream->hstream;
 	if (hstream->posbuf)
 		*hstream->posbuf = 0;

+	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
+
 	/* reset BDL address */
 	snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
 			  sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL,
@@ -459,6 +462,8 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
 				sd_offset + SOF_HDA_ADSP_REG_SD_LVI,
 				0xffff, (hstream->frags - 1));

+	mask = 0x1 << hstream->index;
+
 	/* decouple host and link DMA, enable DSP features */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
 				mask, mask);
--
2.40.0

Re: [PATCH] ASoC: SOF: Intel: hda-stream: Move three variable assignments behind condition checks in hda_dsp_iccmax_stream_hw_params()

[Pierre-Louis Bossart]

On 4/19/23 13:54, Markus Elfring wrote:
> Date: Wed, 19 Apr 2023 20:42:19 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “hda_dsp_iccmax_stream_hw_params”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for three local variables behind some condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 7d88b9608142f95ccdd3dfb190da4a5faddb1cc7 ("ASoC: SOF: Intel: hdac_ext_stream: consistent prefixes for variables/members")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Yes indeed, for some reason this was fixed in
hda_dsp_stream_hw_params() but not in the
hda_dsp_iccmax_stream_hw_params() variant.

Could we however use the same code as in hda_dsp_stream_hw_params() for
consistency?

	if (!hext_stream) {
		dev_err(sdev->dev, "error: no stream available\n");
		return -ENODEV;
	}

	if (!dmab) {
		dev_err(sdev->dev, "error: no dma buffer allocated!\n");
		return -ENODEV;
	}

	hstream = &hext_stream->hstream;
	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
	mask = BIT(hstream->index);

Thanks!

> ---
>  sound/soc/sof/intel/hda-stream.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index 79d818e6a0fa..9c44127014fc 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -407,10 +407,10 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  				    struct snd_dma_buffer *dmab,
>  				    struct snd_pcm_hw_params *params)
>  {
> -	struct hdac_stream *hstream = &hext_stream->hstream;
> -	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
> +	struct hdac_stream *hstream;
> +	int sd_offset;
>  	int ret;
> -	u32 mask = 0x1 << hstream->index;
> +	u32 mask;
> 
>  	if (!hext_stream) {
>  		dev_err(sdev->dev, "error: no stream available\n");
> @@ -422,9 +422,12 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  		return -ENODEV;
>  	}
> 
> +	hstream = &hext_stream->hstream;
>  	if (hstream->posbuf)
>  		*hstream->posbuf = 0;
> 
> +	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
> +
>  	/* reset BDL address */
>  	snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
>  			  sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL,
> @@ -459,6 +462,8 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  				sd_offset + SOF_HDA_ADSP_REG_SD_LVI,
>  				0xffff, (hstream->frags - 1));
> 
> +	mask = 0x1 << hstream->index;
> +
>  	/* decouple host and link DMA, enable DSP features */
>  	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
>  				mask, mask);
> --
> 2.40.0
> 

Re: [PATCH] ASoC: SOF: Intel: hda-stream: Move three variable assignments behind condition checks in hda_dsp_iccmax_stream_hw_params()

[Markus Elfring]

>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “hda_dsp_iccmax_stream_hw_params”.
>>
>> Thus avoid the risk for undefined behaviour by moving the assignment
>> for three local variables behind some condition checks.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 7d88b9608142f95ccdd3dfb190da4a5faddb1cc7 ("ASoC: SOF: Intel: hdac_ext_stream: consistent prefixes for variables/members")
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> Yes indeed, for some reason this was fixed in
> hda_dsp_stream_hw_params() but not in the
> hda_dsp_iccmax_stream_hw_params() variant.

Would Peter Ujfalusi like to support similar source code adjustments
also according to his commit 09255c7ed8ca1f1ed99357b845d2f63fe2ef3e1e
("ASoC: SOF: Intel: hda-stream: Do not dereference hstream until it is safe")
from 2023-04-04?


> Could we however use the same code as in hda_dsp_stream_hw_params() for consistency?
…
> 	hstream = &hext_stream->hstream;
> 	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
> 	mask = BIT(hstream->index);

Can it matter to move such assignment statements a bit closer to subsequent statements?

Regards,
Markus