Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
On Mon, 7 Sep 2020, Markus Elfring wrote: > >> +@replacement depends on patch@ > >> +expression base, device1, device2, index, private, resource; > >> +@@ > >> +( > >> +-resource = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +- devm_ioremap_resource > >> ++ devm_platform_get_and_ioremap_resource > >> + ( > >> +- & > >> + device1 > >> +- ->dev > >> + , > >> +- resource > >> ++ index, &resource > >> + ); > >> +| > >> +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +- devm_ioremap_resource > >> ++ devm_platform_get_and_ioremap_resource > >> + (device2, > … > >> Both source code change patterns from these branches of a SmPL disjunction > >> work as I would expect it by the current Coccinelle software. > >> Would you like to clarify any remaining related open issues? > > > > In one case the first argument of devm_ioremap_resource has to be changed > > to produce the first argument of devm_platform_get_and_ioremap_resource > > and in the other case there is no such change. "work as I would expect" > > is not a proper explanation of why this is correct. > > Would like to achieve that a SmPL script (like the one which I present here) > can help to adjust the implementation of the functions “mvebu_sei_probe” > and “hi655x_pmic_probe” (for example) for the desired reuse of the wrapper > function “devm_platform_get_and_ioremap_resource”? > > > > Maybe you can point to some previous commits that have made the change in > > this way. I looked at some of the provided examples, but none of them leaves the first argument of devm_ioremap_resource unchanged. julia > > Not yet for these source code examples. > > Some patches were contributed. > https://lore.kernel.org/patchwork/project/lkml/list/?q=devm_platform_get_and_ioremap_resource&archive=both&state=* > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=grep&q=devm_platform_get_and_ioremap_resource > > Regards, > Markus >___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
> +@replacement depends on patch@ > +expression base, device1, device2, index, private, resource; > +@@ > +( > +-resource = platform_get_resource(device1, IORESOURCE_MEM, index); > + base = > +- devm_ioremap_resource > ++ devm_platform_get_and_ioremap_resource > + ( > +- & > + device1 > +- ->dev > + , > +- resource > ++ index, &resource > + ); > +| > +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index); > + base = > +- devm_ioremap_resource > ++ devm_platform_get_and_ioremap_resource > + (device2, > >>> > >>> It is very suspicious that in one case you change the first argument of > >>> devm_platform_get_and_ioremap_resource and in one case you don't. > >> > >> I noticed a few special cases during my source code analysis approach. > > > > This is not a reasonable answer. Does the rule work correctly or not? > > If it doesn't work correctly, it needs to be removed. > > Both source code change patterns from these branches of a SmPL disjunction > work as I would expect it by the current Coccinelle software. > Would you like to clarify any remaining related open issues? In one case the first argument of devm_ioremap_resource has to be changed to produce the first argument of devm_platform_get_and_ioremap_resource and in the other case there is no such change. "work as I would expect" is not a proper explanation of why this is correct. Maybe you can point to some previous commits that have made the change in this way. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
On Mon, 7 Sep 2020, Markus Elfring wrote: > >> +@display depends on context@ > >> +expression base, device1, device2, index, private, resource; > >> +@@ > >> +( > >> +*resource = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +* devm_ioremap_resource > >> + (&device1->dev, resource); > > > > Why do you require these statements to be next to each other? > > I would appreciate indications for a general change acceptance > also according to such a simple transformation approach. > I imagine that it will become more challenging to tolerate extra > source code between these function calls (by the specification > of special SmPL filters). > > > >> +| > >> +*private->res = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +* devm_ioremap_resource > >> + (device2, private->res); > > > > Why do you have this special case? > > The usage of the data structure member “res” triggers corresponding > software design consequences. I don't think this is a reliable rule. You included two examples below. They involve structures of different types. It seems unlikely that there is a guarantee that the res field of all structures is used in the same way. Furthermore, this is the context case. What is the purpose of making the distinction? The user will have to figure out what to do by hand in any case. > The expressions which are passed as the first function call parameters > can be different. > > > >> +@replacement depends on patch@ > >> +expression base, device1, device2, index, private, resource; > >> +@@ > >> +( > >> +-resource = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +- devm_ioremap_resource > >> ++ devm_platform_get_and_ioremap_resource > >> + ( > >> +- & > >> + device1 > >> +- ->dev > >> + , > >> +- resource > >> ++ index, &resource > >> + ); > >> +| > >> +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index); > >> + base = > >> +- devm_ioremap_resource > >> ++ devm_platform_get_and_ioremap_resource > >> + (device2, > > > > It is very suspicious that in one case you change the first argument of > > devm_platform_get_and_ioremap_resource and in one case you don't. > > I noticed a few special cases during my source code analysis approach. This is not a reasonable answer. Does the rule work correctly or not? If it doesn't work correctly, it needs to be removed. > > If you don't know how to make the change in some cases, it would be better > > to do nothing at all. > > How do you think about to take another look at any update candidates? > > Examples: > * mvebu_sei_probe > > https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/irqchip/irq-mvebu-sei.c#L368 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-mvebu-sei.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n368 I don't see any purpose to providing two links for everything. julia > > * hi655x_pmic_probe > > https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/mfd/hi655x-pmic.c#L92 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/hi655x-pmic.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n92 > > Regards, > Markus >___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
>> +@display depends on context@ >> +expression base, device1, device2, index, private, resource; >> +@@ >> +( >> +*resource = platform_get_resource(device1, IORESOURCE_MEM, index); >> + base = >> +* devm_ioremap_resource >> + (&device1->dev, resource); > > Why do you require these statements to be next to each other? I would appreciate indications for a general change acceptance also according to such a simple transformation approach. I imagine that it will become more challenging to tolerate extra source code between these function calls (by the specification of special SmPL filters). >> +| >> +*private->res = platform_get_resource(device1, IORESOURCE_MEM, index); >> + base = >> +* devm_ioremap_resource >> + (device2, private->res); > > Why do you have this special case? The usage of the data structure member “res” triggers corresponding software design consequences. The expressions which are passed as the first function call parameters can be different. >> +@replacement depends on patch@ >> +expression base, device1, device2, index, private, resource; >> +@@ >> +( >> +-resource = platform_get_resource(device1, IORESOURCE_MEM, index); >> + base = >> +- devm_ioremap_resource >> ++ devm_platform_get_and_ioremap_resource >> + ( >> +- & >> + device1 >> +- ->dev >> + , >> +- resource >> ++ index, &resource >> + ); >> +| >> +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index); >> + base = >> +- devm_ioremap_resource >> ++ devm_platform_get_and_ioremap_resource >> + (device2, > > It is very suspicious that in one case you change the first argument of > devm_platform_get_and_ioremap_resource and in one case you don't. I noticed a few special cases during my source code analysis approach. > If you don't know how to make the change in some cases, it would be better > to do nothing at all. How do you think about to take another look at any update candidates? Examples: * mvebu_sei_probe https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/irqchip/irq-mvebu-sei.c#L368 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-mvebu-sei.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n368 * hi655x_pmic_probe https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/mfd/hi655x-pmic.c#L92 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/hi655x-pmic.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n92 Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
On Mon, 7 Sep 2020, Markus Elfring wrote:
> From: Markus Elfring
> Date: Mon, 7 Sep 2020 13:14:44 +0200
>
> Another wrapper function is available since the commit
> 890cc39a879906b63912482dfc41944579df2dc6
> ("drivers: provide devm_platform_get_and_ioremap_resource()").
> Provide design options for the adjustment of affected source code
> by the means of the semantic patch language (Coccinelle software).
>
> Signed-off-by: Markus Elfring
> ---
> ...vm_platform_get_and_ioremap_resource.cocci | 71 +++
> 1 file changed, 71 insertions(+)
> create mode 100644
> scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
>
> diff --git
> a/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
> b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
> new file mode 100644
> index ..8e67359f6b76
> --- /dev/null
> +++ b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Simplify a function call combination by using a known wrapper function.
> +//
> +// Keywords: wrapper function conversion ioremap resources
> +// Confidence: High
> +
> +virtual context, patch, report, org
> +
> +@display depends on context@
> +expression base, device1, device2, index, private, resource;
> +@@
> +(
> +*resource = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base =
> +* devm_ioremap_resource
> + (&device1->dev, resource);
Why do you require these statements to be next to each other?
> +|
> +*private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base =
> +* devm_ioremap_resource
> + (device2, private->res);
Why do you have this special case?
> +)
> +
> +@replacement depends on patch@
> +expression base, device1, device2, index, private, resource;
> +@@
> +(
> +-resource = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base =
> +- devm_ioremap_resource
> ++ devm_platform_get_and_ioremap_resource
> + (
> +- &
> + device1
> +- ->dev
> + ,
> +- resource
> ++ index, &resource
> + );
> +|
> +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base =
> +- devm_ioremap_resource
> ++ devm_platform_get_and_ioremap_resource
> + (device2,
It is very suspicious that in one case you change the first argument of
devm_platform_get_and_ioremap_resource and in one case you don't. If you
don't know how to make the change in some cases, it would be better to do
nothing at all.
julia
> +- private->res
> ++ index, &private->res
> + );
> +)
> +
> +@or depends on org || report@
> +expression base, device1, device2, index, private, resource;
> +position p;
> +@@
> +(
> + resource = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base = devm_ioremap_resource@p(&device1->dev, resource);
> +|
> + private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
> + base = devm_ioremap_resource@p(device2, private->res);
> +)
> +
> +@script:python to_do depends on org@
> +p << or.p;
> +@@
> +coccilib.org.print_todo(p[0], "WARNING: opportunity for
> devm_platform_get_and_ioremap_resource()")
> +
> +@script:python reporting depends on report@
> +p << or.p;
> +@@
> +coccilib.report.print_report(p[0], "WARNING: opportunity for
> devm_platform_get_and_ioremap_resource()")
> --
> 2.28.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
[Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
From: Markus Elfring
Date: Mon, 7 Sep 2020 13:14:44 +0200
Another wrapper function is available since the commit
890cc39a879906b63912482dfc41944579df2dc6
("drivers: provide devm_platform_get_and_ioremap_resource()").
Provide design options for the adjustment of affected source code
by the means of the semantic patch language (Coccinelle software).
Signed-off-by: Markus Elfring
---
...vm_platform_get_and_ioremap_resource.cocci | 71 +++
1 file changed, 71 insertions(+)
create mode 100644
scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
diff --git
a/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
new file mode 100644
index ..8e67359f6b76
--- /dev/null
+++ b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Simplify a function call combination by using a known wrapper function.
+//
+// Keywords: wrapper function conversion ioremap resources
+// Confidence: High
+
+virtual context, patch, report, org
+
+@display depends on context@
+expression base, device1, device2, index, private, resource;
+@@
+(
+*resource = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base =
+* devm_ioremap_resource
+ (&device1->dev, resource);
+|
+*private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base =
+* devm_ioremap_resource
+ (device2, private->res);
+)
+
+@replacement depends on patch@
+expression base, device1, device2, index, private, resource;
+@@
+(
+-resource = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base =
+- devm_ioremap_resource
++ devm_platform_get_and_ioremap_resource
+ (
+- &
+ device1
+- ->dev
+ ,
+- resource
++ index, &resource
+ );
+|
+-private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base =
+- devm_ioremap_resource
++ devm_platform_get_and_ioremap_resource
+ (device2,
+- private->res
++ index, &private->res
+ );
+)
+
+@or depends on org || report@
+expression base, device1, device2, index, private, resource;
+position p;
+@@
+(
+ resource = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base = devm_ioremap_resource@p(&device1->dev, resource);
+|
+ private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
+ base = devm_ioremap_resource@p(device2, private->res);
+)
+
+@script:python to_do depends on org@
+p << or.p;
+@@
+coccilib.org.print_todo(p[0], "WARNING: opportunity for
devm_platform_get_and_ioremap_resource()")
+
+@script:python reporting depends on report@
+p << or.p;
+@@
+coccilib.report.print_report(p[0], "WARNING: opportunity for
devm_platform_get_and_ioremap_resource()")
--
2.28.0
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
