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

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Probe fails completely when devices use randomly generated MAC addresses due to strict MSB validation.
> - [Medium] Strict MAC address MSB validation creates a configuration deadlock, preventing users from modifying MAC addresses.
> --
> 
> commit d9f17b062a5dbe5adb3be07d9c3f501106b25dd8
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration
> 
> This commit extends the hardware MAC address configuration in the airoha
> Ethernet driver to support multiple LAN or WAN interfaces. It calculates
> the minimum and maximum lower MAC address bytes across all devices with the
> same configuration, and enforces that they share the same upper 3 MAC
> address bytes.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index fa37dbd76d579..623cac42dfb71 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > -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++) {
> [ ... ]
> > +			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;
> > +	}
> 
> Does this strict MSB validation cause device initialization to fail if the
> device tree does not provide MAC addresses?
> 
> If the driver falls back to assigning addresses via eth_hw_addr_random(),
> the randomly generated addresses will almost certainly have different upper
> 3 bytes. Will this cause airoha_set_macaddr() to return -EINVAL for the
> second interface, aborting initialization and failing the driver probe?
> 
> Could this also create a configuration deadlock when users try to change
> MAC addresses from userspace?
> 
> Since tools like 'ip link' configure interfaces sequentially, attempting to
> change eth0 to a new OUI would be rejected because its new MSBs won't match
> eth1's current MSBs. Attempting to change eth1 first would similarly fail
> against eth0. Is there a way for users to successfully change the MAC
> address MSBs once the interfaces are initialized?

This is a requirement imposed by the hw design. If two (or multiple) interfaces
are configured as LAN (or WAN), they will need to share the mac address MSBs.
This requirement is not needed if the interfaces are running with different
configuration (e.g. eth0 LAN and eth1 WAN).

Regards,
Lorenzo

> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=11

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