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

Message ID 1409535327-11449-3-git-send-email-Li.Xiubo@freescale.com
State New, archived
Headers show

Commit Message

Xiubo Li Sept. 1, 2014, 1:35 a.m. UTC
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(-)

Comments

Jean-Francois Moine Sept. 1, 2014, 6:54 a.m. UTC | #1
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.
Xiubo Li Sept. 1, 2014, 7:11 a.m. UTC | #2
> > 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/

Patch
diff mbox

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