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

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 [off-list ref]

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.

quoted

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

[ ... ]

quoted

@@ -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).

quoted

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

[ ... ]

quoted

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