[PATCH net-next v3 0/3] net: libwx: improve VF ethtool support

[PATCH net-next v3 0/3] net: libwx: improve VF ethtool support

[Mengyuan Lou]

This series improves ethtool support for Wangxun VF drivers
(ngbevf and txgbevf) in libwx.

This series extends VF support by enabling:
ring parameter configuration via ethtool -G
interrupt coalescing configuration via ethtool -C
hardware statistics reporting via ethtool -S

Patch 1 adds support for set_ringparam in wx_ethtool_ops_vf,
allowing VF users to adjust TX/RX descriptor ring sizes.
Patch 2 enables set_coalesce support for VF devices and updates
EITR programming to use the VF-specific register access helper.
Patch 3 adds VF hardware statistics support so ethtool -S can
report useful runtime counters for VF interfaces.

Changes log:
v3:
- Patch 3:
  Drop netdev_stat_ops support from this patch.
v2: https://lore.kernel.org/all/20260525101115.13151-1-mengyuanlou@net-swift.com
- Patch 3:
  Remove some stats which can be replaced by standard stats defined in
  include/net/netdev_queues.h.
- Patch 1:
  Adding a return value to wx_set_ring to make wx_set_ringparam_vf can
  be passed back to userspace.
  Remove freeing and requesting of IRQs. Ring resize only updates descriptor
  resources and does not change MSI-X vector or interrupt configuration,
  so IRQs do not need to be reallocated.
v1: https://lore.kernel.org/all/20260514103405.42175-1-mengyuanlou@net-swift.com

Mengyuan Lou (3):
  net: libwx: add support for set_ringparam in wx_ethtool_ops_vf
  net: libwx: add support for set_coalesce in wx_ethtool_ops_vf
  net: libwx: support vf hardware statistics

 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 98 +++++++++++++++++--
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  4 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  9 +-
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  4 +-
 .../net/ethernet/wangxun/libwx/wx_vf_common.c |  7 +-
 .../net/ethernet/wangxun/libwx/wx_vf_common.h |  2 +
 6 files changed, 109 insertions(+), 15 deletions(-)

-- 
2.30.1

[PATCH net-next v3 1/3] net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

[Mengyuan Lou]

Add support for the set_ringparam in wx_ethtool_ops_vf,
which is used to set ring sizes for ngbevf and txgbevf.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 60 +++++++++++++++++++
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  9 +--
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  4 +-
 .../net/ethernet/wangxun/libwx/wx_vf_common.c |  4 +-
 .../net/ethernet/wangxun/libwx/wx_vf_common.h |  2 +
 5 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 5df971aca9e3..6d8fcddde6fa 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -9,6 +9,7 @@
 #include "wx_ethtool.h"
 #include "wx_hw.h"
 #include "wx_lib.h"
