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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7852 bytes --]

> 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?

Hw design requires that if two (or more) net_devices are configured as LAN
(or theoretically as WAN), they need to share the mac address upper-3-bytes.
If the user wants to use multiple devices with the same LAN/WAN configuration,
he/she will need to properly store the net_device mac address int the device
eeprom/nvme or in the dts.
Please note this series is not introducing any regression since the user was
not able to run this configuration before this series.

> 
> >  
> >  	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)?

ack, I will fix it in v5.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]