[PATCH net-next v2 0/9] net: sparx5/lan969x: add support for changing the MTU

[PATCH net-next v2 0/9] net: sparx5/lan969x: add support for changing the MTU

[Daniel Machon]

This series adds support for changing the MTU on Sparx5 and lan969x.

Currently, there is no ndo_change_mtu callback, and the MAC maximum
frame length, the FDMA buffers, the queue watermarks and the TAS guard
band are all fixed for a 1518 byte frame. This series makes them follow
the MTU, and sets the maximum MTU from the 14000 byte frame limit of the
MACs. As the FDMA is shared by all ports, new buffers are allocated
while the FDMA keeps running, and the FDMA is only stopped briefly to
replace them. Details are in the commits.

Patches 1-2 are fixes: patch 1 makes the FDMA stop sequence actually
stop the channels, and patch 2 frees the FDMA buffers on init errors
and deinit. They are routed to net-next rather than net, as they
fix issues that are not regressions, and the stop sequence is only used
on deinit until this series uses it to resize the buffers.

Patches 3-4 prepare for the MTU change, by adding a helper for setting
the MAC maximum frame length, and by making the FDMA init and alloc
functions operate on the rings passed to them.

Patches 5-6 size the FDMA buffers from the port MTUs, and add support
for resizing them.

Patches 7-8 scale the queue watermarks and the TAS guard band with the
MTU.

Patch 9 adds the ndo_change_mtu callback and sets the maximum MTU.

Tested on Sparx5 (PCB135) and lan969x (EV23X71A), by forwarding frames
up to the MTU and checking that frames one byte over the MTU are
dropped, by sending jumbo frames to and from the CPU while stepping the
MTU up and down between 1500 and 9216, and by changing the MTU
repeatedly while flooding the CPU with frames in both directions.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
Changes in v2:
- Patch 1: add a Fixes tag.
- Patch 2: also release the lan969x TX buffers that are still in flight
  on deinit, store the length of the mapped TX buffers so that two-step
  PTP skbs are not touched when unmapping, and add Fixes tags.
- Patch 6: reuse the lan969x TX buffer release from patch 2, and mention
  in the commit message that frames still in flight are dropped.
- Rebase on net-next, which now has commit 873363ac591a1 ("net:
  sparx5/lan969x: release the in-flight slot of a stale TX skb"), so
  that a two-step PTP frame dropped by a resize no longer leaks its
  timestamp slot.
- Link to v1: https://lore.kernel.org/r/20261105-sparx5-mtu-change-v1-0-f27bab580463@microchip.com

---
Daniel Machon (9):
      net: sparx5/lan969x: stop the FDMA channels properly
      net: sparx5/lan969x: free the FDMA buffers on init errors and deinit
      net: sparx5/lan969x: add a helper for setting the MAC max frame length
      net: sparx5/lan969x: pass the FDMA rings to the init and alloc functions
      net: sparx5/lan969x: size the FDMA buffers from the port MTUs
      net: sparx5/lan969x: add support for resizing the FDMA buffers
      net: sparx5/lan969x: scale the queue watermarks with the MTU
      net: sparx5/lan969x: scale the TAS guard band with the MTU
      net: sparx5/lan969x: add support for changing the MTU

 .../ethernet/microchip/sparx5/lan969x/lan969x.c    |  10 +
 .../ethernet/microchip/sparx5/lan969x/lan969x.h    |   2 +
 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 193 +++++++++++--
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 321 ++++++++++++++++++---
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |   2 +
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  14 +
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   |  34 +++
 .../net/ethernet/microchip/sparx5/sparx5_netdev.c  |  48 ++-
 .../net/ethernet/microchip/sparx5/sparx5_port.c    | 123 +++++---
 .../net/ethernet/microchip/sparx5/sparx5_port.h    |   5 +
 drivers/net/ethernet/microchip/sparx5/sparx5_tas.c |  43 ++-
 11 files changed, 693 insertions(+), 102 deletions(-)
---
base-commit: 368fbf5d82da9ebce309449c74fc0b128f7d0219
change-id: 20260930-sparx5-mtu-change-d7ebb1a7fcaa

Best regards,
-- 
Daniel Machon <daniel.machon@microchip.com>

[PATCH net-next v2 1/9] net: sparx5/lan969x: stop the FDMA channels properly

[Daniel Machon]

The FDMA channels are activated by setting their bit in
FDMA_CH_ACTIVATE, and are disabled through FDMA_CH_DISABLE, after which
a channel finishes the DCB it is fetching before it becomes inactive,
as shown by FDMA_CH_ACTIVE. The injection and extraction ports are
stopped through INJ_STOP and XTR_STOP.

sparx5_fdma_stop() tries to stop the channels by clearing their bit in
FDMA_CH_ACTIVATE, which has no effect: the register only acts on the
bits that are set, and its bits are cleared immediately by the
hardware. It also never stops the injection port, and waits for
XTR_BUF_IS_EMPTY to be 0, which means that the extraction buffer is not
empty, instead of 1, so the wait always runs until it times out.

Add the FDMA_CH_ACTIVE register, and make sparx5_fdma_stop() disable the
channels and wait for them to become inactive, stop the injection port,
and wait for the extraction buffer to be empty.

The stop sequence is used by the FDMA deinit, and by the FDMA buffer
resize added later in this series.

The issues were discovered by an AI code review agent and confirmed
against the datasheet. The fix was tested by unbinding and rebinding the
switch driver on Sparx5 (PCB135) and lan969x (EV23X71A).

Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 46 ++++++++++++++++------
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 11 ++++++
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 15ec9b053824f..bcae0bbd5e44e 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -20,6 +20,8 @@
 
 #define FDMA_XTR_BUFFER_SIZE		2048
 #define FDMA_WEIGHT			4
+#define SPX5_FDMA_STOP_SLEEP_US		500
+#define SPX5_FDMA_STOP_TIMEOUT_US	10000
 
 static int sparx5_fdma_tx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 				     u64 *dataptr)
@@ -86,10 +88,6 @@ static void sparx5_fdma_rx_deactivate(struct sparx5 *sparx5, struct sparx5_rx *r
 {
 	struct fdma *fdma = &rx->fdma;
 
-	/* Deactivate the RX channel */
-	spx5_rmw(0, BIT(fdma->channel_id) & FDMA_CH_ACTIVATE_CH_ACTIVATE,
-		 sparx5, FDMA_CH_ACTIVATE);
-
 	/* Disable RX channel DB interrupt */
 	spx5_rmw(0, BIT(fdma->channel_id) & FDMA_INTR_DB_ENA_INTR_DB_ENA,
 		 sparx5, FDMA_INTR_DB_ENA);
@@ -125,9 +123,9 @@ static void sparx5_fdma_tx_activate(struct sparx5 *sparx5, struct sparx5_tx *tx)
 
 static void sparx5_fdma_tx_deactivate(struct sparx5 *sparx5, struct sparx5_tx *tx)
 {
-	/* Disable the channel */
-	spx5_rmw(0, BIT(tx->fdma.channel_id) & FDMA_CH_ACTIVATE_CH_ACTIVATE,
-		 sparx5, FDMA_CH_ACTIVATE);
+	/* Stop TX fdma */
+	spx5_rmw(FDMA_PORT_CTRL_INJ_STOP_SET(1), FDMA_PORT_CTRL_INJ_STOP,
+		 sparx5, FDMA_PORT_CTRL(0));
 }
 
 void sparx5_fdma_reload(struct sparx5 *sparx5, struct fdma *fdma)
@@ -447,6 +445,11 @@ static u32 sparx5_fdma_port_ctrl(struct sparx5 *sparx5)
 	return spx5_rd(sparx5, FDMA_PORT_CTRL(0));
 }
 
+static u32 sparx5_fdma_ch_active(struct sparx5 *sparx5)
+{
+	return spx5_rd(sparx5, FDMA_CH_ACTIVE);
+}
+
 int sparx5_fdma_start(struct sparx5 *sparx5)
 {
 	const struct sparx5_ops *ops = sparx5->data->ops;
@@ -468,20 +471,39 @@ int sparx5_fdma_start(struct sparx5 *sparx5)
 
 int sparx5_fdma_stop(struct sparx5 *sparx5)
 {
+	u32 channels = BIT(sparx5->rx.fdma.channel_id) |
+		       BIT(sparx5->tx.fdma.channel_id);
 	struct sparx5_rx *rx = &sparx5->rx;
 	struct sparx5_tx *tx = &sparx5->tx;
 	u32 val;
 
 	napi_disable(&rx->napi);
 
-	/* Stop the fdma and channel interrupts */
+	/* Disable the channels, and wait for them to finish the current DCB */
+	spx5_wr(FDMA_CH_DISABLE_CH_DISABLE_SET(channels), sparx5,
+		FDMA_CH_DISABLE);
+
+	if (read_poll_timeout(sparx5_fdma_ch_active,
+			      val,
+			      !(FDMA_CH_ACTIVE_CH_ACTIVE_GET(val) & channels),
+			      SPX5_FDMA_STOP_SLEEP_US,
+			      SPX5_FDMA_STOP_TIMEOUT_US,
+			      false,
+			      sparx5))
+		dev_warn(sparx5->dev, "Failed to stop FDMA channels\n");
+
+	/* Stop the fdma and the RX channel interrupt */
 	sparx5_fdma_rx_deactivate(sparx5, rx);
 	sparx5_fdma_tx_deactivate(sparx5, tx);
 
-	/* Wait for the RX channel to stop */
-	read_poll_timeout(sparx5_fdma_port_ctrl, val,
-			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val) == 0,
-			  500, 10000, 0, sparx5);
+	/* Wait for the extraction buffer to be empty */
+	read_poll_timeout(sparx5_fdma_port_ctrl,
+			  val,
+			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val),
+			  SPX5_FDMA_STOP_SLEEP_US,
+			  SPX5_FDMA_STOP_TIMEOUT_US,
+			  false,
+			  sparx5);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
index 1464fd1a1d447..df438be0ec552 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
@@ -4713,6 +4713,17 @@ extern const struct sparx5_regs *regs;
 	__REG(TARGET_FDMA, 0, 1, 8, 0, 1, regs->gsize[GW_FDMA_FDMA], 148, r, 8,\
 	      4)
 
+/* FDMA:FDMA:FDMA_CH_ACTIVE */
+#define FDMA_CH_ACTIVE                                                         \
+	__REG(TARGET_FDMA, 0, 1, 8, 0, 1, regs->gsize[GW_FDMA_FDMA], 180, 0, 1,\
+	      4)
+
+#define FDMA_CH_ACTIVE_CH_ACTIVE                 GENMASK(7, 0)
+#define FDMA_CH_ACTIVE_CH_ACTIVE_SET(x)\
+	FIELD_PREP(FDMA_CH_ACTIVE_CH_ACTIVE, x)
+#define FDMA_CH_ACTIVE_CH_ACTIVE_GET(x)\
+	FIELD_GET(FDMA_CH_ACTIVE_CH_ACTIVE, x)
+
 /* FDMA:FDMA:FDMA_CH_CFG */
 #define FDMA_CH_CFG(r)                                                         \
 	__REG(TARGET_FDMA, 0, 1, 8, 0, 1, regs->gsize[GW_FDMA_FDMA], 224, r, 8,\

-- 
2.34.1

Re: [PATCH net-next v2 1/9] net: sparx5/lan969x: stop the FDMA channels properly

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] If the FDMA_CH_ACTIVE poll in sparx5_fdma_stop()
  (sparx5_fdma.c:486-493) times out, the function only calls…
- [Low] In sparx5_fdma_stop()
  (drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c:472-509), the…

--

net: sparx5/lan969x: stop the FDMA channels properly

This commit changes sparx5_fdma_stop() to disable the FDMA channels
through FDMA_CH_DISABLE and to poll the new FDMA_CH_ACTIVE register until
they are inactive. It also stops the injection port through INJ_STOP. The
XTR_BUF_IS_EMPTY poll now waits for the buffer to be empty instead of
non-empty.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index 15ec9b053824f..bcae0bbd5e44e 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c

[ ... ]

> @@ -468,20 +471,39 @@ int sparx5_fdma_start(struct sparx5 *sparx5)
>  
>  int sparx5_fdma_stop(struct sparx5 *sparx5)
>  {
> +	u32 channels = BIT(sparx5->rx.fdma.channel_id) |
> +		       BIT(sparx5->tx.fdma.channel_id);
>  	struct sparx5_rx *rx = &sparx5->rx;
>  	struct sparx5_tx *tx = &sparx5->tx;
>  	u32 val;
>  
>  	napi_disable(&rx->napi);
>  
> -	/* Stop the fdma and channel interrupts */
> +	/* Disable the channels, and wait for them to finish the current DCB */
> +	spx5_wr(FDMA_CH_DISABLE_CH_DISABLE_SET(channels), sparx5,
> +		FDMA_CH_DISABLE);
> +
> +	if (read_poll_timeout(sparx5_fdma_ch_active,
> +			      val,
> +			      !(FDMA_CH_ACTIVE_CH_ACTIVE_GET(val) & channels),
> +			      SPX5_FDMA_STOP_SLEEP_US,
> +			      SPX5_FDMA_STOP_TIMEOUT_US,
> +			      false,
> +			      sparx5))
> +		dev_warn(sparx5->dev, "Failed to stop FDMA channels\n");

[Severity: Medium]
If this FDMA_CH_ACTIVE poll times out, what stops the channels from
continuing to DMA into memory that the callers free right afterwards?

In that case sparx5_fdma_stop() only warns and still returns 0. Both
deinit callers ignore the return value anyway, and free the DMA memory
without resetting the FDMA:

sparx5_fdma_deinit() {
	sparx5_fdma_stop(sparx5);
	fdma_free_phys(&sparx5->rx.fdma);
	fdma_free_phys(&sparx5->tx.fdma);
	...
}

lan969x_fdma_deinit() {
	...
	sparx5_fdma_stop(sparx5);
	fdma_free_coherent(sparx5->dev, &tx->fdma);
	fdma_free_coherent(sparx5->dev, &rx->fdma);
	lan969x_fdma_free_pages(rx);
	page_pool_destroy(rx->page_pool);
	...
}

A channel that is still active keeps the DCB addresses in its LLP
registers. Could it then fetch DCBs from the freed coherent rings, or
write extracted frames into RX pages or skbs that have already gone back
to the allocator? Unbind/remove reaches this through
sparx5_frame_io_deinit()->ops->fdma_deinit()->sparx5_fdma_stop().

The code alone doesn't show whether a channel can really stay active
longer than SPX5_FDMA_STOP_TIMEOUT_US after CH_DISABLE. Still, the timeout
branch exists, and the frees after it run no matter what the stop returns.

Later in this series, "net: sparx5/lan969x: add support for resizing the
FDMA buffers" adds sparx5_fdma_pause(). It follows sparx5_fdma_stop() with
an FDMA_CTRL NRESET before the buffers are freed. That covers the resize
path, but the deinit paths still have no reset.

Would it make sense to return -ETIMEDOUT from sparx5_fdma_stop(), or to
assert FDMA_CTRL NRESET in the deinit paths before releasing the DMA
memory, or both?

> +
> +	/* Stop the fdma and the RX channel interrupt */
>  	sparx5_fdma_rx_deactivate(sparx5, rx);
>  	sparx5_fdma_tx_deactivate(sparx5, tx);
>  
> -	/* Wait for the RX channel to stop */
> -	read_poll_timeout(sparx5_fdma_port_ctrl, val,
> -			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val) == 0,
> -			  500, 10000, 0, sparx5);
> +	/* Wait for the extraction buffer to be empty */
> +	read_poll_timeout(sparx5_fdma_port_ctrl,
> +			  val,
> +			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val),
> +			  SPX5_FDMA_STOP_SLEEP_US,
> +			  SPX5_FDMA_STOP_TIMEOUT_US,
> +			  false,
> +			  sparx5);

[Severity: Low]
Can this poll still time out while RX traffic is arriving?