+#include "wx_vf_common.h"
 
 struct wx_stats {
 	char stat_string[ETH_GSTRING_LEN];
@@ -775,6 +776,64 @@ static int wx_get_link_ksettings_vf(struct net_device *netdev,
 	return 0;
 }
 
+static int wx_set_ringparam_vf(struct net_device *netdev,
+			       struct ethtool_ringparam *ring,
+			       struct kernel_ethtool_ringparam *kernel_ring,
+			       struct netlink_ext_ack *extack)
+{
+	struct wx *wx = netdev_priv(netdev);
+	u32 new_rx_count, new_tx_count;
+	struct wx_ring *temp_ring;
+	int i, err = 0;
+
+	new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD);
+	new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE);
+
+	new_rx_count = clamp_t(u32, ring->rx_pending, WX_MIN_RXD, WX_MAX_RXD);
+	new_rx_count = ALIGN(new_rx_count, WX_REQ_RX_DESCRIPTOR_MULTIPLE);
+
+	if (new_tx_count == wx->tx_ring_count &&
+	    new_rx_count == wx->rx_ring_count)
+		return 0;
+
+	mutex_lock(&wx->reset_lock);
+	set_bit(WX_STATE_RESETTING, wx->state);
+
+	if (!netif_running(wx->netdev)) {
+		for (i = 0; i < wx->num_tx_queues; i++)
+			wx->tx_ring[i]->count = new_tx_count;
+		for (i = 0; i < wx->num_rx_queues; i++)
+			wx->rx_ring[i]->count = new_rx_count;
+		wx->tx_ring_count = new_tx_count;
+		wx->rx_ring_count = new_rx_count;
+
+		goto clear_reset;
+	}
+
+	/* allocate temporary buffer to store rings in */
+	i = max_t(int, wx->num_tx_queues, wx->num_rx_queues);
+	temp_ring = kvmalloc_objs(struct wx_ring, i);
+	if (!temp_ring) {
+		err = -ENOMEM;
+		goto clear_reset;
+	}
+
+	wxvf_down(wx);
+	/* When set_ring fails, the count will not be updated.
+	 * It merely notifies that there is an error in the setting.
+	 */
+	err = wx_set_ring(wx, new_tx_count, new_rx_count, temp_ring);
+	if (err)
+		wx_err(wx, "failed to set ring parameters: %d", err);
+	wx_configure_vf(wx);
+	wxvf_up_complete(wx);
+	kvfree(temp_ring);
+clear_reset:
+	clear_bit(WX_STATE_RESETTING, wx->state);
+	mutex_unlock(&wx->reset_lock);
+	return err;
+}
+
 static const struct ethtool_ops wx_ethtool_ops_vf = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
@@ -782,6 +841,7 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
 	.get_drvinfo		= wx_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
 	.get_ringparam		= wx_get_ringparam,
+	.set_ringparam		= wx_set_ringparam_vf,
 	.get_msglevel		= wx_get_msglevel,
 	.get_coalesce		= wx_get_coalesce,
 	.get_ts_info		= ethtool_op_get_ts_info,
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index d042567b8128..2e5dba88d04a 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -3248,8 +3248,8 @@ netdev_features_t wx_features_check(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(wx_features_check);
 
-void wx_set_ring(struct wx *wx, u32 new_tx_count,
-		 u32 new_rx_count, struct wx_ring *temp_ring)
+int wx_set_ring(struct wx *wx, u32 new_tx_count,
+		u32 new_rx_count, struct wx_ring *temp_ring)
 {
 	int i, err = 0;
 
@@ -3271,7 +3271,7 @@ void wx_set_ring(struct wx *wx, u32 new_tx_count,
 					i--;
 					wx_free_tx_resources(&temp_ring[i]);
 				}
-				return;
+				return err;
 			}
 		}
 
@@ -3299,7 +3299,7 @@ void wx_set_ring(struct wx *wx, u32 new_tx_count,
 					i--;
 					wx_free_rx_resources(&temp_ring[i]);
 				}
-				return;
+				return err;
 			}
 		}
 
@@ -3311,6 +3311,7 @@ void wx_set_ring(struct wx *wx, u32 new_tx_count,
 
 		wx->rx_ring_count = new_rx_count;
 	}
+	return 0;
 }
 EXPORT_SYMBOL(wx_set_ring);
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.h b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
index aed6ea8cf0d6..bc671786978e 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
@@ -36,8 +36,8 @@ netdev_features_t wx_fix_features(struct net_device *netdev,
 netdev_features_t wx_features_check(struct sk_buff *skb,
 				    struct net_device *netdev,
 				    netdev_features_t features);
-void wx_set_ring(struct wx *wx, u32 new_tx_count,
-		 u32 new_rx_count, struct wx_ring *temp_ring);
+int wx_set_ring(struct wx *wx, u32 new_tx_count,
+		u32 new_rx_count, struct wx_ring *temp_ring);
 void wx_service_event_schedule(struct wx *wx);
 void wx_service_event_complete(struct wx *wx);
 void wx_service_timer(struct timer_list *t);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 0d2db8d38cd5..26de78e9a69e 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -269,7 +269,7 @@ static void wxvf_irq_enable(struct wx *wx)
 	wr32(wx, WX_VXIMC, wx->eims_enable_mask);
 }
 
