[Cocci] changing of_get_mac_address() to pass a buffer

[Cocci] changing of_get_mac_address() to pass a buffer

[Michael Walle]

Hi,

so first I need to say I've never used coccinelle before,
so please bear with me ;)

To make of_get_mac_address() work with DSA ports (and a nvmem
provider) I'd need to change the semantics of of_get_mac_address().
Right now it returns a pointer to "const char *", I'd need to change
that so a buffer will be passed as a parameter in which the MAC
address gets stored.

(1) Usually the call is something like:

   const char *mac;
   mac = of_get_mac_address(np);
   if (!IS_ERR(mac))
     ether_addr_copy(ndev->dev_addr, mac);

This would need to be changed to:

   of_get_mac_address(np, ndev->dev_addr);

(2) Unfortunately, there are also other variants of this like. Eg.

   const char *mac;
   mac = of_get_mac_address(np);
   if (IS_ERR(mac))
     eth_hw_addr_random(ndev);
   else
     ether_addr_copy(ndev->dev_addr, mac);

This would need to be changed to:

   ret = of_get_mac_address(np, ndev->dev_addr);
   if (ret)
     eth_hw_addr_random(ndev);

(3) Of course there are more variants like "if .. else if .. else"
     and so on.

This is what I've come up for now:

<spml>
virtual patch

@depends on patch@
type T;
identifier mac;
expression node, dev_addr;
statement S;
statement list SL;
@@

- T mac;
<+...
- mac = of_get_mac_address(node);
(
+ of_get_mac_address(node, dev_addr);
- if (!IS_ERR(mac))
-   ether_addr_copy(dev_addr, mac);
|
+ ret_tbd = of_get_mac_address(node, dev_addr);
+ if (!ret_tdb) {
- if (!IS_ERR(mac)) {
     ... when != mac
-   ether_addr_copy(dev_addr, mac);
     ... when != mac
   }
|
+ ret_tbd = of_get_mac_address(node, dev_addr);
+ if (!ret_tdb) {
- if (!IS_ERR(mac)) {
     ... when != mac
-   ether_addr_copy(dev_addr, mac);
     ... when != mac
   } else {
     ... when != mac
   }
|
- if (IS_ERR(mac))
-   S
- else
-   ether_addr_copy(dev_addr, mac);
+ ret_tbd = of_get_mac_address(node, dev_addr);
+ if (ret_tbd) S
|
- if (IS_ERR(mac)) {
-   SL
- } else {
-   ether_addr_copy(dev_addr, mac);
- }
+ ret_tbd = of_get_mac_address(node, dev_addr);
+ if (ret_tbd) { SL }
)
...+>

@depends on patch@
type T;
identifier mac;
expression node, dev_addr;
statement S;
@@

- T mac = of_get_mac_address(node);
+ of_get_mac_address(node, dev_addr);
- if (!IS_ERR(mac))
-   ether_addr_copy(dev_addr, mac);
   ... when != mac
</spml>

Maybe someone could have a brief look at it. Maybe (well I'm sure)
there is something to improve here. There is some repeated code
here, esp. for the different "if" and  "if else" branches. Is there
a better way to tell coccinelle, that the else might be optional?
And I guess, I need to go over the (maybe) new ret_tbd manually.

Of course, this won't match all occurrences, I'll go over the
remaining ones manually.

Thanks,
-michael
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] changing of_get_mac_address() to pass a buffer

[Markus Elfring]

>    mac = of_get_mac_address(np);
>    if (!IS_ERR(mac))
>      ether_addr_copy(ndev->dev_addr, mac);
>
> This would need to be changed to:
>
>    of_get_mac_address(np, ndev->dev_addr);

Can such a function call variant fail?

How do you think about take any special return values into account?


> Maybe (well I'm sure) there is something to improve here.

I suggest once more to group another bit of SmPL code like “+ ret_tbd = …”
by using nested SmPL disjunctions so that duplicate specifications can be reduced.

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] changing of_get_mac_address() to pass a buffer

[Mansour Moufid]

On Thu, Apr 1, 2021 at 4:13 AM Michael Walle <michael@walle.cc> wrote:
>
> Hi,
>
> so first I need to say I've never used coccinelle before,
> so please bear with me ;)
>
> To make of_get_mac_address() work with DSA ports (and a nvmem
> provider) I'd need to change the semantics of of_get_mac_address().
> Right now it returns a pointer to "const char *", I'd need to change
> that so a buffer will be passed as a parameter in which the MAC
> address gets stored.
>
> (1) Usually the call is something like:
>
>    const char *mac;
>    mac = of_get_mac_address(np);
>    if (!IS_ERR(mac))
>      ether_addr_copy(ndev->dev_addr, mac);
>
> This would need to be changed to:
>
>    of_get_mac_address(np, ndev->dev_addr);

Here is one possible approach, doing the API change first then
handling the conditionals. It seems to work.

@a@
identifier x;
expression y, z;
@@
-   x = of_get_mac_address(y);
+   x = of_get_mac_address(y, z);
    <...
-   ether_addr_copy(z, x);
    ...>

@@
identifier a.x;
@@
-   if (<+... x ...+>) {}

@@
identifier a.x;
@@
    if (<+... x ...+>) {
        ...
    }
-   else {}

@@
identifier a.x;
expression e;
@@
-   if (<+... x ...+>@e)
-       {}
-   else
+   if (!(e))
        {...}

@@
expression x, y, z;
@@
-   x = of_get_mac_address(y, z);
+   of_get_mac_address(y, z);
    ... when != x
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] changing of_get_mac_address() to pass a buffer

