Re: [PATCH net] net: airoha: fix max receive size configuration

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Set the GDM maximum receive size to AIROHA_MAX_RX_SIZE unconditionally
> during hardware initialization instead of updating it according to the
> configured MTU. This avoids dropping incoming frames that exceed the
> current MTU but could still be processed by the networking stack, which
> is able to fragment the reply on the TX side (e.g. ICMP echo requests).
> Move the per-port MTU configuration to the PPE egress path where it
> belongs, and set the tx frame size running airoha_ppe_set_xmit_frame_size()
> to dynamically track the maximum MTU across running interfaces sharing
> the same PPE instance.
> Fix the PPE MTU register addressing to pack two port entries per
> register word and add WAN_MTU0 configuration for non-LAN GDM devices.

commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260625-airoha-fix-rx-max-len-v1-1-45b9b827358d%40kernel.org

> 
> Fixes: 54d989d58d2a ("net: airoha: Move min/max packet len configuration in airoha_dev_open()")
> Tested-by: Madhur Agrawal <madhur.agrawal@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c  | 68 ++++++++++---------------------
>  drivers/net/ethernet/airoha/airoha_eth.h  |  2 +
>  drivers/net/ethernet/airoha/airoha_ppe.c  | 39 +++++++++++++-----
>  drivers/net/ethernet/airoha/airoha_regs.h |  9 ++--
>  4 files changed, 58 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 932b3a3df2e5..3f451c2d4c24 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -178,10 +178,15 @@ static void airoha_fe_maccr_init(struct airoha_eth *eth)
>  {
>  	int p;
>  
> -	for (p = 1; p <= ARRAY_SIZE(eth->ports); p++)
> +	for (p = 1; p <= ARRAY_SIZE(eth->ports); p++) {
>  		airoha_fe_set(eth, REG_GDM_FWD_CFG(p),
>  			      GDM_TCP_CKSUM_MASK | GDM_UDP_CKSUM_MASK |
>  			      GDM_IP4_CKSUM_MASK | GDM_DROP_CRC_ERR_MASK);
> +		airoha_fe_rmw(eth, REG_GDM_LEN_CFG(p),
> +			      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> +			      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> +			      FIELD_PREP(GDM_LONG_LEN_MASK, AIROHA_MAX_RX_SIZE));
> +	}
>  
>  	airoha_fe_rmw(eth, REG_CDM_VLAN_CTRL(1), CDM_VLAN_MASK,
>  		      FIELD_PREP(CDM_VLAN_MASK, 0x8100));
> @@ -1831,13 +1836,24 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
>  	spin_unlock(&port->stats_lock);

- This is a pre-existing issue, but can the spin_lock() used in
  airoha_update_hw_stats() cause a deadlock?
  If a process context holds port->stats_lock via spin_lock() and is preempted
  by a networking softirq on the same CPU that calls dev_get_stats()
  (which invokes ndo_get_stats64 -> airoha_update_hw_stats()), will the softirq
  spin forever trying to acquire the same lock? Should this use spin_lock_bh()
  instead?
  - The reported issue has not been introduced by this patch. Moreover, I do
    not think this is a real problem since in the current codebase
    airoha_update_hw_stats() is always run in process context and not in-irq
    context.

>  }
>  
> +static void airoha_dev_set_xmit_frame_size(struct net_device *netdev)
> +{
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +
> +	airoha_ppe_set_xmit_frame_size(dev);
> +	if (!airoha_is_lan_gdm_dev(dev))
> +		airoha_fe_rmw(dev->eth, REG_WAN_MTU0, WAN_MTU0_MASK,
> +			      FIELD_PREP(WAN_MTU0_MASK,
> +					 VLAN_ETH_HLEN + netdev->mtu));
> +}

- Does this unconditional write to REG_WAN_MTU0 break sibling network devices
  sharing the same WAN port? 
  If multiple interfaces share the same hardware port, this appears to overwrite
  the shared register using only the current interface's MTU, ignoring the
  maximum MTU of any active sibling interfaces. Could this cause the hardware to
  drop frames for sibling interfaces if their MTU is larger than the most
  recently configured interface?
  - This is not a real issue since we can have at most a single WAN port in the
    system

