[PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

As of commit cb18cd26039f ("ASoC: soc-core: do rtd->id trick at
snd_soc_add_pcm_runtime()") the ID stored in the PCM runtime data can
no longer be safely used to index the priv->dai_props array. This is
because the ID may be modified during snd_soc_add_pcm_runtime(), thus
resulting in an ID that's no longer a valid array index.

To fix this, use the position of the dai_link stored inside the PCM
runtime data relative to the start of the dai_link array as index into
the priv->dai_props array.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 include/sound/simple_card_utils.h     |  3 +++
 sound/soc/generic/simple-card-utils.c | 10 +++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index 3360d9eab068..0122f7871a40 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -89,6 +89,9 @@ struct simple_util_priv {
 #define simple_props_to_dai_codec(props, i)	((props)->codec_dai + i)
 #define simple_props_to_codec_conf(props, i)	((props)->codec_conf + i)
 
+#define runtime_simple_priv_to_props(priv, rtd)					\
+	((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))
+
 #define for_each_prop_dlc_cpus(props, i, cpu)				\
 	for ((i) = 0;							\
 	     ((i) < (props)->num.cpus) &&				\
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index a0c3111f7e08..c0f94e1cf0e9 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -296,7 +296,7 @@ int simple_util_startup(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
 	struct simple_util_priv *priv = snd_soc_card_get_drvdata(rtd->card);
-	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->id);
+	struct simple_dai_props *props = runtime_simple_priv_to_props(priv, rtd);
 	struct simple_util_dai *dai;
 	unsigned int fixed_sysclk = 0;
 	int i1, i2, i;
@@ -357,7 +357,7 @@ void simple_util_shutdown(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
 	struct simple_util_priv *priv = snd_soc_card_get_drvdata(rtd->card);
-	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->id);
+	struct simple_dai_props *props = runtime_simple_priv_to_props(priv, rtd);
 	struct simple_util_dai *dai;
 	int i;
 
@@ -446,7 +446,7 @@ int simple_util_hw_params(struct snd_pcm_substream *substream,
 	struct simple_util_dai *pdai;
 	struct snd_soc_dai *sdai;
 	struct simple_util_priv *priv = snd_soc_card_get_drvdata(rtd->card);
-	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->id);
+	struct simple_dai_props *props = runtime_simple_priv_to_props(priv, rtd);
 	unsigned int mclk, mclk_fs = 0;
 	int i, ret;
 
@@ -517,7 +517,7 @@ int simple_util_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
 				   struct snd_pcm_hw_params *params)
 {
 	struct simple_util_priv *priv = snd_soc_card_get_drvdata(rtd->card);
-	struct simple_dai_props *dai_props = simple_priv_to_props(priv, rtd->id);
+	struct simple_dai_props *dai_props = runtime_simple_priv_to_props(priv, rtd);
 	struct simple_util_data *data = &dai_props->adata;
 	struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
 	struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
@@ -628,7 +628,7 @@ static int simple_init_for_codec2codec(struct snd_soc_pcm_runtime *rtd,
 int simple_util_dai_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct simple_util_priv *priv = snd_soc_card_get_drvdata(rtd->card);
-	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->id);
+	struct simple_dai_props *props = runtime_simple_priv_to_props(priv, rtd);
 	struct simple_util_dai *dai;
 	int i, ret;
 
-- 
2.34.1

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Kuninori Morimoto]

Hi Laurentiu

Thank you for the patch

> As of commit cb18cd26039f ("ASoC: soc-core: do rtd->id trick at
> snd_soc_add_pcm_runtime()") the ID stored in the PCM runtime data can
> no longer be safely used to index the priv->dai_props array. This is
> because the ID may be modified during snd_soc_add_pcm_runtime(), thus
> resulting in an ID that's no longer a valid array index.
> 
> To fix this, use the position of the dai_link stored inside the PCM
> runtime data relative to the start of the dai_link array as index into
> the priv->dai_props array.
> 
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
(snip)
> +#define runtime_simple_priv_to_props(priv, rtd)					\
> +	((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))

Oh yes, indeed.

But I wonder it is needed not only utils, but all drivers
(= simple-card/audio-graph-card/audio-graph-card2).

Why don't you just replace macro ?

	- #define simple_priv_to_props(priv, i) // old macro
	+ #define simple_priv_to_props(priv, i) // new macro

Or, do you have any reasons ?

In replase case, we would like to have the comment not only git-log but
on header too.


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Laurentiu Mihalcea]

On 12/23/2024 2:48 AM, Kuninori Morimoto wrote:
> Hi Laurentiu
>
> Thank you for the patch
>
>> As of commit cb18cd26039f ("ASoC: soc-core: do rtd->id trick at
>> snd_soc_add_pcm_runtime()") the ID stored in the PCM runtime data can
>> no longer be safely used to index the priv->dai_props array. This is
>> because the ID may be modified during snd_soc_add_pcm_runtime(), thus
>> resulting in an ID that's no longer a valid array index.
>>
>> To fix this, use the position of the dai_link stored inside the PCM
>> runtime data relative to the start of the dai_link array as index into
>> the priv->dai_props array.
>>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> (snip)
>> +#define runtime_simple_priv_to_props(priv, rtd)					\
>> +	((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))
> Oh yes, indeed.
>
> But I wonder it is needed not only utils, but all drivers
> (= simple-card/audio-graph-card/audio-graph-card2).

At this point I'd say there's no need to do the replacement anywhere else. That's because the code
still using simple_priv_to_props() makes use of the link number to index the array, which is fine.
This is opposed to the PCM runtime data ID that may not correspond to a link number, thus making it
unfit for use as an array index.

>
> Why don't you just replace macro ?
>
> 	- #define simple_priv_to_props(priv, i) // old macro
> 	+ #define simple_priv_to_props(priv, i) // new macro
>
> Or, do you have any reasons ?
>
> In replase case, we would like to have the comment not only git-log but
> on header too.
>
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto

Thanks for taking your time to review this and sorry for the (very) late reply!

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Kuninori Morimoto]

Hi Laurentiu

> >> +#define runtime_simple_priv_to_props(priv, rtd)
> >> +	((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))
> > Oh yes, indeed.
> >
> > But I wonder it is needed not only utils, but all drivers
> > (= simple-card/audio-graph-card/audio-graph-card2).
> 
> At this point I'd say there's no need to do the replacement anywhere else. That's because the code
> still using simple_priv_to_props() makes use of the link number to index the array, which is fine.
> This is opposed to the PCM runtime data ID that may not correspond to a link number, thus making it
> unfit for use as an array index.

Oops, my previous mail indicates strange sample.

I would like to tell was update simple_priv_to_props() itself

-	#define simple_priv_to_props(priv, i)	((priv)->dai_props + (i))
+	#define simple_priv_to_props(priv, rtd)	\
+		((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))

...

-	simple_priv_to_props(priv, rtd->id);
+	simple_priv_to_props(priv, rtd);


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Laurentiu Mihalcea]

On 1/9/2025 2:07 AM, Kuninori Morimoto wrote:
> Hi Laurentiu
>
>>>> +#define runtime_simple_priv_to_props(priv, rtd)
>>>> +	((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))
>>> Oh yes, indeed.
>>>
>>> But I wonder it is needed not only utils, but all drivers
>>> (= simple-card/audio-graph-card/audio-graph-card2).
>> At this point I'd say there's no need to do the replacement anywhere else. That's because the code
>> still using simple_priv_to_props() makes use of the link number to index the array, which is fine.
>> This is opposed to the PCM runtime data ID that may not correspond to a link number, thus making it
>> unfit for use as an array index.
> Oops, my previous mail indicates strange sample.
>
> I would like to tell was update simple_priv_to_props() itself
>
> -	#define simple_priv_to_props(priv, i)	((priv)->dai_props + (i))
> +	#define simple_priv_to_props(priv, rtd)	\
> +		((priv)->dai_props + ((rtd)->dai_link - (priv)->dai_link))
>
> ...
>
> -	simple_priv_to_props(priv, rtd->id);
> +	simple_priv_to_props(priv, rtd);

ah, thanks for the clarification! correct me if I'm wrong here but the issue
that I see with the suggested approach is that we have some places in which
simple_priv_to_props() is used before the RTD is created.

Example scenario: in audio-graph-card2 we call graph_link_init() (which uses simple_priv_to_props)
before the card is registered (during which the RTD is created).

If the current naming might confuse people then perhaps we could change it to something like

simple_priv_to_props -> simple_props_by_link_idx
runtime_simple_priv_to_props -> simple_props_by_rtd

while also adding some comments on how/when the macros should be used?

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Kuninori Morimoto]

Hi Laurentiu

Thank you for your reply

> > -	simple_priv_to_props(priv, rtd->id);
> > +	simple_priv_to_props(priv, rtd);
> 
> ah, thanks for the clarification! correct me if I'm wrong here but the issue
> that I see with the suggested approach is that we have some places in which
> simple_priv_to_props() is used before the RTD is created.
> 
> Example scenario: in audio-graph-card2 we call graph_link_init() (which uses simple_priv_to_props)
> before the card is registered (during which the RTD is created).

Do you mean like this ?

Before created RTD: ID is not yet updated : use         simple_priv_to_props()
After  created RTD: ID is         updated : use runtime_simple_priv_to_props()

Is this correct ?
If so, do we need to use them differently ?
In the end, it end up doing the same thing, I think.

If my understanding was correct, it will just makes people confuse, and I
don't want to makes code complex. I think just adding comment why it
don't/can't use rtd->id directly is simple and enough (almost all user
don't care small detail of macros :) but what do you think ?


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH RFC] ASoC: simple-card-utils: fix priv->dai_props indexing

[Laurentiu Mihalcea]

On 1/10/2025 4:25 AM, Kuninori Morimoto wrote:
> Hi Laurentiu
>
> Thank you for your reply
>
>>> -	simple_priv_to_props(priv, rtd->id);
>>> +	simple_priv_to_props(priv, rtd);
>> ah, thanks for the clarification! correct me if I'm wrong here but the issue
>> that I see with the suggested approach is that we have some places in which
>> simple_priv_to_props() is used before the RTD is created.
>>
>> Example scenario: in audio-graph-card2 we call graph_link_init() (which uses simple_priv_to_props)
>> before the card is registered (during which the RTD is created).
> Do you mean like this ?
>
> Before created RTD: ID is not yet updated : use         simple_priv_to_props()
> After  created RTD: ID is         updated : use runtime_simple_priv_to_props()
>
> Is this correct ?

yep, pretty much.

> If so, do we need to use them differently ?
> In the end, it end up doing the same thing, I think.

yep, they do the exact same thing. The only difference is the way they do it. As for the usage, the "rule" would be:

if you have an RTD structure =>
    use runtime_simple_priv_to_props(priv, rtd) if simple_priv_to_props(priv, rtd->id) might be invalid

otherwise =>
    use simple_priv_to_props()

>
> If my understanding was correct, it will just makes people confuse, and I
> don't want to makes code complex. I think just adding comment why it
> don't/can't use rtd->id directly is simple and enough (almost all user
> don't care small detail of macros :) but what do you think ?

i agree, no need to overly-complicate things. Will just add a comment in code as you suggested.

Thanks for the input!

>
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto