Coccinelle Archive on lore.kernel.org
 help / color / Atom feed
* [Cocci] changing of_get_mac_address() to pass a buffer
@ 2021-04-01  8:13 Michael Walle
  2021-04-01 17:06 ` Markus Elfring
  2021-04-04 17:48 ` Mansour Moufid
  0 siblings, 2 replies; 6+ messages in thread
From: Michael Walle @ 2021-04-01  8:13 UTC (permalink / raw)
  To: cocci

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cocci] changing of_get_mac_address() to pass a buffer
  2021-04-01  8:13 [Cocci] changing of_get_mac_address() to pass a buffer Michael Walle
@ 2021-04-01 17:06 ` Markus Elfring
  2021-04-04 17:48 ` Mansour Moufid
  1 sibling, 0 replies; 6+ messages in thread
From: Markus Elfring @ 2021-04-01 17:06 UTC (permalink / raw)
  To: Michael Walle; +Cc: cocci

>    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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cocci] changing of_get_mac_address() to pass a buffer
  2021-04-01  8:13 [Cocci] changing of_get_mac_address() to pass a buffer Michael Walle
  2021-04-01 17:06 ` Markus Elfring
@ 2021-04-04 17:48 ` Mansour Moufid
  2021-04-04 18:34   ` Julia Lawall
  2021-04-05 21:50   ` Michael Walle
  1 sibling, 2 replies; 6+ messages in thread
From: Mansour Moufid @ 2021-04-04 17:48 UTC (permalink / raw)
  To: Michael Walle; +Cc: cocci

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cocci] changing of_get_mac_address() to pass a buffer
  2021-04-04 17:48 ` Mansour Moufid
@ 2021-04-04 18:34   ` Julia Lawall
  2021-04-05 21:50   ` Michael Walle
  1 sibling, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2021-04-04 18:34 UTC (permalink / raw)
  To: Mansour Moufid; +Cc: cocci



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cocci] changing of_get_mac_address() to pass a buffer
  2021-04-04 17:48 ` Mansour Moufid
  2021-04-04 18:34   ` Julia Lawall
@ 2021-04-05 21:50   ` Michael Walle
  2021-04-05 21:58     ` Julia Lawall
  1 sibling, 1 reply; 6+ messages in thread
From: Michael Walle @ 2021-04-05 21:50 UTC (permalink / raw)
  To: Mansour Moufid; +Cc: cocci

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Cocci] changing of_get_mac_address() to pass a buffer
  2021-04-05 21:50   ` Michael Walle
@ 2021-04-05 21:58     ` Julia Lawall
  0 siblings, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2021-04-05 21:58 UTC (permalink / raw)
  To: Michael Walle; +Cc: cocci



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01  8:13 [Cocci] changing of_get_mac_address() to pass a buffer Michael Walle
2021-04-01 17:06 ` Markus Elfring
2021-04-04 17:48 ` Mansour Moufid
2021-04-04 18:34   ` Julia Lawall
2021-04-05 21:50   ` Michael Walle
2021-04-05 21:58     ` Julia Lawall

Coccinelle Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr
	public-inbox-index cocci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git