[PATCHv2 0/2] ASoC: simple-card: Fix bug about DT node's refcount

[PATCHv2 0/2] ASoC: simple-card: Fix bug about DT node's refcount

[Xiubo Li]

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(-)

-- 
1.8.4

[PATCHv2 1/2] ASoC: simple-card: Fix bug of forgetting decrement DT node's refcount

[Xiubo Li]

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);
-- 
1.8.4

[PATCHv2 2/2] ASoC: simple-card: Fix bug of wrong decrement DT node's refcount

[Xiubo Li]

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);
-- 
1.8.4

Re: [PATCHv2 1/2] ASoC: simple-card: Fix bug of forgetting decrement DT node's refcount

[Jean-Francois Moine]

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);
>  	} else {
>  		ret = asoc_simple_card_dai_link_of(node, dev,
>  						   dai_link, dai_props, true);

No. np is NULL at end of loop.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

RE: [PATCHv2 1/2] ASoC: simple-card: Fix bug of forgetting decrement DT node's refcount

[Li.Xiubo]

> > 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);
> 
> No. np is NULL at end of loop.
> 

Well, yes, you are right.

Thanks,

BRs
Xiubo

Re: [PATCHv2 2/2] ASoC: simple-card: Fix bug of wrong decrement DT node's refcount

[Jean-Francois Moine]

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

RE: [PATCHv2 2/2] ASoC: simple-card: Fix bug of wrong decrement DT node's refcount

[Li.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.
> 
> 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/