Re: [PATCH net-next v3 04/12] net: airoha: Rely on net_device pointer in HTB callbacks

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "For your consideration."
---
net: airoha: Introduce WAN device flag

This commit introduces a WAN flag to specify whether a device is used to
transmit or receive WAN or LAN traffic. It enables specifying multiple LAN
devices but limits the configuration to a single WAN device.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 5b0cd37b155e4..9988011dca53f 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1796,36 +1796,77 @@ static int airoha_set_gdm2_loopback(struct airoha_gdm_dev *dev)

[ ... ]

> +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:
> -		/* If GDM2 is active we can't enable loopback */
> -		if (!eth->ports[1]) {
> -			int err;
> +	case AIROHA_GDM4_IDX: {
> +		struct airoha_eth *eth = dev->eth;
>  
> -			err = airoha_set_gdm2_loopback(dev);
> -			if (err)
> -				return err;
> -		}
> +		if (eth->ports[1] || airoha_get_wan_gdm_dev(eth))
> +			break;

If a board's Device Tree enables both GDM3 and GDM4, and GDM2 is not present,
does this sequence permanently assign the WAN role to GDM3 since it is
initialized first? Is there a Device Tree property or fallback configuration
mechanism to allow users to specify GDM4 as the WAN port instead?

> +		fallthrough;
> +	}
> +	case AIROHA_GDM2_IDX:
> +		/* GDM2 is always used as wan */
> +		dev->flags |= PRIV_FLAG_WAN;
>  		break;
>  	default:
>  		break;
>  	}
>  
> -	for (i = 0; i < eth->soc->num_ppe; i++)
> -		airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev));
> +	airoha_dev_set_qdma(dev);
> +	airoha_set_macaddr(dev, netdev->dev_addr);

Since the commit message notes it is possible to specify multiple LAN
devices, does unconditionally calling airoha_set_macaddr() for each
device overwrite the single shared hardware LAN MAC register?

If so, could this cause only the last initialized LAN device to have its MAC
correctly programmed into the hardware, leading to dropped unicast
packets for the other LAN interfaces?

> +
> +	if (!airoha_is_lan_gdm_dev(dev) &&
> +	    (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)) {
> +		int err;

[ ... ]