[PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Himanshu Jha]

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

Re: [PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Julia Lawall]

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

Re: [PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Julia Lawall]

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

Re: [PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

> +// 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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

> +- 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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Julia Lawall]

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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

>>> +- 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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Enrico Weigelt, metux IT consult]

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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Julia Lawall]

[-- 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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

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

[PATCH] drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Markus Elfring]

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

Re: [Cocci] [PATCH] drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Julia Lawall]

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
>

Re: [PATCH] drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Greg Kroah-Hartman]

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*

Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Markus Elfring]

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

Re: [PATCH] drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Enrico Weigelt, metux IT consult]

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

Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Markus Elfring]

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

Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Enrico Weigelt, metux IT consult]

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

Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Dan Carpenter]

Greg already commented on this thread.  No need to discuss it further.

regards,
dan carpenter

Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions

[Markus Elfring]

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

Re: [Cocci] [PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Masahiro Yamada]

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

Re: [Cocci] [PATCH] coccinelle: api: add devm_platform_ioremap_resource script

[Julia Lawall]

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
>

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Masahiro Yamada]

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

Re: Coccinelle: api: add devm_platform_ioremap_resource script

[Markus Elfring]

> 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