> +
>  static int airoha_dev_open(struct net_device *netdev)
>  {
> -	int err, len = ETH_HLEN + netdev->mtu + ETH_FCS_LEN;
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
>  	struct airoha_gdm_port *port = dev->port;
> -	u32 cur_len, pse_port = FE_PSE_PORT_PPE1;
>  	struct airoha_qdma *qdma = dev->qdma;
> +	u32 pse_port = FE_PSE_PORT_PPE1;
> +	int err;
>  
>  	netif_tx_start_all_queues(netdev);
>  	err = airoha_set_vip_for_gdm_port(dev, true);

[...]

>  static int airoha_dev_stop(struct net_device *netdev)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> @@ -1909,7 +1889,7 @@ static int airoha_dev_stop(struct net_device *netdev)
>  	airoha_set_vip_for_gdm_port(dev, false);
>  
>  	if (--port->users)
> -		airoha_set_port_mtu(dev->eth, port);
> +		airoha_ppe_set_xmit_frame_size(dev);

- Does this stop path fail to update the WAN MTU limit?
  When an interface is stopped, airoha_ppe_set_xmit_frame_size() recalculates
  the PPE MTU, but it looks like the global REG_WAN_MTU0 register is not updated
  here. Will this prevent the MTU limit from correctly shrinking when an
  interface is brought down?
  - This is not a real issue since, as pointed out above, we can have at most a
    single WAN port in the system, so there is no point to reconfigure
    REG_WAN_MTU0 if this interface is stopped.

>  	else
>  		airoha_set_gdm_port_fwd_cfg(qdma->eth,
>  					    REG_GDM_FWD_CFG(port->id),
> @@ -1962,10 +1942,6 @@ static int airoha_enable_gdm2_loopback(struct airoha_gdm_dev *dev)
>  		      FIELD_PREP(LPBK_CHAN_MASK, chan) |
>  		      LBK_GAP_MODE_MASK | LBK_LEN_MODE_MASK |
>  		      LBK_CHAN_MODE_MASK | LPBK_EN_MASK);
> -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(AIROHA_GDM2_IDX),
> -		      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> -		      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> -		      FIELD_PREP(GDM_LONG_LEN_MASK, AIROHA_MAX_MTU));
>  	/* Forward the traffic to the proper GDM port */
>  	pse_port = port->id == AIROHA_GDM3_IDX ? FE_PSE_PORT_GDM3
>  					       : FE_PSE_PORT_GDM4;
> @@ -2098,7 +2074,7 @@ static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
>  
>  	WRITE_ONCE(netdev->mtu, mtu);
>  	if (port->users)
> -		airoha_set_port_mtu(dev->eth, port);
> +		airoha_dev_set_xmit_frame_size(netdev);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index d7ff8c5200e2..0c3fb6e5d7f1 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -23,6 +23,7 @@
>  #define AIROHA_MAX_DSA_PORTS		7
>  #define AIROHA_MAX_NUM_RSTS		3
>  #define AIROHA_MAX_MTU			9220
> +#define AIROHA_MAX_RX_SIZE		16128
>  #define AIROHA_MAX_PACKET_SIZE		2048
>  #define AIROHA_NUM_QOS_CHANNELS		4
>  #define AIROHA_NUM_QOS_QUEUES		8
> @@ -676,6 +677,7 @@ int airoha_get_fe_port(struct airoha_gdm_dev *dev);
>  bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
>  			     struct airoha_gdm_dev *dev);
>  
> +void airoha_ppe_set_xmit_frame_size(struct airoha_gdm_dev *dev);
>  void airoha_ppe_set_cpu_port(struct airoha_gdm_dev *dev, u8 ppe_id, u8 fport);
>  bool airoha_ppe_is_enabled(struct airoha_eth *eth, int index);
>  void airoha_ppe_check_skb(struct airoha_ppe_dev *dev, struct sk_buff *skb,
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> index 42f4b0f21d17..e7c78293002a 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -97,6 +97,33 @@ void airoha_ppe_set_cpu_port(struct airoha_gdm_dev *dev, u8 ppe_id, u8 fport)
>  		      __field_prep(DFT_CPORT_MASK(fport), fe_cpu_port));
>  }
>  
> +void airoha_ppe_set_xmit_frame_size(struct airoha_gdm_dev *dev)
> +{
> +	struct airoha_gdm_port *port = dev->port;
> +	struct airoha_eth *eth = dev->eth;
> +	int i, ppe_id, index;
> +	u32 len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> +		struct airoha_gdm_dev *d = port->devs[i];
> +		struct net_device *netdev;
> +
> +		if (!d)
> +			continue;
> +
> +		netdev = netdev_from_priv(d);
> +		if (netif_running(netdev))
> +			len = max_t(u32, len, netdev->mtu);
> +	}
> +	len += VLAN_ETH_HLEN;
> +
> +	ppe_id = !airoha_is_lan_gdm_dev(dev) && airoha_ppe_is_enabled(eth, 1);
> +	index = port->id == AIROHA_GDM4_IDX ? 7 : port->id;
> +	airoha_fe_rmw(eth, REG_PPE_MTU(ppe_id, index),
> +		      FP_EGRESS_MTU_MASK(index),
> +		      __field_prep(FP_EGRESS_MTU_MASK(index), len));

