Re: [PATCH net-next v6 3/3] net: libwx: support vf per-queue statistics via ethtool -S

2026年7月7日 17:50，Paolo Abeni [off-list ref] 写道：

From: AI Reviewer <redacted>

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/
---
Now I have enough information to produce the report.

commit e59c58bb080afb34cac34c91b8c320a1f05134e6
Author: Mengyuan Lou [off-list ref]

net: libwx: support vf per-queue statistics via ethtool -S

The patch wires wx_get_sset_count, wx_get_strings, and wx_get_ethtool_stats
into wx_ethtool_ops_vf, adds a small vf-only global stats table, and starts
calling wx_update_stats() from wxvf_service_task. It also adds NULL checks
for wx->rx_ring[i] / wx->tx_ring[i] inside wx_update_stats.

quoted

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 22037f015ded..f72908630ea4 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c

[ ... ]

quoted

@@ -114,7 +133,7 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)

ethtool_sprintf(&p, "tx_queue_%u_packets", i);
ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
}
- for (i = 0; i < WX_NUM_RX_QUEUES; i++) {
+ for (i = 0; i < WX_NUM_RX_QUEUES(netdev); i++) {
ethtool_sprintf(&p, "rx_queue_%u_packets", i);
ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
}

[ ... ]

quoted

@@ -852,6 +878,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,
};

[Medium]
Before this change wx_ethtool_ops_vf had no get_sset_count / get_strings /
get_ethtool_stats callbacks, so the VF exposed no ethtool -S output at all.
Wiring these in also picks up the shared per-queue string loop that emits
tx_queue_%u_packets / tx_queue_%u_bytes and rx_queue_%u_packets /
rx_queue_%u_bytes.

Per-queue TX/RX packet and byte counters are a category that already has a
standard uAPI (netdev_stat_ops with queue_stats_rx / queue_stats_tx,
surfaced by netlink and ynl). Documentation/networking/statistics.rst and
the ethtool -S guidance state that new counters covering values that have a
standard uAPI should not be added to ethtool -S even when the driver does
not yet implement the standard interface.

The wangxun tree does not implement netdev_stat_ops (no
get_base_stats / get_queue_stats_rx / get_queue_stats_tx callbacks are
present under drivers/net/ethernet/wangxun/).

Ok，I will remove patch3（support vf per-queue statistics via ethtool -S).

Would it be preferable to expose the per-queue counters on the VF via
netdev_stat_ops instead of adding them to ethtool -S here?

netdev_stat_ops support is deferred to a follow-up patch; this one only extends ethtool ops.

quoted

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..349afbe60132 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c

@@ -2886,6 +2886,8 @@ void wx_update_stats(struct wx *wx)

for (i = 0; i < wx->num_rx_queues; i++) {
struct wx_ring *rx_ring = wx->rx_ring[i];

+ if (!rx_ring)
+ continue;
non_eop_descs += rx_ring->rx_stats.non_eop_descs;

[ ... ]

quoted

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 26de78e9a69e..1d84fe35d7d9 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c

@@ -409,6 +410,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);
}

[High]
Can this call race with VF PCI suspend and shutdown to produce a
use-after-free on wx->rx_ring[i] / wx->tx_ring[i]?

Both ngbevf and txgbevf use wxvf_suspend as their PM suspend callback and
wxvf_shutdown (which calls wxvf_suspend) as their pci_driver ->shutdown
callback:

   static DEFINE_SIMPLE_DEV_PM_OPS(ngbevf_pm_ops, wxvf_suspend, wxvf_resume);
   ...
   .shutdown = wxvf_shutdown,

wxvf_suspend() in wx_vf_common.c does:

   int wxvf_suspend(struct device *dev_d)
   {
       struct pci_dev *pdev = to_pci_dev(dev_d);
       struct wx *wx = pci_get_drvdata(pdev);

       netif_device_detach(wx->netdev);
       wx_clear_interrupt_scheme(wx);
       pci_disable_device(pdev);

       return 0;
   }

wx_clear_interrupt_scheme() ends up freeing the q_vectors (via
kfree_rcu), and the ring allocations are embedded in the q_vector, so
wx->rx_ring[i] / wx->tx_ring[i] end up pointing at memory that will be
freed after an RCU grace period.

wxvf_suspend / wxvf_shutdown do not call timer_shutdown_sync on
wx->service_timer or cancel_work_sync on wx->service_task, and they do
not set WX_STATE_DOWN.

With this hunk in place, the service_task now walks wx->rx_ring[] and
wx->tx_ring[]:

   void wx_update_stats(struct wx *wx)
   {
       ...
       for (i = 0; i < wx->num_rx_queues; i++) {
           struct wx_ring *rx_ring = wx->rx_ring[i];

           if (!rx_ring)
               continue;
           non_eop_descs += rx_ring->rx_stats.non_eop_descs;
           ...
       }
       ...
       for (i = 0; i < wx->num_tx_queues; i++) {
           struct wx_ring *tx_ring = wx->tx_ring[i];

           if (!tx_ring)
               continue;
           restart_queue += tx_ring->tx_stats.restart_queue;
           ...
       }

If the service_task latches a non-NULL rx_ring / tx_ring pointer just
before wx_clear_interrupt_scheme runs, the added NULL check does not
help, and the subsequent ->rx_stats / ->tx_stats reads touch memory
that has been kfree_rcu'd. wxvf_remove() covers this by calling
timer_shutdown_sync + cancel_work_sync before wx_clear_interrupt_scheme,
but wxvf_suspend / wxvf_shutdown do not.

Should wxvf_suspend (and hence wxvf_shutdown) quiesce the service_timer
and service_task, or set WX_STATE_DOWN, before wx_clear_interrupt_scheme
now that wx_update_stats is on the service_task's call path?
-- 
This is an AI-generated review.