[Julia Lawall]

On Sun, 4 Apr 2021, Mansour Moufid wrote:

> On Thu, Apr 1, 2021 at 4:13 AM Michael Walle <michael@walle.cc> wrote:
> >
> > Hi,
> >
> > so first I need to say I've never used coccinelle before,
> > so please bear with me ;)
> >
> > To make of_get_mac_address() work with DSA ports (and a nvmem
> > provider) I'd need to change the semantics of of_get_mac_address().
> > Right now it returns a pointer to "const char *", I'd need to change
> > that so a buffer will be passed as a parameter in which the MAC
> > address gets stored.
> >
> > (1) Usually the call is something like:
> >
> >    const char *mac;
> >    mac = of_get_mac_address(np);
> >    if (!IS_ERR(mac))
> >      ether_addr_copy(ndev->dev_addr, mac);
> >
> > This would need to be changed to:
> >
> >    of_get_mac_address(np, ndev->dev_addr);
>
> Here is one possible approach, doing the API change first then
> handling the conditionals. It seems to work.
>
> @a@
> identifier x;
> expression y, z;
> @@
> -   x = of_get_mac_address(y);
> +   x = of_get_mac_address(y, z);
>     <...
> -   ether_addr_copy(z, x);
>     ...>
>
> @@
> identifier a.x;
> @@
> -   if (<+... x ...+>) {}
>
> @@
> identifier a.x;
> @@
>     if (<+... x ...+>) {
>         ...
>     }
> -   else {}
>
> @@
> identifier a.x;
> expression e;
> @@
> -   if (<+... x ...+>@e)
> -       {}
> -   else
> +   if (!(e))
>         {...}
>
> @@
> expression x, y, z;
> @@
> -   x = of_get_mac_address(y, z);
> +   of_get_mac_address(y, z);
>     ... when != x

This seems like a good approach.  It would also be possible to have
special cases for when the removed call is in a {} by itself, but it seems
like a lot of trouble for little benefit.  Presumably the existing code
doesn't contain {}, so the only code that would be affected by the cleanup
rules would be the code that was introduced by the first rule.  Thanks for
th suggestion.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] changing of_get_mac_address() to pass a buffer

[Michael Walle]

Hi Mansour,

