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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The MAC address MSB validation introduces a configuration deadlock, permanently preventing MAC address MSB updates when multiple interfaces are registered.
> - [High] Unconditional overwrite of `ref_addr` in the interface evaluation loop breaks MSB validation and corrupts hardware L2 bounds.
> --
> 
> commit ca5c4a90447cc1f9bdc3a2831da1cfe304652070
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration
> 
> The EN7581 and AN7583 SoCs provide registers to configure hardware LAN/WAN
> MAC addresses. This commit extends the logic to derive the correct minimum
> and maximum values for MAC LSB bounds when multiple interfaces are configured.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 64ee526da2414..8f2608293bb72 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -71,20 +71,76 @@ static void airoha_qdma_irq_disable(struct airoha_irq_bank *irq_bank,
> [ ... ]
> > -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)
> >  {
> > +	u8 ref_addr[ETH_ALEN] __aligned(2);
> >  	struct airoha_eth *eth = dev->eth;
> > -	u32 val, reg;
> > +	u32 reg, val, lmin, lmax;
> > +	int i;
> > +
> > +	eth_zero_addr(ref_addr);
> > +	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;
> [ ... ]
> > +			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;
> > +		}
> > +	}
> 
> [Severity: High]
> Does this loop unconditionally overwrite ref_addr with the last evaluated
> interface? If there are multiple interfaces with mismatched MSBs (e.g., from
> random boot MACs), it seems ref_addr will only retain the MSB of the final
> interface.
> 
> Could this cause the validation check below to incorrectly pass if the new
> MAC matches the last interface's MSB but not the earlier ones, while
> erroneously accumulating LSBs from mismatched interfaces into lmin and
> lmax?

As pointed out before the hw design requires all the interfaces configured as
LAN (or WAN) to share the mac address MSBs since we have a single register to
configure it (we can't configure a range like for lmin or lamx). A
misconfiguration will just affect hw acceleration and the user will be able to
log and fix the configuration (according to the reported log) if the provided 
mac addresses do not follow this hw limitation.
The choice of the reference interface is arbitrary and if multiple interfaces
have 'wrong' mac address based on the hw requirements, the user should log and
fix them one by one.
Please note this limitation is only valid if multiple net_devices are
configured as LAN (or WAN). Since in the current codebase we do not support
multiple interfaces configured as LAN or WAN, we are not introducing any
regression.

> 
> > +
> > +	if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3)) {
> 
> [Severity: High]
> Can this validation check create a configuration deadlock?
> 
> Since the driver requires all LAN or WAN interfaces to share the same
> MAC address 3-byte MSB, and userspace APIs configure interfaces sequentially
> via ndo_set_mac_address, changing the first interface's MSB will always fail
> this check against the remaining interfaces.
> 
> How can a user successfully configure a unified custom MAC MSB across all
> devices if the interfaces are assigned random MAC addresses at boot with
> different MSBs?

As pointed out above, the user is supposed to configure the mac addresses based
on the hw design.

Regards,
Lorenzo

> 
> > +		/* According to the HW design, hw mac address MSBs must be
> > +		 * the same for each net_device with the same LAN/WAN
> > +		 * configuration.
> > +		 */
> > +		struct net_device *netdev = netdev_from_priv(dev);
> > +
> > +		dev_warn(eth->dev,
> > +			 "%s: wrong mac addr, MSBs must be %02x:%02x:%02x\n",
> > +			 netdev->name, ref_addr[0], ref_addr[1],
> > +			 ref_addr[2]);
> > +		dev_warn(eth->dev, "FE hw forwarding won't work properly\n");
> > +
> > +		return -EINVAL;
> > +	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260603-airoha-eth-multi-serdes-v9-0-5d476bc2f426@kernel.org?part=6

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