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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> 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?

This issue will be fixed with following series that allows to dynamically
select LAN or WAN configuration at runtime.

> 
> > +		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;
> 
> [ ... ]

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