[PATCH v3 net-next 0/5] net: fec: add the Jumbo frame support

[PATCH v3 net-next 0/5] net: fec: add the Jumbo frame support

[Shenwei Wang]

Changes in v3:
 - modify the OPT_FRAME_SIZE definition and drop the condition logic
 - address the review comments from Wei Fang

Changes in v2:
 - split the v1 patch per Andrew's feedback.

Shenwei Wang (5):
  net: fec: use a member variable for maximum buffer size
  net: fec: add pagepool_order to support variable page size
  net: fec: add rx_frame_size to support configurable RX length
  net: fec: add change_mtu to support dynamic buffer allocation
  net: fec: enable the Jumbo frame support for i.MX8QM

 drivers/net/ethernet/freescale/fec.h      |  6 ++
 drivers/net/ethernet/freescale/fec_main.c | 82 ++++++++++++++++++++---
 2 files changed, 77 insertions(+), 11 deletions(-)

--
2.43.0

[PATCH v3 net-next 1/5] net: fec: use a member variable for maximum buffer size

[Shenwei Wang]

Refactor code to support Jumbo frame functionality by adding a member
variable in the fec_enet_private structure to store PKT_MAXBUF_SIZE.

Cc: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      | 1 +
 drivers/net/ethernet/freescale/fec_main.c | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 5c8fdcef759b..2969088dda09 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -619,6 +619,7 @@ struct fec_enet_private {
 
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
+	unsigned int max_buf_size;
 
 	struct	platform_device *pdev;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1383918f8a3f..5a21000aca59 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -253,7 +253,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
     defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
     defined(CONFIG_ARM64)
-#define	OPT_FRAME_SIZE	(PKT_MAXBUF_SIZE << 16)
+#define	OPT_FRAME_SIZE	(fep->max_buf_size << 16)
 #else
 #define	OPT_FRAME_SIZE	0
 #endif
@@ -1083,7 +1083,7 @@ static void fec_enet_enable_ring(struct net_device *ndev)
 	for (i = 0; i < fep->num_rx_queues; i++) {
 		rxq = fep->rx_queue[i];
 		writel(rxq->bd.dma, fep->hwp + FEC_R_DES_START(i));
-		writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_R_BUFF_SIZE(i));
+		writel(fep->max_buf_size, fep->hwp + FEC_R_BUFF_SIZE(i));
 
 		/* enable DMA1/2 */
 		if (i)
@@ -1191,7 +1191,7 @@ fec_restart(struct net_device *ndev)
 		else
 			val &= ~FEC_RACC_OPTIONS;
 		writel(val, fep->hwp + FEC_RACC);
-		writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
+		writel(fep->max_buf_size, fep->hwp + FEC_FTRL);
 	}
 #endif
 
@@ -4559,7 +4559,8 @@ fec_probe(struct platform_device *pdev)
 	fec_enet_clk_enable(ndev, false);
 	pinctrl_pm_select_sleep_state(&pdev->dev);
 
-	ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN;
+	fep->max_buf_size = PKT_MAXBUF_SIZE;
+	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
 
 	ret = register_netdev(ndev);
 	if (ret)
-- 
2.43.0

[PATCH v3 net-next 2/5] net: fec: add pagepool_order to support variable page size

[Shenwei Wang]

