Re: [RFC net-next 3/6] ethernet: prestera: use eth_hw_addr_set_port()

On Mon, Oct 18, 2021 at 10:54:56AM -0700, Shannon Nelson wrote:

On 10/18/21 10:33 AM, Ido Schimmel wrote:

quoted

On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:

quoted

On 10/18/21 7:19 AM, Jakub Kicinski wrote:

quoted

On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:

quoted

As a potential consumer of these helpers, I'd rather do my own mac
address byte twiddling and then use eth_hw_addr_set() to put it into place.

This is disproved by many upstream drivers, I only converted the ones
that jumped out at me on Friday, but I'm sure there is more. If your
driver is _really_ doing something questionable^W I mean "special"
nothing is stopping you from open coding it. For others the helper will
be useful.

IOW I don't understand your comment.

To try to answer your RFC more clearly: I feel that this particular helper
obfuscates the operation more than it helps.

FWIW, it at least helped me realize that we are going to have a bug with
Spectrum-4. Currently we have:

ether_addr_copy(addr, mlxsw_sp->base_mac);
addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;

As a preparation for Spectrum-4 we are promoting 'local_port' to u16
since at least 257 and 258 are valid local port values.

With the current code, the netdev corresponding to local port 1 will
have the same MAC as the netdev corresponding to local port 257.

After Jakub's conversion and changing the 'id' argument to 'unsigned
int' [1], it should work correctly.

[1] https://lore.kernel.org/netdev/20211018070845.68ba815d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/ (local)

I would think that it might be clearer to do something like

    u64 addr64;

    addr64 = ether_addr_to_64(mlxsw_sp->base_mac);
    addr64 += mlxsw_sp_port->local_port;
    u64_to_ether_addr(addr64, addr);
    eth_hw_addr_set(dev, addr);

This is basically what Jakub's helper is doing...

I don't know how to argue with "clearer", but the fact is that we are
not doing what you suggested right now (hindsight is always 20/20) and
that it would have taken me time to debug it.

The suggested helper already helped to avoid one bug and it's not even
merged yet, so it's safe to assume it will help to avoid more bugs in
the future.