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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 12499 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.
> 
> 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>

commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260625-airoha-fix-rx-max-len-v1-1-45b9b827358d%40kernel.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);
>  }
>  
> +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));
> +}

- Could the WAN_MTU0 update here use the same max-across-siblings
  aggregation as airoha_ppe_set_xmit_frame_size()?
  - This is same issue reported by sashiko-gemini. There is just one WAN device
    in the system so we do not need calculate the max MTU here.

> +
>  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);
> @@ -1851,19 +1867,7 @@ static int airoha_dev_open(struct net_device *netdev)
>  		airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
>  				GDM_STAG_EN_MASK);
>  
> -	cur_len = airoha_fe_get(qdma->eth, REG_GDM_LEN_CFG(port->id),
> -				GDM_LONG_LEN_MASK);
> -	if (!port->users || len > cur_len) {
> -		/* Opening a sibling net_device with a larger MTU updates the
> -		 * MTU of already running devices. This is required to allow
> -		 * multiple net_devices with different MTUs to share the same
> -		 * GDM port.
> -		 */
> -		airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> -			      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> -			      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> -			      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> -	}
> +	airoha_dev_set_xmit_frame_size(netdev);
>  	port->users++;
>  
>  	if (!airoha_is_lan_gdm_dev(dev) &&
> @@ -1875,30 +1879,6 @@ static int airoha_dev_open(struct net_device *netdev)
>  	return 0;
>  }
>  
> -static void airoha_set_port_mtu(struct airoha_eth *eth,
> -				struct airoha_gdm_port *port)
> -{
> -	u32 len = 0;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> -		struct airoha_gdm_dev *dev = port->devs[i];
> -		struct net_device *netdev;
> -
> -		if (!dev)
> -			continue;
> -
> -		netdev = netdev_from_priv(dev);
> -		if (netif_running(netdev))
> -			len = max_t(u32, len, netdev->mtu);
> -	}
> -	len += ETH_HLEN + ETH_FCS_LEN;
> -
> -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> -		      GDM_LONG_LEN_MASK,
> -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> -}
> -
>  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);

- On the close path, the call is to airoha_ppe_set_xmit_frame_size()
  directly rather than the airoha_dev_set_xmit_frame_size() wrapper.
  Does this mean WAN_MTU0 is never refreshed when a WAN dev is closed?
  For example, if a small-MTU sibling is closed while a larger-MTU dev
  remains running, the PPE MTU register gets recomputed to the larger
  value but WAN_MTU0 retains the smaller value written at the last open
  or change_mtu.
  The commit message states:
    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.
  Is the asymmetry between PPE MTU (max across siblings) and WAN_MTU0
  (per-netdev write) intentional?
  - This is same issue reported by sashiko-gemini. There is just one WAN device
    in the system so there is no point to update WAN_MTU0 if the WAN device is
    stopped.

Regards,
Lorenzo

>  	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));
> +}
> +
>  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 --]