- Does this leave the egress MTU limit uninitialized for other PPE engines?
  The patch removes the loop in airoha_ppe_hw_init() that previously initialized
  REG_PPE_MTU for all ports on all available PPEs. This function now only
  configures it for a single ppe_id.
  During cross-PPE routing (such as WAN-to-LAN), if PPE1 (WAN) forwards a packet
  to a LAN port, it will check REG_PPE_MTU(1, LAN_port). Since this register was
  only configured for PPE0, will the uninitialized limit (0) cause the packet to
  be dropped?
  - This is not a real issue since every airoha_gdm_dev/net_device is
    associated to a PPE engine/QDMA according to the logic in
    airoha_dev_open()/airoha_dev_set_qdma(). The other PPE engine's MTU will be
    updated according to the assigned net_device.

Regards,
Lorenzo

> +}
> +
>  static void airoha_ppe_hw_init(struct airoha_ppe *ppe)
>  {
>  	u32 sram_ppe_num_data_entries = PPE_SRAM_NUM_ENTRIES, sram_num_entries;
> @@ -115,8 +142,6 @@ static void airoha_ppe_hw_init(struct airoha_ppe *ppe)
>  		PPE_RAM_NUM_ENTRIES_SHIFT(sram_ppe_num_data_entries);
>  
>  	for (i = 0; i < eth->soc->num_ppe; i++) {
> -		int p;
> -
>  		airoha_fe_wr(eth, REG_PPE_TB_BASE(i),
>  			     ppe->foe_dma + sram_tb_size);
>  
> @@ -166,15 +191,6 @@ static void airoha_ppe_hw_init(struct airoha_ppe *ppe)
>  		airoha_fe_wr(eth, REG_PPE_HASH_SEED(i), PPE_HASH_SEED);
>  		airoha_fe_clear(eth, REG_PPE_PPE_FLOW_CFG(i),
>  				PPE_FLOW_CFG_IP6_6RD_MASK);
> -
> -		for (p = 0; p < ARRAY_SIZE(eth->ports); p++)
> -			airoha_fe_rmw(eth, REG_PPE_MTU(i, p),
> -				      FP0_EGRESS_MTU_MASK |
> -				      FP1_EGRESS_MTU_MASK,
> -				      FIELD_PREP(FP0_EGRESS_MTU_MASK,
> -						 AIROHA_MAX_MTU) |
> -				      FIELD_PREP(FP1_EGRESS_MTU_MASK,
> -						 AIROHA_MAX_MTU));
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> @@ -196,6 +212,7 @@ static void airoha_ppe_hw_init(struct airoha_ppe *ppe)
>  				 airoha_ppe_is_enabled(eth, 1);
>  			fport = airoha_get_fe_port(dev);
>  			airoha_ppe_set_cpu_port(dev, ppe_id, fport);
> +			airoha_ppe_set_xmit_frame_size(dev);
>  		}
>  	}
>  }
> diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> index 436f3c8779c1..6fed63d013b4 100644
> --- a/drivers/net/ethernet/airoha/airoha_regs.h
> +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> @@ -327,9 +327,8 @@
>  #define PPE_SRAM_TABLE_EN_MASK			BIT(0)
>  
>  #define REG_PPE_MTU_BASE(_n)			(((_n) ? PPE2_BASE : PPE1_BASE) + 0x304)
> -#define REG_PPE_MTU(_m, _n)			(REG_PPE_MTU_BASE(_m) + ((_n) << 2))
> -#define FP1_EGRESS_MTU_MASK			GENMASK(29, 16)
> -#define FP0_EGRESS_MTU_MASK			GENMASK(13, 0)
> +#define REG_PPE_MTU(_m, _n)			(REG_PPE_MTU_BASE(_m) + (((_n) / 2) << 2))
> +#define FP_EGRESS_MTU_MASK(_n)			GENMASK(13 + (((_n) % 2) << 4), ((_n) % 2) << 4)
>  
>  #define REG_PPE_RAM_CTRL(_n)			(((_n) ? PPE2_BASE : PPE1_BASE) + 0x31c)
>  #define PPE_SRAM_CTRL_ACK_MASK			BIT(31)
> @@ -377,6 +376,10 @@
>  #define REG_SRC_PORT_FC_MAP6		0x2298
>  #define FC_ID_OF_SRC_PORT_MASK(_n)	GENMASK(4 + ((_n) << 3), ((_n) << 3))
>  
> +#define REG_WAN_MTU0			0x2300
> +#define WAN_MTU1_MASK			GENMASK(29, 16)
> +#define WAN_MTU0_MASK			GENMASK(13, 0)
> +
>  #define REG_CDM5_RX_OQ1_DROP_CNT	0x29d4
>  
>  /* QDMA */
> 
> ---
> base-commit: fd1269e454089abda0e4f9e5e25ecd02a90ab009
> change-id: 20260618-airoha-fix-rx-max-len-57654b661646
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

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