Re: [PATCH net-next 4/5] net: rnpgbe: Allow changing the primary MAC address

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit 'net: rnpgbe: Allow changing the primary MAC address'
  adds two entries to rnpgbe_netdev_ops in…

--

net: rnpgbe: Allow changing the primary MAC address

This commit adds ndo_set_mac_address support to the rnpgbe driver, so the
primary MAC address can be changed while the interface is up or down. The
new address is validated with eth_prepare_mac_addr_change(), written to
RAR 0, and then committed to the netdev.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 31871b244882d..09d499dc10eeb 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -160,6 +160,8 @@ static const struct net_device_ops rnpgbe_netdev_ops = {
>  	.ndo_stop		 = rnpgbe_close,
>  	.ndo_start_xmit		 = rnpgbe_xmit_frame,
>  	.ndo_set_rx_mode_async = rnpgbe_set_rx_mode,
>  	.ndo_set_mac_address	 = rnpgbe_set_mac_address,
> +	.ndo_validate_addr	 = eth_validate_addr,

[Severity: Low]
This isn't a bug, but should the commit message also mention the
ndo_validate_addr hook added to rnpgbe_netdev_ops?

The commit message only covers ndo_set_mac_address:

  Add ndo_set_mac_address support to change the primary MAC address
  while the interface is up or down. Validate the new address with the
  standard Ethernet helpers, then update the primary receive address
  filter and the netdev address.

The "Validate the new address" sentence refers to the
eth_prepare_mac_addr_change() call in rnpgbe_set_mac_address().

eth_validate_addr is a separate change. __dev_open() calls it on every
ifup, so opening the device can now fail with -EADDRNOTAVAIL if
netdev->dev_addr is not a valid unicast address. Before this patch, open
did no such check.

As far as I can tell this check cannot fire today. In
rnpgbe_add_adapter(), probe either sets a firmware MAC that has already
passed is_valid_ether_addr() in rnpgbe_get_permanent_mac(), or it falls
back to eth_hw_addr_random(). After probe, rnpgbe_set_mac_address()
rejects invalid addresses.

Could the commit message mention the open-time validation, or could the
hook move to its own patch?

>  	.ndo_get_stats64	 = rnpgbe_get_stats64,
>  };

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/FB722C2F25FE9005%2B20261106112420.43050-1-dong100%40mucse.com