The extraction channel is disabled and polled inactive first, while the
extraction port is still running. XTR_STOP is only set after that, in
sparx5_fdma_rx_deactivate():

	/* Stop RX fdma */
	spx5_rmw(FDMA_PORT_CTRL_XTR_STOP_SET(1), FDMA_PORT_CTRL_XTR_STOP,
		 sparx5, FDMA_PORT_CTRL(0));

Frames may keep reaching the CPU extraction queue during that window, and
CPU forwarding (QFWD_SWITCH_PORT_MODE_PORT_ENA) stays enabled on the
deinit path. If so, there may be no active channel left to move data out
of the extraction buffer, and XTR_BUF_IS_EMPTY might never become 1.

The result of this read_poll_timeout() is also discarded and nothing is
printed. A non-empty buffer therefore looks the same as a clean stop.

The commit message says the old wait "always runs until it times out".
Could the new wait still do that in this case?

Later in the series, sparx5_fdma_pause() disables CPU forwarding and
drains the CPU queues before it calls sparx5_fdma_stop(). That suggests
the stop sequence alone does not handle extraction that is still in
progress.

Would it help to set XTR_STOP before disabling the channel and poll
XTR_BUF_IS_EMPTY while the channel is still active? Another option is to
flush the buffer with FDMA_PORT_CTRL_XTR_BUF_RST, which is defined in
sparx5_main_regs.h but never used. At the very least, checking and
reporting the poll result would keep the diagnostic.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com

[PATCH net-next v2 2/9] net: sparx5/lan969x: free the FDMA buffers on init errors and deinit

[Daniel Machon]

The FDMA RX buffers are skbs on Sparx5, and pages from a page_pool on
lan969x, which are handed to the FDMA through the RX DCBs. On lan969x,
the TX DCBs also have an array holding the state of the TX buffers.

These are not all freed. On Sparx5, the RX skbs are never freed, and if
the TX buffers cannot be allocated, the RX DCBs are not freed. On
lan969x, if the RX DCBs cannot be allocated, the page_pool is not
destroyed, if the TX buffers cannot be allocated, only the RX DCBs are
freed, and lan969x_fdma_deinit() never frees the TX buffer state, nor
the TX buffers that are still in flight. On both, the return value of
fdma_dcbs_init() is also ignored for RX, so if a buffer cannot be
allocated while filling the RX ring, the FDMA is started with a partly
filled ring, and on lan969x, lan969x_fdma_free_pages() later puts the
pages that were never allocated.

Add sparx5_fdma_rx_free(), which frees the RX skbs and DCBs,
lan969x_fdma_rx_free(), which returns the pages to the page_pool and
frees the page_pool and the RX DCBs, and lan969x_fdma_tx_free(), which
releases the TX buffers still in flight and frees the TX buffer state
and the TX DCBs, and use them on the error paths and on deinit. Check
the return value of fdma_dcbs_init() for RX. To know which RX buffers
are still owned by the driver, clear the RX buffer slot once the buffer
has been passed to the stack on Sparx5, and make
lan969x_fdma_free_pages() skip and clear the empty slots of the ring.
The two-step PTP skbs are owned by the PTP code, which may already have
freed them when a TX buffer is released, so store the length of the
mapped TX buffer, and use it instead of the skb length.

The issues were discovered by AI code review agents and confirmed by
code inspection; they were not reproduced. Tested by unbinding and
rebinding the switch driver on lan969x (EV23X71A).

Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Fixes: 8fec1cea941d ("net: sparx5: use the FDMA library for allocation of rx buffers")
Fixes: d84ad2c0d80c ("net: lan969x: add FDMA implementation")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 89 +++++++++++++++++-----
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 30 +++++++-
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  1 +
 3 files changed, 99 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index 053d125eda658..8e0f1ddcbf2c4 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -67,13 +67,13 @@ static void lan969x_fdma_tx_clear_buf(struct sparx5 *sparx5, int weight)
 		if (!fdma_db_is_done(fdma_db_get(fdma, i, 0)))
 			continue;
 
-		db->dev->stats.tx_bytes += db->skb->len;
+		db->dev->stats.tx_bytes += db->len;
 		db->dev->stats.tx_packets++;
 		sparx5->tx.packets++;
 
 		dma_unmap_single(sparx5->dev,
 				 db->dma_addr,
-				 db->skb->len,
+				 db->len,
 				 DMA_TO_DEVICE);
 
 		if (!db->ptp)
@@ -90,12 +90,29 @@ static void lan969x_fdma_free_pages(struct sparx5_rx *rx)
 	struct fdma *fdma = &rx->fdma;
 
 	for (int i = 0; i < fdma->n_dcbs; ++i) {
-		for (int j = 0; j < fdma->n_dbs; ++j)
+		for (int j = 0; j < fdma->n_dbs; ++j) {
+			if (!rx->page[i][j])
+				continue;
+
 			page_pool_put_full_page(rx->page_pool,
 						rx->page[i][j], false);
+			rx->page[i][j] = NULL;
+		}
 	}
 }
 
+/* Return the pages in the RX ring to the page_pool, and free the page_pool
+ * and the RX DCBs.
+ */
+static void lan969x_fdma_rx_free(struct sparx5 *sparx5)
+{
+	struct sparx5_rx *rx = &sparx5->rx;
+
+	lan969x_fdma_free_pages(rx);
+	fdma_free_coherent(sparx5->dev, &rx->fdma);
+	page_pool_destroy(rx->page_pool);
+}
+
 static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 						 struct sparx5_rx *rx)
 {
@@ -169,14 +186,18 @@ static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5)
 		return PTR_ERR(rx->page_pool);
 
 	err = fdma_alloc_coherent(sparx5->dev, fdma);
-	if (err)
+	if (err) {
+		page_pool_destroy(rx->page_pool);
 		return err;
+	}
 
-	fdma_dcbs_init(fdma,
-		       FDMA_DCB_INFO_DATAL(fdma->db_size),
-		       FDMA_DCB_STATUS_INTR);
+	err = fdma_dcbs_init(fdma,
+			     FDMA_DCB_INFO_DATAL(fdma->db_size),
+			     FDMA_DCB_STATUS_INTR);
+	if (err)
+		lan969x_fdma_rx_free(sparx5);
 
-	return 0;
+	return err;
 }
 
 static int lan969x_fdma_tx_alloc(struct sparx5 *sparx5)
@@ -202,6 +223,45 @@ static int lan969x_fdma_tx_alloc(struct sparx5 *sparx5)
 	return 0;
 }
 