Am 2021-04-04 19:48, schrieb Mansour Moufid:
> On Thu, Apr 1, 2021 at 4:13 AM Michael Walle <michael@walle.cc> wrote:
>> 
>> Hi,
>> 
>> so first I need to say I've never used coccinelle before,
>> so please bear with me ;)
>> 
>> To make of_get_mac_address() work with DSA ports (and a nvmem
>> provider) I'd need to change the semantics of of_get_mac_address().
>> Right now it returns a pointer to "const char *", I'd need to change
>> that so a buffer will be passed as a parameter in which the MAC
>> address gets stored.
>> 
>> (1) Usually the call is something like:
>> 
>>    const char *mac;
>>    mac = of_get_mac_address(np);
>>    if (!IS_ERR(mac))
>>      ether_addr_copy(ndev->dev_addr, mac);
>> 
>> This would need to be changed to:
>> 
>>    of_get_mac_address(np, ndev->dev_addr);
> 
> Here is one possible approach, doing the API change first then
> handling the conditionals. It seems to work.
> 
> @a@
> identifier x;
> expression y, z;
> @@
> -   x = of_get_mac_address(y);
> +   x = of_get_mac_address(y, z);
>     <...
> -   ether_addr_copy(z, x);
>     ...>
> 
> @@
> identifier a.x;
> @@
> -   if (<+... x ...+>) {}
> 
> @@
> identifier a.x;
> @@
>     if (<+... x ...+>) {
>         ...
>     }
> -   else {}
> 
> @@
> identifier a.x;
> expression e;
> @@
> -   if (<+... x ...+>@e)
> -       {}
> -   else
> +   if (!(e))
>         {...}
> 
> @@
> expression x, y, z;
> @@
> -   x = of_get_mac_address(y, z);
> +   of_get_mac_address(y, z);
>     ... when != x

Thanks a lot!

See also 
https://lore.kernel.org/netdev/20210405164643.21130-1-michael@walle.cc/

There were some "if (!(!IS_ERR(x))", which I needed to simplify
manually. Didn't noticed that in my previous script. I'm just
curious, is that also something coccinelle can simplify on its
own?

-michael
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] changing of_get_mac_address() to pass a buffer

[Julia Lawall]

On Mon, 5 Apr 2021, Michael Walle wrote:

> Hi Mansour,
>
> Am 2021-04-04 19:48, schrieb Mansour Moufid:
> > On Thu, Apr 1, 2021 at 4:13 AM Michael Walle <michael@walle.cc> wrote:
> > >
> > > Hi,
> > >
> > > so first I need to say I've never used coccinelle before,
> > > so please bear with me ;)
> > >
> > > To make of_get_mac_address() work with DSA ports (and a nvmem
> > > provider) I'd need to change the semantics of of_get_mac_address().
> > > Right now it returns a pointer to "const char *", I'd need to change
> > > that so a buffer will be passed as a parameter in which the MAC
> > > address gets stored.
> > >
> > > (1) Usually the call is something like:
> > >
> > >    const char *mac;
> > >    mac = of_get_mac_address(np);
> > >    if (!IS_ERR(mac))
> > >      ether_addr_copy(ndev->dev_addr, mac);
> > >
> > > This would need to be changed to:
> > >
> > >    of_get_mac_address(np, ndev->dev_addr);
> >
> > Here is one possible approach, doing the API change first then
> > handling the conditionals. It seems to work.
> >
> > @a@
> > identifier x;
> > expression y, z;
> > @@
> > -   x = of_get_mac_address(y);
> > +   x = of_get_mac_address(y, z);
> >     <...
> > -   ether_addr_copy(z, x);
> >     ...>
> >
> > @@
> > identifier a.x;
> > @@
> > -   if (<+... x ...+>) {}
> >
> > @@
> > identifier a.x;
> > @@
> >     if (<+... x ...+>) {
> >         ...
> >     }
> > -   else {}
> >
> > @@
> > identifier a.x;
> > expression e;
> > @@
> > -   if (<+... x ...+>@e)
> > -       {}
> > -   else
> > +   if (!(e))

Maybe try the above line without the parentheses around e?


> >         {...}
> >
> > @@
> > expression x, y, z;
> > @@
> > -   x = of_get_mac_address(y, z);
> > +   of_get_mac_address(y, z);
> >     ... when != x
>
> Thanks a lot!
>
> See also
> https://lore.kernel.org/netdev/20210405164643.21130-1-michael@walle.cc/
>
> There were some "if (!(!IS_ERR(x))", which I needed to simplify
> manually. Didn't noticed that in my previous script. I'm just
> curious, is that also something coccinelle can simplify on its
> own?

You can certainly write another rule for it:

- !(!e)
+ e

julia


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