| Message ID | f9111564acc6c9ac011318a7eb3c6843ea0529ce.1725518229.git.zhoubinbin@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Some issues about loongson i2s | expand |
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@loongson.cn> > --- > 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
Hi Krzysztof: Thanks for your detailed review. On Fri, Sep 6, 2024 at 4:29 PM Krzysztof Kozlowski <krzk@kernel.org> 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@loongson.cn> > > --- > > 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 >
On 09/09/2024 09:51, Binbin Zhou wrote: >> >>> + 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. You drop reference from a pointer which is a random stack value. Best regards, Krzysztof
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); + 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) + 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) + goto free_codec; + + *dai_node = of_parse_phandle(codec, "sound-dai", 0); + if (!*dai_node) ret = -EINVAL; - goto free_cpu; + +free_codec: + 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); + of_node_put(cpu_dai_node); return ret; }
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@loongson.cn> --- sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 26 deletions(-)