| Message ID | 1409535327-11449-3-git-send-email-Li.Xiubo@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/
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);
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(-)