+/* Release the TX buffers that are still in flight. The two-step PTP skbs are
+ * owned by the port tx_skbs list, so they are only unmapped.
+ */
+static void lan969x_fdma_tx_release(struct sparx5 *sparx5)
+{
+	struct sparx5_tx *tx = &sparx5->tx;
+	struct sparx5_tx_buf *db;
+
+	for (int i = 0; i < tx->fdma.n_dcbs; ++i) {
+		db = &tx->dbs[i];
+
+		if (!db->used)
+			continue;
+
+		db->dev->stats.tx_dropped++;
+		tx->dropped++;
+
+		dma_unmap_single(sparx5->dev,
+				 db->dma_addr,
+				 db->len,
+				 DMA_TO_DEVICE);
+
+		if (!db->ptp)
+			dev_kfree_skb_any(db->skb);
+
+		db->used = false;
+	}
+}
+
+/* Free the TX buffers, the TX buffer state and the TX DCBs */
+static void lan969x_fdma_tx_free(struct sparx5 *sparx5)
+{
+	struct sparx5_tx *tx = &sparx5->tx;
+
+	lan969x_fdma_tx_release(sparx5);
+	fdma_free_coherent(sparx5->dev, &tx->fdma);
+	kfree(tx->dbs);
+}
+
 static void lan969x_fdma_rx_init(struct sparx5 *sparx5)
 {
 	struct fdma *fdma = &sparx5->rx.fdma;
@@ -330,6 +390,7 @@ int lan969x_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 
 	db_buf->dev = dev;
 	db_buf->skb = skb;
+	db_buf->len = skb->len;
 	db_buf->ptp = false;
 	db_buf->used = true;
 
@@ -353,7 +414,6 @@ int lan969x_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 
 int lan969x_fdma_init(struct sparx5 *sparx5)
 {
-	struct sparx5_rx *rx = &sparx5->rx;
 	int err;
 
 	lan969x_fdma_rx_init(sparx5);
@@ -375,7 +435,7 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 
 	err = lan969x_fdma_tx_alloc(sparx5);
 	if (err) {
-		fdma_free_coherent(sparx5->dev, &rx->fdma);
+		lan969x_fdma_rx_free(sparx5);
 		dev_err(sparx5->dev, "Failed to allocate TX buffers: %d\n",
 			err);
 		return err;
@@ -390,14 +450,9 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 
 int lan969x_fdma_deinit(struct sparx5 *sparx5)
 {
-	struct sparx5_rx *rx = &sparx5->rx;
-	struct sparx5_tx *tx = &sparx5->tx;
-
 	sparx5_fdma_stop(sparx5);
-	fdma_free_coherent(sparx5->dev, &tx->fdma);
-	fdma_free_coherent(sparx5->dev, &rx->fdma);
-	lan969x_fdma_free_pages(rx);
-	page_pool_destroy(rx->page_pool);
+	lan969x_fdma_tx_free(sparx5);
+	lan969x_fdma_rx_free(sparx5);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index bcae0bbd5e44e..f4e8ac00dbe2f 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -174,6 +174,10 @@ static bool sparx5_fdma_rx_get_frame(struct sparx5 *sparx5, struct sparx5_rx *rx
 	skb->dev->stats.rx_bytes += skb->len;
 	skb->dev->stats.rx_packets++;
 	rx->packets++;
+	/* The skb is passed to the stack, so clear its slot in the ring, so
+	 * that sparx5_fdma_rx_free() does not free it.
+	 */
+	rx->skb[fdma->dcb_index][fdma->db_index] = NULL;
 	netif_receive_skb(skb);
 	return true;
 }
@@ -239,6 +243,21 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+/* Free the RX skbs owned by the driver, and the RX DCBs */
+static void sparx5_fdma_rx_free(struct sparx5_rx *rx)
+{
+	struct fdma *fdma = &rx->fdma;
+
+	for (int i = 0; i < fdma->n_dcbs; i++) {
+		for (int j = 0; j < fdma->n_dbs; j++) {
+			consume_skb(rx->skb[i][j]);
+			rx->skb[i][j] = NULL;
+		}
+	}
+
+	fdma_free_phys(fdma);
+}
+
 static int sparx5_fdma_rx_alloc(struct sparx5 *sparx5)
 {
 	struct sparx5_rx *rx = &sparx5->rx;
@@ -249,10 +268,12 @@ static int sparx5_fdma_rx_alloc(struct sparx5 *sparx5)
 	if (err)
 		return err;
 
-	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
-		       FDMA_DCB_STATUS_INTR);
+	err = fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
+			     FDMA_DCB_STATUS_INTR);
+	if (err)
+		sparx5_fdma_rx_free(rx);
 
-	return 0;
+	return err;
 }
 
 static int sparx5_fdma_tx_alloc(struct sparx5 *sparx5)
@@ -425,6 +446,7 @@ int sparx5_fdma_init(struct sparx5 *sparx5)
 	}
 	err = sparx5_fdma_tx_alloc(sparx5);
 	if (err) {
+		sparx5_fdma_rx_free(&sparx5->rx);
 		dev_err(sparx5->dev, "Could not allocate TX buffers: %d\n", err);
 		return err;
 	}
@@ -434,7 +456,7 @@ int sparx5_fdma_init(struct sparx5 *sparx5)
 int sparx5_fdma_deinit(struct sparx5 *sparx5)
 {
 	sparx5_fdma_stop(sparx5);
-	fdma_free_phys(&sparx5->rx.fdma);
+	sparx5_fdma_rx_free(&sparx5->rx);
 	fdma_free_phys(&sparx5->tx.fdma);
 
 	return 0;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 8dc3a500c1c91..e08b1635cfcd9 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -179,6 +179,7 @@ struct sparx5_tx_buf {
 	dma_addr_t dma_addr;
 	bool used;
 	bool ptp;
+	u32 len;
 };
 
 /* Frame DMA transmit state:

-- 
2.34.1

Re: [PATCH net-next v2 2/9] net: sparx5/lan969x: free the FDMA buffers on init errors and deinit

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On lan969x, the ownership tracking for RX pages is still
  incomplete in the free_page error path of lan969x_fdma_rx_get_frame().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but both NAPI
  handlers ignore the return value of fdma_dcb_add() when they refill RX…

--

net: sparx5/lan969x: free the FDMA buffers on init errors and deinit

This patch adds sparx5_fdma_rx_free(), lan969x_fdma_rx_free() and
lan969x_fdma_tx_free(). They release the RX skbs/pages, the page_pool,
the TX buffer state and the in-flight TX buffers on init error paths and
on deinit. It also checks the return value of fdma_dcbs_init() for RX,
and stores the mapped TX length so it no longer reads skb->len.

> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> index 053d125eda658..8e0f1ddcbf2c4 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c

[ ... ]

> @@ -90,12 +90,29 @@ static void lan969x_fdma_free_pages(struct sparx5_rx *rx)
>  	struct fdma *fdma = &rx->fdma;
>  
>  	for (int i = 0; i < fdma->n_dcbs; ++i) {
> -		for (int j = 0; j < fdma->n_dbs; ++j)
> +		for (int j = 0; j < fdma->n_dbs; ++j) {
> +			if (!rx->page[i][j])
> +				continue;
> +
>  			page_pool_put_full_page(rx->page_pool,
>  						rx->page[i][j], false);
> +			rx->page[i][j] = NULL;
> +		}
>  	}
>  }

[Severity: High]
Is a non-NULL slot here always a page that the driver still owns?

lan969x_fdma_rx_get_frame() takes its free_page error path when
WARN_ON(!port) fires or build_skb() fails:

free_page:
	page_pool_recycle_direct(rx->page_pool, page);

	return NULL;

That returns the page to the pool, but
rx->page[fdma->dcb_index][fdma->db_index] still points at it.
lan969x_fdma_napi_poll() then breaks out of the loop without advancing
or refilling that DB:

		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
		if (!skb)
			break;

On a later teardown through
lan969x_fdma_deinit()->lan969x_fdma_rx_free()->lan969x_fdma_free_pages(),
wouldn't page_pool_put_full_page() run on that stale slot a second time?

A page in the pool cache still has pp_ref_count == 1. So
page_pool_unref_netmem() takes the second put as the last reference and
recycles the page again, this time into the ptr_ring, while it also
sits in the alloc cache. page_pool_destroy() would then DMA-unmap the
page twice and call put_page() on it twice.

The commit message says:

  "To know which RX buffers are still owned by the driver, clear the RX
  buffer slot once the buffer has been passed to the stack on Sparx5, and
  make lan969x_fdma_free_pages() skip and clear the empty slots of the
  ring."

However, lan969x does not clear any slot at this commit. A later patch
in the series, "net: sparx5/lan969x: add support for resizing the FDMA
buffers", fixes the success path: it sets rx->page[...] = NULL before
returning the skb. The free_page path still leaves the slot set there.

That later patch also adds lan969x_fdma_resize(), which calls
lan969x_fdma_rx_free() on an MTU change. After it, the double put can
happen without an unbind.

Separately, and this part predates the patch: the stuck DB stays DONE,
so the next poll processes the same, already recycled page again. If
build_skb() succeeds that time, the stack gets an skb on a page that
the refill can hand straight back to the hardware.

Would it work to set rx->page[fdma->dcb_index][fdma->db_index] = NULL
after page_pool_recycle_direct()? The poll loop would also need to
consume or refill that DB, or reuse the page in place, instead of
recycling the page while the DB still references it.

[ ... ]

> @@ -169,14 +186,18 @@ static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5)
>  		return PTR_ERR(rx->page_pool);
>  
>  	err = fdma_alloc_coherent(sparx5->dev, fdma);
> -	if (err)
> +	if (err) {
> +		page_pool_destroy(rx->page_pool);
>  		return err;
> +	}
>  
> -	fdma_dcbs_init(fdma,
> -		       FDMA_DCB_INFO_DATAL(fdma->db_size),
> -		       FDMA_DCB_STATUS_INTR);
> +	err = fdma_dcbs_init(fdma,
> +			     FDMA_DCB_INFO_DATAL(fdma->db_size),
> +			     FDMA_DCB_STATUS_INTR);
> +	if (err)
> +		lan969x_fdma_rx_free(sparx5);
>  
> -	return 0;
> +	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the same allocation
failure is still unchecked when the RX DCBs are refilled at runtime. The
patch only adds the check at init time.

lan969x_fdma_napi_poll() ignores the return value of fdma_dcb_add():

		fdma_dcb_add(fdma,
			     old_dcb,
			     FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);

So does sparx5_fdma_napi_callback() in sparx5_fdma.c:

		fdma_dcb_add(fdma, fdma->dcb_index,
			     FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);

__fdma_db_add() in fdma_api.c writes the status, with DONE cleared,
before it calls the dataptr callback:

	db->status = cpu_to_le64(status);

	err = cb(fdma, dcb_idx, db_idx, &dataptr);
	if (unlikely(err))
		return err;

Suppose page_pool_dev_alloc_pages() in lan969x_fdma_rx_dataptr_cb() or
the GFP_ATOMIC __netdev_alloc_skb() in sparx5_fdma_rx_dataptr_cb()
fails. __fdma_dcb_add() then returns before it links the DCB or updates
fdma->last_dcb. The next successful add links around that DCB, so the
hardware skips it.

Once dcb_index wraps around to that DCB, fdma_has_frames() and
fdma_db_is_done() return false. Would both NAPI handlers then stop at
that DCB on every poll? The hardware would fill the remaining DCBs, but
nothing would consume or refill them, and CPU RX would stay stalled
until the driver is rebound.

This code is unchanged through the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com

[PATCH net-next v2 3/9] net: sparx5/lan969x: add a helper for setting the MAC max frame length

[Daniel Machon]

The maximum frame length accepted by a port is set in the MAC_MAXLEN_CFG
register of its MAC. All ports have a 2G5 device, the 5G, 10G and 25G
ports also have a high speed device, and the lan969x RGMII ports have an
RGMII device instead. The register is currently written open-coded in
sparx5_port_init(), with a fixed length of 1518 bytes.

Add sparx5_port_max_len_set(), which sets the maximum frame length in the
devices of a port, and use it in sparx5_port_init(). To support the
RGMII devices, add the DEVRGMII_MAC_MAXLEN_CFG register, and make
lan969x_port_dev_mapping() return the RGMII device index for the RGMII
ports.

The helper is used to change the maximum frame length with the MTU, later
in this series.

No functional change intended.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../ethernet/microchip/sparx5/lan969x/lan969x.c    |  9 ++++
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 11 +++++
 .../net/ethernet/microchip/sparx5/sparx5_port.c    | 50 ++++++++++++++++------
 .../net/ethernet/microchip/sparx5/sparx5_port.h    |  4 ++
 4 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
index c02499d1b297a..e8c30049b54cf 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
@@ -180,6 +180,15 @@ static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
 		}
 	}
 
+	if (lan969x_port_is_rgmii(port)) {
+		switch (port) {
+		case 28:
+			return 0;
+		case 29:
+			return 1;
+		}
+	}
+
 	/* 2g5 port */
 	return port;
 }
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
index df438be0ec552..5302c7686e207 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
@@ -8430,6 +8430,17 @@ extern const struct sparx5_regs *regs;
 #define DEVRGMII_MAC_ENA_CFG_TX_ENA_GET(x)\
 	FIELD_GET(DEVRGMII_MAC_ENA_CFG_TX_ENA, x)
 
+/* LAN969X ONLY */
+/* DEV1G:MAC_CFG_STATUS:MAC_MAXLEN_CFG */
+#define DEVRGMII_MAC_MAXLEN_CFG(t)                                             \
+	__REG(TARGET_DEVRGMII, t, 2, 36, 0, 1, 36, 8, 0, 1, 4)
+
+#define DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN          GENMASK(15, 0)
+#define DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN_SET(x)\
+	FIELD_PREP(DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN, x)
+#define DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN_GET(x)\
+	FIELD_GET(DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN, x)
+
 /* LAN969X ONLY */
 /* DEV1G:MAC_CFG_STATUS:MAC_TAGS_CFG */
 #define DEVRGMII_MAC_TAGS_CFG(t)                                               \
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
index ef06bed3a9cc4..701d310df0ba6 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
@@ -613,6 +613,42 @@ static int sparx5_port_max_tags_set(struct sparx5 *sparx5,
 	return 0;
 }
 
+/* Set the maximum frame length accepted by the MAC of a port */
+void sparx5_port_max_len_set(struct sparx5 *sparx5,
+			     struct sparx5_port *port,
+			     u32 len)
+{
+	u32 pix = sparx5_port_dev_index(sparx5, port->portno);
+	const struct sparx5_ops *ops = sparx5->data->ops;
+	void __iomem *devinst;
+
+	if (ops->is_port_rgmii(port->portno)) {
+		spx5_rmw(DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN_SET(len),
+			 DEVRGMII_MAC_MAXLEN_CFG_MAX_LEN,
+			 sparx5,
+			 DEVRGMII_MAC_MAXLEN_CFG(pix));
+		return;
+	}
+
+	/* All other ports have a 2G5 (shadow) device */
+	spx5_rmw(DEV2G5_MAC_MAXLEN_CFG_MAX_LEN_SET(len),
+		 DEV2G5_MAC_MAXLEN_CFG_MAX_LEN,
+		 sparx5,
+		 DEV2G5_MAC_MAXLEN_CFG(port->portno));
+
+	if (ops->is_port_2g5(port->portno))
+		return;
+
+	/* The MAXLEN_CFG register layout is shared by the high speed devices */
+	devinst = spx5_inst_get(sparx5,
+				sparx5_to_high_dev(sparx5, port->portno), pix);
+
+	spx5_inst_rmw(DEV10G_MAC_MAXLEN_CFG_MAX_LEN_SET(len),
+		      DEV10G_MAC_MAXLEN_CFG_MAX_LEN,
+		      devinst,
+		      DEV10G_MAC_MAXLEN_CFG(0));
+}
+
 int sparx5_port_fwd_urg(struct sparx5 *sparx5, u32 speed)
 {
 	u32 clk_period_ps = 1600; /* 625Mhz for now */
@@ -1068,18 +1104,15 @@ int sparx5_port_init(struct sparx5 *sparx5,
 	u32 pause_start = sparx5_wm_enc(6  * (ETH_MAXLEN / SPX5_BUFFER_CELL_SZ));
 	u32 atop = sparx5_wm_enc(20 * (ETH_MAXLEN / SPX5_BUFFER_CELL_SZ));
 	const struct sparx5_ops *ops = sparx5->data->ops;
-	u32 devhigh = sparx5_to_high_dev(sparx5, port->portno);
 	u32 pix = sparx5_port_dev_index(sparx5, port->portno);
 	u32 pcs = sparx5_to_pcs_dev(sparx5, port->portno);
 	bool sd_pol = port->signd_active_high;
 	bool sd_sel = !port->signd_internal;
 	bool sd_ena = port->signd_enable;
 	u32 pause_stop = 0xFFF - 1; /* FC generate disabled */
-	void __iomem *devinst;
 	void __iomem *pcsinst;
 	int err;
 
-	devinst = spx5_inst_get(sparx5, devhigh, pix);
 	pcsinst = spx5_inst_get(sparx5, pcs, pix);
 
 	/* Set the mux port mode  */
@@ -1119,10 +1152,7 @@ int sparx5_port_init(struct sparx5 *sparx5,
 		return err;
 
 	/* Set Max Length */
-	spx5_rmw(DEV2G5_MAC_MAXLEN_CFG_MAX_LEN_SET(ETH_MAXLEN),
-		 DEV2G5_MAC_MAXLEN_CFG_MAX_LEN,
-		 sparx5,
-		 DEV2G5_MAC_MAXLEN_CFG(port->portno));
+	sparx5_port_max_len_set(sparx5, port, ETH_MAXLEN);
 
 	/* 1G/2G5: Signal Detect configuration */
 	spx5_wr(DEV2G5_PCS1G_SD_CFG_SD_POL_SET(sd_pol) |
@@ -1171,12 +1201,6 @@ int sparx5_port_init(struct sparx5 *sparx5,
 	if (sparx5_is_baser(conf->portmode))
 		sparx5_dev_switch(sparx5, port->portno, true);
 
-	/* Set Max Length */
-	spx5_inst_rmw(DEV10G_MAC_MAXLEN_CFG_MAX_LEN_SET(ETH_MAXLEN),
-		      DEV10G_MAC_MAXLEN_CFG_MAX_LEN,
-		      devinst,
-		      DEV10G_MAC_MAXLEN_CFG(0));
-
 	/* Handle Signal Detect in 10G PCS */
 	spx5_inst_wr(PCS10G_BR_PCS_SD_CFG_SD_POL_SET(sd_pol) |
 		     PCS10G_BR_PCS_SD_CFG_SD_SEL_SET(sd_sel) |
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.h b/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
index c8a37468a3d1d..912f97a506768 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
@@ -88,6 +88,10 @@ int sparx5_port_init(struct sparx5 *sparx5,
 		     struct sparx5_port *spx5_port,
 		     struct sparx5_port_config *conf);
 
+void sparx5_port_max_len_set(struct sparx5 *sparx5,
+			     struct sparx5_port *port,
+			     u32 len);
+
 int sparx5_port_config(struct sparx5 *sparx5,
 		       struct sparx5_port *spx5_port,
 		       struct sparx5_port_config *conf);

-- 
2.34.1

[PATCH net-next v2 4/9] net: sparx5/lan969x: pass the FDMA rings to the init and alloc functions

[Daniel Machon]

The FDMA RX and TX rings are initialized and allocated by functions that
take struct sparx5, and operate on the rings in it. The RX buffer
callbacks also find the RX ring through struct sparx5, and the DB size
is computed in the RX and TX init functions.

In preparation for MTU support, where a new set of rings must be set up
while the FDMA keeps running on the current ones, make these functions
operate on the ring passed to them, and pass the DB size to the Sparx5
init functions. Find the RX ring in the RX buffer callbacks from the FDMA
that is passed to them. On lan969x, TX frames are mapped directly from
the skbs, so only the RX ring has to be replaced. Therefore, only its RX
functions take a ring, and its RX init function keeps computing the DB
size.

No functional change intended.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 28 ++++++++++----------
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 30 ++++++++++------------
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index 8e0f1ddcbf2c4..d435c4df5effa 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -25,7 +25,7 @@ static int lan969x_fdma_tx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 static int lan969x_fdma_rx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 				      u64 *dataptr)
 {
-	struct sparx5_rx *rx = &FDMA_PRIV(fdma)->rx;
+	struct sparx5_rx *rx = container_of(fdma, struct sparx5_rx, fdma);
 	struct page *page;
 
 	page = page_pool_dev_alloc_pages(rx->page_pool);
@@ -104,10 +104,8 @@ static void lan969x_fdma_free_pages(struct sparx5_rx *rx)
 /* Return the pages in the RX ring to the page_pool, and free the page_pool
  * and the RX DCBs.
  */
-static void lan969x_fdma_rx_free(struct sparx5 *sparx5)
+static void lan969x_fdma_rx_free(struct sparx5 *sparx5, struct sparx5_rx *rx)
 {
-	struct sparx5_rx *rx = &sparx5->rx;
-
 	lan969x_fdma_free_pages(rx);
 	fdma_free_coherent(sparx5->dev, &rx->fdma);
 	page_pool_destroy(rx->page_pool);
@@ -163,9 +161,8 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	return NULL;
 }
 
-static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5)
+static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5, struct sparx5_rx *rx)
 {
-	struct sparx5_rx *rx = &sparx5->rx;
 	struct fdma *fdma = &rx->fdma;
 	int err;
 
@@ -195,7 +192,7 @@ static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5)
 			     FDMA_DCB_INFO_DATAL(fdma->db_size),
 			     FDMA_DCB_STATUS_INTR);
 	if (err)
-		lan969x_fdma_rx_free(sparx5);
+		lan969x_fdma_rx_free(sparx5, rx);
 
 	return err;
 }
@@ -262,9 +259,9 @@ static void lan969x_fdma_tx_free(struct sparx5 *sparx5)
 	kfree(tx->dbs);
 }
 
-static void lan969x_fdma_rx_init(struct sparx5 *sparx5)
+static void lan969x_fdma_rx_init(struct sparx5 *sparx5, struct sparx5_rx *rx)
 {
-	struct fdma *fdma = &sparx5->rx.fdma;
+	struct fdma *fdma = &rx->fdma;
 
 	fdma->channel_id = FDMA_XTR_CHANNEL;
 	fdma->n_dcbs = FDMA_DCB_MAX;
@@ -280,7 +277,7 @@ static void lan969x_fdma_rx_init(struct sparx5 *sparx5)
 		struct sparx5_port *port = sparx5->ports[idx];
 
 		if (port && port->ndev) {
-			sparx5->rx.ndev = port->ndev;
+			rx->ndev = port->ndev;
 			break;
 		}
 	}
@@ -414,9 +411,10 @@ int lan969x_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 
 int lan969x_fdma_init(struct sparx5 *sparx5)
 {
+	struct sparx5_rx *rx = &sparx5->rx;
 	int err;
 
-	lan969x_fdma_rx_init(sparx5);
+	lan969x_fdma_rx_init(sparx5, rx);
 	lan969x_fdma_tx_init(sparx5);
 	sparx5_fdma_injection_mode(sparx5);
 
@@ -426,7 +424,7 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 		return err;
 	}
 
-	err = lan969x_fdma_rx_alloc(sparx5);
+	err = lan969x_fdma_rx_alloc(sparx5, rx);
 	if (err) {
 		dev_err(sparx5->dev, "Failed to allocate RX buffers: %d\n",
 			err);
@@ -435,7 +433,7 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 
 	err = lan969x_fdma_tx_alloc(sparx5);
 	if (err) {
-		lan969x_fdma_rx_free(sparx5);
+		lan969x_fdma_rx_free(sparx5, rx);
 		dev_err(sparx5->dev, "Failed to allocate TX buffers: %d\n",
 			err);
 		return err;
@@ -450,9 +448,11 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 
 int lan969x_fdma_deinit(struct sparx5 *sparx5)
 {
+	struct sparx5_rx *rx = &sparx5->rx;
+
 	sparx5_fdma_stop(sparx5);
 	lan969x_fdma_tx_free(sparx5);
-	lan969x_fdma_rx_free(sparx5);
+	lan969x_fdma_rx_free(sparx5, rx);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index f4e8ac00dbe2f..7ac5887abe6ee 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -35,8 +35,7 @@ static int sparx5_fdma_tx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 static int sparx5_fdma_rx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 				     u64 *dataptr)
 {
-	struct sparx5 *sparx5 = fdma->priv;
-	struct sparx5_rx *rx = &sparx5->rx;
+	struct sparx5_rx *rx = container_of(fdma, struct sparx5_rx, fdma);
 	struct sk_buff *skb;
 
 	skb = __netdev_alloc_skb(rx->ndev, fdma->db_size, GFP_ATOMIC);
@@ -258,9 +257,8 @@ static void sparx5_fdma_rx_free(struct sparx5_rx *rx)
 	fdma_free_phys(fdma);
 }
 
-static int sparx5_fdma_rx_alloc(struct sparx5 *sparx5)
+static int sparx5_fdma_rx_alloc(struct sparx5_rx *rx)
 {
-	struct sparx5_rx *rx = &sparx5->rx;
 	struct fdma *fdma = &rx->fdma;
 	int err;
 
@@ -276,9 +274,8 @@ static int sparx5_fdma_rx_alloc(struct sparx5 *sparx5)
 	return err;
 }
 
-static int sparx5_fdma_tx_alloc(struct sparx5 *sparx5)
+static int sparx5_fdma_tx_alloc(struct sparx5_tx *tx)
 {
-	struct sparx5_tx *tx = &sparx5->tx;
 	struct fdma *fdma = &tx->fdma;
 	int err;
 
@@ -293,7 +290,7 @@ static int sparx5_fdma_tx_alloc(struct sparx5 *sparx5)
 }
 
 static void sparx5_fdma_rx_init(struct sparx5 *sparx5,
-				struct sparx5_rx *rx, int channel)
+				struct sparx5_rx *rx, int channel, u32 db_size)
 {
 	struct fdma *fdma = &rx->fdma;
 	int idx;
@@ -302,8 +299,8 @@ static void sparx5_fdma_rx_init(struct sparx5 *sparx5,
 	fdma->n_dcbs = FDMA_DCB_MAX;
 	fdma->n_dbs = FDMA_RX_DCB_MAX_DBS;
 	fdma->priv = sparx5;
-	fdma->db_size = ALIGN(FDMA_XTR_BUFFER_SIZE, PAGE_SIZE);
-	fdma->size = fdma_get_size(&sparx5->rx.fdma);
+	fdma->db_size = db_size;
+	fdma->size = fdma_get_size(fdma);
 	fdma->ops.dataptr_cb = &sparx5_fdma_rx_dataptr_cb;
 	fdma->ops.nextptr_cb = &fdma_nextptr_cb;
 	/* Fetch a netdev for SKB and NAPI use, any will do */
@@ -318,7 +315,7 @@ static void sparx5_fdma_rx_init(struct sparx5 *sparx5,
 }
 
 static void sparx5_fdma_tx_init(struct sparx5 *sparx5,
-				struct sparx5_tx *tx, int channel)
+				struct sparx5_tx *tx, int channel, u32 db_size)
 {
 	struct fdma *fdma = &tx->fdma;
 
@@ -326,8 +323,8 @@ static void sparx5_fdma_tx_init(struct sparx5 *sparx5,
 	fdma->n_dcbs = FDMA_DCB_MAX;
 	fdma->n_dbs = FDMA_TX_DCB_MAX_DBS;
 	fdma->priv = sparx5;
-	fdma->db_size = ALIGN(FDMA_XTR_BUFFER_SIZE, PAGE_SIZE);
-	fdma->size = fdma_get_size_contiguous(&sparx5->tx.fdma);
+	fdma->db_size = db_size;
+	fdma->size = fdma_get_size_contiguous(fdma);
 	fdma->ops.dataptr_cb = &sparx5_fdma_tx_dataptr_cb;
 	fdma->ops.nextptr_cb = &fdma_nextptr_cb;
 }
@@ -421,6 +418,7 @@ void sparx5_fdma_injection_mode(struct sparx5 *sparx5)
 
 int sparx5_fdma_init(struct sparx5 *sparx5)
 {
+	u32 db_size = ALIGN(FDMA_XTR_BUFFER_SIZE, PAGE_SIZE);
 	int err;
 
 	/* Reset FDMA state */
@@ -437,14 +435,14 @@ int sparx5_fdma_init(struct sparx5 *sparx5)
 		 sparx5, CPU_PROC_CTRL);
 
 	sparx5_fdma_injection_mode(sparx5);
-	sparx5_fdma_rx_init(sparx5, &sparx5->rx, FDMA_XTR_CHANNEL);
-	sparx5_fdma_tx_init(sparx5, &sparx5->tx, FDMA_INJ_CHANNEL);
-	err = sparx5_fdma_rx_alloc(sparx5);
+	sparx5_fdma_rx_init(sparx5, &sparx5->rx, FDMA_XTR_CHANNEL, db_size);
+	sparx5_fdma_tx_init(sparx5, &sparx5->tx, FDMA_INJ_CHANNEL, db_size);
+	err = sparx5_fdma_rx_alloc(&sparx5->rx);
 	if (err) {
 		dev_err(sparx5->dev, "Could not allocate RX buffers: %d\n", err);
 		return err;
 	}
-	err = sparx5_fdma_tx_alloc(sparx5);
+	err = sparx5_fdma_tx_alloc(&sparx5->tx);
 	if (err) {
 		sparx5_fdma_rx_free(&sparx5->rx);
 		dev_err(sparx5->dev, "Could not allocate TX buffers: %d\n", err);

-- 
2.34.1

[PATCH net-next v2 5/9] net: sparx5/lan969x: size the FDMA buffers from the port MTUs

[Daniel Machon]

The FDMA buffers must hold the largest frame that can be received by the
CPU, including the IFH, and on Sparx5 also the largest frame that is
transmitted, as Sparx5 copies the frames into its TX buffers. On Sparx5,
the RX buffers are skbs, and the TX buffers are a contiguous area. On
lan969x, the RX buffers are page_pool pages, which are turned into skbs
with build_skb(), and TX frames are mapped directly from the skbs. The
buffers are currently fixed to a single page, which is not large enough
to hold jumbo frames.

Instead, size the buffers from the largest MTU of the ports, plus the
Ethernet header, FCS, IFH and two VLAN tags. On Sparx5, use this length,
rounded up to the 128 byte granularity required by the FDMA, as the RX
and TX buffer size. As the TX buffers can now be smaller than a page,
make sparx5_fdma_xmit() drop frames that do not fit. On lan969x, use the
page order needed to hold the frame and the skb_shared_info, as lan966x
does, and limit the RX buffer size so that the FDMA never writes into
the skb_shared_info at the end of the page.

With the default MTU of 1500, the Sparx5 buffers become smaller, and the
lan969x pages are unchanged.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 22 ++++++++++----
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 35 ++++++++++++++++++++--
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  2 ++
 3 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index d435c4df5effa..40cdb0323eb49 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -131,7 +131,7 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	if (WARN_ON(!port))
 		goto free_page;
 
-	skb = build_skb(page_address(page), fdma->db_size);
+	skb = build_skb(page_address(page), PAGE_SIZE << rx->page_order);
 	if (unlikely(!skb))
 		goto free_page;
 
@@ -161,21 +161,27 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	return NULL;
 }
 
+/* Get the page order needed for a page to hold the largest frame */
+static u8 lan969x_fdma_get_page_order(struct sparx5 *sparx5)
+{
+	return get_order(ALIGN(sparx5_fdma_get_max_frame(sparx5), 128) +
+			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+}
+
 static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5, struct sparx5_rx *rx)
 {
 	struct fdma *fdma = &rx->fdma;
 	int err;
 
 	struct page_pool_params pp_params = {
-		.order = 0,
+		.order = rx->page_order,
 		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
 		.pool_size = fdma->n_dcbs * fdma->n_dbs,
 		.nid = NUMA_NO_NODE,
 		.dev = sparx5->dev,
 		.dma_dir = DMA_FROM_DEVICE,
 		.offset = 0,
-		.max_len = fdma->db_size -
-			   SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
+		.max_len = fdma->db_size,
 	};
 
 	rx->page_pool = page_pool_create(&pp_params);
@@ -268,7 +274,12 @@ static void lan969x_fdma_rx_init(struct sparx5 *sparx5, struct sparx5_rx *rx)
 	fdma->n_dbs = 1;
 	fdma->priv = sparx5;
 	fdma->size = fdma_get_size(fdma);
-	fdma->db_size = PAGE_SIZE;
+	/* Keep the FDMA from writing into the skb_shared_info at the end of
+	 * the page. The DB size must be a multiple of 128 bytes.
+	 */
+	fdma->db_size = ALIGN_DOWN((PAGE_SIZE << rx->page_order) -
+				   SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
+				   128);
 	fdma->ops.dataptr_cb = &lan969x_fdma_rx_dataptr_cb;
 	fdma->ops.nextptr_cb = &fdma_nextptr_cb;
 
@@ -414,6 +425,7 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 	struct sparx5_rx *rx = &sparx5->rx;
 	int err;
 
+	rx->page_order = lan969x_fdma_get_page_order(sparx5);
 	lan969x_fdma_rx_init(sparx5, rx);
 	lan969x_fdma_tx_init(sparx5);
 	sparx5_fdma_injection_mode(sparx5);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 7ac5887abe6ee..3a74a305d55d4 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -18,7 +18,6 @@
 #include "sparx5_main.h"
 #include "sparx5_port.h"
 
-#define FDMA_XTR_BUFFER_SIZE		2048
 #define FDMA_WEIGHT			4
 #define SPX5_FDMA_STOP_SLEEP_US		500
 #define SPX5_FDMA_STOP_TIMEOUT_US	10000
@@ -218,6 +217,10 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 	struct fdma *fdma = &tx->fdma;
 	void *virt_addr;
 
+	/* Drop frames that do not fit in the TX buffers */
+	if (unlikely(IFH_LEN * 4 + skb->len + ETH_FCS_LEN > fdma->db_size))
+		return -EMSGSIZE;
+
 	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
 		return -EINVAL;
 
@@ -289,6 +292,33 @@ static int sparx5_fdma_tx_alloc(struct sparx5_tx *tx)
 	return 0;
 }
 
+/* Get the size of the largest frame, including IFH and VLAN tags, that can be
+ * received on any of the ports.
+ */
+u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5)
+{
+	u32 max_mtu = 0;
+
+	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
+		struct sparx5_port *port = sparx5->ports[i];
+
+		if (!port)
+			continue;
+
+		max_mtu = max(max_mtu, READ_ONCE(port->ndev->mtu));
+	}
+
+	return SPX5_HW_MTU(max_mtu) + IFH_LEN * sizeof(u32) + VLAN_HLEN * 2;
+}
+
+/* Get the DB size needed to hold the largest frame. The DB size must be a
+ * multiple of 128 bytes.
+ */
+static u32 sparx5_fdma_get_db_size(struct sparx5 *sparx5)
+{
+	return ALIGN(sparx5_fdma_get_max_frame(sparx5), 128);
+}
+
 static void sparx5_fdma_rx_init(struct sparx5 *sparx5,
 				struct sparx5_rx *rx, int channel, u32 db_size)
 {
@@ -418,7 +448,7 @@ void sparx5_fdma_injection_mode(struct sparx5 *sparx5)
 
 int sparx5_fdma_init(struct sparx5 *sparx5)
 {
-	u32 db_size = ALIGN(FDMA_XTR_BUFFER_SIZE, PAGE_SIZE);
+	u32 db_size;
 	int err;
 
 	/* Reset FDMA state */
@@ -435,6 +465,7 @@ int sparx5_fdma_init(struct sparx5 *sparx5)
 		 sparx5, CPU_PROC_CTRL);
 
 	sparx5_fdma_injection_mode(sparx5);
+	db_size = sparx5_fdma_get_db_size(sparx5);
 	sparx5_fdma_rx_init(sparx5, &sparx5->rx, FDMA_XTR_CHANNEL, db_size);
 	sparx5_fdma_tx_init(sparx5, &sparx5->tx, FDMA_INJ_CHANNEL, db_size);
 	err = sparx5_fdma_rx_alloc(&sparx5->rx);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index e08b1635cfcd9..1ff096b65f1b4 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -109,6 +109,7 @@ enum sparx5_feature {
 #define SPX5_BUFFER_CELL_SZ    184   /* Cell size  */
 #define SPX5_BUFFER_MEMORY     4194280 /* 22795 words * 184 bytes */
 #define SPX5_TAS_ENTRIES_PER_PORT 2 /* TAS lists per port */
+#define SPX5_HW_MTU(mtu)       ((mtu) + ETH_HLEN + ETH_FCS_LEN)
 
 #define XTR_QUEUE     0
 #define INJ_QUEUE     0
@@ -480,6 +481,7 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 irqreturn_t sparx5_fdma_handler(int irq, void *args);
 void sparx5_fdma_reload(struct sparx5 *sparx5, struct fdma *fdma);
 void sparx5_fdma_injection_mode(struct sparx5 *sparx5);
+u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5);
 
 /* sparx5_mactable.c */
 int sparx5_mact_learn(struct sparx5 *sparx5, int port,

-- 
2.34.1

[PATCH net-next v2 6/9] net: sparx5/lan969x: add support for resizing the FDMA buffers

[Daniel Machon]

The FDMA buffers are sized from the largest MTU of the ports when the
FDMA is initialized, so they must be resized when an MTU change affects
that size. As the FDMA is shared by all ports, it must be stopped while
its buffers are replaced.

Add an fdma_resize operation, which allocates new buffers for the new
MTU while the FDMA keeps running on the current ones, so that a failed
allocation leaves the FDMA untouched. It then stops the port TX queues
and the FDMA, replaces the buffers, and restarts the FDMA and the TX
queues. On Sparx5 both the RX and TX buffers are replaced. On lan969x,
where TX frames are mapped directly from the skbs, only the RX buffers
and page_pool are replaced.

The forwarding to the CPU ports is stopped, and the frames already
queued for the CPU are extracted, before the FDMA is stopped. Otherwise,
with frames arriving for the CPU, extraction to the CPU can stop for
good after the restart.

Frames that are still in the FDMA buffers, or queued for the CPU, when
the FDMA is stopped are dropped.

On lan969x, to know which RX pages are still owned by the driver when
freeing them, clear the RX buffer slot once the page has been passed to
the stack.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../ethernet/microchip/sparx5/lan969x/lan969x.c    |   1 +
 .../ethernet/microchip/sparx5/lan969x/lan969x.h    |   2 +
 .../microchip/sparx5/lan969x/lan969x_fdma.c        |  78 +++++++-
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    | 198 ++++++++++++++++++++-
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |   1 +
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |   9 +-
 6 files changed, 277 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
index e8c30049b54cf..2ea3bb6023f92 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
@@ -356,6 +356,7 @@ static const struct sparx5_ops lan969x_ops = {
 	.fdma_deinit             = &lan969x_fdma_deinit,
 	.fdma_poll               = &lan969x_fdma_napi_poll,
 	.fdma_xmit               = &lan969x_fdma_xmit,
+	.fdma_resize             = &lan969x_fdma_resize,
 };
 
 const struct sparx5_match_data lan969x_desc = {
diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.h b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.h
index 529fde3d4deba..c34ce0ed4e555 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.h
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.h
@@ -78,5 +78,7 @@ int lan969x_fdma_deinit(struct sparx5 *sparx5);
 int lan969x_fdma_napi_poll(struct napi_struct *napi, int weight);
 int lan969x_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 		      struct net_device *dev);
+int lan969x_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
+			int new_mtu);
 
 #endif
diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index 40cdb0323eb49..6f827cad49229 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -153,6 +153,12 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	skb->dev->stats.rx_bytes += skb->len;
 	skb->dev->stats.rx_packets++;
 
+	/* The page is passed to the stack in the skb, so clear its slot in the
+	 * ring, so that lan969x_fdma_rx_free() does not return it to the
+	 * page_pool.
+	 */
+	rx->page[fdma->dcb_index][fdma->db_index] = NULL;
+
 	return skb;
 
 free_page:
@@ -162,9 +168,11 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 }
 
 /* Get the page order needed for a page to hold the largest frame */
-static u8 lan969x_fdma_get_page_order(struct sparx5 *sparx5)
+static u8 lan969x_fdma_get_page_order(struct sparx5 *sparx5,
+				      struct sparx5_port *port, int new_mtu)
 {
-	return get_order(ALIGN(sparx5_fdma_get_max_frame(sparx5), 128) +
+	return get_order(ALIGN(sparx5_fdma_get_max_frame(sparx5, port, new_mtu),
+			       128) +
 			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
 }
 
@@ -425,7 +433,7 @@ int lan969x_fdma_init(struct sparx5 *sparx5)
 	struct sparx5_rx *rx = &sparx5->rx;
 	int err;
 
-	rx->page_order = lan969x_fdma_get_page_order(sparx5);
+	rx->page_order = lan969x_fdma_get_page_order(sparx5, NULL, 0);
 	lan969x_fdma_rx_init(sparx5, rx);
 	lan969x_fdma_tx_init(sparx5);
 	sparx5_fdma_injection_mode(sparx5);
@@ -468,3 +476,67 @@ int lan969x_fdma_deinit(struct sparx5 *sparx5)
 
 	return 0;
 }
+
+/* Release the TX buffers that were in flight when the FDMA was reset, and
+ * initialize the TX DCBs again.
+ */
+static void lan969x_fdma_tx_reset(struct sparx5 *sparx5)
+{
+	struct fdma *fdma = &sparx5->tx.fdma;
+
+	/* Complete the frames that were transmitted before the reset. BHs are
+	 * disabled as tx_clear_buf() expects to run from NAPI.
+	 */
+	local_bh_disable();
+	lan969x_fdma_tx_clear_buf(sparx5, 0);
+	local_bh_enable();
+
+	lan969x_fdma_tx_release(sparx5);
+
+	fdma_dcbs_init(fdma,
+		       FDMA_DCB_INFO_DATAL(fdma->db_size),
+		       FDMA_DCB_STATUS_DONE);
+}
+
+int lan969x_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
+			int new_mtu)
+{
+	struct sparx5_rx *rx = &sparx5->rx;
+	struct sparx5_rx *rx_new;
+	u8 page_order;
+	int err;
+
+	page_order = lan969x_fdma_get_page_order(sparx5, port, new_mtu);
+	if (page_order == rx->page_order)
+		return 0;
+
+	rx_new = kzalloc_obj(*rx_new);
+	if (!rx_new)
+		return -ENOMEM;
+
+	rx_new->page_order = page_order;
+	lan969x_fdma_rx_init(sparx5, rx_new);
+
+	err = lan969x_fdma_rx_alloc(sparx5, rx_new);
+	if (err)
+		goto out;
+
+	sparx5_fdma_pause(sparx5);
+
+	/* The FDMA is stopped and reset, and NAPI and the TX queues are
+	 * disabled, so nothing uses the buffers until sparx5_fdma_resume().
+	 */
+	lan969x_fdma_tx_reset(sparx5);
+	lan969x_fdma_rx_free(sparx5, rx);
+
+	rx->fdma = rx_new->fdma;
+	rx->page_pool = rx_new->page_pool;
+	rx->page_order = rx_new->page_order;
+	memcpy(rx->page, rx_new->page, sizeof(rx->page));
+
+	sparx5_fdma_resume(sparx5);
+out:
+	kfree(rx_new);
+
+	return err;
+}
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 3a74a305d55d4..f4bb420d3f27b 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -21,6 +21,13 @@
 #define FDMA_WEIGHT			4
 #define SPX5_FDMA_STOP_SLEEP_US		500
 #define SPX5_FDMA_STOP_TIMEOUT_US	10000
+#define SPX5_FDMA_TX_DRAIN_SLEEP_US	10
+#define SPX5_FDMA_TX_DRAIN_TIMEOUT_US	5000
+#define SPX5_FDMA_XTR_DRAIN_SLEEP_US	100
+#define SPX5_FDMA_XTR_DRAIN_TIMEOUT_US	100000
+
+/* Base of the QRES resources tracking memory per destination port */
+#define SPX5_QRES_DST_MEM		2048
 
 static int sparx5_fdma_tx_dataptr_cb(struct fdma *fdma, int dcb, int db,
 				     u64 *dataptr)
@@ -293,19 +300,23 @@ static int sparx5_fdma_tx_alloc(struct sparx5_tx *tx)
 }
 
 /* Get the size of the largest frame, including IFH and VLAN tags, that can be
- * received on any of the ports.
+ * received on any of the ports. If a port is given, its MTU is taken to be
+ * new_mtu instead of its current MTU, as the MTU is only updated once the
+ * buffers have been resized.
  */
-u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5)
+u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5,
+			      struct sparx5_port *port, int new_mtu)
 {
 	u32 max_mtu = 0;
 
 	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
-		struct sparx5_port *port = sparx5->ports[i];
+		struct sparx5_port *p = sparx5->ports[i];
 
-		if (!port)
+		if (!p)
 			continue;
 
-		max_mtu = max(max_mtu, READ_ONCE(port->ndev->mtu));
+		max_mtu = max_t(u32, max_mtu, p == port ?
+				new_mtu : READ_ONCE(p->ndev->mtu));
 	}
 
 	return SPX5_HW_MTU(max_mtu) + IFH_LEN * sizeof(u32) + VLAN_HLEN * 2;
