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

[Ido Schimmel <idosch@idosch.org>]

On Mon, Oct 18, 2021 at 10:54:56AM -0700, Shannon Nelson wrote:
> On 10/18/21 10:33 AM, Ido Schimmel wrote:
> > On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:
> > > On 10/18/21 7:19 AM, Jakub Kicinski wrote:
> > > > On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:
> > > > > 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/
> 
> 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.