Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”

[Julia Lawall <julia.lawall@inria.fr>]

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