[alsa-devel] [PATCHv2 0/2] ASoC: simple-card: Fix bug about DT node's refcount
Change for v2: - Just rebased to the next branch newest head.
Xiubo Li (2): ASoC: simple-card: Fix bug of forgetting decrement DT node's refcount ASoC: simple-card: Fix bug of wrong decrement DT node's refcount
sound/soc/generic/simple-card.c | 53 ++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 24 deletions(-)
We shouldn't forget decrement the last DT node when the for_each_child_of_node() has finished searching.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/generic/simple-card.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index fd8b045..9e170fe 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -360,6 +360,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, } i++; } + of_node_put(np); } else { ret = asoc_simple_card_dai_link_of(node, dev, dai_link, dai_props, true);
On Mon, 1 Sep 2014 09:35:26 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
We shouldn't forget decrement the last DT node when the for_each_child_of_node() has finished searching.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index fd8b045..9e170fe 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -360,6 +360,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, } i++; }
of_node_put(np);
No. np is NULL at end of loop.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
card.c
index fd8b045..9e170fe 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -360,6 +360,7 @@ static int asoc_simple_card_parse_of(struct device_node
*node,
} i++; }
of_node_put(np);No. np is NULL at end of loop.
Well, yes, you are right.
Thanks,
BRs Xiubo
DAI links's cpu_of_node's and codec_of_node's refcounts shouldn't be decremented immediately at the end of the probe() fucntion. Because we will still use them before the audio card is removed.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/generic/simple-card.c | 52 ++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 9e170fe..08c5d7d 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -307,6 +307,22 @@ dai_link_of_err: return ret; }
+static inline void +asoc_simple_card_unref(const struct snd_soc_dai_link *dai_link, + int num_links) +{ + struct device_node *np; + + while (num_links--) { + np = dai_link[num_links].cpu_of_node; + if (np) + of_node_put(np); + np = dai_link[num_links].codec_of_node; + if (np) + of_node_put(np); + } +} + static int asoc_simple_card_parse_of(struct device_node *node, struct simple_card_data *priv, struct device *dev, @@ -355,6 +371,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, dai_props + i, false); if (ret < 0) { + asoc_simple_card_unref(dai_link, i + 1); of_node_put(np); return ret; } @@ -374,27 +391,6 @@ static int asoc_simple_card_parse_of(struct device_node *node, return 0; }
-/* update the reference count of the devices nodes at end of probe */ -static int asoc_simple_card_unref(struct platform_device *pdev) -{ - struct snd_soc_card *card = platform_get_drvdata(pdev); - struct snd_soc_dai_link *dai_link; - struct device_node *np; - int num_links; - - for (num_links = 0, dai_link = card->dai_link; - num_links < card->num_links; - num_links++, dai_link++) { - np = (struct device_node *) dai_link->cpu_of_node; - if (np) - of_node_put(np); - np = (struct device_node *) dai_link->codec_of_node; - if (np) - of_node_put(np); - } - return 0; -} - static int asoc_simple_card_probe(struct platform_device *pdev) { struct simple_card_data *priv; @@ -441,7 +437,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev) if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(dev, "parse error %d\n", ret); - goto err; + return ret; }
} else { @@ -483,11 +479,18 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
-err: - asoc_simple_card_unref(pdev); return ret; }
+static int asoc_simple_card_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + + asoc_simple_card_unref(card->dai_link, card->num_links); + + return 0; +} + static const struct of_device_id asoc_simple_of_match[] = { { .compatible = "simple-audio-card", }, {}, @@ -501,6 +504,7 @@ static struct platform_driver asoc_simple_card = { .of_match_table = asoc_simple_of_match, }, .probe = asoc_simple_card_probe, + .remove = asoc_simple_card_remove, };
module_platform_driver(asoc_simple_card);
On Mon, 1 Sep 2014 09:35:27 +0800 Xiubo Li Li.Xiubo@freescale.com wrote:
DAI links's cpu_of_node's and codec_of_node's refcounts shouldn't be decremented immediately at the end of the probe() fucntion. Because we will still use them before the audio card is removed.
Right, but your patch seems a bit complicated. See below.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 52 ++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 9e170fe..08c5d7d 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -307,6 +307,22 @@ dai_link_of_err: return ret; }
+static inline void +asoc_simple_card_unref(const struct snd_soc_dai_link *dai_link,
int num_links)+{
- struct device_node *np;
- while (num_links--) {
np = dai_link[num_links].cpu_of_node;if (np)of_node_put(np);np = dai_link[num_links].codec_of_node;if (np)of_node_put(np);- }
+}
static int asoc_simple_card_parse_of(struct device_node *node, struct simple_card_data *priv, struct device *dev, @@ -355,6 +371,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, dai_props + i, false); if (ret < 0) {
asoc_simple_card_unref(dai_link, i + 1); of_node_put(np); return ret; }@@ -374,27 +391,6 @@ static int asoc_simple_card_parse_of(struct device_node *node, return 0; }
-/* update the reference count of the devices nodes at end of probe */ -static int asoc_simple_card_unref(struct platform_device *pdev) -{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- struct snd_soc_dai_link *dai_link;
- struct device_node *np;
- int num_links;
- for (num_links = 0, dai_link = card->dai_link;
num_links < card->num_links;num_links++, dai_link++) {np = (struct device_node *) dai_link->cpu_of_node;if (np)of_node_put(np);np = (struct device_node *) dai_link->codec_of_node;if (np)of_node_put(np);- }
- return 0;
-}
static int asoc_simple_card_probe(struct platform_device *pdev) { struct simple_card_data *priv; @@ -441,7 +437,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev) if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(dev, "parse error %d\n", ret);
goto err;
return ret;}
} else {
@@ -483,11 +479,18 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
-err:
- asoc_simple_card_unref(pdev); return ret;
}
The main problem is there: don't unref the nodes is no error. Why not simply:
ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card); + if (ret >= 0) + return ret; /* success */
err: asoc_simple_card_unref(pdev); return ret; }
? Then, the above stuff is not needed.
+static int asoc_simple_card_remove(struct platform_device *pdev) +{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- asoc_simple_card_unref(card->dai_link, card->num_links);
- return 0;
+}
static const struct of_device_id asoc_simple_of_match[] = { { .compatible = "simple-audio-card", }, {}, @@ -501,6 +504,7 @@ static struct platform_driver asoc_simple_card = { .of_match_table = asoc_simple_of_match, }, .probe = asoc_simple_card_probe,
- .remove = asoc_simple_card_remove,
};
module_platform_driver(asoc_simple_card);
OK.
DAI links's cpu_of_node's and codec_of_node's refcounts shouldn't be decremented immediately at the end of the probe() fucntion. Because we will still use them before the audio card is removed.
Right, but your patch seems a bit complicated. See below.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com
sound/soc/generic/simple-card.c | 52 ++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
card.c
index 9e170fe..08c5d7d 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -307,6 +307,22 @@ dai_link_of_err: return ret; }
+static inline void +asoc_simple_card_unref(const struct snd_soc_dai_link *dai_link,
int num_links)+{
- struct device_node *np;
- while (num_links--) {
np = dai_link[num_links].cpu_of_node;if (np)of_node_put(np);np = dai_link[num_links].codec_of_node;if (np)of_node_put(np);- }
+}
static int asoc_simple_card_parse_of(struct device_node *node, struct simple_card_data *priv, struct device *dev, @@ -355,6 +371,7 @@ static int asoc_simple_card_parse_of(struct device_node
*node,
dai_props + i, false); if (ret < 0) {
asoc_simple_card_unref(dai_link, i + 1); of_node_put(np); return ret; }@@ -374,27 +391,6 @@ static int asoc_simple_card_parse_of(struct device_node
*node,
return 0; }
-/* update the reference count of the devices nodes at end of probe */ -static int asoc_simple_card_unref(struct platform_device *pdev) -{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- struct snd_soc_dai_link *dai_link;
- struct device_node *np;
- int num_links;
- for (num_links = 0, dai_link = card->dai_link;
num_links < card->num_links;num_links++, dai_link++) {np = (struct device_node *) dai_link->cpu_of_node;if (np)of_node_put(np);np = (struct device_node *) dai_link->codec_of_node;if (np)of_node_put(np);- }
- return 0;
-}
static int asoc_simple_card_probe(struct platform_device *pdev) { struct simple_card_data *priv; @@ -441,7 +437,7 @@ static int asoc_simple_card_probe(struct platform_device
*pdev)
if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(dev, "parse error %d\n", ret);
goto err;
return ret;}
} else {
@@ -483,11 +479,18 @@ static int asoc_simple_card_probe(struct
platform_device *pdev)
ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
-err:
- asoc_simple_card_unref(pdev); return ret;
}
The main problem is there: don't unref the nodes is no error. Why not simply:
ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
- if (ret >= 0)
return ret; /* success */err: asoc_simple_card_unref(pdev); return ret; }
? Then, the above stuff is not needed.
Why I moved the _unref() from the probe() to asoc_simple_card_parse_of() Is that I think we should _unref() those nodes immediately after failed, But this will be a bit more complicated as you said, and also will include one new bug: When devm_snd_soc_register_card() failed, the _unref() won't be ran.
Thanks very much for you comment.
I will fix it in the next version,
BRs Xiubo
+static int asoc_simple_card_remove(struct platform_device *pdev) +{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- asoc_simple_card_unref(card->dai_link, card->num_links);
- return 0;
+}
static const struct of_device_id asoc_simple_of_match[] = { { .compatible = "simple-audio-card", }, {}, @@ -501,6 +504,7 @@ static struct platform_driver asoc_simple_card = { .of_match_table = asoc_simple_of_match, }, .probe = asoc_simple_card_probe,
- .remove = asoc_simple_card_remove,
};
module_platform_driver(asoc_simple_card);
OK.
-- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
participants (3)
-
Jean-Francois Moine -
Li.Xiubo＠freescale.com -
Xiubo Li