-static void wxvf_up_complete(struct wx *wx)
+void wxvf_up_complete(struct wx *wx)
 {
 	/* Always set the carrier off */
 	netif_carrier_off(wx->netdev);
@@ -324,7 +324,7 @@ int wxvf_open(struct net_device *netdev)
 }
 EXPORT_SYMBOL(wxvf_open);
 
-static void wxvf_down(struct wx *wx)
+void wxvf_down(struct wx *wx)
 {
 	struct net_device *netdev = wx->netdev;
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
index cbbb1b178cb2..d45d5d8ac3ab 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
@@ -15,7 +15,9 @@ void wx_set_rx_mode_vf(struct net_device *netdev);
 void wx_configure_vf(struct wx *wx);
 int wx_set_mac_vf(struct net_device *netdev, void *p);
 void wxvf_watchdog_update_link(struct wx *wx);
+void wxvf_up_complete(struct wx *wx);
 int wxvf_open(struct net_device *netdev);
+void wxvf_down(struct wx *wx);
 int wxvf_close(struct net_device *netdev);
 void wxvf_init_service(struct wx *wx);
 
-- 
2.30.1

[PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Mengyuan Lou]

Add support for set_coalesce in wx_ethtool_ops_vf, which
is used to set interrupt coalescing parameters.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_ethtool.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 6d8fcddde6fa..30c6ef6103ac 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -10,6 +10,7 @@
 #include "wx_hw.h"
 #include "wx_lib.h"
 #include "wx_vf_common.h"
+#include "wx_vf_lib.h"
 
 struct wx_stats {
 	char stat_string[ETH_GSTRING_LEN];
@@ -488,7 +489,10 @@ int wx_set_coalesce(struct net_device *netdev,
 		else
 			/* rx only or mixed */
 			q_vector->itr = rx_itr_param;
-		wx_write_eitr(q_vector);
+		if (wx->pdev->is_virtfn)
+			wx_write_eitr_vf(q_vector);
+		else
+			wx_write_eitr(q_vector);
 	}
 
 	wx_update_rsc(wx);
@@ -844,6 +848,7 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
 	.set_ringparam		= wx_set_ringparam_vf,
 	.get_msglevel		= wx_get_msglevel,
 	.get_coalesce		= wx_get_coalesce,
+	.set_coalesce		= wx_set_coalesce,
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_link_ksettings	= wx_get_link_ksettings_vf,
 };
-- 
2.30.1

[PATCH net-next v3 3/3] net: libwx: support vf hardware statistics

[Mengyuan Lou]

Add support to show hardware statistics for ethtool -S ethx.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 31 +++++++++++++++----
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  4 +++
 .../net/ethernet/wangxun/libwx/wx_vf_common.c |  3 ++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 30c6ef6103ac..a12e1205ce80 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -48,6 +48,10 @@ static const struct wx_stats wx_gstrings_stats[] = {
 	WX_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared),
 };
 
+static const struct wx_stats wx_gstrings_stats_vf[] = {
+	WX_STAT("non_eop_descs", non_eop_descs),
+};
+
 static const struct wx_stats wx_gstrings_fdir_stats[] = {
 	WX_STAT("fdir_match", stats.fdirmatch),
 	WX_STAT("fdir_miss", stats.fdirmiss),
@@ -69,7 +73,9 @@ static const struct wx_stats wx_gstrings_rsc_stats[] = {
 #define WX_QUEUE_STATS_LEN ( \
 		(WX_NUM_TX_QUEUES + WX_NUM_RX_QUEUES) * \
 		(sizeof(struct wx_queue_stats) / sizeof(u64)))
-#define WX_GLOBAL_STATS_LEN  ARRAY_SIZE(wx_gstrings_stats)
+#define WX_GLOBAL_STATS_LEN (wx->pdev->is_virtfn ? \
+		ARRAY_SIZE(wx_gstrings_stats_vf) : \
+		ARRAY_SIZE(wx_gstrings_stats))
 #define WX_FDIR_STATS_LEN  ARRAY_SIZE(wx_gstrings_fdir_stats)
 #define WX_RSC_STATS_LEN  ARRAY_SIZE(wx_gstrings_rsc_stats)
 #define WX_STATS_LEN (WX_GLOBAL_STATS_LEN + WX_QUEUE_STATS_LEN)
