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

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

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 uses our helpers to keep common actions safe, but also keeps the 
vendor specific logic (add N to the base_mac) in the driver.

sln