| Message ID | 1440957911-7687-2-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8985729578cb42f9e781a8e38e5b6b1ee90c1018 |
| Headers | show |
Am 30.08.2015 20:05, schrieb Julia Lawall: > Apply PTR_ERR to the value that was recently assigned. > > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression x,y; > @@ > > if (IS_ERR(x) || ...) { > ... when any > when != IS_ERR(...) > ( > PTR_ERR(x) > | > * PTR_ERR(y) > ) > ... when any > } > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > sound/soc/qcom/lpass-cpu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c > index 23f3d59..94beb99 100644 > --- a/sound/soc/qcom/lpass-cpu.c > +++ b/sound/soc/qcom/lpass-cpu.c > @@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > dev_err(&pdev->dev, > "%s() error getting mi2s-bit-clk: %ld\n", > - __func__, PTR_ERR(drvdata->mi2s_bit_clk[i])); > + __func__, > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > } > } > just a note: using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help to make the code more readable (yes, the other code is alike). something like: struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); if (IS_ERR(tmp)) { dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: %ld\n",__func__, PTR_ERR(tmp)); return PTR_ERR(tmp); } drvdata->mi2s_bit_clk[dai_id]=tmp; just one minor: the dev_warn() just before says: " error getting mi2s-osr-clk" may be it should be "warnig ..." That will make it more easy to rep for real error in a log. re, wh
On Sun, 30 Aug 2015, walter harms wrote: > > > Am 30.08.2015 20:05, schrieb Julia Lawall: > > Apply PTR_ERR to the value that was recently assigned. > > > > The semantic match that finds this problem is as follows: > > (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @@ > > expression x,y; > > @@ > > > > if (IS_ERR(x) || ...) { > > ... when any > > when != IS_ERR(...) > > ( > > PTR_ERR(x) > > | > > * PTR_ERR(y) > > ) > > ... when any > > } > > // </smpl> > > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > > > --- > > sound/soc/qcom/lpass-cpu.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c > > index 23f3d59..94beb99 100644 > > --- a/sound/soc/qcom/lpass-cpu.c > > +++ b/sound/soc/qcom/lpass-cpu.c > > @@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) > > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > > dev_err(&pdev->dev, > > "%s() error getting mi2s-bit-clk: %ld\n", > > - __func__, PTR_ERR(drvdata->mi2s_bit_clk[i])); > > + __func__, > > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > > } > > } > > > > just a note: > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help to make the code > more readable (yes, the other code is alike). something like: > > struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); Where do you suggest to put this? Maybe it would be reasonable to declare a variable struct clk *clk; at the top of the function, and then use that as a temporary variable for all three calls. However, now I see that the first call, unlike the other two doesn't cause a return from the function. if (IS_ERR(drvdata->mi2s_osr_clk[dai_id])) { dev_warn(&pdev->dev, "%s() error getting mi2s-osr-clk: %ld\n", __func__, PTR_ERR(drvdata->mi2s_osr_clk[dai_id])); } Is that intentional? thanks, julia > if (IS_ERR(tmp)) { > dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: %ld\n",__func__, PTR_ERR(tmp)); > return PTR_ERR(tmp); > } > drvdata->mi2s_bit_clk[dai_id]=tmp; > > > just one minor: > the dev_warn() just before says: " error getting mi2s-osr-clk" may be it should be "warnig ..." > That will make it more easy to rep for real error in a log. > > re, > wh > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Sun, Aug 30, 2015 at 11:05:10AM -0700, Julia Lawall wrote: > Apply PTR_ERR to the value that was recently assigned. > > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression x,y; > @@ > > if (IS_ERR(x) || ...) { > ... when any > when != IS_ERR(...) > ( > PTR_ERR(x) > | > * PTR_ERR(y) > ) > ... when any > } > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- The patch itself looks good. Thanks. Acked-by: Kenneth Westfield <kwestfie@codeaurora.org>
On Sun, Aug 30, 2015 at 11:54:33AM -0700, walter harms wrote: > Am 30.08.2015 20:05, schrieb Julia Lawall: > > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > > dev_err(&pdev->dev, > > "%s() error getting mi2s-bit-clk: %ld\n", > > - __func__, > PTR_ERR(drvdata->mi2s_bit_clk[i])); > > + __func__, > > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > > } > > } > > > > just a note: > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help > to make the code > more readable (yes, the other code is alike). something like: > > struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); > > if (IS_ERR(tmp)) { > dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: > %ld\n",__func__, PTR_ERR(tmp)); > return PTR_ERR(tmp); > } > drvdata->mi2s_bit_clk[dai_id]=tmp; Yes, it would make the code more readable. > > > just one minor: > the dev_warn() just before says: " error getting mi2s-osr-clk" may be it > should be "warnig ..." > That will make it more easy to rep for real error in a log. "error [gs]etting" could be re-phrased to "could not [gs]et". The term "error" was not meant to indicate the log level, but evidently, can cause some confusion for someone reading the logs.
On Sun, Aug 30, 2015 at 12:54:15PM -0700, Julia Lawall wrote: > On Sun, 30 Aug 2015, walter harms wrote: > > Am 30.08.2015 20:05, schrieb Julia Lawall: > > > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > > > dev_err(&pdev->dev, > > > "%s() error getting mi2s-bit-clk: %ld\n", > > > - __func__, > PTR_ERR(drvdata->mi2s_bit_clk[i])); > > > + __func__, > > > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > > > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > > > } > > > } > > > > > > > just a note: > > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould > help to make the code > > more readable (yes, the other code is alike). something like: > > > > struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); > > Where do you suggest to put this? > > Maybe it would be reasonable to declare a variable struct clk *clk; at the > > top of the function, and then use that as a temporary variable for all > three calls. > > However, now I see that the first call, unlike the other two doesn't cause > > a return from the function. > > if (IS_ERR(drvdata->mi2s_osr_clk[dai_id])) { > dev_warn(&pdev->dev, > "%s() error getting mi2s-osr-clk: %ld\n", > __func__, > PTR_ERR(drvdata->mi2s_osr_clk[dai_id])); > } > > Is that intentional? Yes, that was intentional as the presense of the OSR clock in the DT node is optional.
On Sun, Aug 30, 2015 at 08:05:10PM +0200, Julia Lawall wrote: > Apply PTR_ERR to the value that was recently assigned. > > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) I have no idea what's going on with this stuff without spending more time than it looks like should need, there's a moderately big thread and some patches posted in the middle of it. Can you repost whatever the current state is please?
On Mon, 14 Sep 2015, Mark Brown wrote: > On Sun, Aug 30, 2015 at 08:05:10PM +0200, Julia Lawall wrote: > > Apply PTR_ERR to the value that was recently assigned. > > > > The semantic match that finds this problem is as follows: > > (http://coccinelle.lip6.fr/) > > I have no idea what's going on with this stuff without spending more > time than it looks like should need, there's a moderately big thread and > some patches posted in the middle of it. Can you repost whatever the > current state is please? The discussion was about introducing a temporary variable to simplify the code. But that makes a lot of changes, so I think it would be better to just apply the original bug fixing patch as is, and then the cleanup could be applied on top of that. I will submit the original patch again. julia
On Thu, Sep 17, 2015 at 10:46:16AM +0200, Julia Lawall wrote: > The discussion was about introducing a temporary variable to simplify the > code. But that makes a lot of changes, so I think it would be better to > just apply the original bug fixing patch as is, and then the cleanup could > be applied on top of that. I will submit the original patch again. OK, makes sense - thanks.
diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index 23f3d59..94beb99 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { dev_err(&pdev->dev, "%s() error getting mi2s-bit-clk: %ld\n", - __func__, PTR_ERR(drvdata->mi2s_bit_clk[i])); + __func__, + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); } }
Apply PTR_ERR to the value that was recently assigned. The semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ expression x,y; @@ if (IS_ERR(x) || ...) { ... when any when != IS_ERR(...) ( PTR_ERR(x) | * PTR_ERR(y) ) ... when any } // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- sound/soc/qcom/lpass-cpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)