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
>