@@ -101,7 +107,10 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	switch (stringset) {
 	case ETH_SS_STATS:
 		for (i = 0; i < WX_GLOBAL_STATS_LEN; i++)
-			ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
+			if (wx->pdev->is_virtfn)
+				ethtool_puts(&p, wx_gstrings_stats_vf[i].stat_string);
+			else
+				ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
 		if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags)) {
 			for (i = 0; i < WX_FDIR_STATS_LEN; i++)
 				ethtool_puts(&p, wx_gstrings_fdir_stats[i].stat_string);
@@ -135,9 +144,15 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 	wx_update_stats(wx);
 
 	for (i = 0; i < WX_GLOBAL_STATS_LEN; i++) {
-		p = (char *)wx + wx_gstrings_stats[i].stat_offset;
-		data[i] = (wx_gstrings_stats[i].sizeof_stat ==
-			   sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
+		if (wx->pdev->is_virtfn) {
+			p = (char *)wx + wx_gstrings_stats_vf[i].stat_offset;
+			data[i] = (wx_gstrings_stats_vf[i].sizeof_stat ==
+				   sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
+		} else {
+			p = (char *)wx + wx_gstrings_stats[i].stat_offset;
+			data[i] = (wx_gstrings_stats[i].sizeof_stat ==
+				   sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
+		}
 	}
 
 	if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags)) {
@@ -219,9 +234,10 @@ EXPORT_SYMBOL(wx_get_pause_stats);
 
 void wx_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
 {
-	unsigned int stats_len = WX_STATS_LEN;
 	struct wx *wx = netdev_priv(netdev);
+	unsigned int stats_len;
 
+	stats_len = WX_STATS_LEN;
 	if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags))
 		stats_len += WX_FDIR_STATS_LEN;
 
@@ -851,6 +867,9 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
 	.set_coalesce		= wx_set_coalesce,
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_link_ksettings	= wx_get_link_ksettings_vf,
+	.get_sset_count		= wx_get_sset_count,
+	.get_strings		= wx_get_strings,
+	.get_ethtool_stats	= wx_get_ethtool_stats,
 };
 
 void wx_set_ethtool_ops_vf(struct net_device *netdev)
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 260e14d5d541..ce172b6deb80 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -2917,6 +2917,9 @@ void wx_update_stats(struct wx *wx)
 	wx->restart_queue = restart_queue;
 	wx->tx_busy = tx_busy;
 
+	if (wx->pdev->is_virtfn)
+		goto skip_hw_stats;
+
 	wx_update_xoff_rx_lfc(wx);
 
 	hwstats->gprc += rd32(wx, WX_RDM_PKT_CNT);
@@ -2956,6 +2959,7 @@ void wx_update_stats(struct wx *wx)
 		hwstats->qmprc += rd32_wrap(wx, WX_PX_MPRC(i),
 					    &wx->last_stats.qmprc[i]);
 
+skip_hw_stats:
 	spin_unlock(&wx->hw_stats_lock);
 }
 EXPORT_SYMBOL(wx_update_stats);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 26de78e9a69e..ee0b10c71b56 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -5,6 +5,7 @@
 #include <linux/pci.h>
 
 #include "wx_type.h"
+#include "wx_hw.h"
 #include "wx_mbx.h"
 #include "wx_lib.h"
 #include "wx_vf.h"
@@ -336,6 +337,7 @@ void wxvf_down(struct wx *wx)
 	netif_tx_disable(netdev);
 	netif_carrier_off(netdev);
 	wx_napi_disable_all(wx);
+	wx_update_stats(wx);
 	wx_reset_vf(wx);
 
 	wx_clean_all_tx_rings(wx);
