[PATCH net-next v12 0/4] net: mana: add ethtool private flag for full-page RX buffers

[PATCH net-next v12 0/4] net: mana: add ethtool private flag for full-page RX buffers

[Dipayaan Roy]

On some ARM64 platforms with 4K PAGE_SIZE, utilizing page_pool
fragments for allocation in the RX refill path (~2kB buffer per fragment)
causes 15-20% throughput regression under high connection counts
(>16 TCP streams at 180+ Gbps). Using full-page buffers on these
platforms shows no regression and restores line-rate performance.

This behavior is observed on a single platform; other platforms
perform better with page_pool fragments, indicating this is not a
page_pool issue but platform-specific.

This series adds an ethtool private flag "full-page-rx" to let the
user opt in to one RX buffer per page:

  ethtool --set-priv-flags eth0 full-page-rx on

There is no behavioral change by default. The flag can be persisted
via udev rule for affected platforms.

Patches 2 and 4 harden the detach/attach path so that ethtool
operations (ring size, channel count, priv-flags) can recover the
port via the queue_reset_work handler when mana_attach() fails,
instead of leaving the port permanently dead.

This series depends on the following fixes now merged in net-next:
  commit 17bfe0a8c014 ("net: mana: Add NULL guards in teardown path to prevent panic on attach failure")
  commit 5b05aa36ee24 ("net: mana: Skip redundant detach on already-detached port")

Changes in v12:
  - Added patch 2 to ensure mana_detach() always completes its full
    teardown even if mana_dealloc_queues() fails, keeping port state
    consistent for recovery.
  - Added patch 4 to schedule queue_reset_work when mana_attach()
    fails during ethtool ring size or channel count changes, with
    fallback values that maximize recovery chances.
Changes in v11:
  - Rebased on net-next
Changes in v10:
  - Rebased on net-next which now includes the prerequisite fixes.
  - Recovery logic in mana_set_priv_flags() leverages the idempotent
    mana_detach() from the merged fixes.
Changes in v9:
  - Added correct tree.
Changes in v8:
  - Fixed queue_reset_work recovery by restoring port_is_up before
    scheduling reset so the handler can properly re-attach.
  - Simplified "err && schedule_port_reset" to "schedule_port_reset".
Changes in v7:
  - Rebased onto net-next.
  - Retained private flag approach after David Wei's testing on
    Grace (ARM64) confirmed that fragment mode outperforms
    full-page mode on other platforms, validating this is a
    single-platform workaround rather than a generic issue.
Changes in v6:
  - Added missed maintainers.
Changes in v5:
  - Split prep refactor into separate patch (patch 1/2)
Changes in v4:
  - Dropping the smbios string parsing and add ethtool priv flag
    to reconfigure the queues with full page rx buffers.
Changes in v3:
  - changed u8* to char*
Changes in v2:
  - separate reading string index and the string, remove inline.

Dipayaan Roy (4):
  net: mana: refactor mana_get_strings() and mana_get_sset_count() to
    use switch
  net: mana: do not bail out of mana_detach on dealloc failure
  net: mana: force full-page RX buffers via ethtool private flag
  net: mana: recover port on attach failure in ethtool operations

 drivers/net/ethernet/microsoft/mana/mana_en.c |  26 +-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 223 +++++++++++++++---
 include/net/mana/mana.h                       |   8 +
 3 files changed, 220 insertions(+), 37 deletions(-)

-- 
2.43.0

[PATCH net-next v12 1/4] net: mana: refactor mana_get_strings() and mana_get_sset_count() to use switch

[Dipayaan Roy]

Refactor mana_get_strings() and mana_get_sset_count() from if/else to
switch statements in preparation for adding ethtool private flags
support which requires handling ETH_SS_PRIV_FLAGS.

No functional change.

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
 .../ethernet/microsoft/mana/mana_ethtool.c    | 75 ++++++++++++-------
 1 file changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 9e31e2595ae3..482cd16009ab 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -138,53 +138,70 @@ static int mana_get_sset_count(struct net_device *ndev, int stringset)
 	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int num_queues = apc->num_queues;
 