Add a new pagepool_order member in the fec_enet_private struct
to allow dynamic configuration of page size for an instance. This
change clears the hardcoded page size assumptions.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      | 1 +
 drivers/net/ethernet/freescale/fec_main.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 2969088dda09..47317346b2f3 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -620,6 +620,7 @@ struct fec_enet_private {
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
 	unsigned int max_buf_size;
+	unsigned int pagepool_order;
 
 	struct	platform_device *pdev;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 5a21000aca59..f046d32a62fb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1780,7 +1780,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 	 * These get messed up if we get called due to a busy condition.
 	 */
 	bdp = rxq->bd.cur;
-	xdp_init_buff(&xdp, PAGE_SIZE, &rxq->xdp_rxq);
+	xdp_init_buff(&xdp, (PAGE_SIZE << fep->pagepool_order), &rxq->xdp_rxq);
 
 	while (!((status = fec16_to_cpu(bdp->cbd_sc)) & BD_ENET_RX_EMPTY)) {
 
@@ -1850,7 +1850,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 		 * include that when passing upstream as it messes up
 		 * bridging applications.
 		 */
-		skb = build_skb(page_address(page), PAGE_SIZE);
+		skb = build_skb(page_address(page), (PAGE_SIZE << fep->pagepool_order));
 		if (unlikely(!skb)) {
 			page_pool_recycle_direct(rxq->page_pool, page);
 			ndev->stats.rx_dropped++;
@@ -4559,6 +4559,7 @@ fec_probe(struct platform_device *pdev)
 	fec_enet_clk_enable(ndev, false);
 	pinctrl_pm_select_sleep_state(&pdev->dev);
 
+	fep->pagepool_order = 0;
 	fep->max_buf_size = PKT_MAXBUF_SIZE;
 	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
 
-- 
2.43.0

[PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Shenwei Wang]

Add a new rx_frame_size member in the fec_enet_private structure to
decouple frame size configuration from max_buf_size. This allows more
precise control over RX frame length settings. It is particularly useful
for Jumbo frame support because the RX frame size may possible larger than
the allocated RX buffer.

Configure TRUNC_FL (Frame Truncation Length) based on the RX buffer size.
Frames exceeding this limit will be treated as error packets and dropped.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      | 1 +
 drivers/net/ethernet/freescale/fec_main.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 47317346b2f3..f1032a11aa76 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -621,6 +621,7 @@ struct fec_enet_private {
 	unsigned int total_rx_ring_size;
 	unsigned int max_buf_size;
 	unsigned int pagepool_order;
+	unsigned int rx_frame_size;
 
 	struct	platform_device *pdev;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f046d32a62fb..3f25db23d0de 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1191,7 +1191,7 @@ fec_restart(struct net_device *ndev)
 		else
 			val &= ~FEC_RACC_OPTIONS;
 		writel(val, fep->hwp + FEC_RACC);
-		writel(fep->max_buf_size, fep->hwp + FEC_FTRL);
+		writel(fep->rx_frame_size, fep->hwp + FEC_FTRL);
 	}
 #endif
 
@@ -4560,6 +4560,7 @@ fec_probe(struct platform_device *pdev)
 	pinctrl_pm_select_sleep_state(&pdev->dev);
 
 	fep->pagepool_order = 0;
+	fep->rx_frame_size = FEC_ENET_RX_FRSIZE;
 	fep->max_buf_size = PKT_MAXBUF_SIZE;
 	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
 
-- 
2.43.0

[PATCH v3 net-next 4/5] net: fec: add change_mtu to support dynamic buffer allocation

[Shenwei Wang]

Add a fec_change_mtu() handler to recalculate the pagepool_order based
on the new_mtu value. It will update the rx_frame_size accordingly if
the pagepool_order is changed.

If the interface is running, it stops RX/TX, and recreate the pagepool
with the new configuration.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 46 ++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3f25db23d0de..aeeb2f81313c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -470,14 +470,14 @@ fec_enet_create_page_pool(struct fec_enet_private *fep,
 {
 	struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
 	struct page_pool_params pp_params = {
-		.order = 0,
+		.order = fep->pagepool_order,
 		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
 		.pool_size = size,
 		.nid = dev_to_node(&fep->pdev->dev),
 		.dev = &fep->pdev->dev,
 		.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE,
 		.offset = FEC_ENET_XDP_HEADROOM,
-		.max_len = FEC_ENET_RX_FRSIZE,
+		.max_len = fep->rx_frame_size,
 	};
 	int err;
 
@@ -4020,6 +4020,47 @@ static int fec_hwtstamp_set(struct net_device *ndev,
 	return fec_ptp_set(ndev, config, extack);
 }
 
+static int fec_change_mtu(struct net_device *ndev, int new_mtu)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	int order, done;
+
+	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
+	if (fep->pagepool_order == order) {
+		WRITE_ONCE(ndev->mtu, new_mtu);
+		return 0;
+	}
+
+	fep->pagepool_order = order;
+	fep->rx_frame_size = (PAGE_SIZE << order) - FEC_ENET_XDP_HEADROOM
+			     - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+
+	if (!netif_running(ndev)) {
+		WRITE_ONCE(ndev->mtu, new_mtu);
+		return 0;
+	}
+
+	/* Stop TX/RX and free the buffers */
+	napi_disable(&fep->napi);
+	netif_tx_disable(ndev);
+	read_poll_timeout(fec_enet_rx_napi, done, (done == 0),
+			  10, 1000, false, &fep->napi, 10);
+	fec_stop(ndev);
+	fec_enet_free_buffers(ndev);
+
+	WRITE_ONCE(ndev->mtu, new_mtu);
+
+	/* Create the pagepool according the new mtu */
+	if (fec_enet_alloc_buffers(ndev) < 0)
+		return -ENOMEM;
+
+	fec_restart(ndev);
+	napi_enable(&fep->napi);
+	netif_tx_start_all_queues(ndev);
+
+	return 0;
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
@@ -4029,6 +4070,7 @@ static const struct net_device_ops fec_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_tx_timeout		= fec_timeout,
 	.ndo_set_mac_address	= fec_set_mac_address,
+	.ndo_change_mtu		= fec_change_mtu,
 	.ndo_eth_ioctl		= phy_do_ioctl_running,
 	.ndo_set_features	= fec_set_features,
 	.ndo_bpf		= fec_enet_bpf,
-- 
2.43.0

[PATCH v3 net-next 5/5] net: fec: enable the Jumbo frame support for i.MX8QM

[Shenwei Wang]

Certain i.MX SoCs, such as i.MX8QM and i.MX8QXP, feature enhanced
FEC hardware that supports Ethernet Jumbo frames with packet sizes
up to 16K bytes.

When Jumbo frames are enabled, the TX FIFO may not be large enough
to hold an entire frame. To accommodate this, the FIFO should be
configured to operate in cut-through mode, which allows transmission
to begin once the FIFO reaches a certain threshold.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      |  3 +++
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index f1032a11aa76..6802773c5f34 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -513,6 +513,9 @@ struct bufdesc_ex {
  */
 #define FEC_QUIRK_HAS_MDIO_C45		BIT(24)
 
+/* Jumbo Frame support */
+#define FEC_QUIRK_JUMBO_FRAME		BIT(25)
+
 struct bufdesc_prop {
 	int qid;
 	/* Address of Rx and Tx buffers */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index aeeb2f81313c..d932417f4d4c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -167,7 +167,8 @@ static const struct fec_devinfo fec_imx8qm_info = {
 		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
 		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
 		  FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES |
-		  FEC_QUIRK_DELAYED_CLKS_SUPPORT | FEC_QUIRK_HAS_MDIO_C45,
+		  FEC_QUIRK_DELAYED_CLKS_SUPPORT | FEC_QUIRK_HAS_MDIO_C45 |
+		  FEC_QUIRK_JUMBO_FRAME,
 };
 
 static const struct fec_devinfo fec_s32v234_info = {
@@ -233,6 +234,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
  * 2048 byte skbufs are allocated. However, alignment requirements
  * varies between FEC variants. Worst case is 64, so round down by 64.
  */
+#define MAX_JUMBO_BUF_SIZE	(round_down(16384 - 64, 64))
 #define PKT_MAXBUF_SIZE		(round_down(2048 - 64, 64))
 #define PKT_MINBUF_SIZE		64
 
@@ -1278,8 +1280,16 @@ fec_restart(struct net_device *ndev)
 	if (fep->quirks & FEC_QUIRK_ENET_MAC) {
 		/* enable ENET endian swap */
 		ecntl |= FEC_ECR_BYTESWP;
-		/* enable ENET store and forward mode */
-		writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
+
+		/* When Jumbo Frame is enabled, the FIFO may not be large enough
+		 * to hold an entire frame. In this case, configure the interface
+		 * to operate in cut-through mode, triggered by the FIFO threshold.
+		 * Otherwise, enable the ENET store-and-forward mode.
+		 */
+		if (fep->quirks & FEC_QUIRK_JUMBO_FRAME)
+			writel(0xF, fep->hwp + FEC_X_WMRK);
+		else
+			writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
 	}
 
 	if (fep->bufdesc_ex)
@@ -4603,7 +4613,12 @@ fec_probe(struct platform_device *pdev)
 
 	fep->pagepool_order = 0;
 	fep->rx_frame_size = FEC_ENET_RX_FRSIZE;
-	fep->max_buf_size = PKT_MAXBUF_SIZE;
+
+	if (fep->quirks & FEC_QUIRK_JUMBO_FRAME)
+		fep->max_buf_size = MAX_JUMBO_BUF_SIZE;
+	else
+		fep->max_buf_size = PKT_MAXBUF_SIZE;
+
 	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
 
 	ret = register_netdev(ndev);
-- 
2.43.0

Re: [PATCH v3 net-next 1/5] net: fec: use a member variable for maximum buffer size

[Andrew Lunn]

On Sat, Aug 23, 2025 at 02:01:06PM -0500, Shenwei Wang wrote:
> Refactor code to support Jumbo frame functionality by adding a member
> variable in the fec_enet_private structure to store PKT_MAXBUF_SIZE.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>

This looks sensible now. Thanks

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 2/5] net: fec: add pagepool_order to support variable page size

[Andrew Lunn]

On Sat, Aug 23, 2025 at 02:01:07PM -0500, Shenwei Wang wrote:
> Add a new pagepool_order member in the fec_enet_private struct
> to allow dynamic configuration of page size for an instance. This
> change clears the hardcoded page size assumptions.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Andrew Lunn]

On Sat, Aug 23, 2025 at 02:01:08PM -0500, Shenwei Wang wrote:
> Add a new rx_frame_size member in the fec_enet_private structure to
> decouple frame size configuration from max_buf_size. This allows more
> precise control over RX frame length settings. It is particularly useful
> for Jumbo frame support because the RX frame size may possible larger than
> the allocated RX buffer.

Please could you extend that a little. What happens if the received
frame is bigger than the buffer? Does the hardware fragment it over
two buffers?

> 
> Configure TRUNC_FL (Frame Truncation Length) based on the RX buffer size.
> Frames exceeding this limit will be treated as error packets and dropped.

This bit confuses me. You want to allow rx_frame_size to be bigger
than the buffer size, but you also want to discard frames bigger than
the buffer size?

    Andrew

---
pw-bot: cr

Re: [PATCH v3 net-next 4/5] net: fec: add change_mtu to support dynamic buffer allocation

[Andrew Lunn]

> +static int fec_change_mtu(struct net_device *ndev, int new_mtu)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	int order, done;
> +
> +	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
> +	if (fep->pagepool_order == order) {
> +		WRITE_ONCE(ndev->mtu, new_mtu);
> +		return 0;
> +	}
> +
> +	fep->pagepool_order = order;
> +	fep->rx_frame_size = (PAGE_SIZE << order) - FEC_ENET_XDP_HEADROOM
> +			     - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +
> +	if (!netif_running(ndev)) {
> +		WRITE_ONCE(ndev->mtu, new_mtu);
> +		return 0;
> +	}
> +
> +	/* Stop TX/RX and free the buffers */
> +	napi_disable(&fep->napi);
> +	netif_tx_disable(ndev);
> +	read_poll_timeout(fec_enet_rx_napi, done, (done == 0),
> +			  10, 1000, false, &fep->napi, 10);
> +	fec_stop(ndev);
> +	fec_enet_free_buffers(ndev);
> +
> +	WRITE_ONCE(ndev->mtu, new_mtu);
> +
> +	/* Create the pagepool according the new mtu */
> +	if (fec_enet_alloc_buffers(ndev) < 0)
> +		return -ENOMEM;
> +

This is the wrong order. You need to first allocate the new buffers
and then free the old ones. If the allocation fails, you still have a
working interface using the smaller buffers, and the MTU change just
returns -ENOMEM. If you free the buffers and the allocation of the new
buffers fail, you interface is dead, because it has no buffers.

	Andrew

Re: [PATCH v3 net-next 5/5] net: fec: enable the Jumbo frame support for i.MX8QM

[Andrew Lunn]

> @@ -1278,8 +1280,16 @@ fec_restart(struct net_device *ndev)
>  	if (fep->quirks & FEC_QUIRK_ENET_MAC) {
>  		/* enable ENET endian swap */
>  		ecntl |= FEC_ECR_BYTESWP;
> -		/* enable ENET store and forward mode */
> -		writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
> +
> +		/* When Jumbo Frame is enabled, the FIFO may not be large enough
> +		 * to hold an entire frame. In this case, configure the interface
> +		 * to operate in cut-through mode, triggered by the FIFO threshold.
> +		 * Otherwise, enable the ENET store-and-forward mode.
> +		 */
> +		if (fep->quirks & FEC_QUIRK_JUMBO_FRAME)
> +			writel(0xF, fep->hwp + FEC_X_WMRK);

The quirk indicates the hardware is capable of jumbo frames, not that
jumbo frames are enabled. Don't you need to compare the mtu with
ETH_FRAME_LEN + ETH_FCS_LEN to say jumbo is enabled?

Is there a counter or other indication that the FIFO experienced an
underflow?

	Andrew

Re: [PATCH v3 net-next 4/5] net: fec: add change_mtu to support dynamic buffer allocation

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, August 23, 2025 2:17 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Stanislav Fomichev <sdf@fomichev.me>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-
> linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net-next 4/5] net: fec: add change_mtu to support
> dynamic buffer allocation
> > +
> > +     WRITE_ONCE(ndev->mtu, new_mtu);
> > +
> > +     /* Create the pagepool according the new mtu */
> > +     if (fec_enet_alloc_buffers(ndev) < 0)
> > +             return -ENOMEM;
> > +
> 
> This is the wrong order. You need to first allocate the new buffers and then free
> the old ones. If the allocation fails, you still have a working interface using the
> smaller buffers, and the MTU change just returns -ENOMEM. If you free the
> buffers and the allocation of the new buffers fail, you interface is dead, because it
> has no buffers.
> 

I've considered that. I was wondering-if there's no available memory, should it still be 
treated as a critical issue for the system in this case?

The current API doesn't support allocating memory first and freeing it only upon success. 
Supporting such behavior would require a redesign of the buffer management flow.

As an alternative, we could attempt to fall back to allocating buffers of the original size if the 
operation fails. However, this still doesn't guarantee success.

Regards,
Shenwei

>         Andrew

Re: [PATCH v3 net-next 5/5] net: fec: enable the Jumbo frame support for i.MX8QM

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, August 23, 2025 2:26 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Stanislav Fomichev <sdf@fomichev.me>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-
> linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net-next 5/5] net: fec: enable the Jumbo frame
> support for i.MX8QM
> > -             /* enable ENET store and forward mode */
> > -             writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
> > +
> > +             /* When Jumbo Frame is enabled, the FIFO may not be large enough
> > +              * to hold an entire frame. In this case, configure the interface
> > +              * to operate in cut-through mode, triggered by the FIFO threshold.
> > +              * Otherwise, enable the ENET store-and-forward mode.
> > +              */
> > +             if (fep->quirks & FEC_QUIRK_JUMBO_FRAME)
> > +                     writel(0xF, fep->hwp + FEC_X_WMRK);
> 
> The quirk indicates the hardware is capable of jumbo frames, not that jumbo
> frames are enabled. Don't you need to compare the mtu with ETH_FRAME_LEN +
> ETH_FCS_LEN to say jumbo is enabled?
> 

The comments here do have some confusion. The goal is to enable cut-through mode 
when the hardware supports Jumbo frames.  But we can limit the scope to enable it
only when MTU is less than 2k bytes. 

> Is there a counter or other indication that the FIFO experienced an underflow?
> 

There is a Underrun bit in the status field in the TX buffer descriptor. The hardware 
supports retransmit frames if high memory latency is encountered due to other 
high-priority bus masters.

Thanks,
Shenwei

>         Andrew

Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, August 23, 2025 2:11 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Stanislav Fomichev <sdf@fomichev.me>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-
> linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to
> support configurable RX length
> On Sat, Aug 23, 2025 at 02:01:08PM -0500, Shenwei Wang wrote:
> > Add a new rx_frame_size member in the fec_enet_private structure to
> > decouple frame size configuration from max_buf_size. This allows more
> > precise control over RX frame length settings. It is particularly
> > useful for Jumbo frame support because the RX frame size may possible
> > larger than the allocated RX buffer.
> 
> Please could you extend that a little. What happens if the received frame is bigger
> than the buffer? Does the hardware fragment it over two buffers?
> 

The hardware doesn't have the capability to fragment received frames that exceed the MAX_FL 
value. Instead, it flags an overrun error in the status register when such frames are encountered.

> >
> > Configure TRUNC_FL (Frame Truncation Length) based on the RX buffer size.
> > Frames exceeding this limit will be treated as error packets and dropped.
> 
> This bit confuses me. You want to allow rx_frame_size to be bigger than the
> buffer size, but you also want to discard frames bigger than the buffer size?
> 

MAX_FL defines the maximum allowable frame length, while TRUNC_FL specifies the 
threshold beyond which frames are truncated.  

Here, TRUNC_FL is configured based on the RX buffer size, allowing the hardware to 
handle oversized frame errors automatically without requiring software intervention.

Regards,
Shenwei

>     Andrew
> 
> ---
> pw-bot: cr

Re: [PATCH v3 net-next 5/5] net: fec: enable the Jumbo frame support for i.MX8QM

[Andrew Lunn]

> > > -             /* enable ENET store and forward mode */
> > > -             writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
> > > +
> > > +             /* When Jumbo Frame is enabled, the FIFO may not be large enough
> > > +              * to hold an entire frame. In this case, configure the interface
> > > +              * to operate in cut-through mode, triggered by the FIFO threshold.
> > > +              * Otherwise, enable the ENET store-and-forward mode.
> > > +              */
> > > +             if (fep->quirks & FEC_QUIRK_JUMBO_FRAME)
> > > +                     writel(0xF, fep->hwp + FEC_X_WMRK);
> > 
> > The quirk indicates the hardware is capable of jumbo frames, not that jumbo
> > frames are enabled. Don't you need to compare the mtu with ETH_FRAME_LEN +
> > ETH_FCS_LEN to say jumbo is enabled?
> > 
> 
> The comments here do have some confusion. The goal is to enable cut-through mode 
> when the hardware supports Jumbo frames.  But we can limit the scope to enable it
> only when MTU is less than 2k bytes. 

That would be good. I've no idea how many embedded systems actually
use jumbo packets. Maybe your marketing people can tell you. 1%? 5%?
But you are changing this for 100% of your customers, which might
cause regressions. So it would be better to only enable this when
needed.

> > Is there a counter or other indication that the FIFO experienced an underflow?
> > 
> 
> There is a Underrun bit in the status field in the TX buffer descriptor. The hardware 
> supports retransmit frames if high memory latency is encountered due to other 
> high-priority bus masters.

So this:
                        if (status & BD_ENET_TX_UN)  /* Underrun */
                                ndev->stats.tx_fifo_errors++;

Good to see there is a counter for it.

	Andrew

Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Andrew Lunn]

> > > Add a new rx_frame_size member in the fec_enet_private structure to
> > > decouple frame size configuration from max_buf_size. This allows more
> > > precise control over RX frame length settings. It is particularly
> > > useful for Jumbo frame support because the RX frame size may possible
> > > larger than the allocated RX buffer.
> > 
> > Please could you extend that a little. What happens if the received frame is bigger
> > than the buffer? Does the hardware fragment it over two buffers?
> > 
> 
> The hardware doesn't have the capability to fragment received frames that exceed the MAX_FL 
> value. Instead, it flags an overrun error in the status register when such frames are encountered.

And how is this useful for jumbo? Why would i want the RX frame size
bigger than the RX buffer size?

> > >
> > > Configure TRUNC_FL (Frame Truncation Length) based on the RX buffer size.
> > > Frames exceeding this limit will be treated as error packets and dropped.
> > 
> > This bit confuses me. You want to allow rx_frame_size to be bigger than the
> > buffer size, but you also want to discard frames bigger than the buffer size?
> > 
> 
> MAX_FL defines the maximum allowable frame length, while TRUNC_FL specifies the 
> threshold beyond which frames are truncated.  
> 
> Here, TRUNC_FL is configured based on the RX buffer size, allowing the hardware to 
> handle oversized frame errors automatically without requiring software intervention.

Please could you expand the commit message.

I still don't quite get it. Why not set MAX_FL = TRUNC_FL = RX buffer
size?

	Andrew

Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, August 23, 2025 3:55 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Stanislav Fomichev <sdf@fomichev.me>;
> imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-
> linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to
> support configurable RX length
> > >
> > > Please could you extend that a little. What happens if the received
> > > frame is bigger than the buffer? Does the hardware fragment it over two
> buffers?
> > >
> >
> > The hardware doesn't have the capability to fragment received frames
> > that exceed the MAX_FL value. Instead, it flags an overrun error in the status
> register when such frames are encountered.
> 
> And how is this useful for jumbo? Why would i want the RX frame size bigger than
> the RX buffer size?
> 

I would say this is purely a software implementation, as the driver simply sets MAX_FL 
to a fixed value, PKT_MAXBUF_SIZE. The patch does not alter that logic. I agree the 
comments need to be rephrased.

> >
> > MAX_FL defines the maximum allowable frame length, while TRUNC_FL
> > specifies the threshold beyond which frames are truncated.
> >
> > Here, TRUNC_FL is configured based on the RX buffer size, allowing the
> > hardware to handle oversized frame errors automatically without requiring
> software intervention.
> 
> Please could you expand the commit message.
> 
> I still don't quite get it. Why not set MAX_FL = TRUNC_FL = RX buffer size?
> 

That's the logic in v2. We added a logic to change MAX_FL.

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1148,8 +1148,8 @@ fec_restart(struct net_device *ndev)
 	u32 rcntl = FEC_RCR_MII;
 	u32 ecntl = FEC_ECR_ETHEREN;
 
-	if (fep->max_buf_size == OPT_FRAME_SIZE)
-		rcntl |= (fep->max_buf_size << 16);
+	if (OPT_FRAME_SIZE != 0)
+		rcntl |= (fep->rx_frame_size << 16);

After further consideration, I think we can simply keep MAX_FL set to max_buf_size and 
only update TRUNC_FL as needed. This approach is also sufficient, so the logic introduced 
in patch v2 has been removed. Let me know if you want to add back the above logic.

Thanks,
Shenwei

>         Andrew

Re: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Andrew Lunn]

> After further consideration, I think we can simply keep MAX_FL set to max_buf_size and 
> only update TRUNC_FL as needed. This approach is also sufficient, so the logic introduced 
> in patch v2 has been removed. Let me know if you want to add back the above logic.

For me, higher priority is you review all the commit messages and
comments. A lot of the discussion here has been because the comments
don't fit the code, and the commit messages don't give enough details
to explain the changes. They are just as important as the code.

	Andrew

RE: [PATCH v3 net-next 1/5] net: fec: use a member variable for maximum buffer size

[Wei Fang]

> Refactor code to support Jumbo frame functionality by adding a member
> variable in the fec_enet_private structure to store PKT_MAXBUF_SIZE.
> 

Reviewed-by: Wei Fang <wei.fang@nxp.com>

RE: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Wei Fang]

> -		writel(fep->max_buf_size, fep->hwp + FEC_FTRL);
> +		writel(fep->rx_frame_size, fep->hwp + FEC_FTRL);
>  	}
>  #endif
> 
> @@ -4560,6 +4560,7 @@ fec_probe(struct platform_device *pdev)
>  	pinctrl_pm_select_sleep_state(&pdev->dev);
> 
>  	fep->pagepool_order = 0;
> +	fep->rx_frame_size = FEC_ENET_RX_FRSIZE;
>  	fep->max_buf_size = PKT_MAXBUF_SIZE;
>  	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
> 

The same comments as in v2, if max_buf_size < rx_frame_size, the packet
will be received by multiple Rx BDs, but the current driver only supports
one packet per Rx BD, so the Rx error statistic will be doubled with this
patch.

RE: [PATCH v3 net-next 4/5] net: fec: add change_mtu to support dynamic buffer allocation

[Wei Fang]

> +static int fec_change_mtu(struct net_device *ndev, int new_mtu) {
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	int order, done;
> +
> +	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
> +	if (fep->pagepool_order == order) {
> +		WRITE_ONCE(ndev->mtu, new_mtu);
> +		return 0;
> +	}
> +
> +	fep->pagepool_order = order;
> +	fep->rx_frame_size = (PAGE_SIZE << order) - FEC_ENET_XDP_HEADROOM
> +			     - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

Same comments as in v2, please check the new value whether is reasonable,
if it exceeds hardware capability, then return an error.

RE: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support configurable RX length

[Shenwei Wang]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Sunday, August 24, 2025 9:47 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Clark Wang <xiaoning.wang@nxp.com>; Stanislav Fomichev
> <sdf@fomichev.me>; imx@lists.linux.dev; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel
> Borkmann <daniel@iogearbox.net>; Jesper Dangaard Brouer
> <hawk@kernel.org>; John Fastabend <john.fastabend@gmail.com>
> Subject: RE: [PATCH v3 net-next 3/5] net: fec: add rx_frame_size to support
> configurable RX length
> 
> > -		writel(fep->max_buf_size, fep->hwp + FEC_FTRL);
> > +		writel(fep->rx_frame_size, fep->hwp + FEC_FTRL);
> >  	}
> >  #endif
> >
> > @@ -4560,6 +4560,7 @@ fec_probe(struct platform_device *pdev)
> >  	pinctrl_pm_select_sleep_state(&pdev->dev);
> >
> >  	fep->pagepool_order = 0;
> > +	fep->rx_frame_size = FEC_ENET_RX_FRSIZE;
> >  	fep->max_buf_size = PKT_MAXBUF_SIZE;
> >  	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;
> >
> 
> The same comments as in v2, if max_buf_size < rx_frame_size, the packet will be
> received by multiple Rx BDs, but the current driver only supports one packet per
> Rx BD, so the Rx error statistic will be doubled with this patch.

That's not the case. In the old implementation, the oversized packets were counted
as the rx_length_errors. In this change, the oversized packets are truncated so the error
becomes rx_errors.

To maintain the same error behavior, I will keep the MAX_FL  and TRUNC_FL the same value
as the original implementation.

Thanks,
Shenwei