[1/4] ASoC: simple-card: Fix device node locks

Commit Message

Jean-Francois Moine Feb. 20, 2014, 5:25 p.m. UTC

Some device nodes stay locked and some other ones are not locked
while being used during the card lifetime.

Signed-off-by: Jean-Francois Moine <[e-mail]>
---
 sound/soc/generic/simple-card.c | 51 +++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 12 deletions(-)

Comments

Xiubo Li Feb. 24, 2014, 2:17 a.m. UTC | #1

> @@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node
> *node,
>  	/* CPU sub-node */
>  	ret = -EINVAL;
>  	np = of_get_child_by_name(node, "simple-audio-card,cpu");
> -	if (np)
> +	if (np) {
>  		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
>  						  &priv->cpu_dai,
>  						  &dai_link->cpu_of_node,
>  						  &dai_link->cpu_dai_name);
> +		of_node_put(np);

Does the of_node_put(np) is really needed here ?


> +	}
>  	if (ret < 0)
>  		return ret;
> 
>  	/* CODEC sub-node */
>  	ret = -EINVAL;
>  	np = of_get_child_by_name(node, "simple-audio-card,codec");
> -	if (np)
> +	if (np) {
>  		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
>  						  &priv->codec_dai,
>  						  &dai_link->codec_of_node,
>  						  &dai_link->codec_dai_name);
> +		of_node_put(np);

The same here...

> +	}
>  	if (ret < 0)
>  		return ret;
> 

Thanks,

--
Best Regards,
Xiubo

Jean-Francois Moine Feb. 25, 2014, 7:30 a.m. UTC | #2

On Mon, 24 Feb 2014 02:17:00 +0000
"[e-mail]" <[e-mail]> wrote:

> > @@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node
> > *node,
> >  	/* CPU sub-node */
> >  	ret = -EINVAL;
> >  	np = of_get_child_by_name(node, "simple-audio-card,cpu");
> > -	if (np)
> > +	if (np) {
> >  		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
> >  						  &priv->cpu_dai,
> >  						  &dai_link->cpu_of_node,
> >  						  &dai_link->cpu_dai_name);
> > +		of_node_put(np);
> 
> Does the of_node_put(np) is really needed here ?
	[snip]

Yes, of_get_child_by_name() increments the node refcount and np is not
used afterwards.

But, you are right, this creates a bug in the next patch when using
of_get_next_child(). I will fix it.

Thanks.

Mark Brown Feb. 25, 2014, 12:42 p.m. UTC | #3

On Thu, Feb 20, 2014 at 06:25:12PM +0100, Jean-Francois Moine wrote:
> Some device nodes stay locked and some other ones are not locked
> while being used during the card lifetime.

Please pay more attention to describing your patches clearly in
changelogs and to splitting them up for review.  This is a frequent
issue and it does make it much harder to check what's happening.

In this case you're not actually working with locking but rather with
reference counting and there's no real description of the actual errors
either.  Saying something like "function X takes a reference which we
need to drop" would help a lot, and it would let us think about if it's
sensible for the function to return holding the reference in the first
place and how long the reference needs to be held for.

Patch

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 4fe8abc..8809ab4 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -92,7 +92,7 @@  asoc_simple_card_sub_parse_of(struct device_node *np,
 	/* get dai->name */
 	ret = snd_soc_of_get_dai_name(np, name);
 	if (ret < 0)
-		goto parse_error;
+		return ret;
 
 	/*
 	 * bitclock-inversion, frame-inversion
@@ -112,7 +112,7 @@  asoc_simple_card_sub_parse_of(struct device_node *np,
 		clk = of_clk_get(np, 0);
 		if (IS_ERR(clk)) {
 			ret = PTR_ERR(clk);
-			goto parse_error;
+			return ret;
 		}
 
 		dai->sysclk = clk_get_rate(clk);
@@ -126,12 +126,7 @@  asoc_simple_card_sub_parse_of(struct device_node *np,
 			dai->sysclk = clk_get_rate(clk);
 	}
 
-	ret = 0;
-
-parse_error:
-	of_node_put(node);
-
-	return ret;
+	return 0;
 }
 
 static int asoc_simple_card_parse_of(struct device_node *node,
@@ -169,22 +164,26 @@  static int asoc_simple_card_parse_of(struct device_node *node,
 	/* CPU sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,cpu");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
 						  &dai_link->cpu_dai_name);
+		of_node_put(np);
+	}
 	if (ret < 0)
 		return ret;
 
 	/* CODEC sub-node */
 	ret = -EINVAL;
 	np = of_get_child_by_name(node, "simple-audio-card,codec");
-	if (np)
+	if (np) {
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->codec_dai,
 						  &dai_link->codec_of_node,
 						  &dai_link->codec_dai_name);
+		of_node_put(np);
+	}
 	if (ret < 0)
 		return ret;
 
@@ -219,6 +218,26 @@  static int asoc_simple_card_parse_of(struct device_node *node,
 	return 0;
 }
 
+static int asoc_simple_card_purge(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;
@@ -246,7 +265,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);
-			return ret;
+			goto err;
 		}
 	} else {
 		struct asoc_simple_card_info *cinfo;
@@ -287,7 +306,14 @@  static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	snd_soc_card_set_drvdata(&priv->snd_card, priv);
 
-	return devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
+	ret = devm_snd_soc_register_card(&pdev->dev, &priv->snd_card);
+	if (ret < 0)
+		goto err;
+	return 0;
+
+err:
+	asoc_simple_card_purge(pdev);
+	return ret;
 }
 
 static const struct of_device_id asoc_simple_of_match[] = {
@@ -303,6 +329,7 @@  static struct platform_driver asoc_simple_card = {
 		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
+	.remove		= asoc_simple_card_purge,
 };
 
 module_platform_driver(asoc_simple_card);