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

[Simon Horman <horms@kernel.org>]

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);

[ ... ]