* Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <5f9fdd59-4b0b-1cb5-c3a2-92efc5bb3841@web.de> @ 2020-09-07 12:00 ` Julia Lawall [not found] ` <477abcea-e008-e509-d03f-f2753ebdfb20@web.de> [not found] ` <4b505afc-ae48-d8eb-f4e1-8e3f7192f73d@web.de> 1 sibling, 1 reply; 6+ messages in thread From: Julia Lawall @ 2020-09-07 12:00 UTC (permalink / raw) To: Markus Elfring Cc: Coccinelle, Dejin Zheng, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML On Mon, 7 Sep 2020, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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 <elfring@users.sourceforge.net> > --- > ...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 000000000000..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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <477abcea-e008-e509-d03f-f2753ebdfb20@web.de>]
* Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <477abcea-e008-e509-d03f-f2753ebdfb20@web.de> @ 2020-09-07 13:11 ` Julia Lawall [not found] ` <46d314d5-822e-3d73-2d70-015794556e56@web.de> 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2020-09-07 13:11 UTC (permalink / raw) To: Markus Elfring Cc: Coccinelle, Dejin Zheng, Denis Efremov, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML [-- Attachment #1: Type: text/plain, Size: 3570 bytes --] 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <46d314d5-822e-3d73-2d70-015794556e56@web.de>]
* Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <46d314d5-822e-3d73-2d70-015794556e56@web.de> @ 2020-09-07 13:46 ` Julia Lawall [not found] ` <8aaa799f-0ec4-cfd0-854a-e1006d0d200a@web.de> 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2020-09-07 13:46 UTC (permalink / raw) To: Markus Elfring Cc: Coccinelle, Dejin Zheng, Denis Efremov, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML > >>>> +@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <8aaa799f-0ec4-cfd0-854a-e1006d0d200a@web.de>]
* Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <8aaa799f-0ec4-cfd0-854a-e1006d0d200a@web.de> @ 2020-09-07 14:18 ` Julia Lawall 0 siblings, 0 replies; 6+ messages in thread From: Julia Lawall @ 2020-09-07 14:18 UTC (permalink / raw) To: Markus Elfring Cc: Dejin Zheng, Coccinelle, Denis Efremov, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML [-- Attachment #1: Type: text/plain, Size: 2381 bytes --] 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <4b505afc-ae48-d8eb-f4e1-8e3f7192f73d@web.de>]
* Re: [Cocci] [PATCH v2] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <4b505afc-ae48-d8eb-f4e1-8e3f7192f73d@web.de> @ 2020-09-07 17:03 ` Julia Lawall [not found] ` <8fb7782c-538b-b657-af13-da71124e6afa@web.de> 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2020-09-07 17:03 UTC (permalink / raw) To: Markus Elfring Cc: Coccinelle, Dejin Zheng, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML On Mon, 7 Sep 2020, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 7 Sep 2020 18:38:04 +0200 > > Another wrapper function is available since the commit 890cc39a879906b63912482dfc41944579df2dc6 > ("drivers: provide devm_platform_get_and_ioremap_resource()"). > Provide safe design options for the adjustment of affected source code > by the means of the semantic patch language (Coccinelle software). > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > > v2: > Julia Lawall requested to omit case distinctions (disjunctions) from > the first SmPL script. > The usage of different expression metavariables for the first parameter > of function calls was too questionable for the proposed source > code transformation. > > > ...vm_platform_get_and_ioremap_resource.cocci | 48 +++++++++++++++++++ > 1 file changed, 48 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 000000000000..319583716ac8 > --- /dev/null > +++ b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci > @@ -0,0 +1,48 @@ > +// 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 Shouldn't there be some options? --no-includes and perhaps --include-headers would seem reasonable. Please also remove all of the rule names except for "or" which is necessary. The rest just add clutter and make it harder to find the useful information, which is the depends on. Rules only need names if some other rule inherits their metavariables. julia > + > +virtual context, patch, report, org > + > +@display depends on context@ > +expression base, device, index, resource; > +@@ > +*resource = platform_get_resource(device, IORESOURCE_MEM, index); > + base = > +* devm_ioremap_resource > + (&device->dev, resource); > + > +@replacement depends on patch@ > +expression base, device, index, resource; > +@@ > +-resource = platform_get_resource(device, IORESOURCE_MEM, index); > + base = > +- devm_ioremap_resource > ++ devm_platform_get_and_ioremap_resource > + ( > +- & > + device > +- ->dev > + , > +- resource > ++ index, &resource > + ); > + > +@or depends on org || report@ > +expression base, device, index, resource; > +position p; > +@@ > + resource = platform_get_resource(device, IORESOURCE_MEM, index); > + base = devm_ioremap_resource@p(&device->dev, resource); > + > +@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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <8fb7782c-538b-b657-af13-da71124e6afa@web.de>]
* Re: [PATCH v2] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” [not found] ` <8fb7782c-538b-b657-af13-da71124e6afa@web.de> @ 2020-09-07 17:31 ` Julia Lawall 0 siblings, 0 replies; 6+ messages in thread From: Julia Lawall @ 2020-09-07 17:31 UTC (permalink / raw) To: Markus Elfring Cc: Coccinelle, Dejin Zheng, Denis Efremov, Gilles Muller, Masahiro Yamada, Michal Marek, Nicolas Palix, kernel-janitors, LKML On Mon, 7 Sep 2020, Markus Elfring wrote: > >> +++ b/scripts/coccinelle/api/use_devm_platform_get_and_ioremap_resource.cocci > >> @@ -0,0 +1,48 @@ > >> +// 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 > > > > Shouldn't there be some options? --no-includes and perhaps > > --include-headers would seem reasonable. > > I imagine that the proposed source code transformation can work also without > additional semantic patch command options. It can. But it will be more efficient and more exhaustive if you include them. julia ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-07 17:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <25b804fd-0d04-475d-f614-26c03c9fd544@web.de>
[not found] ` <5f9fdd59-4b0b-1cb5-c3a2-92efc5bb3841@web.de>
2020-09-07 12:00 ` [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci” Julia Lawall
[not found] ` <477abcea-e008-e509-d03f-f2753ebdfb20@web.de>
2020-09-07 13:11 ` Julia Lawall
[not found] ` <46d314d5-822e-3d73-2d70-015794556e56@web.de>
2020-09-07 13:46 ` Julia Lawall
[not found] ` <8aaa799f-0ec4-cfd0-854a-e1006d0d200a@web.de>
2020-09-07 14:18 ` Julia Lawall
[not found] ` <4b505afc-ae48-d8eb-f4e1-8e3f7192f73d@web.de>
2020-09-07 17:03 ` [Cocci] [PATCH v2] " Julia Lawall
[not found] ` <8fb7782c-538b-b657-af13-da71124e6afa@web.de>
2020-09-07 17:31 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).