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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: airoha: Introduce WAN device flag
> 
> This commit replaces the port->id == 1 LAN/WAN test with a per-dev
> PRIV_FLAG_WAN bit that is set during airoha_dev_init() based on the
> port id and the presence of other WAN devices. It also extracts the
> QDMA selection/PPE cpu port programming into a new helper and reworks
> the GDM2 loopback path to key off the new flag.
> 
> A few questions about the new policy and error paths.
> 
> [Medium]
> The commit message says:
> 
>     Introduce WAN flag to specify if a given device is used to
>     transmit/receive WAN or LAN traffic. [...] Please note it is possible
>     to specify multiple LAN devices but just a single WAN one.
> 
> Under the old helper (port->id == 1), GDM2/GDM3/GDM4 were all treated as
> WAN. Under the new logic, when GDM2 (eth->ports[1]) is present, the
> GDM3/GDM4 case breaks out of the switch without setting PRIV_FLAG_WAN, so
> GDM3 and GDM4 are now classified as LAN. That flips the MAC register
> (REG_FE_WAN_MAC_H -> REG_FE_LAN_MAC_H in airoha_set_macaddr()), the QDMA
> binding (qdma[1] -> qdma[0] in airoha_dev_set_qdma()), and the PPE cpu
> port (CDM2 -> CDM1). Is that role change intentional, and could the
> commit message call it out so bisecting a board that relied on the old
> GDM3/GDM4=WAN semantics is easier?

This change is intentional and this is currently not visible to the user
since airoha_eth driver currently supports just the internal phy available
via the MT7530 DSA switch and there are no WAN interfaces officially
supported since PCS/external phy is not merged mainline yet (it is posted just
as RFC in [0]). I will add this info in the commimit log in v5.

[0] https://lore.kernel.org/netdev/20260505182713.27644-1-ansuelsmth@gmail.com/

> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 1c4927c1aeb0..8617dd4f7932 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1863,36 +1863,77 @@ static int airoha_set_gdm2_loopback(struct airoha_gdm_dev *dev)
> >  	return 0;
> >  }
> >  
> > -static int airoha_dev_init(struct net_device *netdev)
> > +static struct airoha_gdm_dev *
> > +airoha_get_wan_gdm_dev(struct airoha_eth *eth)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> > +		struct airoha_gdm_port *port = eth->ports[i];
> > +		int j;
> > +
> > +		if (!port)
> > +			continue;
> > +
> > +		for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> > +			struct airoha_gdm_dev *dev = port->devs[j];
> > +
> > +			if (dev && !airoha_is_lan_gdm_dev(dev))
> > +				return dev;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> [High]
> airoha_register_gdm_devices() invokes airoha_dev_init() for each dev in
> port->devs[] sequentially. On EN7581 GDM3 supports two devs (nbq 4/5 for
> PCIE0/PCIE1 in airoha_alloc_gdm_device()). When both devs are registered,
> devs[0] sets PRIV_FLAG_WAN and then devs[1] sees the first via
> airoha_get_wan_gdm_dev() and falls through to LAN. Is that the intended
> result for two devs on the same physical GDM3/GDM4 port?
> 
> The two devs then diverge:
> 
>   airoha_dev_set_qdma()
>     dev->qdma = &eth->qdma[!airoha_is_lan_gdm_dev(dev)];
> 
> so devs[1] binds to qdma[0] while devs[0] is on qdma[1];
> 
>   airoha_set_macaddr()
>     reg = airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H
>                                      : REG_FE_WAN_MAC_H;
> 
> so devs[1] writes into the LAN MAC range that belongs to GDM1; and in
> airoha_ppe_foe_entry_prepare() pse_port becomes port->id instead of 2
> (GDM2 loopback) only for devs[1]. Is there an assumption that sibling
> devs on the same port must share the WAN/LAN classification?

A subsequent patch in the series adds suppport for multiple net_device that
share the same LAN/WAN mac address register space.
Moreover, I will post subsequent patches (not in the series) to dynamically
set/get net_device LAN/WAN configuration.

> 
> > +
> > +static void airoha_dev_set_qdma(struct airoha_gdm_dev *dev)
> >  {
> > -	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > -	struct airoha_gdm_port *port = dev->port;
> >  	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;
> > -	airoha_set_macaddr(dev, netdev->dev_addr);
> > +
> > +	for (i = 0; i < eth->soc->num_ppe; i++)
> > +		airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(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;
> 
> [Medium]
> Is the eth->ports[1] check equivalent to asking whether a GDM2 dev
> actually exists? airoha_alloc_gdm_port() assigns eth->ports[p] before
> iterating the "airoha,eth-port" child nodes, so with all GDM2 child nodes
> marked status="disabled" and GDM3 or GDM4 enabled, eth->ports[1] is
> non-NULL but port->devs[] is empty. In that case this branch still breaks
> out of the switch and the uplink GDM3/GDM4 dev ends up LAN. Would checking
> for an actual dev under eth->ports[1] be safer than checking the port
> pointer alone?

ack, I will fix it in v5.

> 
> [Medium, Low]
> The policy that "just a single WAN one" is allowed is enforced by silently
> breaking here when airoha_get_wan_gdm_dev() returns non-NULL. Would a
> netdev_warn() or -EINVAL help an administrator notice that a second WAN
> candidate was demoted to LAN? Also, with both GDM3 and GDM4 enabled and
> GDM2 absent, the winner of PRIV_FLAG_WAN depends on eth->ports[] iteration
> order and the port/dev allocation order; is there a deterministic,
> documented way to choose which dev becomes WAN (DT property, for example)?

We can't return -EINVAL here since we still need to run airoha_dev_set_qdma()
and airoha_set_macaddr(). Moreover, I will post subsequent patches (not in the
series) to dynamically set/get net_device LAN/WAN configuration.

> 
> > +		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);
> > +
> > +	if (!airoha_is_lan_gdm_dev(dev) &&
> > +	    (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)) {
> > +		int err;
> > +
> > +		err = airoha_set_gdm2_loopback(dev);
> > +		if (err) {
> > +			dev->flags &= ~PRIV_FLAG_WAN;
> > +			return err;
> > +		}
> > +	}
> 
> [Medium]
> On the error path, is clearing PRIV_FLAG_WAN sufficient? By the time
> airoha_set_gdm2_loopback() returns a failure, airoha_dev_set_qdma() has
> already pointed dev->qdma at &eth->qdma[1] and set dev->dev->irq to the
> WAN IRQ, airoha_set_macaddr() has written into REG_FE_WAN_MAC_H,
> airoha_ppe_set_cpu_port() has programmed CDM2 as the PPE cpu port, and
> airoha_set_gdm2_loopback() itself may have already programmed
> REG_GDM_FWD_CFG(GDM2), REG_GDM_TXCHN_EN/RXCHN_EN, REG_GDM_LPBK_CFG,
> REG_GDM_LEN_CFG, REG_FE_VIP_PORT_EN and REG_FE_IFC_PORT_EN before the
> get_sport() branch failed.
> 
> After clearing the flag airoha_is_lan_gdm_dev(dev) reports LAN but
> dev->qdma and the hardware registers above still describe the WAN setup.
> Would it be cleaner to either unwind those steps or drop the flag clear?

In this case net_device registration will fail and the pending hw configuration
will not affect other running devices. I guess we can avoid to add unnecessary
code complexity.

> 
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index 3a313ac439e7..5715b03e630b 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -540,12 +540,17 @@ struct airoha_qdma {
> >  	u64 fwd_tx_packets;
> >  };
> >  
> > +enum airoha_priv_flags {
> > +	PRIV_FLAG_WAN = BIT(0),
> > +};
> 
> [Low]
> This isn't a bug, but PRIV_FLAG_WAN and the u32 flags field below overlap
> conceptually with struct net_device::priv_flags and the IFF_* namespace.
> Would a driver-prefixed name such as AIROHA_DEV_F_WAN make the
> driver-local scope obvious to future readers?

ack, I will fix it in v5.

Regards,
Lorenzo

> 
> > +
> >  struct airoha_gdm_dev {
> >  	struct airoha_gdm_port *port;
> >  	struct airoha_qdma *qdma;
> >  	struct airoha_eth *eth;
> >  	struct net_device *dev;
> >  
> > +	u32 flags;
> >  	int nbq;
> >  };
> 
> [ ... ]

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