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

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

[Shenwei Wang]

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
  et: 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 | 88 ++++++++++++++++++++---
 2 files changed, 85 insertions(+), 9 deletions(-)

--
2.43.0

[PATCH v2 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 | 12 ++++++++----
 2 files changed, 9 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..24ce808b0c05 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -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)
@@ -1145,9 +1145,12 @@ static void
 fec_restart(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	u32 rcntl = OPT_FRAME_SIZE | FEC_RCR_MII;
+	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 (fep->bufdesc_ex)
 		fec_ptp_save_state(fep);
 
@@ -1191,7 +1194,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 +4562,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 v2 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 24ce808b0c05..b6ce56051c79 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1783,7 +1783,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)) {
 
@@ -1853,7 +1853,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++;
@@ -4562,6 +4562,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 v2 net-next 3/5] et: 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.

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

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 b6ce56051c79..6d0d97bb077c 100644
--- 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);
 
 	if (fep->bufdesc_ex)
 		fec_ptp_save_state(fep);
@@ -1194,7 +1194,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
 
@@ -4563,6 +4563,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 v2 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 | 44 +++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 6d0d97bb077c..aa85a6d0b44f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4023,6 +4023,49 @@ 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;
+	bool running;
+
+	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));
+
+	running = netif_running(ndev);
+	if (!running) {
+		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,
@@ -4032,6 +4075,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 v2 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 | 28 +++++++++++++++++++----
 2 files changed, 27 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 aa85a6d0b44f..160d49e6f86c 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
 
@@ -481,6 +483,11 @@ fec_enet_create_page_pool(struct fec_enet_private *fep,
 	};
 	int err;
 
+	if (fep->pagepool_order != 0) {
+		pp_params.order = fep->pagepool_order;
+		pp_params.max_len = fep->rx_frame_size;
+	}
+
 	rxq->page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(rxq->page_pool)) {
 		err = PTR_ERR(rxq->page_pool);
@@ -1281,8 +1288,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)
@@ -4608,7 +4623,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 v2 net-next 1/5] net: fec: use a member variable for maximum buffer size

[Andrew Lunn]

On Thu, Aug 21, 2025 at 01:33:32PM -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.

This is better, thanks.

> @@ -1145,9 +1145,12 @@ static void
>  fec_restart(struct net_device *ndev)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	u32 rcntl = OPT_FRAME_SIZE | FEC_RCR_MII;
> +	u32 rcntl = FEC_RCR_MII;
>  	u32 ecntl = FEC_ECR_ETHEREN;
>  
> +	if (fep->max_buf_size == OPT_FRAME_SIZE)
> +		rcntl |= (fep->max_buf_size << 16);

I was expecting something like s/OPT_FRAME_SIZE/fep->max_buf_size/g

This is introducing extra logic. I think the if (...) belongs in
another patch. The assignment is however what i expected.

    Andrew

