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

[Binbin Zhou <zhoubb.aaron@xxxxxxxxx>]

Hi Krzysztof:

Thanks for your detailed review.

On Fri, Sep 6, 2024 at 4:29 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> 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.

OK.. the ret param is unnecessary also.
>
> > +     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.

Sorry, I can not get your point, I think there is nothing that should be put.
>
> > +             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.

Yes, I should of_node_put(args.np); here
>
> > +             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()..
>

OK, I will rename the label name as codec_put.

Thanks.
Binbin
>
> > +     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
>