@@ -314,9 +325,10 @@ u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5)
 /* Get the DB size needed to hold the largest frame. The DB size must be a
  * multiple of 128 bytes.
  */
-static u32 sparx5_fdma_get_db_size(struct sparx5 *sparx5)
+static u32 sparx5_fdma_get_db_size(struct sparx5 *sparx5,
+				   struct sparx5_port *port, int new_mtu)
 {
-	return ALIGN(sparx5_fdma_get_max_frame(sparx5), 128);
+	return ALIGN(sparx5_fdma_get_max_frame(sparx5, port, new_mtu), 128);
 }
 
 static void sparx5_fdma_rx_init(struct sparx5 *sparx5,
@@ -465,7 +477,7 @@ int sparx5_fdma_init(struct sparx5 *sparx5)
 		 sparx5, CPU_PROC_CTRL);
 
 	sparx5_fdma_injection_mode(sparx5);
-	db_size = sparx5_fdma_get_db_size(sparx5);
+	db_size = sparx5_fdma_get_db_size(sparx5, NULL, 0);
 	sparx5_fdma_rx_init(sparx5, &sparx5->rx, FDMA_XTR_CHANNEL, db_size);
 	sparx5_fdma_tx_init(sparx5, &sparx5->tx, FDMA_INJ_CHANNEL, db_size);
 	err = sparx5_fdma_rx_alloc(&sparx5->rx);
