2021-04-13 07:37:56

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH net-next v4 1/2] of: net: pass the dst buffer to of_get_mac_address()

On Mon, Apr 12, 2021 at 07:47:17PM +0200, Michael Walle wrote:
> of_get_mac_address() returns a "const void*" pointer to a MAC address.
> Lately, support to fetch the MAC address by an NVMEM provider was added.
> But this will only work with platform devices. It will not work with
> PCI devices (e.g. of an integrated root complex) and esp. not with DSA
> ports.
>
> There is an of_* variant of the nvmem binding which works without
> devices. The returned data of a nvmem_cell_read() has to be freed after
> use. On the other hand the return of_get_mac_address() points to some
> static data without a lifetime. The trick for now, was to allocate a
> device resource managed buffer which is then returned. This will only
> work if we have an actual device.
>
> Change it, so that the caller of of_get_mac_address() has to supply a
> buffer where the MAC address is written to. Unfortunately, this will
> touch all drivers which use the of_get_mac_address().
>
> Usually the code looks like:
>
> const char *addr;
> addr = of_get_mac_address(np);
> if (!IS_ERR(addr))
> ether_addr_copy(ndev->dev_addr, addr);
>
> This can then be simply rewritten as:
>
> of_get_mac_address(np, ndev->dev_addr);
>
> Sometimes is_valid_ether_addr() is used to test the MAC address.
> of_get_mac_address() already makes sure, it just returns a valid MAC
> address. Thus we can just test its return code. But we have to be
> careful if there are still other sources for the MAC address before the
> of_get_mac_address(). In this case we have to keep the
> is_valid_ether_addr() call.
>
> The following coccinelle patch was used to convert common cases to the
> new style. Afterwards, I've manually gone over the drivers and fixed the
> return code variable: either used a new one or if one was already
> available use that. Mansour Moufid, thanks for that coccinelle patch!
>
> <spml>
> @[email protected]
> 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
> </spml>
>
> All drivers, except drivers/net/ethernet/aeroflex/greth.c, were
> compile-time tested.
>
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Michael Walle <[email protected]>

I cannot say i looked at all the changes, but the ones i did exam
seemed O.K.

Reviewed-by: Andrew Lunn <[email protected]>

Andrew