---
pw-bot: cr

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, August 21, 2025 1:56 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 v2 net-next 1/5] net: fec: use a member variable for
> maximum buffer size
> 
> > @@ -1145,9 +1145,12 @@ static void
> >  fec_restart(struct net_device *ndev)
> >  {
> >       struct fec_enet_private *fep = netdev_priv(ndev);
> > -     u32 rcntl = OPT_FRAME_SIZE | FEC_RCR_MII;
> > +     u32 rcntl = FEC_RCR_MII;
> >       u32 ecntl = FEC_ECR_ETHEREN;
> >
> > +     if (fep->max_buf_size == OPT_FRAME_SIZE)
> > +             rcntl |= (fep->max_buf_size << 16);
> 
> I was expecting something like s/OPT_FRAME_SIZE/fep->max_buf_size/g
> 

We can't simply replace the value here because it needs to be left-shifted by 16 bits. I intentionally 
preserved the original logic for consistency. In patch #3, the max_buf_size may become a different
one for Jumbo frame mode and the condition is updated to:
+	if (OPT_FRAME_SIZE != 0)

Thanks,
Shenwei

> This is introducing extra logic. I think the if (...) belongs in another patch. The
> assignment is however what i expected.
> 
>     Andrew
> 
> ---
> pw-bot: cr

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, August 21, 2025 1:56 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 v2 net-next 1/5] net: fec: use a member variable for
> maximum buffer size
> 
> > @@ -1145,9 +1145,12 @@ static void
> >  fec_restart(struct net_device *ndev)
> >  {
> >       struct fec_enet_private *fep = netdev_priv(ndev);
> > -     u32 rcntl = OPT_FRAME_SIZE | FEC_RCR_MII;
> > +     u32 rcntl = FEC_RCR_MII;
> >       u32 ecntl = FEC_ECR_ETHEREN;
> >
> > +     if (fep->max_buf_size == OPT_FRAME_SIZE)
> > +             rcntl |= (fep->max_buf_size << 16);
> 
> I was expecting something like s/OPT_FRAME_SIZE/fep->max_buf_size/g
> 
> This is introducing extra logic. I think the if (...) belongs in another patch. The
> assignment is however what i expected.

We just had an internal discussion and found that updating the macro definition is the simplest 
and most straightforward solution, as shown below. With this change, the above modifications 
are no longer necessary.

-#define        OPT_FRAME_SIZE  (PKT_MAXBUF_SIZE << 16)
+#define        OPT_FRAME_SIZE  (fep->max_buf_size << 16)
 #else
 #define        OPT_FRAME_SIZE  0

Thanks,
Shenwei

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

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

[Wei Fang]

>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	u32 rcntl = OPT_FRAME_SIZE | FEC_RCR_MII;
> +	u32 rcntl = FEC_RCR_MII;

nit: move this line under 'u32 ecntl = FEC_ECR_ETHEREN;'
see https://elixir.bootlin.com/linux/v6.17-rc2/source/Documentation/process/maintainer-netdev.rst#L368

>  	u32 ecntl = FEC_ECR_ETHEREN;
> 
> +	if (fep->max_buf_size == OPT_FRAME_SIZE)

fep->max_buf_size = PKT_MAXBUF_SIZE;
#define	OPT_FRAME_SIZE	(PKT_MAXBUF_SIZE << 16)

So fep->max_buf_size will never be equal to OPT_FRAME_SIZE,
so always take the false branch.

> +		rcntl |= (fep->max_buf_size << 16);

nit: the parentheses are unnecessary

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

[Wei Fang]

> Subject: [PATCH v2 net-next 3/5] et: fec: add rx_frame_size to support

nit: 'et' --> 'net'

RE: [PATCH v2 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;
> +	bool running;
> +
> +	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
> +	if (fep->pagepool_order == order) {
> +		WRITE_ONCE(ndev->mtu, new_mtu);

No need to write ndev->mtu, same below, because __netif_set_mtu() will
help update it.

> +		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));
> +
> +	running = netif_running(ndev);
> +	if (!running) {

running is only used once, so we can use netif_running(ndev) directly
to simplify the code.

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

I'm not sure whether read_poll_timeout() is necessary, because I
get the info from the kernel doc of napi_disable()

/**
 * napi_disable() - prevent NAPI from scheduling
 * @n: NAPI context
 *
 * Stop NAPI from being scheduled on this context.
 * Waits till any outstanding processing completes.
 * Takes netdev_lock() for associated net_device.
 */

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

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

[Wei Fang]

> @@ -481,6 +483,11 @@ fec_enet_create_page_pool(struct fec_enet_private
> *fep,
>  	};
>  	int err;
> 
> +	if (fep->pagepool_order != 0) {
> +		pp_params.order = fep->pagepool_order;
> +		pp_params.max_len = fep->rx_frame_size;
> +	}
> +

We can move this logic to the initialization of pp_params, just as follows.

struct page_pool_params pp_params = {
		.order = fep->pagepool_order,
		[...]
		.max_len = fep->rx_frame_size,
};

A question: shouldn't this change be in the 4th patch ("net: fec: add change
mtu to support dynamic buffer allocation") ?

RE: [PATCH v2 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;
> +	bool running;
> +
> +	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));
> +

I think we need to add a check for rx_frame_size, as FEC_R_CNTRL[MAX_FL]
and FEC_FTRL[TRUNC_FL] only have 14 bits.

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

[Wei Fang]

> @@ -4563,6 +4563,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;

According to the RM, to allow one maximum size frame per buffer,
FEC_R_BUFF_SIZE must be set to FEC_R_CNTRL [MAX_FL] or larger.
FEC_ENET_RX_FRSIZE is greater than PKT_MAXBUF_SIZE, I'm not
sure whether it will cause some unknown issues.

>  	fep->max_buf_size = PKT_MAXBUF_SIZE;
>  	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;

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

[Shenwei Wang]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, August 22, 2025 5:10 AM
> 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 v2 net-next 3/5] et: fec: add rx_frame_size to support
> configurable RX length
> 
> > @@ -4563,6 +4563,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;
> 
> According to the RM, to allow one maximum size frame per buffer,
> FEC_R_BUFF_SIZE must be set to FEC_R_CNTRL [MAX_FL] or larger.
> FEC_ENET_RX_FRSIZE is greater than PKT_MAXBUF_SIZE, I'm not sure whether it
> will cause some unknown issues.

