Re: [PATCH net-next v4 10/10] net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration

This patch extends airoha_set_macaddr() to derive REG_FE_MAC_LMIN and
REG_FE_MAC_LMAX by iterating over all sibling LAN/WAN net_devices, and
rejects addresses whose upper 3 bytes do not match the already-registered
peers. airoha_dev_set_macaddr() is also changed to split eth_mac_addr()
into eth_prepare_mac_addr_change() and eth_commit_mac_addr_change() around
the hardware programming step.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 8617dd4f7932..fbfefd0ec355 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -71,20 +71,67 @@ static void airoha_qdma_irq_disable(struct airoha_irq_bank *irq_bank,
>  	airoha_qdma_set_irqmask(irq_bank, index, mask, 0);
>  }
>  
> -static void airoha_set_macaddr(struct airoha_gdm_dev *dev, const u8 *addr)
> +static int airoha_set_macaddr(struct airoha_gdm_dev *dev, const u8 *addr)
>  {
>  	struct airoha_eth *eth = dev->eth;
> -	u32 val, reg;
> +	u8 ref_addr[ETH_ALEN] = {};
> +	u32 reg, val, lmin, lmax;
> +	int i;
> +
> +	lmin = (addr[3] << 16) | (addr[4] << 8) | addr[5];
> +	lmax = lmin;
> +
> +	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> +		struct airoha_gdm_port *port = eth->ports[i];
> +		int j;
> +
> +		if (!port)
> +			continue;
> +
> +		for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> +			struct airoha_gdm_dev *iter_dev;
> +			struct net_device *netdev;
> +
> +			iter_dev = port->devs[j];
> +			if (!iter_dev || iter_dev == dev)
> +				continue;
> +
> +			if (airoha_is_lan_gdm_dev(iter_dev) !=
> +			    airoha_is_lan_gdm_dev(dev))
> +				continue;
> +
> +			netdev = iter_dev->dev;
> +			if (netdev->reg_state != NETREG_REGISTERED)
> +				continue;
> +
> +			ether_addr_copy(ref_addr, netdev->dev_addr);
> +			val = (netdev->dev_addr[3] << 16) |
> +			      (netdev->dev_addr[4] << 8) | netdev->dev_addr[5];
> +			if (val < lmin)
> +				lmin = val;
> +			if (val > lmax)
> +				lmax = val;
> +		}
> +	}
> +
> +	if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3)) {
> +		/* According to the HW design, hw mac address MS bits
> +		 * must be the same for each net_device with the same
> +		 * LAN/WAN configuration.
> +		 */
> +		return -EINVAL;
> +	}

[Medium]
The NETREG_REGISTERED filter here means the check only sees peers that
have already completed register_netdev(). airoha_dev_init() is invoked
as ndo_init from register_netdev() before dev->reg_state transitions
to NETREG_REGISTERED, so the constraint is only enforced across
already-registered siblings.

When the device tree does not supply a mac-address property,
airoha_alloc_gdm_device() falls back to eth_hw_addr_random() and each
sibling gets an independently generated random MAC. With two or more
LAN (or two or more WAN) net_devices going through this fallback, the
second and later siblings' upper 3 bytes will almost always differ
from the first, so airoha_set_macaddr() returns -EINVAL:

	if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3))
		return -EINVAL;

airoha_dev_init() now propagates that error, which fails
register_netdev() and airoha_register_gdm_devices(), aborting probe.

Before this patch the same configuration still came up (with the single
REG_FE_{LAN,WAN}_MAC_H being last-writer-wins). Should the random
fallback path derive sibling MACs from a shared base, or should the
upper-3-bytes constraint only be enforced for user-triggered
ndo_set_mac_address rather than for init-time programming?

>  
>  	reg = airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H : REG_FE_WAN_MAC_H;
>  	val = (addr[0] << 16) | (addr[1] << 8) | addr[2];
>  	airoha_fe_wr(eth, reg, val);
>  
> -	val = (addr[3] << 16) | (addr[4] << 8) | addr[5];
> -	airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), val);
> -	airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), val);
> +	airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), lmin);
> +	airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), lmax);
>  
>  	airoha_ppe_init_upd_mem(dev);
> +
> +	return 0;
>  }
>  
>  static void airoha_set_gdm_port_fwd_cfg(struct airoha_eth *eth, u32 addr,
> @@ -1792,13 +1839,18 @@ static int airoha_dev_stop(struct net_device *netdev)
>  static int airoha_dev_set_macaddr(struct net_device *netdev, void *p)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct sockaddr *addr = p;
>  	int err;
>  
> -	err = eth_mac_addr(netdev, p);
> +	err = eth_prepare_mac_addr_change(netdev, p);
>  	if (err)
>  		return err;
>  
> -	airoha_set_macaddr(dev, netdev->dev_addr);
> +	err = airoha_set_macaddr(dev, addr->sa_data);
> +	if (err)
> +		return err;
> +
> +	eth_commit_mac_addr_change(netdev, p);
>  
>  	return 0;
>  }

[High]
After this reordering, does airoha_ppe_init_upd_mem() end up
programming the PPE update memory with the old MAC rather than the new
one?

eth_prepare_mac_addr_change() only validates the sockaddr and does not
touch netdev->dev_addr; the actual write to netdev->dev_addr happens in
eth_commit_mac_addr_change() via eth_hw_addr_set():

net/ethernet/eth.c:eth_prepare_mac_addr_change() {
	if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
		return -EBUSY;
	if (!is_valid_ether_addr(addr->sa_data))
		return -EADDRNOTAVAIL;
	return 0;
}

net/ethernet/eth.c:eth_commit_mac_addr_change() {
	eth_hw_addr_set(dev, addr->sa_data);
}

airoha_set_macaddr() is invoked between those two, so when it reaches
airoha_ppe_init_upd_mem(dev) the commit has not yet run. That helper
reads netdev->dev_addr directly rather than using the addr argument:

drivers/net/ethernet/airoha/airoha_ppe.c:airoha_ppe_init_upd_mem() {
	const u8 *addr = netdev->dev_addr;
	...
	val = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
	airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
	...
	val = (addr[0] << 8) | addr[1];
	airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
	...
}

So the FE registers REG_FE_{LAN,WAN}_MAC_H / REG_FE_MAC_LMIN /
REG_FE_MAC_LMAX are written from the new addr parameter, but the PPE
REG_UPDMEM_DATA/REG_UPDMEM_CTRL entries are written from the still-old
netdev->dev_addr.

Previously eth_mac_addr() (prepare + commit combined) ran first, so
netdev->dev_addr was the new MAC by the time airoha_set_macaddr() and
airoha_ppe_init_upd_mem() ran, keeping the two blocks in sync.

Would it be cleaner to either pass addr through to
airoha_ppe_init_upd_mem(), or invoke eth_commit_mac_addr_change() before
airoha_set_macaddr() (with a rollback path if the hardware programming
fails)?

> @@ -1903,6 +1955,7 @@ static int airoha_dev_init(struct net_device *netdev)
[ ... ]