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

["mengyuanlou@net-swift.com" <mengyuanlou@net-swift.com>]

> 2026年7月7日 17:50，Paolo Abeni <pabeni@redhat.com> 写道：
> 
> 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/
> ---
> Now I have enough information to produce the report.
> 
> commit e59c58bb080afb34cac34c91b8c320a1f05134e6
> Author: Mengyuan Lou <mengyuanlou@net-swift.com>
> 
> 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.
> 
>> 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
> 
> [ ... ]
> 
>> @@ -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);
>> }
> 
> [ ... ]
> 
>> @@ -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.
> 
>> 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;
> 
> [ ... ]
> 
>> 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.
> 
>