MAX_FL defines the maximum allowable frame length, while TRUNC_FL specifies the threshold beyond 
which frames are truncated. Based on this logic, the documentation appears to be incorrect-TRUNC_FL 
should never exceed MAX_FL, as doing so would make the truncation mechanism ineffective. 

This has been confirmed through testing data.

Regards,
Shenwei

> 
> >  	fep->max_buf_size = PKT_MAXBUF_SIZE;
> >  	ndev->max_mtu = fep->max_buf_size - ETH_HLEN - ETH_FCS_LEN;

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

[Shenwei Wang]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, August 22, 2025 3:45 AM
> 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 v2 net-next 4/5] net: fec: add change_mtu to support
> dynamic buffer allocation
> 
> > +static int fec_change_mtu(struct net_device *ndev, int new_mtu) {
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	int order, done;
> > +	bool running;
> > +
> > +	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
> > +	if (fep->pagepool_order == order) {
> > +		WRITE_ONCE(ndev->mtu, new_mtu);
> 
> No need to write ndev->mtu, same below, because __netif_set_mtu() will help
> update it.

It will only update the ndev->mtu if the driver doesn't have its own chang_mtu handler.

int __netif_set_mtu(struct net_device *dev, int new_mtu)
{
	const struct net_device_ops *ops = dev->netdev_ops;

	if (ops->ndo_change_mtu)
		return ops->ndo_change_mtu(dev, new_mtu);

	/* Pairs with all the lockless reads of dev->mtu in the stack */
	WRITE_ONCE(dev->mtu, new_mtu);
	return 0;
}

Regards,
Shenwei

> 
> > +		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));
> > +
> > +	running = netif_running(ndev);
> > +	if (!running) {
> 
> running is only used once, so we can use netif_running(ndev) directly to simplify
> the code.
> 
> > +		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);
> 
> I'm not sure whether read_poll_timeout() is necessary, because I get the info
> from the kernel doc of napi_disable()
> 
> /**
>  * napi_disable() - prevent NAPI from scheduling
>  * @n: NAPI context
>  *
>  * Stop NAPI from being scheduled on this context.
>  * Waits till any outstanding processing completes.
>  * Takes netdev_lock() for associated net_device.
>  */
> 
> > +	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;
> > +}
> > +

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

[Shenwei Wang]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, August 22, 2025 4:49 AM
> 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 v2 net-next 4/5] net: fec: add change_mtu to support
> dynamic buffer allocation
> 
> > +static int fec_change_mtu(struct net_device *ndev, int new_mtu) {
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	int order, done;
> > +	bool running;
> > +
> > +	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));
> > +
> 
> I think we need to add a check for rx_frame_size, as FEC_R_CNTRL[MAX_FL] and
> FEC_FTRL[TRUNC_FL] only have 14 bits.

That would be redundant, since rx_frame_size cannot exceed max_buf_size which value
would either be PKT_MAXBUF_SIZE or MAX_JUMBO_BUF_SIZE.

Regards,
Shenwei

