Use recently introduced devm_platform_ioremap_resource helper which wraps platform_get_resource() and devm_ioremap_resource() together. This helps produce much cleaner code while removing local `struct resource` declaration. Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> --- Tree wide changes has been tested through 0-day test service with build success. BUILD SUCCESS 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 tree/branch: https://github.com/himanshujha199640/linux-next 20190401-devm_platform_ioremap_resource-final branch HEAD: 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 Coccinelle: api: Add devm_platform_ioremap_resource.cocci elapsed time: 385m configs tested: 162 Stats: 916 files changed, 1028 insertions(+), 2921 deletions(-) Note: cases where the `struct resource *res` variable is used subsequently in the function have been ignored out because those cases produce: eg., drivers/bus/da8xx-mstpri.c warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized] due to: if (prio_descr->reg + sizeof(u32) > resource_size(res)) { which seems correct as `res` isn't initialized in the scope of the function(da8xx_mstpri_probe) and instead initialized inside: void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, unsigned int index) { struct resource *res; res = platform_get_resource(pdev, IORESOURCE_MEM, index); return devm_ioremap_resource(&pdev->dev, res); } EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); .../api/devm_platform_ioremap_resource.cocci | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci new file mode 100644 index 000000000000..a28274af14df --- /dev/null +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci @@ -0,0 +1,63 @@ +/// Use devm_platform_ioremap_resource helper which wraps +/// platform_get_resource() and devm_ioremap_resource() together. +/// +// Confidence: High +// Copyright: (C) 2019 Himanshu Jha GPLv2. +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. +// Keywords: platform_get_resource, devm_ioremap_resource, +// Keywords: devm_platform_ioremap_resource + +virtual patch +virtual report + +@r depends on patch && !report@ +expression e1, e2, arg1, arg2, arg3, arg4; +identifier id; +@@ + +( +- id = platform_get_resource(arg1, arg2, arg3); +| +- struct resource *id = platform_get_resource(arg1, arg2, arg3); +) + ... when != id +- e1 = devm_ioremap_resource(arg4, id); ++ e1 = devm_platform_ioremap_resource(arg1, arg3); + ... when != id +? id = e2 + +@r1 depends on patch && !report@ +identifier r.id; +type T; +@@ + +- T *id; + ...when != id + +// ---------------------------------------------------------------------------- + +@r2 depends on report && !patch@ +identifier id; +expression e1, e2, arg1, arg2, arg3, arg4; +position j0; +@@ + +( + id = platform_get_resource(arg1, arg2, arg3); +| + struct resource *id = platform_get_resource(arg1, arg2, arg3); +) + ... when != id + e1@j0 = devm_ioremap_resource(arg4, id); + ... when != id +? id = e2 + +// ---------------------------------------------------------------------------- + +@script:python depends on report && !patch@ +e1 << r2.e1; +j0 << r2.j0; +@@ + +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) +coccilib.report.print_report(j0[0], msg) -- 2.17.1 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sat, 6 Apr 2019, Himanshu Jha wrote: > Use recently introduced devm_platform_ioremap_resource > helper which wraps platform_get_resource() and > devm_ioremap_resource() together. This helps produce much > cleaner code while removing local `struct resource` declaration. > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> Acked-by: Julia Lawall <julia.lawall@lip6.fr> Thanks for taking up this issue. julia > --- > > Tree wide changes has been tested through 0-day test service > with build success. > > BUILD SUCCESS 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 > tree/branch: https://github.com/himanshujha199640/linux-next 20190401-devm_platform_ioremap_resource-final > branch HEAD: 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 Coccinelle: api: Add devm_platform_ioremap_resource.cocci > > elapsed time: 385m > configs tested: 162 > > > Stats: > 916 files changed, 1028 insertions(+), 2921 deletions(-) > > Note: cases where the `struct resource *res` variable is > used subsequently in the function have been ignored out because > those cases produce: > > eg., drivers/bus/da8xx-mstpri.c > > warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized] > > due to: > if (prio_descr->reg + sizeof(u32) > resource_size(res)) { > > which seems correct as `res` isn't initialized in the scope of > the function(da8xx_mstpri_probe) and instead initialized inside: > > void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > unsigned int index) > { > struct resource *res; > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > return devm_ioremap_resource(&pdev->dev, res); > } > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > .../api/devm_platform_ioremap_resource.cocci | 63 +++++++++++++++++++ > 1 file changed, 63 insertions(+) > create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > new file mode 100644 > index 000000000000..a28274af14df > --- /dev/null > +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > @@ -0,0 +1,63 @@ > +/// Use devm_platform_ioremap_resource helper which wraps > +/// platform_get_resource() and devm_ioremap_resource() together. > +/// > +// Confidence: High > +// Copyright: (C) 2019 Himanshu Jha GPLv2. > +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. > +// Keywords: platform_get_resource, devm_ioremap_resource, > +// Keywords: devm_platform_ioremap_resource > + > +virtual patch > +virtual report > + > +@r depends on patch && !report@ > +expression e1, e2, arg1, arg2, arg3, arg4; > +identifier id; > +@@ > + > +( > +- id = platform_get_resource(arg1, arg2, arg3); > +| > +- struct resource *id = platform_get_resource(arg1, arg2, arg3); > +) > + ... when != id > +- e1 = devm_ioremap_resource(arg4, id); > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); > + ... when != id > +? id = e2 > + > +@r1 depends on patch && !report@ > +identifier r.id; > +type T; > +@@ > + > +- T *id; > + ...when != id > + > +// ---------------------------------------------------------------------------- > + > +@r2 depends on report && !patch@ > +identifier id; > +expression e1, e2, arg1, arg2, arg3, arg4; > +position j0; > +@@ > + > +( > + id = platform_get_resource(arg1, arg2, arg3); > +| > + struct resource *id = platform_get_resource(arg1, arg2, arg3); > +) > + ... when != id > + e1@j0 = devm_ioremap_resource(arg4, id); > + ... when != id > +? id = e2 > + > +// ---------------------------------------------------------------------------- > + > +@script:python depends on report && !patch@ > +e1 << r2.e1; > +j0 << r2.j0; > +@@ > + > +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) > +coccilib.report.print_report(j0[0], msg) > -- > 2.17.1 > > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sat, 6 Apr 2019, Julia Lawall wrote: > > > On Sat, 6 Apr 2019, Himanshu Jha wrote: > > > Use recently introduced devm_platform_ioremap_resource > > helper which wraps platform_get_resource() and > > devm_ioremap_resource() together. This helps produce much > > cleaner code while removing local `struct resource` declaration. > > > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > Thanks for taking up this issue. Maybe this should be Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> since I contributed two lines to the script :) julia > > julia > > > --- > > > > Tree wide changes has been tested through 0-day test service > > with build success. > > > > BUILD SUCCESS 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 > > tree/branch: https://github.com/himanshujha199640/linux-next 20190401-devm_platform_ioremap_resource-final > > branch HEAD: 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 Coccinelle: api: Add devm_platform_ioremap_resource.cocci > > > > elapsed time: 385m > > configs tested: 162 > > > > > > Stats: > > 916 files changed, 1028 insertions(+), 2921 deletions(-) > > > > Note: cases where the `struct resource *res` variable is > > used subsequently in the function have been ignored out because > > those cases produce: > > > > eg., drivers/bus/da8xx-mstpri.c > > > > warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized] > > > > due to: > > if (prio_descr->reg + sizeof(u32) > resource_size(res)) { > > > > which seems correct as `res` isn't initialized in the scope of > > the function(da8xx_mstpri_probe) and instead initialized inside: > > > > void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > > unsigned int index) > > { > > struct resource *res; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > > return devm_ioremap_resource(&pdev->dev, res); > > } > > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > > > > .../api/devm_platform_ioremap_resource.cocci | 63 +++++++++++++++++++ > > 1 file changed, 63 insertions(+) > > create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > > diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > new file mode 100644 > > index 000000000000..a28274af14df > > --- /dev/null > > +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > @@ -0,0 +1,63 @@ > > +/// Use devm_platform_ioremap_resource helper which wraps > > +/// platform_get_resource() and devm_ioremap_resource() together. > > +/// > > +// Confidence: High > > +// Copyright: (C) 2019 Himanshu Jha GPLv2. > > +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. > > +// Keywords: platform_get_resource, devm_ioremap_resource, > > +// Keywords: devm_platform_ioremap_resource > > + > > +virtual patch > > +virtual report > > + > > +@r depends on patch && !report@ > > +expression e1, e2, arg1, arg2, arg3, arg4; > > +identifier id; > > +@@ > > + > > +( > > +- id = platform_get_resource(arg1, arg2, arg3); > > +| > > +- struct resource *id = platform_get_resource(arg1, arg2, arg3); > > +) > > + ... when != id > > +- e1 = devm_ioremap_resource(arg4, id); > > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); > > + ... when != id > > +? id = e2 > > + > > +@r1 depends on patch && !report@ > > +identifier r.id; > > +type T; > > +@@ > > + > > +- T *id; > > + ...when != id > > + > > +// ---------------------------------------------------------------------------- > > + > > +@r2 depends on report && !patch@ > > +identifier id; > > +expression e1, e2, arg1, arg2, arg3, arg4; > > +position j0; > > +@@ > > + > > +( > > + id = platform_get_resource(arg1, arg2, arg3); > > +| > > + struct resource *id = platform_get_resource(arg1, arg2, arg3); > > +) > > + ... when != id > > + e1@j0 = devm_ioremap_resource(arg4, id); > > + ... when != id > > +? id = e2 > > + > > +// ---------------------------------------------------------------------------- > > + > > +@script:python depends on report && !patch@ > > +e1 << r2.e1; > > +j0 << r2.j0; > > +@@ > > + > > +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) > > +coccilib.report.print_report(j0[0], msg) > > -- > > 2.17.1 > > > > > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +// Copyright: (C) 2019 Himanshu Jha GPLv2. How do you think about to use a SPDX identifier? > +// ---… I would prefer a SmPL script without such comment lines as delimiters here. > +position j0; Would the variable name “p” be nicer? > +@script:python depends on report && !patch@ > +e1 << r2.e1; > +j0 << r2.j0; > +@@ > + > +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) > +coccilib.report.print_report(j0[0], msg) I suggest to print such a message without the extra variable “msg” because the string format expression can be passed directly. Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +- e1 = devm_ioremap_resource(arg4, id); > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); Can the following specification variant matter for the shown SmPL change approach? + e1 = +- devm_ioremap_resource(arg4, id ++ devm_platform_ioremap_resource(arg1, arg3 + ); Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sat, 8 Jun 2019, Markus Elfring wrote: > > +- e1 = devm_ioremap_resource(arg4, id); > > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); > > Can the following specification variant matter for the shown SmPL > change approach? > > + e1 = > +- devm_ioremap_resource(arg4, id > ++ devm_platform_ioremap_resource(arg1, arg3 > + ); In the latter case, the original formatting of e1 will be preserved. But there is not usually any interesting formatting on the left side of an assignment (ie typically no newlines or comments). I can see no purpose to factorizing the right parenthesis. julia _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>>> +- e1 = devm_ioremap_resource(arg4, id); >>> ++ e1 = devm_platform_ioremap_resource(arg1, arg3); >> >> Can the following specification variant matter for the shown SmPL >> change approach? >> >> + e1 = >> +- devm_ioremap_resource(arg4, id >> ++ devm_platform_ioremap_resource(arg1, arg3 >> + ); > > In the latter case, the original formatting of e1 will be preserved. I would like to point the possibility out to express only required changes also by SmPL specifications. > But there is not usually any interesting formatting on the left side of an > assignment (ie typically no newlines or comments). Is there any need to trigger additional source code reformatting? > I can see no purpose to factorizing the right parenthesis. These characters at the end of such a function call should be kept unchanged. I got another software development concern according to the discussed software update “drivers: provide devm_platform_ioremap_resource()” (from 2019-02-21). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/platform.c?id=7945f929f1a77a1c8887a97ca07f87626858ff42 The flag “IORESOURCE_MEM” is passed as the second parameter for the call of the function “platform_get_resource” in this refactoring. Should this detail be specified also in the proposed script for the semantic patch language instead of using the metavariable “arg2” in SmPL disjunctions? How do you think about to delete error detection and corresponding exception handling code for the previous function call? Is the SmPL code specification “when != id” really sufficient for the exclusion of variable reassignments here? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On 09.06.19 10:55, Markus Elfring wrote: <snip> >> But there is not usually any interesting formatting on the left side of an >> assignment (ie typically no newlines or comments). > > Is there any need to trigger additional source code reformatting? > >> I can see no purpose to factorizing the right parenthesis. > > These characters at the end of such a function call should be kept unchanged. Agreed. OTOH, we all know that spatch results still need to be carefully checked. I suspect trying to teach it all the formatting rules of the kernel isn't an easy task. > The flag “IORESOURCE_MEM” is passed as the second parameter for the call > of the function “platform_get_resource” in this refactoring. In that particular case, we maybe should consider separate inline helpers instead of passing this is a parameter. Maybe it would even be more efficient to have completely separate versions of devm_platform_ioremap_resource(), so we don't even have to pass that parameter on stack. --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 1213 bytes --] On Tue, 11 Jun 2019, Enrico Weigelt, metux IT consult wrote: > On 09.06.19 10:55, Markus Elfring wrote: > > <snip> > > >> But there is not usually any interesting formatting on the left side of an > >> assignment (ie typically no newlines or comments). > > > > Is there any need to trigger additional source code reformatting? > > > >> I can see no purpose to factorizing the right parenthesis. > > > > These characters at the end of such a function call should be kept unchanged. > > Agreed. OTOH, we all know that spatch results still need to be carefully > checked. I suspect trying to teach it all the formatting rules of the > kernel isn't an easy task. > > > The flag “IORESOURCE_MEM” is passed as the second parameter for the call > > of the function “platform_get_resource” in this refactoring. > > In that particular case, we maybe should consider separate inline > helpers instead of passing this is a parameter. > > Maybe it would even be more efficient to have completely separate > versions of devm_platform_ioremap_resource(), so we don't even have > to pass that parameter on stack. I'm lost as to why this discussion suddenly appeared. What problem is actually being discussed? julia [-- Attachment #2: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> The flag “IORESOURCE_MEM” is passed as the second parameter for the call >> of the function “platform_get_resource” in this refactoring. > > In that particular case, we maybe should consider separate inline > helpers instead of passing this is a parameter. > > Maybe it would even be more efficient to have completely separate > versions of devm_platform_ioremap_resource(), so we don't even have > to pass that parameter on stack. Would you like to add another function which should be called instead of the combination of the functions “platform_get_resource” and “devm_ioremap_resource”? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
From: Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 14 Jun 2019 11:05:33 +0200 Two function calls were combined in this function implementation. Inline corresponding code so that extra error checks can be avoided here. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/base/platform.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4d1729853d1a..baadca72f949 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -80,8 +80,8 @@ struct resource *platform_get_resource(struct platform_device *dev, EXPORT_SYMBOL_GPL(platform_get_resource); /** - * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform - * device + * devm_platform_ioremap_resource + * Achieve devm_ioremap_resource() functionality for a platform device * * @pdev: platform device to use both for memory resource lookup as well as * resource management @@ -91,10 +91,39 @@ EXPORT_SYMBOL_GPL(platform_get_resource); void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, unsigned int index) { - struct resource *res; + u32 i; - res = platform_get_resource(pdev, IORESOURCE_MEM, index); - return devm_ioremap_resource(&pdev->dev, res); + for (i = 0; i < pdev->num_resources; i++) { + struct resource *res = &pdev->resource[i]; + + if (resource_type(res) == IORESOURCE_MEM && index-- == 0) { + struct device *dev = &pdev->dev; + resource_size_t size = resource_size(res); + void __iomem *dest; + + if (!devm_request_mem_region(dev, + res->start, + size, + dev_name(dev))) { + dev_err(dev, + "can't request region for resource %pR\n", + res); + return IOMEM_ERR_PTR(-EBUSY); + } + + dest = devm_ioremap(dev, res->start, size); + if (!dest) { + dev_err(dev, + "ioremap failed for resource %pR\n", + res); + devm_release_mem_region(dev, res->start, size); + dest = IOMEM_ERR_PTR(-ENOMEM); + } + + return dest; + } + } + return IOMEM_ERR_PTR(-EINVAL); } EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); #endif /* CONFIG_HAS_IOMEM */ -- 2.22.0 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Fri, 14 Jun 2019, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 14 Jun 2019 11:05:33 +0200 > > Two function calls were combined in this function implementation. > Inline corresponding code so that extra error checks can be avoided here. I don't see any point to this at all. By inlining the code, you have created a clone, which will introduce extra work to maintain in the future. julia > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/base/platform.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 4d1729853d1a..baadca72f949 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -80,8 +80,8 @@ struct resource *platform_get_resource(struct platform_device *dev, > EXPORT_SYMBOL_GPL(platform_get_resource); > > /** > - * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform > - * device > + * devm_platform_ioremap_resource > + * Achieve devm_ioremap_resource() functionality for a platform device > * > * @pdev: platform device to use both for memory resource lookup as well as > * resource management > @@ -91,10 +91,39 @@ EXPORT_SYMBOL_GPL(platform_get_resource); > void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > unsigned int index) > { > - struct resource *res; > + u32 i; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, index); > - return devm_ioremap_resource(&pdev->dev, res); > + for (i = 0; i < pdev->num_resources; i++) { > + struct resource *res = &pdev->resource[i]; > + > + if (resource_type(res) == IORESOURCE_MEM && index-- == 0) { > + struct device *dev = &pdev->dev; > + resource_size_t size = resource_size(res); > + void __iomem *dest; > + > + if (!devm_request_mem_region(dev, > + res->start, > + size, > + dev_name(dev))) { > + dev_err(dev, > + "can't request region for resource %pR\n", > + res); > + return IOMEM_ERR_PTR(-EBUSY); > + } > + > + dest = devm_ioremap(dev, res->start, size); > + if (!dest) { > + dev_err(dev, > + "ioremap failed for resource %pR\n", > + res); > + devm_release_mem_region(dev, res->start, size); > + dest = IOMEM_ERR_PTR(-ENOMEM); > + } > + > + return dest; > + } > + } > + return IOMEM_ERR_PTR(-EINVAL); > } > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > #endif /* CONFIG_HAS_IOMEM */ > -- > 2.22.0 > > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Fri, Jun 14, 2019 at 11:22:40AM +0200, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 14 Jun 2019 11:05:33 +0200 > > Two function calls were combined in this function implementation. > Inline corresponding code so that extra error checks can be avoided here. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/base/platform.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) Hey, looks like you timed out from my kill-file and this snuck through somehow. Let me go add you again to it, so I'm not bothered by pointless stuff like this anymore. *plonk* _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> Two function calls were combined in this function implementation. >> Inline corresponding code so that extra error checks can be avoided here. > > I don't see any point to this at all. Would you like to take another look at corresponding design options? How do you think about to check run time characteristics any more? > By inlining the code, you have created a clone, > which will introduce extra work to maintain in the future. Would you find the shown software transformation acceptable if a C compiler will be able to generate a similar code structure? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On 14.06.19 11:22, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 14 Jun 2019 11:05:33 +0200 > > Two function calls were combined in this function implementation. > Inline corresponding code so that extra error checks can be avoided here. What exactly is the purpose of this ? Looks like a notable code duplication ... I thought we usually try to reduce this, instead of introducing new ones. --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> Two function calls were combined in this function implementation. >> Inline corresponding code so that extra error checks can be avoided here. > > What exactly is the purpose of this ? I suggest to take another look at the need and relevance of involved error checks in the discussed function combination. > Looks like a notable code duplication ... This can be. > I thought we usually try to reduce this, instead of introducing new ones. Would you like to check the software circumstances once more for the generation of a similar code structure by a C compiler (or optimiser)? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On 18.06.19 07:37, Markus Elfring wrote: >>> Two function calls were combined in this function implementation. >>> Inline corresponding code so that extra error checks can be avoided here. >> >> What exactly is the purpose of this ? > > I suggest to take another look at the need and relevance of involved > error checks in the discussed function combination. Sorry, don't have the time for guessing and trying to reproduce your thoughts. That's why we have patch descriptions / commit messages. It would be a lot easier for all of us if you just desribe the exact problem you'd like to solve and your approach to do so. >> Looks like a notable code duplication ... > > This can be. I doubt that code duplication is appreciated, as this increases the maintenance overhead. (actually, we're usually trying to reduce that, eg. by using lots of generic helpers). >> I thought we usually try to reduce this, instead of introducing new ones. > > Would you like to check the software circumstances once more > for the generation of a similar code structure by a C compiler > (or optimiser)? As said: unfortunately, I don't have the time to do that - you'd have to tell us, what exactly you've got in mind. If it's just about some error checks which happen to be redundant in a particular case, you'll have to show that this case is a *really* hot path (eg. irq, syscall, scheduling, etc) - but I don't see that here. What's the exact scenario you're trying to optimize ? Any actual measurements on how your patch improves that ? Look, I understand that you'd like to squeeze out maximum performance, but this has to be practically maintainable. I could list a lot of things that I don't need in particular use cases and would like to introduce build knobs for, but I have to understand that maintainers have to be pretty reluctant towards those things. --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Greg already commented on this thread. No need to discuss it further. regards, dan carpenter _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> Would you like to check the software circumstances once more >> for the generation of a similar code structure by a C compiler >> (or optimiser)? > > As said: unfortunately, I don't have the time to do that I became curious if you would like to adjust your software development attention a bit more also in this area. > - you'd have to tell us, what exactly you've got in mind. I try to point possibilities out to improve the combination of two functions. > If it's just about some error checks which happen to be redundant in a > particular case, you'll have to show that this case is a *really* hot > path (eg. irq, syscall, scheduling, etc) - but I don't see that here. 1. May the check “resource_type(res) == IORESOURCE_MEM” be performed in a local loop? 2. How hot do you find the null pointer check for the device input parameter of the function “devm_ioremap_resource”? > Any actual measurements on how your patch improves that ? Not yet. - Which benchmarks would you trust here? > Look, I understand that you'd like to squeeze out maximum performance, I hope so. > but this has to be practically maintainable. This can be achieved if more contributors would find proposed adjustments helpful for another software transformation. Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sat, Apr 6, 2019 at 3:34 PM Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > On Sat, 6 Apr 2019, Julia Lawall wrote: > > > > > > > On Sat, 6 Apr 2019, Himanshu Jha wrote: > > > > > Use recently introduced devm_platform_ioremap_resource > > > helper which wraps platform_get_resource() and > > > devm_ioremap_resource() together. This helps produce much > > > cleaner code while removing local `struct resource` declaration. > > > > > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > > > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > > > Thanks for taking up this issue. > > Maybe this should be > > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> > > since I contributed two lines to the script :) I will apply with Julia's Signed-off-by instead of Acked-by. I will also add SPDX tag. Is this OK? > julia > > > > > julia > > > > > --- > > > > > > Tree wide changes has been tested through 0-day test service > > > with build success. > > > > > > BUILD SUCCESS 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 > > > tree/branch: https://github.com/himanshujha199640/linux-next 20190401-devm_platform_ioremap_resource-final > > > branch HEAD: 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 Coccinelle: api: Add devm_platform_ioremap_resource.cocci > > > > > > elapsed time: 385m > > > configs tested: 162 > > > > > > > > > Stats: > > > 916 files changed, 1028 insertions(+), 2921 deletions(-) > > > > > > Note: cases where the `struct resource *res` variable is > > > used subsequently in the function have been ignored out because > > > those cases produce: > > > > > > eg., drivers/bus/da8xx-mstpri.c > > > > > > warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized] > > > > > > due to: > > > if (prio_descr->reg + sizeof(u32) > resource_size(res)) { > > > > > > which seems correct as `res` isn't initialized in the scope of > > > the function(da8xx_mstpri_probe) and instead initialized inside: > > > > > > void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > > > unsigned int index) > > > { > > > struct resource *res; > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > > > return devm_ioremap_resource(&pdev->dev, res); > > > } > > > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > > > > > > > .../api/devm_platform_ioremap_resource.cocci | 63 +++++++++++++++++++ > > > 1 file changed, 63 insertions(+) > > > create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > > > > diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > new file mode 100644 > > > index 000000000000..a28274af14df > > > --- /dev/null > > > +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > @@ -0,0 +1,63 @@ > > > +/// Use devm_platform_ioremap_resource helper which wraps > > > +/// platform_get_resource() and devm_ioremap_resource() together. > > > +/// > > > +// Confidence: High > > > +// Copyright: (C) 2019 Himanshu Jha GPLv2. > > > +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. > > > +// Keywords: platform_get_resource, devm_ioremap_resource, > > > +// Keywords: devm_platform_ioremap_resource > > > + > > > +virtual patch > > > +virtual report > > > + > > > +@r depends on patch && !report@ > > > +expression e1, e2, arg1, arg2, arg3, arg4; > > > +identifier id; > > > +@@ > > > + > > > +( > > > +- id = platform_get_resource(arg1, arg2, arg3); > > > +| > > > +- struct resource *id = platform_get_resource(arg1, arg2, arg3); > > > +) > > > + ... when != id > > > +- e1 = devm_ioremap_resource(arg4, id); > > > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); > > > + ... when != id > > > +? id = e2 > > > + > > > +@r1 depends on patch && !report@ > > > +identifier r.id; > > > +type T; > > > +@@ > > > + > > > +- T *id; > > > + ...when != id > > > + > > > +// ---------------------------------------------------------------------------- > > > + > > > +@r2 depends on report && !patch@ > > > +identifier id; > > > +expression e1, e2, arg1, arg2, arg3, arg4; > > > +position j0; > > > +@@ > > > + > > > +( > > > + id = platform_get_resource(arg1, arg2, arg3); > > > +| > > > + struct resource *id = platform_get_resource(arg1, arg2, arg3); > > > +) > > > + ... when != id > > > + e1@j0 = devm_ioremap_resource(arg4, id); > > > + ... when != id > > > +? id = e2 > > > + > > > +// ---------------------------------------------------------------------------- > > > + > > > +@script:python depends on report && !patch@ > > > +e1 << r2.e1; > > > +j0 << r2.j0; > > > +@@ > > > + > > > +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) > > > +coccilib.report.print_report(j0[0], msg) > > > -- > > > 2.17.1 > > > > > > > > > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci -- Best Regards Masahiro Yamada _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sat, 6 Jul 2019, Masahiro Yamada wrote: > On Sat, Apr 6, 2019 at 3:34 PM Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > > On Sat, 6 Apr 2019, Julia Lawall wrote: > > > > > > > > > > > On Sat, 6 Apr 2019, Himanshu Jha wrote: > > > > > > > Use recently introduced devm_platform_ioremap_resource > > > > helper which wraps platform_get_resource() and > > > > devm_ioremap_resource() together. This helps produce much > > > > cleaner code while removing local `struct resource` declaration. > > > > > > > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com> > > > > > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > > > > > Thanks for taking up this issue. > > > > Maybe this should be > > > > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> > > > > since I contributed two lines to the script :) > > I will apply with Julia's Signed-off-by instead of Acked-by. > I will also add SPDX tag. > > Is this OK? Yes, thanks. julia > > > > > julia > > > > > > > > julia > > > > > > > --- > > > > > > > > Tree wide changes has been tested through 0-day test service > > > > with build success. > > > > > > > > BUILD SUCCESS 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 > > > > tree/branch: https://github.com/himanshujha199640/linux-next 20190401-devm_platform_ioremap_resource-final > > > > branch HEAD: 74ebaaca5d14d3d9b03e911f0b4995b78a4d60f0 Coccinelle: api: Add devm_platform_ioremap_resource.cocci > > > > > > > > elapsed time: 385m > > > > configs tested: 162 > > > > > > > > > > > > Stats: > > > > 916 files changed, 1028 insertions(+), 2921 deletions(-) > > > > > > > > Note: cases where the `struct resource *res` variable is > > > > used subsequently in the function have been ignored out because > > > > those cases produce: > > > > > > > > eg., drivers/bus/da8xx-mstpri.c > > > > > > > > warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized] > > > > > > > > due to: > > > > if (prio_descr->reg + sizeof(u32) > resource_size(res)) { > > > > > > > > which seems correct as `res` isn't initialized in the scope of > > > > the function(da8xx_mstpri_probe) and instead initialized inside: > > > > > > > > void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, > > > > unsigned int index) > > > > { > > > > struct resource *res; > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > > > > return devm_ioremap_resource(&pdev->dev, res); > > > > } > > > > EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); > > > > > > > > > > > > .../api/devm_platform_ioremap_resource.cocci | 63 +++++++++++++++++++ > > > > 1 file changed, 63 insertions(+) > > > > create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > > > > > > diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > > new file mode 100644 > > > > index 000000000000..a28274af14df > > > > --- /dev/null > > > > +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > > > > @@ -0,0 +1,63 @@ > > > > +/// Use devm_platform_ioremap_resource helper which wraps > > > > +/// platform_get_resource() and devm_ioremap_resource() together. > > > > +/// > > > > +// Confidence: High > > > > +// Copyright: (C) 2019 Himanshu Jha GPLv2. > > > > +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. > > > > +// Keywords: platform_get_resource, devm_ioremap_resource, > > > > +// Keywords: devm_platform_ioremap_resource > > > > + > > > > +virtual patch > > > > +virtual report > > > > + > > > > +@r depends on patch && !report@ > > > > +expression e1, e2, arg1, arg2, arg3, arg4; > > > > +identifier id; > > > > +@@ > > > > + > > > > +( > > > > +- id = platform_get_resource(arg1, arg2, arg3); > > > > +| > > > > +- struct resource *id = platform_get_resource(arg1, arg2, arg3); > > > > +) > > > > + ... when != id > > > > +- e1 = devm_ioremap_resource(arg4, id); > > > > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); > > > > + ... when != id > > > > +? id = e2 > > > > + > > > > +@r1 depends on patch && !report@ > > > > +identifier r.id; > > > > +type T; > > > > +@@ > > > > + > > > > +- T *id; > > > > + ...when != id > > > > + > > > > +// ---------------------------------------------------------------------------- > > > > + > > > > +@r2 depends on report && !patch@ > > > > +identifier id; > > > > +expression e1, e2, arg1, arg2, arg3, arg4; > > > > +position j0; > > > > +@@ > > > > + > > > > +( > > > > + id = platform_get_resource(arg1, arg2, arg3); > > > > +| > > > > + struct resource *id = platform_get_resource(arg1, arg2, arg3); > > > > +) > > > > + ... when != id > > > > + e1@j0 = devm_ioremap_resource(arg4, id); > > > > + ... when != id > > > > +? id = e2 > > > > + > > > > +// ---------------------------------------------------------------------------- > > > > + > > > > +@script:python depends on report && !patch@ > > > > +e1 << r2.e1; > > > > +j0 << r2.j0; > > > > +@@ > > > > + > > > > +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) > > > > +coccilib.report.print_report(j0[0], msg) > > > > -- > > > > 2.17.1 > > > > > > > > > > > > > _______________________________________________ > > Cocci mailing list > > Cocci@systeme.lip6.fr > > https://systeme.lip6.fr/mailman/listinfo/cocci > > > > -- > Best Regards > Masahiro Yamada > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> I will apply with Julia's Signed-off-by instead of Acked-by. >> I will also add SPDX tag. >> >> Is this OK? > > Yes, thanks. Will the clarification for following implementation details get any more software development attention? https://systeme.lip6.fr/pipermail/cocci/2019-June/005975.html https://lore.kernel.org/lkml/7b4fe770-dadd-80ba-2ba4-0f2bc90984ef@web.de/ * The flag “IORESOURCE_MEM” * Exclusion of variable assignments by SmPL when constraints Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
On Sun, Jul 7, 2019 at 6:56 PM Markus Elfring <Markus.Elfring@web.de> wrote: > > >> I will apply with Julia's Signed-off-by instead of Acked-by. > > >> I will also add SPDX tag. > > >> > > >> Is this OK? > > > > > > Yes, thanks. > > > Will the clarification for following implementation details get any more > software development attention? > https://systeme.lip6.fr/pipermail/cocci/2019-June/005975.html > https://lore.kernel.org/lkml/7b4fe770-dadd-80ba-2ba4-0f2bc90984ef@web.de/ > > * The flag “IORESOURCE_MEM” > > * Exclusion of variable assignments by SmPL when constraints OK, for this refactoring to happen, the second argument should be IORESOURCE_MEM instead of generic 'arg2'. Himanshu, Will you send v2? -- Best Regards Masahiro Yamada _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> OK, for this refactoring to happen, > the second argument should be IORESOURCE_MEM > instead of generic 'arg2'. A corresponding adjustment was committed on 2019-07-17. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci?id=d09778d16e20bc4f1f4971cc9a9fd7ff6ba898ff I constructed another variant of a script for the semantic patch language based on this contribution. I tried out to delete a bit of exception handling code after a call of the function “platform_get_resource”. Yesterday I sent a selection of patches from this transformation approach. (An analysis based on “Linux next-20190916” pointed 56 source files as update candidates out.) Will it be useful to integrate such a case distinction into the SmPL script directory? The analysis based on the committed SmPL script pointed 657 source files as update candidates out. So there are further opportunities for collateral software evolution. Can it become easier to check how many update suggestions are already in corresponding patch review queues? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci