Re: [PATCH net-next v5 08/12] net: airoha: Support multiple net_devices for a single FE GDM port

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Bringing up a device overwrites the global WAN port and GDM loopback configuration of previously opened devices sharing the same port.
> - [Medium] Standard netdev statistics and ethtool MAC/RMON stats return the combined hardware counters for the entire GDM port instead of per-interface statistics.
> --
> 
> commit 6b561eda8762bee9a19daa9304286dbd64cc6eac
> Author: Lorenzo Bianconi <[email protected]>
> 
> net: airoha: Support multiple net_devices for a single FE GDM port
> 
> Introduce support for multiple net_devices connected to the same Frame Engine
> (FE) GDM port (GDM3 or GDM4) via an external hw arbiter.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 904f8da936005..be31f639f4be3 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -1829,7 +1833,7 @@ static int airoha_set_gdm2_loopback(struct airoha_gdm_dev *dev)
> >  	airoha_fe_clear(eth, REG_FE_VIP_PORT_EN, BIT(AIROHA_GDM2_IDX));
> >  	airoha_fe_clear(eth, REG_FE_IFC_PORT_EN, BIT(AIROHA_GDM2_IDX));
> >  
> > -	src_port = eth->soc->ops.get_sport(port, port->nbq);
> > +	src_port = eth->soc->ops.get_sport(port, dev->nbq);
> 
> Does bringing up a device overwrite the global WAN port and GDM loopback
> configuration of previously opened devices sharing the same port?
> 
> Since multiple devices can now share the same GDM port and each has a
> different nbq (and therefore a different src_port), wouldn't the
> initialization of the second device clobber the WAN port configuration of the
> first one, breaking its forwarding behavior?
> 
> I noticed this is fixed later in the series in commit 8e8f8223af5be ("net:
> airoha: Introduce WAN device flag"), but wanted to confirm the regression
> between commits.

Correct, airoha_set_gdm2_loopback() is run just for WAN net_device and a
subsequent patch limits the support to a single WAN device.

> 
> >  	if (src_port < 0)
> >  		return src_port;
> >  
> [ ... ]
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index 694e83bfd1e70..8d6887e334130 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> [ ... ]
> > @@ -539,19 +540,20 @@ struct airoha_qdma {
> >  struct airoha_gdm_dev {
> >  	struct airoha_gdm_port *port;
> >  	struct airoha_qdma *qdma;
> > -	struct net_device *dev;
> >  	struct airoha_eth *eth;
> > +	struct net_device *dev;
> >  
> >  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> >  	/* qos stats counters */
> >  	u64 cpu_tx_packets;
> >  	u64 fwd_tx_packets;
> > +
> > +	int nbq;
> >  };
> >  
> >  struct airoha_gdm_port {
> > -	struct airoha_gdm_dev *dev;
> > +	struct airoha_gdm_dev *devs[AIROHA_MAX_NUM_GDM_DEVS];
> >  	int id;
> > -	int nbq;
> >  
> >  	struct airoha_hw_stats stats;
> 
> Since standard netdev statistics and ethtool MAC/RMON stats are tied to the
> hardware counters on the GDM port, do they now return the combined hardware
> counters for the entire GDM port instead of per-interface statistics?
> 
> Prior to this patch, each GDM port mapped to a single net_device, making
> port-wide hardware stats accurate for the interface. With multiple net_devices
> sharing a single GDM port, querying statistics for one interface might
> incorrectly return the aggregated traffic of all interfaces connected to that
> port, violating standard rtnl_link_stats64 semantics.
> 
> I noticed this is fixed later in the series in commit 29334a9bde18f ("net:
> airoha: Better handle MIB for GDM with multiple port attached").
> 

Correct, this is fixed by a subsequent patch.

Regards,
Lorenzo

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCagGLKgAKCRA6cBh0uS2t
rMF3AP0RLVH1BUBktGa8mzV8rfg1YVHHkb0u2MSuklmwoqDnxwD/f21aDQk4lpzG
k6fW/ysM0wQA78uX4Sh42xeIMjGA6wQ=
=jThD
-----END PGP SIGNATURE-----