RE: [PATCH v2 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;
> > > +	bool running;
> > > +
> > > +	order = get_order(new_mtu + ETH_HLEN + ETH_FCS_LEN);
> > > +	if (fep->pagepool_order == order) {
> > > +		WRITE_ONCE(ndev->mtu, new_mtu);
> >
> > No need to write ndev->mtu, same below, because __netif_set_mtu() will
> > help update it.
> 
> It will only update the ndev->mtu if the driver doesn't have its own chang_mtu
> handler.
> 
> int __netif_set_mtu(struct net_device *dev, int new_mtu) {
> 	const struct net_device_ops *ops = dev->netdev_ops;
> 
> 	if (ops->ndo_change_mtu)
> 		return ops->ndo_change_mtu(dev, new_mtu);
> 
> 	/* Pairs with all the lockless reads of dev->mtu in the stack */
> 	WRITE_ONCE(dev->mtu, new_mtu);
> 	return 0;
> }

Oh, I misread the code, so sorry.

RE: [PATCH v2 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;
> > > +	bool running;
> > > +
> > > +	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));
> > > +
> >
> > I think we need to add a check for rx_frame_size, as FEC_R_CNTRL[MAX_FL]
> and
> > FEC_FTRL[TRUNC_FL] only have 14 bits.
> 
> That would be redundant, since rx_frame_size cannot exceed max_buf_size
> which value
> would either be PKT_MAXBUF_SIZE or MAX_JUMBO_BUF_SIZE.
> 

Looked at the entire patch set, the rx_frame_size is set to FEC_FTRL[TRUNC_FL]
and FEC_R_CNTRL[MAX_FL], and both TRUNC_FL and MAX_FL are 14 bits, if the
value set exceeds the hardware capability, the driver should return an error.

For example, the order is 3, so rx_frame_size is 0x7dc0, but MAX_FL will be set
to 0x3dc0, that is not correct.

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

[Wei Fang]

> > > @@ -4563,6 +4563,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;
> >
> > According to the RM, to allow one maximum size frame per buffer,
> > FEC_R_BUFF_SIZE must be set to FEC_R_CNTRL [MAX_FL] or larger.
> > FEC_ENET_RX_FRSIZE is greater than PKT_MAXBUF_SIZE, I'm not sure
> whether it
> > will cause some unknown issues.
> 
> MAX_FL defines the maximum allowable frame length, while TRUNC_FL
> specifies the threshold beyond
> which frames are truncated. Based on this logic, the documentation appears to
> be incorrect-TRUNC_FL
> should never exceed MAX_FL, as doing so would make the truncation
> mechanism ineffective.
> 
> This has been confirmed through testing data.
> 

One obvious issue I can see is that the Rx error statistic would be doubled
the actual number when the FEC receives jumbo frames.

For example, the sender sends 1000 jumbo frames (8000 bytes) to the FEC port,
without this patch set, the Rx error statistic of FEC should be 1000, however,
after applying this patch set (rx_frame_size is 3520, max_buf_size is 1984), I
can see the Rx error statistic is 2000.

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

[Shenwei Wang]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Sunday, August 24, 2025 9:13 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 v2 net-next 4/5] net: fec: add change_mtu to support
> dynamic buffer allocation
> 
> > > > +static int fec_change_mtu(struct net_device *ndev, int new_mtu) {
> > > > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > > > +	int order, done;
> > > > +	bool running;
> > > > +
> > > > +	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));
> > > > +
> > >
> > > I think we need to add a check for rx_frame_size, as
> > > FEC_R_CNTRL[MAX_FL]
> > and
> > > FEC_FTRL[TRUNC_FL] only have 14 bits.
> >
> > That would be redundant, since rx_frame_size cannot exceed
> > max_buf_size which value would either be PKT_MAXBUF_SIZE or
> > MAX_JUMBO_BUF_SIZE.
> >
> 
> Looked at the entire patch set, the rx_frame_size is set to FEC_FTRL[TRUNC_FL]
> and FEC_R_CNTRL[MAX_FL], and both TRUNC_FL and MAX_FL are 14 bits, if the
> value set exceeds the hardware capability, the driver should return an error.
> 
> For example, the order is 3, so rx_frame_size is 0x7dc0, but MAX_FL will be set to
> 0x3dc0, that is not correct.

How can you get the order of 3? If the PAGE_SIZE is 4k, the order is no greater than 2.

Regards,
Shenwei

