linux-kernel - Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2009071506350.2476@hadrien>
Date:   Mon, 7 Sep 2020 15:11:31 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Markus Elfring <Markus.Elfring@....de>
cc:     Coccinelle <cocci@...teme.lip6.fr>,
        Dejin Zheng <zhengdejin5@...il.com>,
        Denis Efremov <efremov@...ux.com>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”



On Mon, 7 Sep 2020, Markus Elfring wrote:

> >> +@...play 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.
>
>
> >> +@...lacement 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
>