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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Strict OUI matching logic breaks random MAC initialization and creates a MAC configuration deadlock.
> - [Medium] Unaligned stack array passed to Ethernet helper functions.
> --
> 
> commit c741e4a94d25c6203f98c2a4392d2ea65e6c3156
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration
> 
> This commit extends the hardware LAN/WAN MAC address configuration logic to
> derive the correct minimum and maximum values when multiple interfaces are
> configured as LAN or WAN. It assumes all interfaces configured in the same
> group share a common upper MAC address.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > --- 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] = {};
> 
> Is this stack array missing a 16-bit alignment attribute?
> 
> ether_addr_copy() and is_zero_ether_addr() require their arguments to be
> 16-bit aligned. On systems lacking CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS,
> passing an unaligned stack array cast to a u16 pointer will cause alignment
> traps.
> 
> Could this be declared with __aligned(2) or as a u16 array?

ack, I will fix it in v7.

> 
> [ ... ]
> > +			if (netdev->reg_state != NETREG_REGISTERED)
> > +				continue;
> > +
> > +			ether_addr_copy(ref_addr, netdev->dev_addr);
> [ ... ]
> > +	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;
> > +	}
> 
> Can this strict OUI matching break device probe or create a configuration
> deadlock?
> 
> If the device tree does not provide MAC addresses, the driver generates random
> MACs via eth_hw_addr_random(), which produces differing OUIs. When the second
> interface attempts to register during probe, this check will fail,
> register_netdev() will return -EINVAL, and the entire driver probe will abort.
> 
> Additionally, can this prevent userspace from migrating the MAC addresses to a
> new OUI using "ip link set"?
> 
> The first interface update attempt would be rejected because its new OUI
> would not match the OUI of the remaining unmodified interfaces.

As already pointed out, this is a requirement imposed by the hw design.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260511-airoha-eth-multi-serdes-v6-0-c899462c4f75@kernel.org?part=11

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