Re: [PATCH net-next v8 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: 3601 bytes --]

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Driver probe fails on systems where MAC addresses are randomly generated.
> - [High] Configuration deadlock prevents users from changing the MAC address OUI.
> --
> 
> commit dbc4f298ffd475d67c32eeea98f201059b73194a
> 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. The hardware design assumes all interfaces configured as LAN
> (or WAN) share a common upper MAC address. This patch extends the logic to
> derive the correct minimum and maximum values when two or more interfaces
> are configured.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 328f94fef2e28..9ae1ba4333097 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -71,20 +71,69 @@ 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)
> >  {
> 
> [ ... ]
> 
> > +			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)) {
> 
> Does this strict OUI validation cause the driver probe to fail when MAC addresses
> are randomly generated?
> 
> If a MAC address is not provided in the device tree, the driver generates a random
> one via eth_hw_addr_random(). For multi-port configurations, each port receives an
> independently generated random MAC address.
> 
> Since the upper 3 bytes of these random addresses will almost certainly differ,
> when register_netdev() is called for the second port, its ndo_init callback
> (airoha_dev_init) will invoke airoha_set_macaddr() and hit this check. Returning
> -EINVAL will fail register_netdev() and abort the entire driver probe, leaving
> the system without networking.
> 
> Does this also prevent users from changing the MAC address OUI?
> 
> When an administrator attempts to change the MAC address via standard userspace
> tools, only one interface can be modified at a time. The driver will compare the
> new MAC against the existing MAC of the other port.
> 
> If the MSBs differ, the change is rejected. This makes it impossible to atomically
> change the OUI of both interfaces simultaneously, locking the MAC address OUI
> to its initial probe-time value.

As pointed out before, this is a hw design requirement when multiple
net_devices are configured as LAN.

Regards,
Lorenzo

> 
> > +		/* According to the HW design, hw mac address MS bits
> > +		 * must be the same for each net_device with the same
> > +		 * LAN/WAN configuration.
> > +		 */
> > +		netdev_err(dev->dev, "wrong mac addr for shared GDM port\n");
> > +		return -EINVAL;
> > +	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6@kernel.org?part=10

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