Re: [PATCH net-next v8 07/10] net: airoha: Support multiple net_devices for a single FE GDM port

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

On May 20, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Port-wide hardware configuration is improperly modified via per-net_device callbacks, breaking the newly introduced 1:N relationship.
> - [High] Sequential initialization of multiple net_devices sharing a GDM port silently overwrites singleton hardware loopback and WAN configuration registers.
> --
> 
> commit d922e17d3b113696fc139ac4b2b3ef81e2b9ad50
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Support multiple net_devices for a single FE GDM port
> 
> This commit introduces support for multiple net_devices connected to the
> same Frame Engine (FE) GDM port (GDM3 or GDM4) via an external hw arbiter.
> It updates the airoha_gdm_port structure to hold an array of devices instead
> of a single device.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index fbb50dc73af84..fc49f0049983e 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> [ ... ]
> > @@ -540,19 +541,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;
> 
> This introduces a 1:N relationship between a GDM port and net_devices, but
> are the net_device lifecycle and MTU callbacks updated to account for shared
> ports?
> 
> Looking at airoha_dev_stop(), the driver unconditionally drops all traffic
> for the port:
> 
> airoha_dev_stop()
>     airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id), FE_PSE_PORT_DROP);
>     
> Since this modifies the forwarding configuration for the entire GDM port,
> does stopping just one net_device silently drop all traffic for the port,
> breaking any other active net_devices sharing it?
> 
> Similarly, airoha_dev_change_mtu() overwrites port-wide hardware registers
> such as REG_GDM_LEN_CFG. Could this lower the port MTU while another device
> still requires a larger MTU?
> 
> I note this regression is addressed later in the series by commit
> 646f1e7bb6021 (net: airoha: Do not stop GDM port if it is shared).
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 27b214ce2a4ed..8d36ab6cd7854 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -1830,7 +1831,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);
> >  	if (src_port < 0)
> >  		return src_port;
> >  
> 
> When multiple net_devices sharing a GDM port are sequentially initialized
> via ndo_init (airoha_dev_init()), they unconditionally call
> airoha_set_gdm2_loopback().
> 
> Inside this function, global singleton hardware registers are modified:
> 
> airoha_set_gdm2_loopback()
>     airoha_fe_rmw(eth, REG_FE_WAN_PORT,
>                   WAN1_EN_MASK | WAN1_MASK | WAN0_MASK,
>                   FIELD_PREP(WAN0_MASK, src_port));
> 
> Does initializing the second device on the same port silently overwrite
> the hardware loopback and WAN configuration of the previously initialized
> device?
> 
> I see this regression is resolved later in the series by commit
> 8a565b44990bb (net: airoha: Introduce WAN device flag).
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6@kernel.org?part=7

Commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6%40kernel.org

- The per-port hardware MIB counters live on the GDM port, but
  airoha_dev_get_stats64() returns dev->port->stats and
  airoha_update_hw_stats() reads REG_FE_GDM_*_CNT_*(port->id) into that
  shared structure followed by REG_FE_GDM_MIB_CLEAR.  With two netdevs now
  sharing a single GDM3 or GDM4 port, would both siblings report identical
  combined byte/packet counters via ip -s link show, since there is no
  per-dev SW counter to disambiguate the shared HW MIB?
  - This issue will be fixed with a dedicated patch not in the series.

Regards,
Lorenzo

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