Re: [Cocci] changing of_get_mac_address() to pass a buffer
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 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-mich...@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
Re: [Cocci] changing of_get_mac_address() to pass a buffer
Hi Mansour, Am 2021-04-04 19:48, schrieb Mansour Moufid: On Thu, Apr 1, 2021 at 4:13 AM Michael Walle 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-mich...@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
On Sun, 4 Apr 2021, Mansour Moufid wrote: > On Thu, Apr 1, 2021 at 4:13 AM Michael Walle 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
On Thu, Apr 1, 2021 at 4:13 AM Michael Walle 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
>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
[Cocci] changing of_get_mac_address() to pass a buffer
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: 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 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