Re: [PATCH v1 06/10] ASoC: loongson: Fix codec detection failure on FDT systems

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote:
> When the Codec is compiled into a module, we can't use
> snd_soc_of_get_dlc() to get the codec dai_name, use
> snd_soc_get_dai_name() instead.
> 
> Also, for the cpu dailink, its dai_name is already defined as
> "loongson-i2s", so just get the corresponding of_node attribute here.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> ---
>  sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++---------
>  1 file changed, 63 insertions(+), 26 deletions(-)
> 
> diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
> index a25287efdd5c..d45a3e77cb90 100644
> --- a/sound/soc/loongson/loongson_card.c
> +++ b/sound/soc/loongson/loongson_card.c
> @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
>  	return 0;
>  }
>  
> -static int loongson_card_parse_of(struct loongson_card_data *data)
> +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node)
>  {
> -	struct snd_soc_card *card = &data->snd_card;
> -	struct device_node *cpu, *codec;
> -	struct device *dev = card->dev;
> -	int ret, i;
> +	struct device_node *cpu;
> +	int ret = 0;
>  
>  	cpu = of_get_child_by_name(dev->of_node, "cpu");
> -	if (!cpu) {
> -		dev_err(dev, "platform property missing or invalid\n");
> +	if (!cpu)
>  		return -EINVAL;
> -	}
> +
> +	*dai_node = of_parse_phandle(cpu, "sound-dai", 0);
> +	if (!*dai_node)
> +		ret = -EINVAL;
> +
> +	of_node_put(cpu);

This goes just after of_parse_phandle, which simplifies your code.

> +	return ret;
> +}
> +
> +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
> +				const char **dai_name)
> +{
> +	struct of_phandle_args args;
> +	struct device_node *codec;
> +	int ret = 0;
>  
>  	codec = of_get_child_by_name(dev->of_node, "codec");
> -	if (!codec) {
> -		dev_err(dev, "audio-codec property missing or invalid\n");
> +	if (!codec)

Hm? So you exit here and then caller does of_node_put on stack value.
This is buggy.

> +		return -EINVAL;
> +
> +	ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args);
> +	if (ret)
> +		goto free_codec;
> +
> +	ret = snd_soc_get_dai_name(&args, dai_name);
> +	if (ret)

Your error paths should clean here. Each function is responsible to
clean up after itself in case of errors, not rely on caller.

> +		goto free_codec;
> +
> +	*dai_node = of_parse_phandle(codec, "sound-dai", 0);
> +	if (!*dai_node)
>  		ret = -EINVAL;
> -		goto free_cpu;
> +
> +free_codec:

You are not freeing any resources here (at least not directly). You are
dropping reference. Use matching label name. free is associated with
kalloc()..


> +	of_node_put(codec);
> +	return ret;
> +}
> +
> +static int loongson_card_parse_of(struct loongson_card_data *data)
> +{
> +	struct device_node *codec_dai_node, *cpu_dai_node;
> +	struct snd_soc_card *card = &data->snd_card;
> +	struct device *dev = card->dev;
> +	const char *codec_dai_name;
> +	int ret = 0, i;
> +
> +	ret = loongson_parse_cpu(dev, &cpu_dai_node);
> +	if (ret) {
> +		dev_err(dev, "cpu property missing or invalid.\n");
> +		goto out;
> +	}
> +
> +	ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name);
> +	if (ret) {
> +		dev_err(dev, "audio-codec property missing or invalid.\n");
> +		goto out;
>  	}
>  
>  	for (i = 0; i < card->num_links; i++) {
> -		ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
> -		if (ret) {
> -			dev_err(dev, "getting cpu dlc error (%d)\n", ret);
> -			goto free_codec;
> -		}
> -
> -		ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
> -		if (ret) {
> -			dev_err(dev, "getting codec dlc error (%d)\n", ret);
> -			goto free_codec;
> -		}
> +		loongson_dai_links[i].platforms->of_node = cpu_dai_node;
> +		loongson_dai_links[i].cpus->of_node = cpu_dai_node;
> +		loongson_dai_links[i].codecs->of_node = codec_dai_node;
> +		loongson_dai_links[i].codecs->dai_name = codec_dai_name;
>  	}
>  
> -free_codec:
> -	of_node_put(codec);
> -free_cpu:
> -	of_node_put(cpu);
> +out:
> +	of_node_put(codec_dai_node);

Yeah, so here we see drop of reference from stack-based pointer...

Best regards,
Krzysztof