@@ -409,6 +411,7 @@ static void wxvf_service_task(struct work_struct *work)
 
 	wxvf_link_config_subtask(wx);
 	wxvf_reset_subtask(wx);
+	wx_update_stats(wx);
 	wx_service_event_complete(wx);
 }
 
-- 
2.30.1

Re: [PATCH net-next v3 1/3] net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
Subject: net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

This commit adds support for the set_ringparam ethtool operation in 
wx_ethtool_ops_vf, enabling ring size configuration for ngbevf and txgbevf.
It also modifies wx_set_ring() to return an error code instead of void to
propagate allocation failures.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index d042567b81284..2e5dba88d04a1 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
[ ... ]
> @@ -3248,8 +3248,8 @@ netdev_features_t wx_features_check(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(wx_features_check);
>  
> -void wx_set_ring(struct wx *wx, u32 new_tx_count,
> -		 u32 new_rx_count, struct wx_ring *temp_ring)
> +int wx_set_ring(struct wx *wx, u32 new_tx_count,
> +		u32 new_rx_count, struct wx_ring *temp_ring)

[Severity: High]
Since the return type of wx_set_ring() was changed to propagate errors, do 
existing callers in txgbe and ngbe need to be updated to check this return 
value? 

If they still ignore the return value, an allocation failure will silently 
leave userspace thinking the operation succeeded.

>  {
>  	int i, err = 0;
>  
[ ... ]
> @@ -3299,7 +3299,7 @@ void wx_set_ring(struct wx *wx, u32 new_tx_count,
>  					i--;
>  					wx_free_rx_resources(&temp_ring[i]);
>  				}
> -				return;
> +				return err;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this break configuration 
atomicity?

If wx_setup_rx_resources() fails here, the TX ring changes were already 
committed and old TX resources freed earlier in this function. This leaves 
the device in a partially updated state with the new TX ring size but the 
old RX ring size, rather than cleanly rolling back the entire ethtool 
operation.

[Severity: High]
This is also a pre-existing issue, but does this error path free 
uninitialized pointers? 

The temp_ring array is created via a shallow memcpy from the old ring, 
inheriting the old next_to_alloc index. Since wx_setup_rx_resources() 
allocates rx_buffer_info via kvmalloc (which doesn't zero memory) and 
doesn't reset next_to_alloc, wx_free_rx_resources() will call 
wx_clean_rx_ring() which iterates up to the inherited next_to_alloc index.

Will this pass uninitialized garbage pointers to dev_kfree_skb() and 
page_pool_put_full_page(), potentially causing a kernel panic?

>  			}
>  		}
>  
[ ... ]

Re: [PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

Add support for set_coalesce in wx_ethtool_ops_vf, which is used to set
interrupt coalescing parameters.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
[ ... ]
> @@ -488,7 +489,10 @@ int wx_set_coalesce(struct net_device *netdev,
>  		else
>  			/* rx only or mixed */
>  			q_vector->itr = rx_itr_param;
> -		wx_write_eitr(q_vector);
> +		if (wx->pdev->is_virtfn)
> +			wx_write_eitr_vf(q_vector);
> +		else
> +			wx_write_eitr(q_vector);
>  	}

[Severity: Medium]
Does this cause hardware register wrap-around for VFs when coalesce values
exceed 127 usecs?

The validation at the top of wx_set_coalesce() uses PF limits:

    max_eitr = WX_SP_MAX_EITR; // 0xFF8
    ...
    if (ec->rx_coalesce_usecs > (max_eitr >> 2))
        return -EINVAL;

For a VF, wx_write_eitr_vf() masks the interval against WX_VXITR_MASK
(0x1FF). Since the user-provided rx_coalesce_usecs is shifted left by 2
before writing:

    wx->rx_itr_setting = ec->rx_coalesce_usecs << 2;

Any value greater than 127 will pass the PF-specific validation limit but
silently wrap around when bitwise-ANDed with 0x1FF in wx_write_eitr_vf().

Could the validation logic be updated to check against the correct VF
limits before writing?

> [ ... ]
> @@ -844,6 +848,7 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
>  	.set_ringparam		= wx_set_ringparam_vf,
>  	.get_msglevel		= wx_get_msglevel,
>  	.get_coalesce		= wx_get_coalesce,
> +	.set_coalesce		= wx_set_coalesce,

[Severity: High]
This isn't a bug introduced by this patch, but does enabling this callback
for VFs expose a potential NULL pointer dereference?

wx_set_coalesce() unconditionally accesses wx->q_vector[0]:

    if (wx->q_vector[0]->tx.count && wx->q_vector[0]->rx.count) {
        ...

If wx->num_q_vectors is 0 (for example, if wx_init_interrupt_scheme() failed
during resume or setup, but the netdev is still registered), this will
dereference a NULL pointer. 

Should there be a check to ensure hardware queues are allocated
(wx->num_q_vectors > 0) before accessing the q_vector array?

>  	.get_ts_info		= ethtool_op_get_ts_info,
>  	.get_link_ksettings	= wx_get_link_ksettings_vf,
>  };

Re: [PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

This change wires wx_set_coalesce() into wx_ethtool_ops_vf and
dispatches the per-vector ITR write through wx_write_eitr_vf() when
operating on a virtual function.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index 6d8fcddde6fa..30c6ef6103ac 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> @@ -488,7 +489,10 @@ int wx_set_coalesce(struct net_device *netdev,
>  		else
>  			/* rx only or mixed */
>  			q_vector->itr = rx_itr_param;
> -		wx_write_eitr(q_vector);
> +		if (wx->pdev->is_virtfn)
> +			wx_write_eitr_vf(q_vector);
> +		else
> +			wx_write_eitr(q_vector);
>  	}

[High]
After this change wx_set_coalesce() is reachable on the VF path, but
the validation and the values computed for q_vector->itr come from
the PF logic earlier in the same function:

	switch (wx->mac.type) {
	case wx_mac_sp:
		max_eitr = WX_SP_MAX_EITR;
		rx_itr_param = WX_20K_ITR;
		tx_itr_param = WX_12K_ITR;
		break;
	case wx_mac_aml:
	case wx_mac_aml40:
		max_eitr = WX_AML_MAX_EITR;
		rx_itr_param = WX_20K_ITR;
		tx_itr_param = WX_12K_ITR;
		break;
	default:
		max_eitr = WX_EM_MAX_EITR;
		rx_itr_param = WX_7K_ITR;
		tx_itr_param = WX_7K_ITR;
		break;
	}

	if ((ec->rx_coalesce_usecs > (max_eitr >> 2)) ||
	    (ec->tx_coalesce_usecs > (max_eitr >> 2)))
		return -EINVAL;

The VF write helper masks with a 9-bit field:

void wx_write_eitr_vf(struct wx_q_vector *q_vector)
{
	...
	itr_reg = q_vector->itr & WX_VXITR_MASK;
	...
}

with WX_VXITR_MASK = GENMASK(8, 0) = 0x1FF, while the PF max_eitr
constants used to bound ec->rx_coalesce_usecs / ec->tx_coalesce_usecs
are WX_SP_MAX_EITR=0xFF8, WX_AML_MAX_EITR=0xFFF and
WX_EM_MAX_EITR=0x7FFC.

Can any q_vector->itr value greater than 511 be silently truncated by
this VF path?  For example, on a wx_mac_sp VF, an
ec->rx_coalesce_usecs of 128 yields q_vector->itr = 512, which
becomes register value 0 (no coalescing) after the mask, while
wx_get_coalesce() reports back the original wx->rx_itr_setting and
hides the mismatch from the user.

Does the wx_mac_em default also misbehave here?  When adaptive is
disabled and the defaults are picked, rx_itr_param/tx_itr_param =
WX_7K_ITR = 595, and 595 & 0x1FF = 83, so the VF programs a value
unrelated to the requested 7K rate.

[High]
For wx_mac_aml / wx_mac_aml40, wx_set_coalesce() computes the same
q_vector->itr for PF and VF, but the two write helpers differ in how
they encode it:

void wx_write_eitr(struct wx_q_vector *q_vector)
{
	...
	case wx_mac_aml:
	case wx_mac_aml40:
		itr_reg = (q_vector->itr >> 3) & WX_AML_MAX_EITR;
		break;
	...
}

void wx_write_eitr_vf(struct wx_q_vector *q_vector)
{
	...
	itr_reg = q_vector->itr & WX_VXITR_MASK;
	...
}

txgbevf_sw_init() sets wx->mac.type = wx_mac_aml, so on aml VFs the
same ec->rx_coalesce_usecs setting now programs the hardware with a
value 8x different from the equivalent PF setting.

Is the VF ITR register on aml expected to have the same time-base as
the PF (in which case the >>3 scaling is missing in
wx_write_eitr_vf() for aml), or a different time-base (in which case
the rx_itr_param / tx_itr_param defaults shared with the PF in the
wx_mac_aml case of wx_set_coalesce() do not match the VF)?

> @@ -844,6 +848,7 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
>  	.set_ringparam		= wx_set_ringparam_vf,
>  	.get_msglevel		= wx_get_msglevel,
>  	.get_coalesce		= wx_get_coalesce,
> +	.set_coalesce		= wx_set_coalesce,
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  	.get_link_ksettings	= wx_get_link_ksettings_vf,
>  };

Re: [PATCH net-next v3 3/3] net: libwx: support vf hardware statistics

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
net: libwx: support vf hardware statistics

This patch adds an ethtool -S path for Wangxun VF netdevs, introduces a
VF-specific stats string array, and gates the hardware register reads in
wx_update_stats() so that VF callers skip them.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index 30c6ef6103ac..a12e1205ce80 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> @@ -48,6 +48,10 @@ static const struct wx_stats wx_gstrings_stats[] = {
>  	WX_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared),
>  };
>  
> +static const struct wx_stats wx_gstrings_stats_vf[] = {
> +	WX_STAT("non_eop_descs", non_eop_descs),
> +};

[Medium]
The subject says "support vf hardware statistics" and the changelog says
"Add support to show hardware statistics for ethtool -S ethx", but the
only entry in wx_gstrings_stats_vf[] is non_eop_descs, which is a pure
software counter accumulated from per-ring rx_stats.non_eop_descs by the
driver fast path in wx_lib.c rather than read from any hardware register.

[Low]
Is there a reason only non_eop_descs is exposed for VFs? wx_update_stats()
already aggregates six per-queue software counters that are equally valid
on a VF (non_eop_descs, alloc_rx_buff_failed, hw_csum_rx_good,
hw_csum_rx_error, restart_queue, tx_busy), and the ethtool -S string list
is in practice a stable interface that monitoring tools key on by name, so
each future addition will churn it.

> +
>  static const struct wx_stats wx_gstrings_fdir_stats[] = {
>  	WX_STAT("fdir_match", stats.fdirmatch),
>  	WX_STAT("fdir_miss", stats.fdirmiss),
> @@ -69,7 +73,9 @@ static const struct wx_stats wx_gstrings_rsc_stats[] = {
>  #define WX_QUEUE_STATS_LEN ( \
>  		(WX_NUM_TX_QUEUES + WX_NUM_RX_QUEUES) * \
>  		(sizeof(struct wx_queue_stats) / sizeof(u64)))
> -#define WX_GLOBAL_STATS_LEN  ARRAY_SIZE(wx_gstrings_stats)
> +#define WX_GLOBAL_STATS_LEN (wx->pdev->is_virtfn ? \
> +		ARRAY_SIZE(wx_gstrings_stats_vf) : \
> +		ARRAY_SIZE(wx_gstrings_stats))

[Low]
WX_GLOBAL_STATS_LEN was previously a compile-time constant. The new
definition silently requires every caller of WX_GLOBAL_STATS_LEN /
WX_STATS_LEN to have a local variable named exactly wx of type
struct wx * already initialised in scope.

That hidden contract is the reason wx_get_drvinfo() had to be reordered in
this same patch:

-	unsigned int stats_len = WX_STATS_LEN;
 	struct wx *wx = netdev_priv(netdev);
+	unsigned int stats_len;

+	stats_len = WX_STATS_LEN;

A future caller using a different identifier (priv, adapter), reaching
the macro before wx is initialised, or using it as a static initialiser /
array size, will fail in non-obvious ways. Would a static inline helper
such as wx_global_stats_len(struct wx *wx) be preferable to a function-like
macro that captures an identifier from the caller's scope?

>  #define WX_FDIR_STATS_LEN  ARRAY_SIZE(wx_gstrings_fdir_stats)
>  #define WX_RSC_STATS_LEN  ARRAY_SIZE(wx_gstrings_rsc_stats)
>  #define WX_STATS_LEN (WX_GLOBAL_STATS_LEN + WX_QUEUE_STATS_LEN)

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 260e14d5d541..ce172b6deb80 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -2917,6 +2917,9 @@ void wx_update_stats(struct wx *wx)
>  	wx->restart_queue = restart_queue;
>  	wx->tx_busy = tx_busy;
>  
> +	if (wx->pdev->is_virtfn)
> +		goto skip_hw_stats;
> +

[Medium]
With this skip in place, none of the values exposed for a VF originate
from hardware, every rd32/rd64 against MMIO is bypassed. Should the
subject and changelog be reworded to describe what the patch actually
does, for example exposing VF software per-queue counters via ethtool -S?

>  	wx_update_xoff_rx_lfc(wx);
>  
>  	hwstats->gprc += rd32(wx, WX_RDM_PKT_CNT);

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index 26de78e9a69e..ee0b10c71b56 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> @@ -336,6 +337,7 @@ void wxvf_down(struct wx *wx)
>  	netif_tx_disable(netdev);
>  	netif_carrier_off(netdev);
>  	wx_napi_disable_all(wx);
> +	wx_update_stats(wx);
>  	wx_reset_vf(wx);

[Medium]
Is this wx_update_stats(wx) call reachable as anything other than a
no-op? wxvf_down() begins with:

	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
		return;

so by the time the new call runs, WX_STATE_DOWN is already set. And
wx_update_stats() starts with:

	if (test_bit(WX_STATE_DOWN, wx->state) ||
	    test_bit(WX_STATE_RESETTING, wx->state))
		return;

The reset path has the same property, since wxvf_reinit_locked() sets
WX_STATE_RESETTING before calling wxvf_down().

If the intent is to fold the final per-queue counters into the wx
aggregates before wx_reset_vf() and the ring cleanup, would it work to
move the call before WX_STATE_DOWN is set, or use a path that bypasses
the gating bits?

>  
>  	wx_clean_all_tx_rings(wx);

[ ... ]

Re: [PATCH net-next v3 1/3] net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

[Simon Horman]

On Wed, Jun 03, 2026 at 04:21:34PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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://sashiko.dev

Sorry, I forwarded this in error.
I don't think anything in this review is
actionable in the context of this patch-set.

Re: [PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Simon Horman]

On Wed, Jun 03, 2026 at 04:21:53PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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://sashiko.dev

Sorry, I forwarded this one in error too.

I meant to forward the review for this patch (and patch 3/3)
from https://netdev-ai.bots.linux.dev/sashiko/
As I believe those reviews are more relevant to the
progress of this patchset.

I have now done that.

Re: [PATCH net-next v3 3/3] net: libwx: support vf hardware statistics

[Simon Horman]

On Wed, Jun 03, 2026 at 04:24:18PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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://sashiko.dev

Sorry, I am not doing so well with forwarding these reviews today.
The URL above should be https://netdev-ai.bots.linux.dev/sashiko/

Re: [PATCH net-next v3 2/3] net: libwx: add support for set_coalesce in wx_ethtool_ops_vf

[Simon Horman]

On Wed, Jun 03, 2026 at 04:23:25PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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://sashiko.dev

Sorry, I am not doing so well with forwarding these reviews today.
The URL above should be https://netdev-ai.bots.linux.dev/sashiko/