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

[sashiko-bot@kernel.org]

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