-	if (stringset != ETH_SS_STATS)
+	switch (stringset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(mana_eth_stats) +
+		       ARRAY_SIZE(mana_phy_stats) +
+		       ARRAY_SIZE(mana_hc_stats)  +
+		       num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+	default:
 		return -EINVAL;
-
-	return ARRAY_SIZE(mana_eth_stats) + ARRAY_SIZE(mana_phy_stats) + ARRAY_SIZE(mana_hc_stats) +
-			num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+	}
 }
 
-static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+static void mana_get_strings_stats(struct mana_port_context *apc, u8 **data)
 {
-	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int num_queues = apc->num_queues;
 	int i, j;
 
-	if (stringset != ETH_SS_STATS)
-		return;
 	for (i = 0; i < ARRAY_SIZE(mana_eth_stats); i++)
-		ethtool_puts(&data, mana_eth_stats[i].name);
+		ethtool_puts(data, mana_eth_stats[i].name);
 
 	for (i = 0; i < ARRAY_SIZE(mana_hc_stats); i++)
-		ethtool_puts(&data, mana_hc_stats[i].name);
+		ethtool_puts(data, mana_hc_stats[i].name);
 
 	for (i = 0; i < ARRAY_SIZE(mana_phy_stats); i++)
-		ethtool_puts(&data, mana_phy_stats[i].name);
+		ethtool_puts(data, mana_phy_stats[i].name);
 
 	for (i = 0; i < num_queues; i++) {
-		ethtool_sprintf(&data, "rx_%d_packets", i);
-		ethtool_sprintf(&data, "rx_%d_bytes", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_drop", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_tx", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_redirect", i);
-		ethtool_sprintf(&data, "rx_%d_pkt_len0_err", i);
+		ethtool_sprintf(data, "rx_%d_packets", i);
+		ethtool_sprintf(data, "rx_%d_bytes", i);
+		ethtool_sprintf(data, "rx_%d_xdp_drop", i);
+		ethtool_sprintf(data, "rx_%d_xdp_tx", i);
+		ethtool_sprintf(data, "rx_%d_xdp_redirect", i);
+		ethtool_sprintf(data, "rx_%d_pkt_len0_err", i);
 		for (j = 0; j < MANA_RXCOMP_OOB_NUM_PPI - 1; j++)
-			ethtool_sprintf(&data, "rx_%d_coalesced_cqe_%d", i, j + 2);
+			ethtool_sprintf(data,
+					"rx_%d_coalesced_cqe_%d",
+					i,
+					j + 2);
 	}
 
 	for (i = 0; i < num_queues; i++) {
-		ethtool_sprintf(&data, "tx_%d_packets", i);
-		ethtool_sprintf(&data, "tx_%d_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_xdp_xmit", i);
-		ethtool_sprintf(&data, "tx_%d_tso_packets", i);
-		ethtool_sprintf(&data, "tx_%d_tso_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_tso_inner_packets", i);
-		ethtool_sprintf(&data, "tx_%d_tso_inner_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_long_pkt_fmt", i);
-		ethtool_sprintf(&data, "tx_%d_short_pkt_fmt", i);
-		ethtool_sprintf(&data, "tx_%d_csum_partial", i);
-		ethtool_sprintf(&data, "tx_%d_mana_map_err", i);
+		ethtool_sprintf(data, "tx_%d_packets", i);
+		ethtool_sprintf(data, "tx_%d_bytes", i);
+		ethtool_sprintf(data, "tx_%d_xdp_xmit", i);
+		ethtool_sprintf(data, "tx_%d_tso_packets", i);
+		ethtool_sprintf(data, "tx_%d_tso_bytes", i);
+		ethtool_sprintf(data, "tx_%d_tso_inner_packets", i);
+		ethtool_sprintf(data, "tx_%d_tso_inner_bytes", i);
+		ethtool_sprintf(data, "tx_%d_long_pkt_fmt", i);
+		ethtool_sprintf(data, "tx_%d_short_pkt_fmt", i);
+		ethtool_sprintf(data, "tx_%d_csum_partial", i);
+		ethtool_sprintf(data, "tx_%d_mana_map_err", i);
+	}
+}
+
+static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	switch (stringset) {
+	case ETH_SS_STATS:
+		mana_get_strings_stats(apc, &data);
+		break;
+	default:
+		break;
 	}
 }
 