RE: [PATCH v2 net-next 3/5] et: 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:26 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 v2 net-next 3/5] et: fec: add rx_frame_size to support
> configurable RX length
> 
> > > > @@ -4563,6 +4563,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;
> > >
> > > According to the RM, to allow one maximum size frame per buffer,
> > > FEC_R_BUFF_SIZE must be set to FEC_R_CNTRL [MAX_FL] or larger.
> > > FEC_ENET_RX_FRSIZE is greater than PKT_MAXBUF_SIZE, I'm not sure
> > whether it
> > > will cause some unknown issues.
> >
> > MAX_FL defines the maximum allowable frame length, while TRUNC_FL
> > specifies the threshold beyond which frames are truncated. Based on
> > this logic, the documentation appears to be incorrect-TRUNC_FL should
> > never exceed MAX_FL, as doing so would make the truncation mechanism
> > ineffective.
> >
> > This has been confirmed through testing data.
> >
> 
> One obvious issue I can see is that the Rx error statistic would be doubled the
> actual number when the FEC receives jumbo frames.
> 
> For example, the sender sends 1000 jumbo frames (8000 bytes) to the FEC port,
> without this patch set, the Rx error statistic of FEC should be 1000, however, after
> applying this patch set (rx_frame_size is 3520, max_buf_size is 1984), I can see
> the Rx error statistic is 2000.

I don't think there is a case that rx_frame_size is 3520 while the max_buf_size is 1984.
With the patch, the rx_frame_size should always less than the max_buf_size. Otherwise,
the implementation might have a logic bug.

Regards,
Shenwei

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

[Wei Fang]

> > > > > @@ -4563,6 +4563,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;
> > > >
> > > > According to the RM, to allow one maximum size frame per buffer,
> > > > FEC_R_BUFF_SIZE must be set to FEC_R_CNTRL [MAX_FL] or larger.
> > > > FEC_ENET_RX_FRSIZE is greater than PKT_MAXBUF_SIZE, I'm not sure
> > > whether it
> > > > will cause some unknown issues.
> > >
> > > MAX_FL defines the maximum allowable frame length, while TRUNC_FL
> > > specifies the threshold beyond which frames are truncated. Based on
> > > this logic, the documentation appears to be incorrect-TRUNC_FL should
> > > never exceed MAX_FL, as doing so would make the truncation mechanism
> > > ineffective.
> > >
> > > This has been confirmed through testing data.
> > >
> >
> > One obvious issue I can see is that the Rx error statistic would be doubled the
> > actual number when the FEC receives jumbo frames.
> >
> > For example, the sender sends 1000 jumbo frames (8000 bytes) to the FEC
> port,
> > without this patch set, the Rx error statistic of FEC should be 1000, however,
> after
> > applying this patch set (rx_frame_size is 3520, max_buf_size is 1984), I can see
> > the Rx error statistic is 2000.
> 
> I don't think there is a case that rx_frame_size is 3520 while the max_buf_size is
> 1984.
> With the patch, the rx_frame_size should always less than the max_buf_size.
> Otherwise,
> the implementation might have a logic bug.
> 

Actually, I have tested this patch set, that is the problem I'm seeing. You can also
have a try, do not enable jumbo frame, just use the default configuration.

RE: [PATCH v2 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;
> > > > > +	bool running;
> > > > > +
> > > > > +	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));
> > > > > +
> > > >
> > > > I think we need to add a check for rx_frame_size, as
> > > > FEC_R_CNTRL[MAX_FL]
> > > and
> > > > FEC_FTRL[TRUNC_FL] only have 14 bits.
> > >
> > > That would be redundant, since rx_frame_size cannot exceed
> > > max_buf_size which value would either be PKT_MAXBUF_SIZE or
> > > MAX_JUMBO_BUF_SIZE.
> > >
> >
> > Looked at the entire patch set, the rx_frame_size is set to
> FEC_FTRL[TRUNC_FL]
> > and FEC_R_CNTRL[MAX_FL], and both TRUNC_FL and MAX_FL are 14 bits, if
> the
> > value set exceeds the hardware capability, the driver should return an error.
> >
> > For example, the order is 3, so rx_frame_size is 0x7dc0, but MAX_FL will be
> set to
> > 0x3dc0, that is not correct.
> 
> How can you get the order of 3? If the PAGE_SIZE is 4k, the order is no greater
> than 2.
> 

I got it, the length has been checked by dev_validate_mtu(), so we
do not need to check it again.