@@ -558,3 +570,173 @@ int sparx5_fdma_stop(struct sparx5 *sparx5)
 
 	return 0;
 }
+
+/* Check if any TX DB is still waiting to be transmitted */
+static bool sparx5_fdma_tx_busy(struct fdma *fdma)
+{
+	for (int i = 0; i < fdma->n_dcbs; i++)
+		if (!fdma_db_is_done(fdma_db_get(fdma, i, 0)))
+			return true;
+
+	return false;
+}
+
+/* Enable or disable the forwarding of frames to the CPU ports */
+static void sparx5_fdma_cpu_fwd_set(struct sparx5 *sparx5, bool ena)
+{
+	for (int cpu = SPX5_PORT_CPU_0; cpu <= SPX5_PORT_CPU_1; cpu++) {
+		int portno = sparx5_get_internal_port(sparx5, cpu);
+
+		spx5_rmw(QFWD_SWITCH_PORT_MODE_PORT_ENA_SET(ena),
+			 QFWD_SWITCH_PORT_MODE_PORT_ENA,
+			 sparx5,
+			 QFWD_SWITCH_PORT_MODE(portno));
+	}
+}
+
+/* Get the memory used by the frames queued to the CPU ports */
+static u32 sparx5_fdma_cpu_queue_use(struct sparx5 *sparx5)
+{
+	u32 use = 0;
+
+	for (int cpu = SPX5_PORT_CPU_0; cpu <= SPX5_PORT_CPU_1; cpu++) {
+		int portno = sparx5_get_internal_port(sparx5, cpu);
+		u32 base;
+
+		base = SPX5_QRES_DST_MEM + SPX5_PRIOS * portno;
+		for (int prio = 0; prio < SPX5_PRIOS; prio++)
+			use += QRES_RES_STAT_CUR_INUSE_GET(spx5_rd(sparx5,
+					QRES_RES_STAT_CUR(base + prio)));
+	}
+
+	return use;
+}
+
+/* Stop the port TX queues and the FDMA, and reset the FDMA, so that the FDMA
+ * buffers can be replaced.
+ */
+void sparx5_fdma_pause(struct sparx5 *sparx5)
+{
+	bool busy;
+	u32 use;
+
+	/* Only ports that are up have TX queues to stop */
+	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
+		struct sparx5_port *port = sparx5->ports[i];
+
+		if (port && netif_running(port->ndev))
+			netif_tx_disable(port->ndev);
+	}
+
+	/* Give the frames already handed to the FDMA a chance to be sent, as
+	 * they are lost when the FDMA is reset.
+	 */
+	if (read_poll_timeout(sparx5_fdma_tx_busy,
+			      busy,
+			      !busy,
+			      SPX5_FDMA_TX_DRAIN_SLEEP_US,
+			      SPX5_FDMA_TX_DRAIN_TIMEOUT_US,
+			      false,
+			      &sparx5->tx.fdma))
+		dev_warn(sparx5->dev, "FDMA TX not drained, frames dropped\n");
+
+	/* Stop the switch core from forwarding frames to the CPU, and let the
+	 * FDMA extract the frames already queued, so that the FDMA is not
+	 * stopped and reset while the switch core is still sending it frames.
+	 */
+	sparx5_fdma_cpu_fwd_set(sparx5, false);
+	if (read_poll_timeout(sparx5_fdma_cpu_queue_use,
+			      use,
+			      !use,
+			      SPX5_FDMA_XTR_DRAIN_SLEEP_US,
+			      SPX5_FDMA_XTR_DRAIN_TIMEOUT_US,
+			      false,
+			      sparx5))
+		dev_warn(sparx5->dev, "CPU queue not drained, frames dropped\n");
+
+	sparx5_fdma_stop(sparx5);
+
+	/* Reset FDMA state */
+	spx5_wr(FDMA_CTRL_NRESET_SET(0), sparx5, FDMA_CTRL);
+	spx5_wr(FDMA_CTRL_NRESET_SET(1), sparx5, FDMA_CTRL);
+
+	/* Discard frames queued for the CPU, as they may not fit the new
+	 * buffers.
+	 */
+	sparx5_xtr_flush(sparx5, XTR_QUEUE);
+}
+
+/* Restart the FDMA and the port TX queues after sparx5_fdma_pause() */
+void sparx5_fdma_resume(struct sparx5 *sparx5)
+{
+	struct sparx5_rx *rx = &sparx5->rx;
+	struct sparx5_tx *tx = &sparx5->tx;
+
+	/* Enable NAPI before the RX interrupt is enabled, as the interrupt
+	 * handler schedules NAPI, as in sparx5_fdma_start().
+	 */
+	napi_enable(&rx->napi);
+
+	sparx5_fdma_rx_activate(sparx5, rx);
+	sparx5_fdma_tx_activate(sparx5, tx);
+
+	/* Forward frames to the CPU again, now that the FDMA can extract them */
+	sparx5_fdma_cpu_fwd_set(sparx5, true);
+
+	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
+		struct sparx5_port *port = sparx5->ports[i];
+
+		if (port && netif_running(port->ndev))
+			netif_tx_wake_all_queues(port->ndev);
+	}
+}
+
+int sparx5_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
+		       int new_mtu)
+{
+	struct sparx5_rx *rx = &sparx5->rx;
+	struct sparx5_tx *tx = &sparx5->tx;
+	struct sparx5_tx tx_new = {};
+	struct sparx5_rx *rx_new;
+	u32 db_size;
+	int err;
+
+	db_size = sparx5_fdma_get_db_size(sparx5, port, new_mtu);
+	if (db_size == rx->fdma.db_size)
+		return 0;
+
+	rx_new = kzalloc_obj(*rx_new);
+	if (!rx_new)
+		return -ENOMEM;
+
+	sparx5_fdma_rx_init(sparx5, rx_new, FDMA_XTR_CHANNEL, db_size);
+	sparx5_fdma_tx_init(sparx5, &tx_new, FDMA_INJ_CHANNEL, db_size);
+
+	err = sparx5_fdma_rx_alloc(rx_new);
+	if (err)
+		goto out;
+
+	err = sparx5_fdma_tx_alloc(&tx_new);
+	if (err) {
+		sparx5_fdma_rx_free(rx_new);
+		goto out;
+	}
+
+	sparx5_fdma_pause(sparx5);
+
+	/* The FDMA is stopped and reset, and NAPI and the TX queues are
+	 * disabled, so nothing uses the buffers until sparx5_fdma_resume().
+	 */
+	sparx5_fdma_rx_free(rx);
+	fdma_free_phys(&tx->fdma);
+
+	rx->fdma = rx_new->fdma;
+	memcpy(rx->skb, rx_new->skb, sizeof(rx->skb));
+	tx->fdma = tx_new.fdma;
+
+	sparx5_fdma_resume(sparx5);
+out:
+	kfree(rx_new);
+
+	return err;
+}
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index e039f5369b95e..cf9cdd494674f 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -1092,6 +1092,7 @@ static const struct sparx5_ops sparx5_ops = {
 	.fdma_deinit             = &sparx5_fdma_deinit,
 	.fdma_poll               = &sparx5_fdma_napi_callback,
 	.fdma_xmit               = &sparx5_fdma_xmit,
+	.fdma_resize             = &sparx5_fdma_resize,
 };
 
 static const struct sparx5_match_data sparx5_desc = {
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 1ff096b65f1b4..97bf8a6cb83f5 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -358,6 +358,8 @@ struct sparx5_ops {
 	int (*fdma_poll)(struct napi_struct *napi, int weight);
 	int (*fdma_xmit)(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 			 struct net_device *dev);
+	int (*fdma_resize)(struct sparx5 *sparx5, struct sparx5_port *port,
+			   int new_mtu);
 };
 
 struct sparx5_main_io_resource {
@@ -481,7 +483,12 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 irqreturn_t sparx5_fdma_handler(int irq, void *args);
 void sparx5_fdma_reload(struct sparx5 *sparx5, struct fdma *fdma);
 void sparx5_fdma_injection_mode(struct sparx5 *sparx5);
-u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5);
+u32 sparx5_fdma_get_max_frame(struct sparx5 *sparx5,
+			      struct sparx5_port *port, int new_mtu);
+void sparx5_fdma_pause(struct sparx5 *sparx5);
+void sparx5_fdma_resume(struct sparx5 *sparx5);
+int sparx5_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
+		       int new_mtu);
 
 /* sparx5_mactable.c */
 int sparx5_mact_learn(struct sparx5 *sparx5, int port,

-- 
2.34.1

Re: [PATCH net-next v2 6/9] net: sparx5/lan969x: add support for resizing the FDMA buffers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On lan969x, the RX error path keeps a pointer to a page it has
  already given back to the page_pool, so lan969x_fdma_resize() can…
- [Medium] This patch adds a one-line lan969x RX slot clear in
  lan969x_fdma_rx_get_frame():…
- [Medium] net: sparx5: GFP_ATOMIC RX skb allocation in
  sparx5_fdma_resize()

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is severe:
  both RX NAPI paths ignore fdma_dcb_add()'s return value when re-arming…

--

net: sparx5/lan969x: add support for resizing the FDMA buffers

This adds an fdma_resize operation for Sparx5 and lan969x. It allocates
new FDMA buffers sized for a new MTU, pauses the TX queues and the FDMA,
swaps in the new buffers and then resumes. On lan969x it also clears the
RX page slot once the page has been passed to the stack.

> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> index 40cdb0323eb49..6f827cad49229 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> @@ -153,6 +153,12 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
>  	skb->dev->stats.rx_bytes += skb->len;
>  	skb->dev->stats.rx_packets++;
>  
> +	/* The page is passed to the stack in the skb, so clear its slot in the
> +	 * ring, so that lan969x_fdma_rx_free() does not return it to the
> +	 * page_pool.
> +	 */
> +	rx->page[fdma->dcb_index][fdma->db_index] = NULL;

[Severity: Medium]
This line also seems to fix an existing double put at deinit, separate
from resizing.

When the NAPI reload loop fails to refill a slot,
lan969x_fdma_rx_dataptr_cb() returns before it overwrites the slot:

	page = page_pool_dev_alloc_pages(rx->page_pool);
	if (unlikely(!page))
		return -ENOMEM;

	rx->page[dcb][db] = page;

The slot then still holds the page that build_skb() already gave to the
stack. Later, lan969x_fdma_deinit()->lan969x_fdma_rx_free()->
lan969x_fdma_free_pages() calls page_pool_put_full_page() on that page.

The earlier fix in this series, "net: sparx5/lan969x: free the FDMA
buffers on init errors and deinit" (Fixes: d84ad2c0d80c), limits the slot
clearing to Sparx5:

  "clear the RX buffer slot once the buffer has been passed to the stack
  on Sparx5"

Could this lan969x line go into that fix, or into its own patch with
Fixes: d84ad2c0d80c? Otherwise stable trees that take the fix would still
have the lan969x double put.

> +
>  	return skb;
>  
>  free_page:

[Severity: High]
The success path now clears the slot. What about the free_page path just
below it?

lan969x_fdma_rx_get_frame() {
    ...
free_page:
	page_pool_recycle_direct(rx->page_pool, page);

	return NULL;
}

This path runs on WARN_ON(!port) or when build_skb() fails. It gives the
page back to the page_pool, but rx->page[fdma->dcb_index][fdma->db_index]
still points at it.

lan969x_fdma_napi_poll() then breaks out before fdma_db_advance():

		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
		if (!skb)
			break;

So the slot is not reloaded. In the same poll, the reload loop can also
hand the just-recycled page from the direct cache to an earlier slot.

After that, an MTU change that changes the page order goes through:

lan969x_fdma_resize()
  lan969x_fdma_rx_free()
    lan969x_fdma_free_pages()
      page_pool_put_full_page(rx->page_pool, rx->page[i][j], false)

Can this put the same page into the page_pool a second time? If so,
page_pool_destroy() would release it twice, with a double put_page() and
a double DMA unmap.

The commit message says the slot is cleared "to know which RX pages are
still owned by the driver when freeing them". The free_page path also
gives up ownership. Should it clear the slot as well?

Before this patch the free only happened at deinit. Now
lan969x_fdma_resize() runs it during normal operation.

[ ... ]

> @@ -468,3 +476,67 @@ int lan969x_fdma_deinit(struct sparx5 *sparx5)

[ ... ]

> +int lan969x_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
> +			int new_mtu)
> +{

[ ... ]

> +	sparx5_fdma_pause(sparx5);
> +
> +	/* The FDMA is stopped and reset, and NAPI and the TX queues are
> +	 * disabled, so nothing uses the buffers until sparx5_fdma_resume().
> +	 */
> +	lan969x_fdma_tx_reset(sparx5);
> +	lan969x_fdma_rx_free(sparx5, rx);
> +
> +	rx->fdma = rx_new->fdma;
> +	rx->page_pool = rx_new->page_pool;
> +	rx->page_order = rx_new->page_order;
> +	memcpy(rx->page, rx_new->page, sizeof(rx->page));
> +
> +	sparx5_fdma_resume(sparx5);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. It
affects the RX refill path that this series makes larger.

Both RX NAPI paths ignore the return value of fdma_dcb_add() when they
re-arm a consumed DCB. In lan969x_fdma_napi_poll():

		fdma_dcb_add(fdma,
			     old_dcb,
			     FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);

		sparx5_fdma_reload(sparx5, fdma);

sparx5_fdma_napi_callback() does the same.

In fdma_api.c, __fdma_db_add() writes the status before the callback
gets a chance to fail:

	db->status = cpu_to_le64(status);

	err = cb(fdma, dcb_idx, db_idx, &dataptr);
	if (unlikely(err))
		return err;

__fdma_dcb_add() then returns before it links prev->nextptr and updates
fdma->last_dcb.

Suppose page_pool_dev_alloc_pages() in lan969x_fdma_rx_dataptr_cb() fails,
or the GFP_ATOMIC __netdev_alloc_skb() in sparx5_fdma_rx_dataptr_cb()
fails. The DCB then has DONE cleared, but it is not in the hardware chain,
and the next successful add links around it.

The software dcb_index still walks the ring in order. When it reaches
this DCB, fdma_has_frames() on lan969x, or fdma_db_is_done() in
sparx5_fdma_rx_get_frame(), sees a DCB that is not done and stops. No
further DCBs are re-armed, and the hardware stops at the last linked DCB.

Can one failed atomic refill stop CPU extraction for every port until
the FDMA is re-initialized?

The later commit "net: sparx5/lan969x: add support for changing the MTU"
enables jumbo MTUs, which turns these refills into higher-order
allocations. The fix commit "net: sparx5/lan969x: free the FDMA buffers
on init errors and deinit" checks errors for the initial fdma_dcbs_init()
fill, but not for this refill at runtime.

[ ... ]

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> @@ -558,3 +570,173 @@ int sparx5_fdma_stop(struct sparx5 *sparx5)
>  
>  	return 0;
>  }
> +
> +/* Check if any TX DB is still waiting to be transmitted */
> +static bool sparx5_fdma_tx_busy(struct fdma *fdma)
> +{
> +	for (int i = 0; i < fdma->n_dcbs; i++)
> +		if (!fdma_db_is_done(fdma_db_get(fdma, i, 0)))
> +			return true;
> +
> +	return false;
> +}
> +
> +/* Enable or disable the forwarding of frames to the CPU ports */
> +static void sparx5_fdma_cpu_fwd_set(struct sparx5 *sparx5, bool ena)
> +{
> +	for (int cpu = SPX5_PORT_CPU_0; cpu <= SPX5_PORT_CPU_1; cpu++) {
> +		int portno = sparx5_get_internal_port(sparx5, cpu);
> +
> +		spx5_rmw(QFWD_SWITCH_PORT_MODE_PORT_ENA_SET(ena),
> +			 QFWD_SWITCH_PORT_MODE_PORT_ENA,
> +			 sparx5,
> +			 QFWD_SWITCH_PORT_MODE(portno));
> +	}
> +}
> +
> +/* Get the memory used by the frames queued to the CPU ports */
> +static u32 sparx5_fdma_cpu_queue_use(struct sparx5 *sparx5)
> +{
> +	u32 use = 0;
> +
> +	for (int cpu = SPX5_PORT_CPU_0; cpu <= SPX5_PORT_CPU_1; cpu++) {
> +		int portno = sparx5_get_internal_port(sparx5, cpu);
> +		u32 base;
> +
> +		base = SPX5_QRES_DST_MEM + SPX5_PRIOS * portno;
> +		for (int prio = 0; prio < SPX5_PRIOS; prio++)
> +			use += QRES_RES_STAT_CUR_INUSE_GET(spx5_rd(sparx5,
> +					QRES_RES_STAT_CUR(base + prio)));
> +	}
> +
> +	return use;
> +}
> +
> +/* Stop the port TX queues and the FDMA, and reset the FDMA, so that the FDMA
> + * buffers can be replaced.
> + */
> +void sparx5_fdma_pause(struct sparx5 *sparx5)
> +{
> +	bool busy;
> +	u32 use;
> +
> +	/* Only ports that are up have TX queues to stop */
> +	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
> +		struct sparx5_port *port = sparx5->ports[i];
> +
> +		if (port && netif_running(port->ndev))
> +			netif_tx_disable(port->ndev);
> +	}
> +
> +	/* Give the frames already handed to the FDMA a chance to be sent, as
> +	 * they are lost when the FDMA is reset.
> +	 */
> +	if (read_poll_timeout(sparx5_fdma_tx_busy,
> +			      busy,
> +			      !busy,
> +			      SPX5_FDMA_TX_DRAIN_SLEEP_US,
> +			      SPX5_FDMA_TX_DRAIN_TIMEOUT_US,
> +			      false,
> +			      &sparx5->tx.fdma))
> +		dev_warn(sparx5->dev, "FDMA TX not drained, frames dropped\n");
> +
> +	/* Stop the switch core from forwarding frames to the CPU, and let the
> +	 * FDMA extract the frames already queued, so that the FDMA is not
> +	 * stopped and reset while the switch core is still sending it frames.
> +	 */
> +	sparx5_fdma_cpu_fwd_set(sparx5, false);
> +	if (read_poll_timeout(sparx5_fdma_cpu_queue_use,
> +			      use,
> +			      !use,
> +			      SPX5_FDMA_XTR_DRAIN_SLEEP_US,
> +			      SPX5_FDMA_XTR_DRAIN_TIMEOUT_US,
> +			      false,
> +			      sparx5))
> +		dev_warn(sparx5->dev, "CPU queue not drained, frames dropped\n");
> +
> +	sparx5_fdma_stop(sparx5);
> +
> +	/* Reset FDMA state */
> +	spx5_wr(FDMA_CTRL_NRESET_SET(0), sparx5, FDMA_CTRL);
> +	spx5_wr(FDMA_CTRL_NRESET_SET(1), sparx5, FDMA_CTRL);
> +
> +	/* Discard frames queued for the CPU, as they may not fit the new
> +	 * buffers.
> +	 */
> +	sparx5_xtr_flush(sparx5, XTR_QUEUE);
> +}
> +
> +/* Restart the FDMA and the port TX queues after sparx5_fdma_pause() */
> +void sparx5_fdma_resume(struct sparx5 *sparx5)
> +{
> +	struct sparx5_rx *rx = &sparx5->rx;
> +	struct sparx5_tx *tx = &sparx5->tx;
> +
> +	/* Enable NAPI before the RX interrupt is enabled, as the interrupt
> +	 * handler schedules NAPI, as in sparx5_fdma_start().
> +	 */
> +	napi_enable(&rx->napi);
> +
> +	sparx5_fdma_rx_activate(sparx5, rx);
> +	sparx5_fdma_tx_activate(sparx5, tx);
> +
> +	/* Forward frames to the CPU again, now that the FDMA can extract them */
> +	sparx5_fdma_cpu_fwd_set(sparx5, true);
> +
> +	for (int i = 0; i < sparx5->data->consts->n_ports; i++) {
> +		struct sparx5_port *port = sparx5->ports[i];
> +
> +		if (port && netif_running(port->ndev))
> +			netif_tx_wake_all_queues(port->ndev);
> +	}
> +}
> +
> +int sparx5_fdma_resize(struct sparx5 *sparx5, struct sparx5_port *port,
> +		       int new_mtu)
> +{
> +	struct sparx5_rx *rx = &sparx5->rx;
> +	struct sparx5_tx *tx = &sparx5->tx;
> +	struct sparx5_tx tx_new = {};
> +	struct sparx5_rx *rx_new;
> +	u32 db_size;
> +	int err;
> +
> +	db_size = sparx5_fdma_get_db_size(sparx5, port, new_mtu);
> +	if (db_size == rx->fdma.db_size)
> +		return 0;
> +
> +	rx_new = kzalloc_obj(*rx_new);
> +	if (!rx_new)
> +		return -ENOMEM;
> +
> +	sparx5_fdma_rx_init(sparx5, rx_new, FDMA_XTR_CHANNEL, db_size);
> +	sparx5_fdma_tx_init(sparx5, &tx_new, FDMA_INJ_CHANNEL, db_size);
> +
> +	err = sparx5_fdma_rx_alloc(rx_new);
> +	if (err)
> +		goto out;
> +
> +	err = sparx5_fdma_tx_alloc(&tx_new);
> +	if (err) {
> +		sparx5_fdma_rx_free(rx_new);
> +		goto out;
> +	}
> +
> +	sparx5_fdma_pause(sparx5);
> +
> +	/* The FDMA is stopped and reset, and NAPI and the TX queues are
> +	 * disabled, so nothing uses the buffers until sparx5_fdma_resume().
> +	 */
> +	sparx5_fdma_rx_free(rx);
> +	fdma_free_phys(&tx->fdma);
> +
> +	rx->fdma = rx_new->fdma;
> +	memcpy(rx->skb, rx_new->skb, sizeof(rx->skb));
> +	tx->fdma = tx_new.fdma;
> +
> +	sparx5_fdma_resume(sparx5);
> +out:
> +	kfree(rx_new);
> +
> +	return err;
> +}

