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

[Li.Xiubo@xxxxxxxxxxxxx]

> > 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@xxxxxxxxxxxxx>
> > ---
> >  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/
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå