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

[PATCH net-next v11 0/2] 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.

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 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 (2):
  net: mana: refactor mana_get_strings() and mana_get_sset_count() to
    use switch
  net: mana: force full-page RX buffers via ethtool private flag

 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
 include/net/mana/mana.h                       |   8 +
 3 files changed, 177 insertions(+), 31 deletions(-)

-- 
2.43.0

[PATCH net-next v11 1/2] 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 94e658d07a27..fa9c49592828 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 v11 2/2] 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    | 103 ++++++++++++++++++
 include/net/mana/mana.h                       |   8 ++
 3 files changed, 131 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 26aef21c6c2c..4bd83c782ea3 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 fa9c49592828..3c498a222965 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;
 	}
@@ -611,6 +630,88 @@ 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;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		apc->priv_flags = priv_flags;
+
+		if (!apc->port_is_up) {
+			/* Port is down, flag updated to apply on next up
+			 * so just return.
+			 */
+			return 0;
+		}
+
+		/* Pre-allocate buffers to prevent failure in mana_attach
+		 * later
+		 */
+		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;
+			return err;
+		}
+
+		err = mana_detach(ndev, false);
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Port is in an inconsistent state. Restore
+			 * 'port_is_up' so that queue reset work handler
+			 * can properly detach and re-attach.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;
+			goto out;
+		}
+
+		err = mana_attach(ndev);
+		if (err) {
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Restore 'port_is_up' so the reset work handler
+			 * can properly detach/attach. Without this,
+			 * the handler sees port_is_up=false and skips
+			 * queue allocation, leaving the port dead.
+			 */
+			apc->port_is_up = true;
+			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;
+}
+
 const struct ethtool_ops mana_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_CQE_FRAMES,
 	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
@@ -631,4 +732,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 13c87baf018e..8dc496f05938 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -30,6 +30,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
@@ -532,6 +538,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

Re: [PATCH net-next v11 0/2] net: mana: add ethtool private flag for full-page RX buffers

[Maciej Fijalkowski]

On Wed, Jul 01, 2026 at 07:15:44AM -0700, Dipayaan Roy wrote:
> 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.

Were you able to track down what is the actual bottleneck on the 'broken'
platform? What is the performance of full-page approach on healthy
platforms? On changelog below you mention the frag approach 'outperforms'
the full-page one.

> 
> 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 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 (2):
>   net: mana: refactor mana_get_strings() and mana_get_sset_count() to
>     use switch
>   net: mana: force full-page RX buffers via ethtool private flag
> 
>  drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
>  .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
>  include/net/mana/mana.h                       |   8 +
>  3 files changed, 177 insertions(+), 31 deletions(-)
> 
> -- 
> 2.43.0
> 
> 

Re: [PATCH net-next v11 0/2] net: mana: add ethtool private flag for full-page RX buffers

[Dipayaan Roy]

On Wed, Jul 01, 2026 at 04:45:14PM +0200, Maciej Fijalkowski wrote:
> On Wed, Jul 01, 2026 at 07:15:44AM -0700, Dipayaan Roy wrote:
> > 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.
> 
> Were you able to track down what is the actual bottleneck on the 'broken'
> platform? What is the performance of full-page approach on healthy
> platforms? On changelog below you mention the frag approach 'outperforms'
> the full-page one.
> 
Hi Maciej,

The HW team identified a PCIE root port stall occurring due to a PCIe
errata in a HW IP used for this platform. Using full pages increases the
time in packet refill path and indirectly helping to reduce the back pressure
in NIC pipeline caused due to the stall in root port. As per them it
will be fixed in next version of this hw which is not anytime soon.

On various other healthy platforms with 4k base page size we tested, we see
improvements using page fragments than full pages around anywhere between 5 to 15%.

Regards
Dipayaan Roy

> > 
> > 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 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 (2):
> >   net: mana: refactor mana_get_strings() and mana_get_sset_count() to
> >     use switch
> >   net: mana: force full-page RX buffers via ethtool private flag
> > 
> >  drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
> >  .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
> >  include/net/mana/mana.h                       |   8 +
> >  3 files changed, 177 insertions(+), 31 deletions(-)
> > 
> > -- 
> > 2.43.0
> > 
> > 

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag

This adds an ethtool private flag "full-page-rx" that forces one RX buffer
per page to avoid a page_pool fragment path throughput regression on some
4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
triggers a mana_detach() + mana_attach() reconfiguration to apply the new
buffer layout.

A few questions inline.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7438ea6b3f26..38f50b4eef87 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
      unrelated to the comments below ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf43..24f6ce63b031 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> @@ -611,6 +630,88 @@ 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;
> +
> +	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> +		apc->priv_flags = priv_flags;

[Low]
Is the storage of priv_flags intentionally nested inside the per-flag
branch?  With only one flag today this is fine, but when a second bit is
added to enum mana_priv_flag_bits, a user toggling only the new bit
passes the !changed and unknown-bits checks, the outer if is skipped,
apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
while mana_get_priv_flags() keeps reporting the old state.

Would it be cleaner to store priv_flags unconditionally after validation
and use per-flag if (changed & BIT(...)) blocks only for the
flag-specific reconfiguration?

> +
> +		if (!apc->port_is_up) {
> +			/* Port is down, flag updated to apply on next up
> +			 * so just return.
> +			 */
> +			return 0;
> +		}
> +
> +		/* Pre-allocate buffers to prevent failure in mana_attach
> +		 * later
> +		 */
> +		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
XDP is attached, mana_use_single_rxbuf_per_page() already returns true
regardless of the new priv flag:

	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;

In that case mana_get_rxbuf_cfg() produces identical output whether the
new flag is on or off, yet toggling the flag here always performs a full
mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
the link.

Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
adding to skip the reconfiguration when nothing actually changes?

[High]
mana_set_priv_flags() drives the same detach/attach sequence used by
mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
does not take apc->vport_mutex and set apc->channel_changing = true
across the window.

The struct comment on channel_changing spells out the invariant:

	/* Set by mana_set_channels() under vport_mutex to block RDMA
	 * from grabbing the vport during the detach/attach window.
	 * Checked by mana_cfg_vport() when called from the RDMA path.
	 */
	bool channel_changing;

Without it, after mana_detach() uncfg's the vport an RDMA client can
call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
will fail with -EBUSY when it re-cfg's the vport.

There is also no equivalent of the '!port_is_up && vport_use_count'
early -EBUSY check that mana_set_channels() performs, so is the flag
allowed to be changed silently while an RDMA client is using the vport?

> +		if (err) {
> +			netdev_err(ndev,
> +				   "Insufficient memory for new allocations\n");
> +			apc->priv_flags = old_priv_flags;
> +			return err;
> +		}
> +
> +		err = mana_detach(ndev, false);

[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
called below.  Is RTNL guaranteed to be held on the SET priv-flags path?

mana registers net_shaper_ops in mana_devops, which makes it an
ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
core acquires rtnl only when the driver opts in via
ETHTOOL_OP_NEEDS_RTNL_* for that specific command.

Looking at mana_ethtool_ops.op_needs_rtnl:

	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
				  ETHTOOL_OP_NEEDS_RTNL_GLINK,

ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
detach/attach helpers below will WARN, and the code races with
RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
apc->tx_qp and apc->port_st_save.

Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
what is already done for SET channels?

> +		if (err) {
> +			netdev_err(ndev, "mana_detach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Port is in an inconsistent state. Restore
> +			 * 'port_is_up' so that queue reset work handler
> +			 * can properly detach and re-attach.
> +			 */
> +			apc->port_is_up = true;
> +			schedule_port_reset = true;
> +			goto out;
> +		}
> +
> +		err = mana_attach(ndev);
> +		if (err) {
> +			netdev_err(ndev, "mana_attach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Restore 'port_is_up' so the reset work handler
> +			 * can properly detach/attach. Without this,
> +			 * the handler sees port_is_up=false and skips
> +			 * queue allocation, leaving the port dead.
> +			 */
> +			apc->port_is_up = true;
> +			schedule_port_reset = true;

[High]
On this branch, mana_detach() has already succeeded and run
mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
down.  Setting apc->port_is_up = true here between now and when
queue_reset_work runs opens a window where callers gate on port_is_up
as their "safe to access rxqs/tx_qp" predicate.

For example, mana_get_ethtool_stats() does:

	if (!apc->port_is_up)
		return;
	...
	rxq = apc->rxqs[q];
	...

A concurrent ethtool -S invocation during that window will pass the
port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.

Is the port_is_up restore actually needed for the reset work handler's
mana_detach() call?  On the reset path, mana_detach() takes its early
return when !netif_device_present(ndev):

	if (!from_close && !netif_device_present(ndev))
		return 0;

That early return does not touch apc->port_st_save, so the saved state
from the earlier successful mana_detach() should already be intact for
the follow-up mana_attach().

There is also no smp_wmb() paired with this write, unlike the pattern
used inside mana_detach()/mana_attach() proper.

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

[ ... remaining hunks unrelated to the comments above ... ]
-- 
This is an AI-generated review.

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag

This adds an ethtool private flag "full-page-rx" that forces one RX buffer
per page to avoid a page_pool fragment path throughput regression on some
4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
triggers a mana_detach() + mana_attach() reconfiguration to apply the new
buffer layout.

A few questions inline.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7438ea6b3f26..38f50b4eef87 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
      unrelated to the comments below ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf43..24f6ce63b031 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> @@ -611,6 +630,88 @@ 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;
> +
> +	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> +		apc->priv_flags = priv_flags;

[Low]
Is the storage of priv_flags intentionally nested inside the per-flag
branch?  With only one flag today this is fine, but when a second bit is
added to enum mana_priv_flag_bits, a user toggling only the new bit
passes the !changed and unknown-bits checks, the outer if is skipped,
apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
while mana_get_priv_flags() keeps reporting the old state.

Would it be cleaner to store priv_flags unconditionally after validation
and use per-flag if (changed & BIT(...)) blocks only for the
flag-specific reconfiguration?

> +
> +		if (!apc->port_is_up) {
> +			/* Port is down, flag updated to apply on next up
> +			 * so just return.
> +			 */
> +			return 0;
> +		}
> +
> +		/* Pre-allocate buffers to prevent failure in mana_attach
> +		 * later
> +		 */
> +		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
XDP is attached, mana_use_single_rxbuf_per_page() already returns true
regardless of the new priv flag:

	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;

In that case mana_get_rxbuf_cfg() produces identical output whether the
new flag is on or off, yet toggling the flag here always performs a full
mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
the link.

Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
adding to skip the reconfiguration when nothing actually changes?

[High]
mana_set_priv_flags() drives the same detach/attach sequence used by
mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
does not take apc->vport_mutex and set apc->channel_changing = true
across the window.

The struct comment on channel_changing spells out the invariant:

	/* Set by mana_set_channels() under vport_mutex to block RDMA
	 * from grabbing the vport during the detach/attach window.
	 * Checked by mana_cfg_vport() when called from the RDMA path.
	 */
	bool channel_changing;

Without it, after mana_detach() uncfg's the vport an RDMA client can
call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
will fail with -EBUSY when it re-cfg's the vport.

There is also no equivalent of the '!port_is_up && vport_use_count'
early -EBUSY check that mana_set_channels() performs, so is the flag
allowed to be changed silently while an RDMA client is using the vport?

> +		if (err) {
> +			netdev_err(ndev,
> +				   "Insufficient memory for new allocations\n");
> +			apc->priv_flags = old_priv_flags;
> +			return err;
> +		}
> +
> +		err = mana_detach(ndev, false);

[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
called below.  Is RTNL guaranteed to be held on the SET priv-flags path?

mana registers net_shaper_ops in mana_devops, which makes it an
ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
core acquires rtnl only when the driver opts in via
ETHTOOL_OP_NEEDS_RTNL_* for that specific command.

Looking at mana_ethtool_ops.op_needs_rtnl:

	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
				  ETHTOOL_OP_NEEDS_RTNL_GLINK,

ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
detach/attach helpers below will WARN, and the code races with
RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
apc->tx_qp and apc->port_st_save.

Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
what is already done for SET channels?

> +		if (err) {
> +			netdev_err(ndev, "mana_detach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Port is in an inconsistent state. Restore
> +			 * 'port_is_up' so that queue reset work handler
> +			 * can properly detach and re-attach.
> +			 */
> +			apc->port_is_up = true;
> +			schedule_port_reset = true;
> +			goto out;
> +		}
> +
> +		err = mana_attach(ndev);
> +		if (err) {
> +			netdev_err(ndev, "mana_attach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Restore 'port_is_up' so the reset work handler
> +			 * can properly detach/attach. Without this,
> +			 * the handler sees port_is_up=false and skips
> +			 * queue allocation, leaving the port dead.
> +			 */
> +			apc->port_is_up = true;
> +			schedule_port_reset = true;

[High]
On this branch, mana_detach() has already succeeded and run
mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
down.  Setting apc->port_is_up = true here between now and when
queue_reset_work runs opens a window where callers gate on port_is_up
as their "safe to access rxqs/tx_qp" predicate.

For example, mana_get_ethtool_stats() does:

	if (!apc->port_is_up)
		return;
	...
	rxq = apc->rxqs[q];
	...

A concurrent ethtool -S invocation during that window will pass the
port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.

Is the port_is_up restore actually needed for the reset work handler's
mana_detach() call?  On the reset path, mana_detach() takes its early
return when !netif_device_present(ndev):

	if (!from_close && !netif_device_present(ndev))
		return 0;

That early return does not touch apc->port_st_save, so the saved state
from the earlier successful mana_detach() should already be intact for
the follow-up mana_attach().

There is also no smp_wmb() paired with this write, unlike the pattern
used inside mana_detach()/mana_attach() proper.

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

[ ... remaining hunks unrelated to the comments above ... ]

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

[Dipayaan Roy]

On Wed, Jul 08, 2026 at 04:57:42PM +0100, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: mana: force full-page RX buffers via ethtool private flag
> 
> This adds an ethtool private flag "full-page-rx" that forces one RX buffer
> per page to avoid a page_pool fragment path throughput regression on some
> 4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
> triggers a mana_detach() + mana_attach() reconfiguration to apply the new
> buffer layout.
> 
> A few questions inline.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 7438ea6b3f26..38f50b4eef87 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> 
> [ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
>       unrelated to the comments below ... ]
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > index 3531abfdaf43..24f6ce63b031 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> 
> [ ... ]
> 
> > @@ -611,6 +630,88 @@ 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;
> > +
> > +	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> > +		apc->priv_flags = priv_flags;
> 
> [Low]
> Is the storage of priv_flags intentionally nested inside the per-flag
> branch?  With only one flag today this is fine, but when a second bit is
> added to enum mana_priv_flag_bits, a user toggling only the new bit
> passes the !changed and unknown-bits checks, the outer if is skipped,
> apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
> while mana_get_priv_flags() keeps reporting the old state.
> 
> Would it be cleaner to store priv_flags unconditionally after validation
> and use per-flag if (changed & BIT(...)) blocks only for the
> flag-specific reconfiguration?
> 

Hi Simon, Paolo,

Agreed on this future concern,
but also we donot plan to have another mana driver private flag.


> > +
> > +		if (!apc->port_is_up) {
> > +			/* Port is down, flag updated to apply on next up
> > +			 * so just return.
> > +			 */
> > +			return 0;
> > +		}
> > +
> > +		/* Pre-allocate buffers to prevent failure in mana_attach
> > +		 * later
> > +		 */
> > +		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> 
> [Low]
> When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
> XDP is attached, mana_use_single_rxbuf_per_page() already returns true
> regardless of the new priv flag:
> 
> 	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;
> 
> In that case mana_get_rxbuf_cfg() produces identical output whether the
> new flag is on or off, yet toggling the flag here always performs a full
> mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
> the link.
> 
> Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
> adding to skip the reconfiguration when nothing actually changes?
Good point will add a pre and post comparison as well to skip the
detach/attach when already in that configuration.

> 
> [High]
> mana_set_priv_flags() drives the same detach/attach sequence used by
> mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
> does not take apc->vport_mutex and set apc->channel_changing = true
> across the window.
> 
> The struct comment on channel_changing spells out the invariant:
> 
> 	/* Set by mana_set_channels() under vport_mutex to block RDMA
> 	 * from grabbing the vport during the detach/attach window.
> 	 * Checked by mana_cfg_vport() when called from the RDMA path.
> 	 */
> 	bool channel_changing;
> 
> Without it, after mana_detach() uncfg's the vport an RDMA client can
> call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
> will fail with -EBUSY when it re-cfg's the vport.
> 
> There is also no equivalent of the '!port_is_up && vport_use_count'
> early -EBUSY check that mana_set_channels() performs, so is the flag
> allowed to be changed silently while an RDMA client is using the vport?
> 
Agreed. Will add the same vport_mutex + channel_changing pattern used by
mana_set_channels() and mana_per_port_queue_reset_work_handler() to
block RDMA from grabbing the vport during the detach/attach window.

> > +		if (err) {
> > +			netdev_err(ndev,
> > +				   "Insufficient memory for new allocations\n");
> > +			apc->priv_flags = old_priv_flags;
> > +			return err;
> > +		}
> > +
> > +		err = mana_detach(ndev, false);
> 
> [High]
> mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
> called below.  Is RTNL guaranteed to be held on the SET priv-flags path?
> 
> mana registers net_shaper_ops in mana_devops, which makes it an
> ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
> core acquires rtnl only when the driver opts in via
> ETHTOOL_OP_NEEDS_RTNL_* for that specific command.
> 
> Looking at mana_ethtool_ops.op_needs_rtnl:
> 
> 	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
> 				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
> 				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
> 
> ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
> detach/attach helpers below will WARN, and the code races with
> RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
> mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
> apc->tx_qp and apc->port_st_save.
> 
> Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
> what is already done for SET channels?
> 
Agreed, Will add ETHTOOL_OP_NEEDS_RTNL_SPFLAGS to .op_needs_rtnl.
> > +		if (err) {
> > +			netdev_err(ndev, "mana_detach failed: %d\n", err);
> > +			apc->priv_flags = old_priv_flags;
> > +
> > +			/* Port is in an inconsistent state. Restore
> > +			 * 'port_is_up' so that queue reset work handler
> > +			 * can properly detach and re-attach.
> > +			 */
> > +			apc->port_is_up = true;
> > +			schedule_port_reset = true;
> > +			goto out;
> > +		}
> > +
> > +		err = mana_attach(ndev);
> > +		if (err) {
> > +			netdev_err(ndev, "mana_attach failed: %d\n", err);
> > +			apc->priv_flags = old_priv_flags;
> > +
> > +			/* Restore 'port_is_up' so the reset work handler
> > +			 * can properly detach/attach. Without this,
> > +			 * the handler sees port_is_up=false and skips
> > +			 * queue allocation, leaving the port dead.
> > +			 */
> > +			apc->port_is_up = true;
> > +			schedule_port_reset = true;
> 
> [High]
> On this branch, mana_detach() has already succeeded and run
> mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
> down.  Setting apc->port_is_up = true here between now and when
> queue_reset_work runs opens a window where callers gate on port_is_up
> as their "safe to access rxqs/tx_qp" predicate.
> 
> For example, mana_get_ethtool_stats() does:
> 
> 	if (!apc->port_is_up)
> 		return;
> 	...
> 	rxq = apc->rxqs[q];
> 	...
> 
> A concurrent ethtool -S invocation during that window will pass the
> port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.
> 
> Is the port_is_up restore actually needed for the reset work handler's
> mana_detach() call?  On the reset path, mana_detach() takes its early
> return when !netif_device_present(ndev):
> 
> 	if (!from_close && !netif_device_present(ndev))
> 		return 0;
> 
> That early return does not touch apc->port_st_save, so the saved state
> from the earlier successful mana_detach() should already be intact for
> the follow-up mana_attach().
> 
> There is also no smp_wmb() paired with this write, unlike the pattern
> used inside mana_detach()/mana_attach() proper.
> 

For the mana_attach() failure path: agreed, will remove port_is_up =
true. After a successful mana_detach(ndev, false), netif_device_detach()
has already been called so the reset handler's mana_detach() takes the
early return via !netif_device_present(), preserving port_st_save for
the subsequent mana_attach().

For the mana_detach() failure path: keeping port_is_up = true is
intentional. Here netif_device_detach() was never reached, so the reset
handler's mana_detach() would re-enter fully and overwrite port_st_save
from port_is_up, if left as false, mana_attach() would skip queue
allocation and leave the port dead.


> > +		}
> > +	}
> > +
> > +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;
> > +}
> > +
> 
> [ ... remaining hunks unrelated to the comments above ... ]

Thanks for the review comments, I will address them in the next
version.


Regards
Dipayaan Roy