[Severity: Medium]
sparx5_fdma_rx_alloc() fills the new ring through fdma_dcbs_init(),
which calls sparx5_fdma_rx_dataptr_cb() for every DB. That callback
always allocates the skb with GFP_ATOMIC:

	skb = __netdev_alloc_skb(rx->ndev, fdma->db_size, GFP_ATOMIC);
	if (unlikely(!skb))
		return -ENOMEM;

That fits the NAPI refill path. Here, though, it runs in process
context under RTNL. It allocates the whole ring of FDMA_DCB_MAX *
FDMA_RX_DCB_MAX_DBS (960) skbs while the old ring is still allocated
and the FDMA is still running on it.

With the jumbo MTUs enabled later in the series, db_size gets close
to 14 KB. Each skb head is then an order-2 allocation, roughly 15 MB
in total. GFP_ATOMIC cannot do direct reclaim or compaction. On a
system with fragmented memory the MTU change can fail with -ENOMEM
where a sleeping allocation would have succeeded.

It also draws on the __GFP_HIGH reserves that the NAPI refill of the
live ring needs at the same time. A failed refill there leads to the
stall raised above for fdma_dcb_add().

Could sparx5_fdma_rx_dataptr_cb() take the gfp mask from its caller?
For example, a field in struct sparx5_rx could let
sparx5_fdma_rx_alloc() use GFP_KERNEL while the NAPI refill keeps
GFP_ATOMIC.

lan969x_fdma_rx_dataptr_cb() looks the same, since
page_pool_dev_alloc_pages() is GFP_ATOMIC too. There the page order
also grows with the MTU.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com

[PATCH net-next v2 7/9] net: sparx5/lan969x: scale the queue watermarks with the MTU

[Daniel Machon]

The queue system of a port has pause watermarks (PAUSE_START and
PAUSE_STOP), which control when pause frames are sent if TX pause is
enabled, and a tail drop watermark (ATOP), above which frames received
on the port are dropped. They are set in units of buffer cells, and are
currently fixed multiples of a 1518 byte frame. With a larger MTU, a
single frame can exceed PAUSE_START, so with TX pause enabled, a pause
frame would be sent for every received frame.

Instead, compute the watermarks from the MTU of the port, plus the
Ethernet header, FCS and two VLAN tags, rounded up to whole buffer
cells. Program them at port init and when pause is resolved, and add
sparx5_port_wm_update() for use when the MTU changes. As the MTU and
phylink paths can run concurrently, protect the watermarks and the
resolved pause mode with a per-port lock.

The top bit of the watermark fields is a x16 multiplier, so take its
position from the field width, which is 11 bits on lan969x and 12 bits
on Sparx5.

