Re: [PATCH net-next v8 09/10] net: airoha: Introduce WAN device flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware CPU port mapping overwrite when multiple devices share a GDM port.
- [High] Strict probe-order dependency leads to hardware misconfiguration and multiple WAN devices.
- [Medium] Global hardware MAC filter overwrite for multiple LAN devices.
--

commit 8a565b44990bb31355dea04fc03116c38aa783db
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Introduce WAN device flag

This commit introduces a WAN flag to specify if a given device is used to
transmit or receive WAN or LAN traffic. It updates the initialization path
to select the proper QDMA and MAC address registers based on this role.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 15ad5a7edd439..328f94fef2e28 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> +static void airoha_dev_set_qdma(struct airoha_gdm_dev *dev)
>  {
>  	struct airoha_eth *eth = dev->eth;
>  	int i;
>  
>  	/* QDMA0 is used for lan ports while QDMA1 is used for WAN ports */
>  	dev->qdma = &eth->qdma[!airoha_is_lan_gdm_dev(dev)];
>  	dev->dev->irq = dev->qdma->irq_banks[0].irq;
> +
> +	for (i = 0; i < eth->soc->num_ppe; i++)
> +		airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev));
> +}

If multiple devices share a physical GDM port, and the first initialized
device assumes the WAN role, it will set the hardware per-physical-port
register to route inbound traffic to the WAN QDMA.

If a second device then initializes on the same physical port, it will
default to the LAN role.

Could this second initialization silently overwrite the previous hardware
mapping since the physical port is shared, effectively routing all traffic
to the LAN QDMA and starving the WAN device?

[ ... ]
> +static int airoha_dev_init(struct net_device *netdev)
> +{
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct airoha_gdm_port *port = dev->port;
>  
>  	switch (port->id) {
>  	case AIROHA_GDM3_IDX:
> +	case AIROHA_GDM4_IDX: {
> +		struct airoha_eth *eth = dev->eth;
>  
> +		/* GDM2 supports a single net_device */
> +		if (eth->ports[1] && eth->ports[1]->devs[0])
> +			break;

Is there a strict probe-order dependency for these interfaces?

If GDM3 or GDM4 is probed before GDM2, eth->ports[1]->devs[0] will be NULL.
This leads GDM3/GDM4 to assume GDM2 is inactive, take the WAN role, and force
GDM2 into loopback mode.

Later, when GDM2 initializes:

> +	case AIROHA_GDM2_IDX:
> +		/* GDM2 is always used as wan */
> +		dev->flags |= AIROHA_PRIV_F_WAN;
>  		break;

It unconditionally assigns itself the WAN role.

Does this probe ordering create multiple WAN devices and leave GDM2's
hardware silently misconfigured since its routing capabilities were disabled
by GDM3's loopback configuration?

>  	default:
>  		break;
>  	}
>  
> +	airoha_dev_set_qdma(dev);
> +	airoha_set_macaddr(dev, netdev->dev_addr);

When setting the MAC address, airoha_set_macaddr() does the following:

static void airoha_set_macaddr(struct airoha_gdm_dev *dev, const u8 *addr)
{
	...
	reg = airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H : REG_FE_WAN_MAC_H;
	...
	airoha_fe_wr(eth, reg, val);
}

Since REG_FE_LAN_MAC_H is a global forwarding engine register rather than
per-port, will initializing subsequent LAN devices overwrite the hardware
MAC filter configured by the first LAN device?

Might this cause the hardware to drop packets for LAN interfaces that do not
match the single overwritten MAC address?

(Note: This was observed to be fixed later in the series by commit "net:
airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration",
but is mentioned here as it appears to introduce a regression in this commit.)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6@kernel.org?part=9