-- 
2.43.0

[PATCH net-next v12 2/4] net: mana: do not bail out of mana_detach on dealloc failure

[Dipayaan Roy]

mana_detach() sets port_is_up = false before calling
mana_dealloc_queues(). If that call were to fail and return early,
netif_device_detach() and mana_cleanup_port_context() are skipped,
leaving the port in an inconsistent state where port_is_up is false
but netif_device_present() still returns true. A subsequent
mana_detach() from the reset work handler would then overwrite
port_st_save with false, causing mana_attach() to skip queue
allocation and leave the port permanently dead.

Remove the early return so that mana_detach() always completes its
full teardown. mana_dealloc_queues() already performs best-effort
cleanup regardless of internal errors (and in practice cannot fail
here since port_is_up is already false), so continuing to
netif_device_detach() and mana_cleanup_port_context() is safe and
ensures the state is always consistent for recovery.

Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 89e7f59f635d..5e3c7a2a2b49 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -3707,10 +3707,8 @@ int mana_detach(struct net_device *ndev, bool from_close)
 
 	if (apc->port_st_save) {
 		err = mana_dealloc_queues(ndev);
-		if (err) {
+		if (err)
 			netdev_err(ndev, "%s failed to deallocate queues: %d\n", __func__, err);
-			return err;
-		}
 	}
 
 	if (!from_close) {
-- 
2.43.0

[PATCH net-next v12 3/4] net: mana: force full-page RX buffers via ethtool private flag

[Dipayaan Roy]

On some ARM64 platforms with 4K PAGE_SIZE, page_pool fragment
allocation in the RX refill path can cause 15-20% throughput
regression under high connection counts (>16 TCP streams).

Add an ethtool private flag "full-page-rx" that allows the user to
force one RX buffer per page, bypassing the page_pool fragment path.
This restores line-rate (180+ Gbps) performance on affected platforms.

Usage:
  ethtool --set-priv-flags eth0 full-page-rx on

There is no behavioral change by default. The flag must be explicitly
enabled by the user or udev rule.

The existing single-buffer-per-page logic for XDP and jumbo frames is
consolidated into a new helper mana_use_single_rxbuf_per_page() which
is now the single decision point for both the automatic and
user-controlled paths.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 +++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 100 ++++++++++++++++++
 include/net/mana/mana.h                       |   8 ++
 3 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 5e3c7a2a2b49..3e5c52e4886b 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -755,6 +755,25 @@ static void *mana_get_rxbuf_pre(struct mana_rxq *rxq, dma_addr_t *da)
 	return va;
 }
 
+static bool
+mana_use_single_rxbuf_per_page(struct mana_port_context *apc, u32 mtu)
+{
+	/* On some platforms with 4K PAGE_SIZE, page_pool fragment allocation
+	 * in the RX refill path (~2kB buffer) can cause significant throughput
+	 * regression under high connection counts. Allow user to force one RX
+	 * buffer per page via ethtool private flag to bypass the fragment
+	 * path.
+	 */
+	if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
+		return true;
+
+	/* For xdp and jumbo frames make sure only one packet fits per page. */
+	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
+		return true;
+
+	return false;
+}
+
 /* Get RX buffer's data size, alloc size, XDP headroom based on MTU */
 static void mana_get_rxbuf_cfg(struct mana_port_context *apc,
 			       int mtu, u32 *datasize, u32 *alloc_size,
@@ -765,8 +784,7 @@ static void mana_get_rxbuf_cfg(struct mana_port_context *apc,
 	/* Calculate datasize first (consistent across all cases) */
 	*datasize = mtu + ETH_HLEN;
 
-	/* For xdp and jumbo frames make sure only one packet fits per page */
-	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc)) {
+	if (mana_use_single_rxbuf_per_page(apc, mtu)) {
 		if (mana_xdp_get(apc)) {
 			*headroom = XDP_PACKET_HEADROOM;
 			*alloc_size = PAGE_SIZE;
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 482cd16009ab..f77509818d07 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -133,6 +133,10 @@ static const struct mana_stats_desc mana_phy_stats[] = {
 	{ "hc_tc7_tx_pause_phy", offsetof(struct mana_ethtool_phy_stats, tx_pause_tc7_phy) },
 };
 
+static const char mana_priv_flags[MANA_PRIV_FLAG_MAX][ETH_GSTRING_LEN] = {
+	[MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF] = "full-page-rx"
+};
+
 static int mana_get_sset_count(struct net_device *ndev, int stringset)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
@@ -144,6 +148,10 @@ static int mana_get_sset_count(struct net_device *ndev, int stringset)
 		       ARRAY_SIZE(mana_phy_stats) +
 		       ARRAY_SIZE(mana_hc_stats)  +
 		       num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+
+	case ETH_SS_PRIV_FLAGS:
+		return MANA_PRIV_FLAG_MAX;
+
 	default:
 		return -EINVAL;
 	}
@@ -192,6 +200,14 @@ static void mana_get_strings_stats(struct mana_port_context *apc, u8 **data)
 	}
 }
 
+static void mana_get_strings_priv_flags(u8 **data)
+{
+	int i;
+
+	for (i = 0; i < MANA_PRIV_FLAG_MAX; i++)
+		ethtool_puts(data, mana_priv_flags[i]);
+}
+
 static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
@@ -200,6 +216,9 @@ static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 	case ETH_SS_STATS:
 		mana_get_strings_stats(apc, &data);
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		mana_get_strings_priv_flags(&data);
+		break;
 	default:
 		break;
 	}
@@ -756,6 +775,84 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static u32 mana_get_priv_flags(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	return apc->priv_flags;
+}
+
+static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	u32 changed = apc->priv_flags ^ priv_flags;
+	u32 old_priv_flags = apc->priv_flags;
+	bool schedule_port_reset = false;
+	int err = 0;
+
+	if (!changed)
+		return 0;
+
+	/* Reject unknown bits */
+	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
+		return -EINVAL;
+
+	apc->priv_flags = priv_flags;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		if (!apc->port_is_up)
+			return 0;
+
+		/* If XDP is attached or MTU is jumbo, single-buffer-per-page
+		 * is already forced regardless of this flag. Skip the
+		 * expensive detach/attach cycle since nothing changes.
+		 */
+		if (ndev->mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 ||
+		    mana_xdp_get(apc))
+			return 0;
+
+		/* Block RDMA from grabbing the vport during detach/attach */
+		mutex_lock(&apc->vport_mutex);
+		apc->channel_changing = true;
+		mutex_unlock(&apc->vport_mutex);
+
+		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
+		if (err) {
+			netdev_err(ndev,
+				   "Insufficient memory for new allocations\n");
+			apc->priv_flags = old_priv_flags;
+			goto clear_flag;
+		}
+
+		err = mana_detach(ndev, false);
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+			goto out;
+		}
+
+		err = mana_attach(ndev);
+		if (err) {
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+			schedule_port_reset = true;
+		}
+	}
+
+out:
+	mana_pre_dealloc_rxbufs(apc);
+clear_flag:
+	mutex_lock(&apc->vport_mutex);
+	apc->channel_changing = false;
+	mutex_unlock(&apc->vport_mutex);
+
+	if (schedule_port_reset)
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
+
+	return err;
+}
+
 const struct ethtool_ops mana_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_CQE_FRAMES |
 				     ETHTOOL_COALESCE_RX_USECS |
@@ -766,6 +863,7 @@ const struct ethtool_ops mana_ethtool_ops = {
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_TX,
 	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
 				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
+				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS |
 				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
 	.get_ethtool_stats	= mana_get_ethtool_stats,
 	.get_sset_count		= mana_get_sset_count,
@@ -783,4 +881,6 @@ const struct ethtool_ops mana_ethtool_ops = {
 	.set_ringparam          = mana_set_ringparam,
 	.get_link_ksettings	= mana_get_link_ksettings,
 	.get_link		= ethtool_op_get_link,
+	.get_priv_flags		= mana_get_priv_flags,
+	.set_priv_flags		= mana_set_priv_flags,
 };
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 226b61504596..768d9f9bf167 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -31,6 +31,12 @@ enum TRI_STATE {
 	TRI_STATE_TRUE = 1
 };
 
+/* MANA ethtool private flag bit positions */
+enum mana_priv_flag_bits {
+	MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF = 0,
+	MANA_PRIV_FLAG_MAX,
+};
+
 /* Number of entries for hardware indirection table must be in power of 2 */
 #define MANA_INDIRECT_TABLE_MAX_SIZE 512
 #define MANA_INDIRECT_TABLE_DEF_SIZE 64
@@ -565,6 +571,8 @@ struct mana_port_context {
 	u32 rxbpre_headroom;
 	u32 rxbpre_frag_count;
 
+	u32 priv_flags;
+
 	struct bpf_prog *bpf_prog;
 
 	/* Create num_queues EQs, SQs, SQ-CQs, RQs and RQ-CQs, respectively. */
-- 
2.43.0

[PATCH net-next v12 4/4] net: mana: recover port on attach failure in ethtool operations

[Dipayaan Roy]

When mana_attach() fails during ethtool ring size or channel count
changes, the port is left in a broken state with no recovery
mechanism, requiring manual intervention to bring the port back up.

On VM SKUs without a netvsc fallback interface, this results in
complete loss of network connectivity to the VM.

Fix by scheduling queue_reset_work when mana_attach() fails. The
preceding patch ensures mana_detach() always completes its full
teardown (netif_device_detach + cleanup), so the reset handler's
mana_detach() takes the "already detached" early return, preserving
port_st_save for a successful mana_attach() recovery.

When mana_attach() fails, choose retry values that maximize recovery
chances: if the operation was an increase, fall back to the previous
working values; if it was a decrease but still above default, fall
back to defaults; otherwise use the minimum supported values.

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Tested-by: Aditya Garg <gargaditya@linux.microsoft.com>
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
 .../ethernet/microsoft/mana/mana_ethtool.c    | 48 +++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index f77509818d07..71e69d5a9a04 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -646,6 +646,7 @@ static int mana_set_channels(struct net_device *ndev,
 	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int new_count = channels->combined_count;
 	unsigned int old_count = apc->num_queues;
+	bool schedule_port_reset = false;
 	int err;
 
 	/* Set channel_changing to block RDMA from grabbing the vport
@@ -675,8 +676,19 @@ static int mana_set_channels(struct net_device *ndev,
 	apc->num_queues = new_count;
 	err = mana_attach(ndev);
 	if (err) {
-		apc->num_queues = old_count;
 		netdev_err(ndev, "mana_attach failed: %d\n", err);
+
+		/* Choose a retry queue count that maximizes recovery
+		 * chances in the reset work handler.
+		 */
+		if (old_count < new_count)
+			apc->num_queues = old_count;
+		else if (new_count > MANA_DEF_NUM_QUEUES)
+			apc->num_queues = MANA_DEF_NUM_QUEUES;
+		else
+			apc->num_queues = 1;
+
+		schedule_port_reset = true;
 	}
 
 out:
@@ -685,6 +697,11 @@ static int mana_set_channels(struct net_device *ndev,
 	mutex_lock(&apc->vport_mutex);
 	apc->channel_changing = false;
 	mutex_unlock(&apc->vport_mutex);
+
+	if (schedule_port_reset)
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
+
 	return err;
 }
 
@@ -707,6 +724,7 @@ static int mana_set_ringparam(struct net_device *ndev,
 			      struct netlink_ext_ack *extack)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
+	bool schedule_port_reset = false;
 	u32 new_tx, new_rx;
 	u32 old_tx, old_rx;
 	int err;
@@ -752,11 +770,35 @@ static int mana_set_ringparam(struct net_device *ndev,
 	err = mana_attach(ndev);
 	if (err) {
 		netdev_err(ndev, "mana_attach failed: %d\n", err);
-		apc->tx_queue_size = old_tx;
-		apc->rx_queue_size = old_rx;
+		NL_SET_ERR_MSG_FMT(extack, "failed to change ring params: %d",
+				   err);
+
+		/* Choose retry ring sizes that maximize recovery
+		 * chances in the reset work handler. Handle RX and
+		 * TX independently.
+		 */
+		if (old_rx < new_rx)
+			apc->rx_queue_size = old_rx;
+		else if (new_rx > DEF_RX_BUFFERS_PER_QUEUE)
+			apc->rx_queue_size = DEF_RX_BUFFERS_PER_QUEUE;
+		else
+			apc->rx_queue_size = MIN_RX_BUFFERS_PER_QUEUE;
+
+		if (old_tx < new_tx)
+			apc->tx_queue_size = old_tx;
+		else if (new_tx > DEF_TX_BUFFERS_PER_QUEUE)
+			apc->tx_queue_size = DEF_TX_BUFFERS_PER_QUEUE;
+		else
+			apc->tx_queue_size = MIN_TX_BUFFERS_PER_QUEUE;
+
+		schedule_port_reset = true;
 	}
 out:
 	mana_pre_dealloc_rxbufs(apc);
+
+	if (schedule_port_reset)
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
 	return err;
 }
 
-- 
2.43.0

Re: [PATCH net-next v12 4/4] net: mana: recover port on attach failure in ethtool operations

[sashiko-bot]

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

New issues:
- [High] The newly introduced background recovery via `queue_reset_work` races with administrative interface state changes (`ip link set down`), causing the port to become permanently stuck in a broken state.
- [High] Clearing `channel_changing` before queuing `queue_reset_work` in `mana_set_channels` creates a race window where the RDMA driver can claim the vport, permanently defeating the recovery.

Pre-existing issues:
- [Critical] A failure in `mana_attach()` leaves the driver vulnerable to a guaranteed NULL pointer dereference when the port is later brought up via `ip link set up`.
- [High] Missing RDMA synchronization via the `channel_changing` flag in `mana_set_ringparam`, `mana_change_mtu`, and `mana_xdp_set` allows the RDMA driver to hijack the vport during configuration changes.
- [Medium] The fix for `mana_attach()` failures leaving the port in a broken state was applied to ethtool ring size and channel count changes, but missed MTU changes and XDP program attachment.
--

--- Patch [4]: [PATCH net-next v12 4/4] net: mana: recover port on attach failure in ethtool operations ---
commit 750fd303cde77a7c9585144b5c7341d83090dbb3
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

net: mana: recover port on attach failure in ethtool operations

This patch adds a recovery mechanism for mana_attach() failures during ethtool 
ring size or channel count changes by scheduling queue_reset_work, while 
adjusting retry values to maximize the chance of recovery.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index f77509818d07f..71e69d5a9a04a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -675,8 +676,19 @@ static int mana_set_channels(struct net_device *ndev,
>  	apc->num_queues = new_count;
>  	err = mana_attach(ndev);
>  	if (err) {
> -		apc->num_queues = old_count;
>  		netdev_err(ndev, "mana_attach failed: %d\n", err);
> +
> +		/* Choose a retry queue count that maximizes recovery
> +		 * chances in the reset work handler.
> +		 */
> +		if (old_count < new_count)
> +			apc->num_queues = old_count;
> +		else if (new_count > MANA_DEF_NUM_QUEUES)
> +			apc->num_queues = MANA_DEF_NUM_QUEUES;
> +		else
> +			apc->num_queues = 1;
> +
> +		schedule_port_reset = true;
>  	}

[Severity: Critical]
This is a pre-existing issue, but a failure in mana_attach() leaves the driver
vulnerable to a NULL pointer dereference when the port is later brought up
manually.

If mana_attach() fails, its failure path calls mana_cleanup_port_context()
which frees apc->rxqs:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_cleanup_port_context() {
    ...
    kfree(apc->rxqs);
    apc->rxqs = NULL;
    ...
}

If the user later brings the interface up, mana_open() triggers the queue
allocation chain without reinitializing the port context:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_open() {
    ...
    err = mana_alloc_queues(ndev);
    ...
}

Which dereferences the NULL pointer in mana_add_rx_queues():

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_add_rx_queues() {
    ...
    apc->rxqs[i] = rxq;
    ...
}

Is there a mechanism to prevent this sequence if recovery fails?

>  out:
> @@ -685,6 +697,11 @@ static int mana_set_channels(struct net_device *ndev,
>  	mutex_lock(&apc->vport_mutex);
>  	apc->channel_changing = false;
>  	mutex_unlock(&apc->vport_mutex);

[Severity: High]
Could clearing channel_changing before the reset work executes create a race
condition where the RDMA driver claims the vport?

If mana_set_channels() fails in mana_attach() and drops vport_mutex while
setting apc->channel_changing = false, a concurrent RDMA QP creation could
call mana_cfg_vport() and successfully claim the hardware vport since
vport_use_count is 0.

When the queued reset work later calls mana_attach() -> mana_create_vport(),
it would see vport_use_count as 1 and fail with -EBUSY, preventing the
recovery from succeeding.

Should channel_changing remain true until the recovery completes?

> +
> +	if (schedule_port_reset)
> +		queue_work(apc->ac->per_port_queue_reset_wq,
> +			   &apc->queue_reset_work);
> +
>  	return err;
>  }

[Severity: High]
Can this asynchronous recovery race with administrative interface state changes,
leaving the port in a state where it cannot be brought up or down?

If an admin runs ip link set down before the reset workqueue executes:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_close() {
    if (!apc->port_is_up)
        return 0;
    ...
}

Because mana_close() returns early, apc->port_st_save remains true, while the
kernel state transitions to DOWN.

When the reset work runs, it calls mana_attach() which sees
port_st_save == true, reallocates queues, increments vport_use_count to 1,
and sets apc->port_is_up = true.

If the admin then runs ip link set up, mana_open() will fail in
mana_create_vport() with -EBUSY because vport_use_count is already 1. A
subsequent ip link set down will do nothing because the kernel state is
already DOWN.

Does the reset work handler need to synchronize against administrative state
changes?

[ ... ]

> @@ -752,11 +770,35 @@ static int mana_set_ringparam(struct net_device *ndev,
>  	err = mana_attach(ndev);
>  	if (err) {

[Severity: High]
This isn't a bug introduced by this patch, but does the detach and attach
sequence here lack synchronization against the RDMA driver?

Unlike mana_set_channels(), mana_set_ringparam() does not acquire
vport_mutex or set apc->channel_changing = true before calling mana_detach().

When it calls mana_detach(ndev, false), vport_use_count drops to 0. A
concurrent RDMA workload could call mana_cfg_vport() and claim the vport
during this window.

When the Ethernet driver subsequently calls mana_attach(), it would fail with
-EBUSY. 

This same lack of synchronization appears to be present in mana_change_mtu()
and mana_xdp_set().

>  		netdev_err(ndev, "mana_attach failed: %d\n", err);
> -		apc->tx_queue_size = old_tx;
> -		apc->rx_queue_size = old_rx;
> +		NL_SET_ERR_MSG_FMT(extack, "failed to change ring params: %d",
> +				   err);
> +
> +		/* Choose retry ring sizes that maximize recovery
> +		 * chances in the reset work handler. Handle RX and
> +		 * TX independently.
> +		 */
> +		if (old_rx < new_rx)
> +			apc->rx_queue_size = old_rx;
> +		else if (new_rx > DEF_RX_BUFFERS_PER_QUEUE)
> +			apc->rx_queue_size = DEF_RX_BUFFERS_PER_QUEUE;
> +		else
> +			apc->rx_queue_size = MIN_RX_BUFFERS_PER_QUEUE;
> +
> +		if (old_tx < new_tx)
> +			apc->tx_queue_size = old_tx;
> +		else if (new_tx > DEF_TX_BUFFERS_PER_QUEUE)
> +			apc->tx_queue_size = DEF_TX_BUFFERS_PER_QUEUE;
> +		else
> +			apc->tx_queue_size = MIN_TX_BUFFERS_PER_QUEUE;
> +
> +		schedule_port_reset = true;
>  	}
>  out:
>  	mana_pre_dealloc_rxbufs(apc);
> +
> +	if (schedule_port_reset)
> +		queue_work(apc->ac->per_port_queue_reset_wq,
> +			   &apc->queue_reset_work);
>  	return err;
>  }

[Severity: Medium]
This is a pre-existing issue, but should this same recovery mechanism be
applied to MTU and XDP program changes?

If mana_attach() fails in mana_change_mtu() or mana_xdp_set(), they return
immediately without scheduling queue_reset_work. This leaves the port detached,
which eventually leads to the NULL pointer dereference in mana_open() if the
user tries to bring the interface up manually.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711041415.3008868-1-dipayanroy@linux.microsoft.com?part=4