With the default MTU of 1500, the watermarks grow from 8 to 9 cells per
frame, as the frame length is now rounded up instead of down.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |  1 +
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  2 +
 .../net/ethernet/microchip/sparx5/sparx5_port.c    | 73 ++++++++++++++--------
 .../net/ethernet/microchip/sparx5/sparx5_port.h    |  1 +
 4 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index cf9cdd494674f..8161685c652b5 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -340,6 +340,7 @@ static int sparx5_create_port(struct sparx5 *sparx5,
 	spx5_port->phylink_pcs.ops = &sparx5_phylink_pcs_ops;
 	spx5_port->is_mrouter = false;
 	INIT_LIST_HEAD(&spx5_port->tc_templates);
+	mutex_init(&spx5_port->wm_lock);
 	sparx5->ports[config->portno] = spx5_port;
 
 	err = sparx5_port_init(sparx5, spx5_port, &config->conf);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 97bf8a6cb83f5..59deeb91ad921 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -215,6 +215,8 @@ struct sparx5_port {
 	struct device_node *of_node;
 	struct phy *serdes;
 	struct sparx5_port_config conf;
+	struct mutex wm_lock; /* Protects the QSYS watermarks and wm_pause */
+	u8 wm_pause;
 	struct phylink_config phylink_config;
 	struct phylink *phylink;
 	struct phylink_pcs phylink_pcs;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
index 701d310df0ba6..87aa0b7841359 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
@@ -373,11 +373,14 @@ static int sparx5_port_disable(struct sparx5 *sparx5, struct sparx5_port *port,
 		 sparx5,
 		 HSCH_PORT_MODE(port->portno));
 
-	/* 5: Disable Flowcontrol */
+	/* 5: Disable Flowcontrol, and keep an MTU change from re-enabling it */
+	mutex_lock(&port->wm_lock);
+	port->wm_pause = 0;
 	spx5_rmw(QSYS_PAUSE_CFG_PAUSE_STOP_SET(0xFFF - 1),
 		 QSYS_PAUSE_CFG_PAUSE_STOP,
 		 sparx5,
 		 QSYS_PAUSE_CFG(port->portno));
+	mutex_unlock(&port->wm_lock);
 
 	spd_prm = spd == SPEED_10 ? 1000 : spd == SPEED_100 ? 100 : 10;
 	/* 6: Wait while the last frame is exiting the queues */
@@ -676,24 +679,53 @@ int sparx5_port_fwd_urg(struct sparx5 *sparx5, u32 speed)
 	return urg / clk_period_ps - 1;
 }
 
+/* The top bit of the watermark field is a x16 multiplier */
 static u16 sparx5_wm_enc(u16 value)
 {
-	if (value >= 2048)
-		return 2048 + value / 16;
+	u16 mult = BIT(regs->fsize[FW_QSYS_ATOP_ATOP] - 1);
+
+	if (value >= mult)
+		return mult + value / 16;
 
 	return value;
 }
 
+/* Pause start/stop and tail drop at 6/4/20 frames of the port MTU */
+static void sparx5_port_wm_set(struct sparx5 *sparx5, struct sparx5_port *port,
+			       u8 pause)
+{
+	u32 len = SPX5_HW_MTU(READ_ONCE(port->ndev->mtu)) + 2 * VLAN_HLEN;
+	u32 cells = DIV_ROUND_UP(len, SPX5_BUFFER_CELL_SZ);
+	u32 pause_stop = 0xFFF - 1; /* FC gen disabled */
+
+	if (pause & MLO_PAUSE_TX)
+		pause_stop = sparx5_wm_enc(4 * cells);
+
+	spx5_rmw(QSYS_PAUSE_CFG_PAUSE_START_SET(sparx5_wm_enc(6 * cells)) |
+		 QSYS_PAUSE_CFG_PAUSE_STOP_SET(pause_stop),
+		 QSYS_PAUSE_CFG_PAUSE_START |
+		 QSYS_PAUSE_CFG_PAUSE_STOP,
+		 sparx5,
+		 QSYS_PAUSE_CFG(port->portno));
+
+	/* Port ATOP. Frames are tail dropped when this WM is hit */
+	spx5_wr(QSYS_ATOP_ATOP_SET(sparx5_wm_enc(20 * cells)),
+		sparx5,
+		QSYS_ATOP(port->portno));
+}
+
+void sparx5_port_wm_update(struct sparx5 *sparx5, struct sparx5_port *port)
+{
+	mutex_lock(&port->wm_lock);
+	sparx5_port_wm_set(sparx5, port, port->wm_pause);
+	mutex_unlock(&port->wm_lock);
+}
+
 static int sparx5_port_fc_setup(struct sparx5 *sparx5,
 				struct sparx5_port *port,
 				struct sparx5_port_config *conf)
 {
 	bool fc_obey = conf->pause & MLO_PAUSE_RX ? 1 : 0;
-	u32 pause_stop = 0xFFF - 1; /* FC gen disabled */
-
-	if (conf->pause & MLO_PAUSE_TX)
-		pause_stop = sparx5_wm_enc(4  * (ETH_MAXLEN /
-						 SPX5_BUFFER_CELL_SZ));
 
 	/* Set HDX flowcontrol */
 	spx5_rmw(DSM_MAC_CFG_HDX_BACKPREASSURE_SET(conf->duplex == DUPLEX_HALF),
@@ -714,10 +746,10 @@ static int sparx5_port_fc_setup(struct sparx5 *sparx5,
 		 QSYS_FWD_PRESSURE(port->portno));
 
 	/* Generate pause frames */
-	spx5_rmw(QSYS_PAUSE_CFG_PAUSE_STOP_SET(pause_stop),
-		 QSYS_PAUSE_CFG_PAUSE_STOP,
-		 sparx5,
-		 QSYS_PAUSE_CFG(port->portno));
+	mutex_lock(&port->wm_lock);
+	port->wm_pause = conf->pause;
+	sparx5_port_wm_set(sparx5, port, conf->pause);
+	mutex_unlock(&port->wm_lock);
 
 	return 0;
 }
@@ -1101,15 +1133,12 @@ int sparx5_port_init(struct sparx5 *sparx5,
 		     struct sparx5_port *port,
 		     struct sparx5_port_config *conf)
 {
-	u32 pause_start = sparx5_wm_enc(6  * (ETH_MAXLEN / SPX5_BUFFER_CELL_SZ));
-	u32 atop = sparx5_wm_enc(20 * (ETH_MAXLEN / SPX5_BUFFER_CELL_SZ));
 	const struct sparx5_ops *ops = sparx5->data->ops;
 	u32 pix = sparx5_port_dev_index(sparx5, port->portno);
 	u32 pcs = sparx5_to_pcs_dev(sparx5, port->portno);
 	bool sd_pol = port->signd_active_high;
 	bool sd_sel = !port->signd_internal;
 	bool sd_ena = port->signd_enable;
-	u32 pause_stop = 0xFFF - 1; /* FC generate disabled */
 	void __iomem *pcsinst;
 	int err;
 
@@ -1120,21 +1149,13 @@ int sparx5_port_init(struct sparx5 *sparx5,
 	if (err)
 		return err;
 
-	/* Set Pause WM hysteresis */
-	spx5_rmw(QSYS_PAUSE_CFG_PAUSE_START_SET(pause_start) |
-		 QSYS_PAUSE_CFG_PAUSE_STOP_SET(pause_stop) |
-		 QSYS_PAUSE_CFG_PAUSE_ENA_SET(1),
-		 QSYS_PAUSE_CFG_PAUSE_START |
-		 QSYS_PAUSE_CFG_PAUSE_STOP |
+	/* Set Pause WM hysteresis and ATOP, FC generate disabled */
+	sparx5_port_wm_set(sparx5, port, 0);
+	spx5_rmw(QSYS_PAUSE_CFG_PAUSE_ENA_SET(1),
 		 QSYS_PAUSE_CFG_PAUSE_ENA,
 		 sparx5,
 		 QSYS_PAUSE_CFG(port->portno));
 
-	/* Port ATOP. Frames are tail dropped when this WM is hit */
-	spx5_wr(QSYS_ATOP_ATOP_SET(atop),
-		sparx5,
-		QSYS_ATOP(port->portno));
-
 	/* Discard pause frame 01-80-C2-00-00-01 */
 	spx5_wr(PAUSE_DISCARD, sparx5, ANA_CL_CAPTURE_BPDU_CFG(port->portno));
 
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.h b/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
index 912f97a506768..06d2becfd9aac 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.h
@@ -91,6 +91,7 @@ int sparx5_port_init(struct sparx5 *sparx5,
 void sparx5_port_max_len_set(struct sparx5 *sparx5,
 			     struct sparx5_port *port,
 			     u32 len);
+void sparx5_port_wm_update(struct sparx5 *sparx5, struct sparx5_port *port);
 
 int sparx5_port_config(struct sparx5 *sparx5,
 		       struct sparx5_port *spx5_port,

-- 
2.34.1

[PATCH net-next v2 8/9] net: sparx5/lan969x: scale the TAS guard band with the MTU

[Daniel Machon]

The TAS guard band (QMAXSDU_VAL, in units of 64 bytes) is how long
before a gate closes the scheduler stops starting frames of a traffic
class, so that a frame started just before the close still finishes in
time. The driver leaves it at the reset value of 24 x 64 = 1536 bytes,
so with a larger MTU, a frame started inside the guard band would
overrun the gate close.

The MAC maximum frame length only limits the frames a port receives, so
a port can transmit frames of the largest MTU of all ports, for example
when forwarding from a port with a larger MTU. Therefore, size the guard
band of all ports for a frame of the largest MTU, plus the Ethernet
header, FCS and two VLAN tags, rounded up to 64 bytes. Add the
HSCH_TAS_QMAXSDU_CFG register, program the guard band at init, and add
sparx5_tas_gb_update() for use when the MTU changes.

With the default MTU of 1500 the guard band is unchanged. As the
scheduler does not start a frame inside the guard band, a gate window
shorter than the guard band at the link speed cannot transmit, as is
already the case for the default 1536 byte guard band.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  1 +
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 12 ++++++
 drivers/net/ethernet/microchip/sparx5/sparx5_tas.c | 43 +++++++++++++++++++++-
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 59deeb91ad921..9bd3f7827b1f2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -746,6 +746,7 @@ int sparx5_tas_enable(struct sparx5_port *port,
 		      struct tc_taprio_qopt_offload *qopt);
 int sparx5_tas_disable(struct sparx5_port *port);
 void sparx5_tas_speed(struct sparx5_port *port, int speed);
+void sparx5_tas_gb_update(struct sparx5 *sparx5);
 
 /* sparx5_mirror.c */
 int sparx5_mirror_add(struct sparx5_mall_entry *entry);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
index 5302c7686e207..8a6de30042b5d 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
@@ -5424,6 +5424,18 @@ extern const struct sparx5_regs *regs;
 #define HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY_GET(x)\
 	FIELD_GET(HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY, x)
 
+/* HSCH:TAS_PROFILE_CFG:TAS_QMAXSDU_CFG */
+#define HSCH_TAS_QMAXSDU_CFG(g, r)                                             \
+	__REG(TARGET_HSCH, 0, 1, regs->gaddr[GA_HSCH_TAS_PROFILE_CFG], g,       \
+	      regs->gcnt[GC_HSCH_TAS_PROFILE_CFG],                              \
+	      regs->gsize[GW_HSCH_TAS_PROFILE_CFG], 0, r, 8, 4)
+
+#define HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL         GENMASK(7, 0)
+#define HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL_SET(x)\
+	FIELD_PREP(HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL, x)
+#define HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL_GET(x)\
+	FIELD_GET(HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL, x)
+
 /* HSCH:TAS_PROFILE_CFG:TAS_PROFILE_CONFIG */
 #define HSCH_TAS_PROFILE_CONFIG(g)                                             \
 	__REG(TARGET_HSCH, 0, 1, regs->gaddr[GA_HSCH_TAS_PROFILE_CFG], g,       \
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c
index ec80bc69371ab..32e502bc4f18f 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c
@@ -29,6 +29,9 @@ enum sparx5_tas_link_speed {
 /* Maximum supported cycle time in nanoseconds */
 #define TAS_MAX_CYCLE_TIME_NS ((1 * NSEC_PER_SEC) - 1) /* 999.999.999 nsec */
 
+/* HSCH_TAS_QMAXSDU_CFG.QMAXSDU_VAL, the guard band, counts 64 byte units */
+#define TAS_QMAXSDU_UNIT 64
+
 /* STARTUP_TIME in units of 256 ns, must be non-zero for a list to start */
 #define TAS_STARTUP_TIME_DEFAULT 8
 
@@ -48,8 +51,8 @@ enum sparx5_tas_link_speed {
  * We use 1 TAS profile per port, shared by both of its lists:
  * index = portno;
  *
- * We do not yet support frame preemption and the default guard band
- * of 1536 bytes is always used on all queues.
+ * We do not yet support frame preemption. All queues of all ports use a
+ * guard band that fits the largest frame that can be forwarded.
  *
  * The gate mask of each entry is written as is to the port's 8 queues,
  * where bit n gates queue (QoS class) n. The mqprio parameters of the
@@ -718,6 +721,40 @@ int sparx5_tas_disable(struct sparx5_port *port)
 	return err;
 }
 
+/* Size the guard band of all ports for a frame of the largest MTU of all
+ * ports, as a port can transmit frames forwarded from any port.
+ */
+void sparx5_tas_gb_update(struct sparx5 *sparx5)
+{
+	u32 max_mtu = 0, gb;
+	int i, tc;
+
+	for (i = 0; i < sparx5->data->consts->n_ports; i++) {
+		if (sparx5->ports[i])
+			max_mtu = max_t(u32, max_mtu,
+					READ_ONCE(sparx5->ports[i]->ndev->mtu));
+	}
+
+	gb = DIV_ROUND_UP(SPX5_HW_MTU(max_mtu) + 2 * VLAN_HLEN,
+			  TAS_QMAXSDU_UNIT);
+
+	mutex_lock(&sparx5->tas_lock);
+
+	/* Each port uses the TAS profile of its own number */
+	for (i = 0; i < sparx5->data->consts->n_ports; i++) {
+		if (!sparx5->ports[i])
+			continue;
+
+		for (tc = 0; tc < SPX5_PRIOS; tc++)
+			spx5_rmw(HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL_SET(gb),
+				 HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL,
+				 sparx5,
+				 HSCH_TAS_QMAXSDU_CFG(i, tc));
+	}
+
+	mutex_unlock(&sparx5->tas_lock);
+}
+
 int sparx5_tas_init(struct sparx5 *sparx5)
 {
 	const struct sparx5_consts *consts = sparx5->data->consts;
@@ -764,6 +801,8 @@ int sparx5_tas_init(struct sparx5 *sparx5)
 		}
 	}
 
+	sparx5_tas_gb_update(sparx5);
+
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH net-next v2 8/9] net: sparx5/lan969x: scale the TAS guard band with the MTU

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] sparx5_tas_gb_update()
  (drivers/net/ethernet/microchip/sparx5/sparx5_tas.c) computes one…
- [Low] sparx5_tas_gb_update() sizes the guard band from L2 bytes only:
  SPX5_HW_MTU(max_mtu) + 2*VLAN_HLEN, i.e. MTU + ETH_HLEN + FCS + two…

--

net: sparx5/lan969x: scale the TAS guard band with the MTU

This commit adds the HSCH_TAS_QMAXSDU_CFG register. It also adds
sparx5_tas_gb_update(), which sizes the TAS guard band of every port and
traffic class for the largest MTU across all ports. The guard band is
programmed at init, and the helper is exported so it can be called when
the MTU changes.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c
> index ec80bc69371ab..32e502bc4f18f 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tas.c

[ ... ]

> @@ -718,6 +721,40 @@ int sparx5_tas_disable(struct sparx5_port *port)
>  	return err;
>  }
>  
> +/* Size the guard band of all ports for a frame of the largest MTU of all
> + * ports, as a port can transmit frames forwarded from any port.
> + */
> +void sparx5_tas_gb_update(struct sparx5 *sparx5)
> +{
> +	u32 max_mtu = 0, gb;
> +	int i, tc;
> +
> +	for (i = 0; i < sparx5->data->consts->n_ports; i++) {
> +		if (sparx5->ports[i])
> +			max_mtu = max_t(u32, max_mtu,
> +					READ_ONCE(sparx5->ports[i]->ndev->mtu));
> +	}
> +
> +	gb = DIV_ROUND_UP(SPX5_HW_MTU(max_mtu) + 2 * VLAN_HLEN,
> +			  TAS_QMAXSDU_UNIT);

[Severity: Low]
Should this calculation include the 8 bytes of preamble and SFD?

The guard band is MTU + ETH_HLEN + ETH_FCS_LEN + 2 * VLAN_HLEN, rounded up
to 64 bytes, so the rounding slack is anywhere from 0 to 63 bytes. For some
MTUs the slack is under 8 bytes. With MTU 9190, for example:

    9190 + 14 + 4 + 8 = 9216 = 144 * 64

which leaves no slack at all.

Suppose the HSCH converts QMAXSDU_VAL * 64 to time at the link speed and
does not add preamble/SFD itself. Then a maximum size frame started just
before the guard band could still be on the wire for up to 8 byte times
after the gate closes.

I couldn't confirm this from the register definitions. The felix driver
uses the same formula for QSYS_PORT_MAX_SDU and adds the 24 byte L1
overhead only when it checks gate lengths, so the hardware may already
handle it. The default MTU of 1500 has 10 bytes of slack and is not
affected.

> +
> +	mutex_lock(&sparx5->tas_lock);
> +
> +	/* Each port uses the TAS profile of its own number */
> +	for (i = 0; i < sparx5->data->consts->n_ports; i++) {
> +		if (!sparx5->ports[i])
> +			continue;
> +
> +		for (tc = 0; tc < SPX5_PRIOS; tc++)
> +			spx5_rmw(HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL_SET(gb),
> +				 HSCH_TAS_QMAXSDU_CFG_QMAXSDU_VAL,
> +				 sparx5,
> +				 HSCH_TAS_QMAXSDU_CFG(i, tc));
> +	}
> +
> +	mutex_unlock(&sparx5->tas_lock);
> +}

[Severity: Medium]
A later patch in this series, "net: sparx5/lan969x: add support for
changing the MTU", makes sparx5_port_change_mtu() call
sparx5_tas_gb_update(). After that, raising the MTU of any one port grows
the guard band of every port. This includes ports that stay at MTU 1500
and already run an offloaded taprio schedule.

For example, port A runs a 1G schedule in which one TC has a 15-20 us
window. That works with the 1536 byte guard band (about 12.3 us at 1G).
Setting MTU 9000 on port B raises the guard band on port A to 9088 bytes
(about 72.7 us at 1G).

Would that TC on port A then stop starting frames, without any error or
log message?

Nothing seems to catch this case. sparx5_tas_enable() checks entry
intervals only against TAS_MIN_CYCLE_TIME_NS and TAS_MAX_CYCLE_TIME_NS,
not against the guard band time at the port speed.
sparx5_tas_gb_update() rewrites the live profiles without looking at
running schedules. Because sparx5_tas_init() sets
HSCH_TAS_CFG_CTRL_ALWAYS_GUARD_BAND_SCH_Q, the new value applies to every
gate close.

For comparison, vsc9959_tas_guard_bands_update() in felix_vsc9959.c
re-checks each TC's minimum gate length against the required frame time
when the MTU changes. When the gate is too short, it enables oversize
frame dropping and prints a dev_info, so the TC does not hang the port.

Sizing for the global maximum looks reasonable, because matchall mirred
and flower redirect can send frames from any port to any egress port.
Could running schedules whose windows become unusable be validated, or at
least reported?

The commit message also says:

    As the scheduler does not start a frame inside the guard band, a gate
    window shorter than the guard band at the link speed cannot transmit,
    as is already the case for the default 1536 byte guard band.

It doesn't mention that this threshold now changes under existing
schedules when another port's MTU changes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com

[PATCH net-next v2 9/9] net: sparx5/lan969x: add support for changing the MTU

[Daniel Machon]

The MTU of a port is currently fixed to 1500, as the MAC maximum frame
length, the FDMA buffers, the queue watermarks and the TAS guard band
are all sized for a 1518 byte frame. The previous patches make the
buffers, watermarks and guard band follow the MTU, so changing the MTU
is a matter of programming the MAC and updating these.

Add an ndo_change_mtu callback that sets the MAC maximum frame length of
the port and resizes the FDMA buffers. The MAC must never accept frames
that are larger than the RX buffers, so when decreasing the MTU, the
maximum frame length is lowered before the buffers are shrunk, and when
increasing it, it is only raised once the buffers have been grown. The
new MTU is only published once the buffers fit it. If the buffers cannot
be grown, the MTU is not changed. If they cannot be shrunk, the current
buffers still fit all frames, so the new MTU is kept. Once the new MTU
is published, update the queue watermarks of the port and the TAS guard
band.

As the FDMA is shared by all ports, resizing its buffers briefly stops
CPU traffic on all ports. This only happens when the change affects the
buffer size needed for the largest MTU of the ports.

Register based extraction, which is used when the FDMA is not, sizes
its skbs from the MTU at the time a frame is read, so frames queued
before an MTU decrease would not fit. Therefore, only allow the MTU to be
changed when the FDMA is used.

The MACs of both Sparx5 and lan969x support frames of up to 14000 bytes,
so set the maximum MTU accordingly when the FDMA is used.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  1 +
 .../net/ethernet/microchip/sparx5/sparx5_netdev.c  | 48 +++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 9bd3f7827b1f2..03643adad4caf 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -110,6 +110,7 @@ enum sparx5_feature {
 #define SPX5_BUFFER_MEMORY     4194280 /* 22795 words * 184 bytes */
 #define SPX5_TAS_ENTRIES_PER_PORT 2 /* TAS lists per port */
 #define SPX5_HW_MTU(mtu)       ((mtu) + ETH_HLEN + ETH_FCS_LEN)
+#define SPX5_MAX_FRAME_LEN     14000 /* Max frame length supported by the MACs */
 
 #define XTR_QUEUE     0
 #define INJ_QUEUE     0
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
index b3725cf2dd44c..095925752f5f6 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
@@ -250,6 +250,44 @@ static int sparx5_port_hwtstamp_set(struct net_device *dev,
 	return sparx5_ptp_hwtstamp_set(sparx5_port, cfg, extack);
 }
 
+static int sparx5_port_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct sparx5_port *port = netdev_priv(dev);
+	struct sparx5 *sparx5 = port->sparx5;
+	bool resize = new_mtu > dev->mtu;
+	int err;
+
+	/* Register based extraction sizes its skbs from the MTU at the time
+	 * a frame is read, so it cannot handle frames queued before an MTU
+	 * decrease.
+	 */
+	if (sparx5->fdma_irq <= 0)
+		return -EOPNOTSUPP;
+
+	/* Never let the MAC accept frames larger than the RX buffers */
+	if (!resize)
+		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
+
+	err = sparx5->data->ops->fdma_resize(sparx5, port, new_mtu);
+	if (err) {
+		if (resize)
+			return err;
+
+		/* The current, larger, buffers still fit all frames */
+		netdev_warn(dev, "Failed to resize FDMA buffers: %d\n", err);
+	}
+
+	WRITE_ONCE(dev->mtu, new_mtu);
+
+	sparx5_port_wm_update(sparx5, port);
+	sparx5_tas_gb_update(sparx5);
+
+	if (resize)
+		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
+
+	return 0;
+}
+
 static const struct net_device_ops sparx5_port_netdev_ops = {
 	.ndo_open               = sparx5_port_open,
 	.ndo_stop               = sparx5_port_stop,
@@ -257,6 +295,7 @@ static const struct net_device_ops sparx5_port_netdev_ops = {
 	.ndo_set_rx_mode_async  = sparx5_set_rx_mode,
 	.ndo_get_phys_port_name = sparx5_port_get_phys_port_name,
 	.ndo_set_mac_address    = sparx5_set_mac_address,
+	.ndo_change_mtu         = sparx5_port_change_mtu,
 	.ndo_validate_addr      = eth_validate_addr,
 	.ndo_get_stats64        = sparx5_get_stats64,
 	.ndo_get_port_parent_id = sparx5_get_port_parent_id,
@@ -306,7 +345,14 @@ int sparx5_register_netdevs(struct sparx5 *sparx5)
 
 	for (portno = 0; portno < sparx5->data->consts->n_ports; portno++)
 		if (sparx5->ports[portno]) {
-			err = register_netdev(sparx5->ports[portno]->ndev);
+			struct net_device *ndev = sparx5->ports[portno]->ndev;
+
+			/* The MTU can only be changed when FDMA is used */
+			if (sparx5->fdma_irq > 0)
+				ndev->max_mtu = SPX5_MAX_FRAME_LEN - ETH_HLEN -
+						ETH_FCS_LEN;
+
+			err = register_netdev(ndev);
 			if (err) {
 				dev_err(sparx5->dev,
 					"port: %02u: netdev registration failed\n",

-- 
2.34.1

Re: [PATCH net-next v2 9/9] net: sparx5/lan969x: add support for changing the MTU

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: sparx5: lan969x: double free of RX page after
  lan969x_fdma_rx_get_frame()
- [Medium] net: sparx5: GFP_ATOMIC RX ring allocation in
  sparx5_port_change_mtu()
- [Medium] net: sparx5: sparx5_port_change_mtu() blocks non-FDMA MTU
  changes
- [Low] An MTU increase can fail even when the current FDMA buffers
  already fit the new MTU.

--

net: sparx5/lan969x: add support for changing the MTU

This adds an ndo_change_mtu callback for sparx5/lan969x ports. It
programs the MAC maximum frame length and resizes the shared FDMA
buffers, and orders these steps so the MAC never accepts frames larger
than the RX buffers. MTU changes are only allowed when the FDMA is
used, and max_mtu is set from the 14000 byte MAC limit.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> index b3725cf2dd44c..095925752f5f6 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> @@ -250,6 +250,44 @@ static int sparx5_port_hwtstamp_set(struct net_device *dev,
>  	return sparx5_ptp_hwtstamp_set(sparx5_port, cfg, extack);
>  }
>  
> +static int sparx5_port_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	struct sparx5_port *port = netdev_priv(dev);
> +	struct sparx5 *sparx5 = port->sparx5;
> +	bool resize = new_mtu > dev->mtu;
> +	int err;

[ ... ]

> +	/* Never let the MAC accept frames larger than the RX buffers */
> +	if (!resize)
> +		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
> +
> +	err = sparx5->data->ops->fdma_resize(sparx5, port, new_mtu);
> +	if (err) {
> +		if (resize)
> +			return err;

[Severity: High]
This error path isn't new in this patch, but wiring up fdma_resize()
for MTU changes makes it reachable at runtime. Can the lan969x resize
double free an RX page?

Once the page belongs to the skb, lan969x_fdma_rx_get_frame() clears
the slot with:

	rx->page[fdma->dcb_index][fdma->db_index] = NULL;

This tells lan969x_fdma_free_pages() that the driver no longer owns the
page. The free_page path is taken when WARN_ON(!port) triggers or
build_skb() returns NULL. There, the page is handed back with
page_pool_recycle_direct(), but the rx->page[][] slot still points at
it.

lan969x_fdma_napi_poll() then breaks out without advancing dcb_index
or refilling the descriptor. The next poll processes the same, already
recycled, page again.

On an MTU change, lan969x_fdma_resize() -> lan969x_fdma_rx_free() ->
lan969x_fdma_free_pages() calls page_pool_put_full_page() on every
non-NULL rx->page[i][j]. That returns the recycled page to
rx->page_pool a second time.

Without the resize path, the ring was only freed from
lan969x_fdma_deinit(). With ndo_change_mtu, a build_skb() failure
under memory pressure followed by an MTU change is enough to corrupt
the page pool.

Should the free_page path also clear the rx->page[][] slot, and reset
or refill the descriptor so the poll loop moves past it? The
underlying bug predates this series, so would it make sense to fix it
separately for net?

[Severity: Medium]
Does sparx5_fdma_resize() end up allocating the whole replacement RX
ring with GFP_ATOMIC?

sparx5_port_change_mtu() runs in process context under RTNL. But
sparx5_fdma_resize() allocates the new ring through
sparx5_fdma_rx_alloc() -> fdma_dcbs_init() -> fdma_dcb_add(), which
calls sparx5_fdma_rx_dataptr_cb() for each of the 64 * 15 = 960
buffers. That callback always allocates with GFP_ATOMIC:

	skb = __netdev_alloc_skb(rx->ndev, fdma->db_size, GFP_ATOMIC);

With max_mtu now raised to 13982, db_size grows to roughly 14 KB. Each
skb head becomes an order-2 allocation, and the replacement ring is
around 15 MB. All of it is allocated while the old ring is still held.

GFP_ATOMIC can't enter direct reclaim or compaction. On a system that
has been up for a while, this is likely to fail with -ENOMEM and make
the MTU increase fail. It can also drain the atomic reserves that NAPI
and interrupt handlers on all the other ports rely on.

Could the gfp flags be passed down to the dataptr callback? Then the
resize and init paths could use GFP_KERNEL, and only the refill from
NAPI would keep GFP_ATOMIC.

[Severity: Low]
Can an MTU increase fail here even though the current FDMA buffers
already fit the new MTU?

When resize is true, sparx5_port_change_mtu() treats any fdma_resize()
error as fatal. But sparx5_fdma_resize() skips the reallocation only
when the size matches exactly:

drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c:sparx5_fdma_resize() {
    ...
	db_size = sparx5_fdma_get_db_size(sparx5, port, new_mtu);
	if (db_size == rx->fdma.db_size)
		return 0;

	rx_new = kzalloc_obj(*rx_new);
    ...
}

lan969x_fdma_resize() does the same with page_order:

drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c:lan969x_fdma_resize() {
    ...
	page_order = lan969x_fdma_get_page_order(sparx5, port, new_mtu);
	if (page_order == rx->page_order)
		return 0;
    ...
}

Take port A at MTU 9000 and all other ports at 1500. Lowering A to 1500
tries to shrink the buffers. If that allocation fails, the 9000-sized
buffers are kept and MTU 1500 is published, as intended.

Then port B is raised to 3000. The needed db_size (or page_order) is
smaller than the current one, so the driver allocates new, smaller
rings. If that allocation also fails, the increase returns an error and
B's MTU stays the same, even though the existing buffers would hold
3000-byte frames.

This doesn't match the commit message:

    If the buffers cannot be grown, the MTU is not changed.

Would a "needed > current" check on the grow path avoid this? Treating
a failed shrink as non-fatal in this case would work too.

> +
> +		/* The current, larger, buffers still fit all frames */
> +		netdev_warn(dev, "Failed to resize FDMA buffers: %d\n", err);
> +	}
> +
> +	WRITE_ONCE(dev->mtu, new_mtu);

[ ... ]

> @@ -250,6 +250,44 @@ static int sparx5_port_hwtstamp_set(struct net_device *dev,
>  	return sparx5_ptp_hwtstamp_set(sparx5_port, cfg, extack);
>  }
>  
> +static int sparx5_port_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	struct sparx5_port *port = netdev_priv(dev);
> +	struct sparx5 *sparx5 = port->sparx5;
> +	bool resize = new_mtu > dev->mtu;
> +	int err;
> +
> +	/* Register based extraction sizes its skbs from the MTU at the time
> +	 * a frame is read, so it cannot handle frames queued before an MTU
> +	 * decrease.
> +	 */
> +	if (sparx5->fdma_irq <= 0)
> +		return -EOPNOTSUPP;
> +
> +	/* Never let the MAC accept frames larger than the RX buffers */
> +	if (!resize)
> +		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
> +
> +	err = sparx5->data->ops->fdma_resize(sparx5, port, new_mtu);
> +	if (err) {
> +		if (resize)
> +			return err;
> +
> +		/* The current, larger, buffers still fit all frames */
> +		netdev_warn(dev, "Failed to resize FDMA buffers: %d\n", err);
> +	}
> +
> +	WRITE_ONCE(dev->mtu, new_mtu);
> +
> +	sparx5_port_wm_update(sparx5, port);
> +	sparx5_tas_gb_update(sparx5);
> +
> +	if (resize)
> +		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops sparx5_port_netdev_ops = {
>  	.ndo_open               = sparx5_port_open,
>  	.ndo_stop               = sparx5_port_stop,

[Severity: Medium]
Doesn't this take away something that works today with register
based extraction?

This affects systems where sparx5->fdma_irq <= 0: Sparx5 rev 0, no
FDMA interrupt in the devicetree, or the fallback to xtr_irq. Without
an ndo_change_mtu, __netif_set_mtu() just updates dev->mtu. Userspace
can currently set any MTU in the [68, 1500] range that ether_setup()
advertises.

With this patch, every such change returns -EOPNOTSUPP. Meanwhile,
sparx5_register_netdevs() still leaves min_mtu at 68 and max_mtu at
1500 on these ports. Lowering the MTU for a tunnel, for example, used
to work and now fails.

For the same reason, the commit message is not quite accurate when it
says the MTU is currently fixed to 1500.

That said, lowering the MTU on the register based path isn't safe
today either. sparx5_xtr_grp() allocates netdev->mtu + ETH_HLEN bytes,
which already leaves out the FCS and any VLAN tags. It then copies
extraction words into rxbuf until EOF without checking skb_tailroom().
With the MAC maximum length still at 1518, a smaller dev->mtu lets a
full sized frame overrun the skb.

Which behaviour is intended here? If MTU changes are not supported
without the FDMA, would it be better to set min_mtu and max_mtu to
ETH_DATA_LEN for these ports? The core would then reject the change,
and the advertised range would match what the driver accepts.

Alternatively, could sparx5_xtr_grp() size its skbs from the MAC
maximum frame length and bound the copy? Then the register based path
could follow the MTU as well. Either way, the missing tailroom check
probably deserves its own fix for net.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com