Re: [PATCH net-next 5/5] net: rnpgbe: Add hardware and queue statistics

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes the rtnl and qstats RX totals count different
  things, so after any RX drop they can never match again.
- [Medium] rnpgbe_read_stats()
  (drivers/net/ethernet/mucse/rnpgbe/rnpgbe_stats.c) adds (u32)(value -…
- [Low] In rnpgbe_get_strings()
  (drivers/net/ethernet/mucse/rnpgbe/rnpgbe_stats.c), the loop index is…

--

net: rnpgbe: Add hardware and queue statistics

This commit adds hardware RX CRC-error and multicast counters to
ndo_get_stats64. The service task accumulates them, and the totals are
kept across down/up cycles. It also reports per-queue RX/TX packet and
byte counters and rx-alloc-fail through netdev queue stats, and exposes
per-queue TX DMA mapping failures through ethtool statistics.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_stats.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_stats.c
> new file mode 100644
> index 0000000000000..595faa16b7d0d
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_stats.c

[ ... ]

> +static void rnpgbe_get_queue_stats_rx(struct net_device *netdev, int idx,
> +				      struct netdev_queue_stats_rx *stats)
> +{
> +	struct mucse *mucse = netdev_priv(netdev);
> +	const struct mucse_queue_stats *queue;
> +	unsigned int start;
> +
> +	queue = &mucse->rx_stats[idx];
> +	do {
> +		start = u64_stats_fetch_begin(&queue->syncp);
> +		stats->packets = queue->delivered_packets;
> +		stats->bytes = queue->delivered_bytes;
> +	} while (u64_stats_fetch_retry(&queue->syncp, start));

[Severity: Medium]
After an RX drop, can the qstats rx-packets/rx-bytes totals ever match the
rtnl totals again?

rnpgbe_get_stats64() fills rx_packets/rx_bytes from rx_stats[i].packets
and rx_stats[i].bytes. rnpgbe_clean_rx_irq() also increments those for
packets the driver drops. One example is an skb build failure at EOP:

rnpgbe_clean_rx_irq() {
    ...
		if (!skb) {
			atomic64_inc(&rx_ring->stats->skb_alloc_failures);
			...
			if (rnpgbe_test_staterr(rx_desc, M_RXD_STAT_EOP)) {
				total_rx_bytes += size;
				total_rx_packets++;
				total_rx_dropped++;
    ...
}

Resource drops at EOP (drop_bytes) and rnpgbe_cleanup_headers() drops with
size >= ETH_HLEN are counted the same way. delivered_packets and
delivered_bytes are only incremented just before napi_gro_receive().
rnpgbe_get_base_stats() also sums the inactive queues through this
callback.

The counters persist across down/up. So after a single drop, qstats stays
lower than rtnl for the life of the device.

pkt_byte_sum() in tools/testing/selftests/drivers/net/stats.py requires
the two to be equal:

        qstat = get_qstat(cfg)
        if stat_cmp(rtstat, qstat) > 0:
            raise KsftFailEx("Qstats are lower, fetched later")

Before this patch the device had no stat_ops, so this test was skipped.
With this patch the test runs, and it fails after any such drop, even on
an idle link.

Could both interfaces report the same values? Alternatively, could the
dropped packets be reported through matching qstats fields so the device
totals agree?

> +	stats->alloc_fail = atomic64_read(&queue->page_alloc_failures) +
> +			    atomic64_read(&queue->skb_alloc_failures);
> +}

[ ... ]

> +static void rnpgbe_read_stats(struct mucse *mucse)
> +{
> +	struct mucse_hw_stats *stats = &mucse->hw_stats;
> +	struct mucse_hw *hw = &mucse->hw;
> +	u32 value;
> +	int i;
> +
> +	/* These 32-bit counters do not clear on read. Unsigned differences
> +	 * extend them across wraparound; hardware reset clears them.
> +	 */
> +	value = mucse_hw_rd32(hw, RNPGBE_ETH_RX_FCS_ERRORS);
> +	stats->rx_crc_errors += (u32)(value - stats->rx_crc_errors_prev);
> +	stats->rx_crc_errors_prev = value;

[Severity: Medium]
What happens here if the counter goes backwards, or if the read returns
0xFFFFFFFF?

The comment says a hardware reset clears these counters. However, the
prev snapshots are only taken from known hardware state once, in
rnpgbe_init_stats() at probe time.

The driver has no pci_error_handlers and no reset_prepare/reset_done
hooks. A runtime reset can therefore clear the counters while the driver
is still bound. Examples are a PCI function reset through sysfs, or AER
recovery of the parent bus. readl() can also return all-ones during a
PCIe error or link-down window.

In either case (u32)(value - prev) wraps, and about 2^32 - prev bogus
events are added to rx_crc_errors, rx_errors and multicast. The totals
are kept across down/up and only reset when the driver is reloaded. One
bad sample from rnpgbe_service_task() or rnpgbe_down() would inflate the
ndo_get_stats64 output until then.

The driver already notes in mucse_check_for_msg_pf() that chip registers
go back to 0 after a reset:

	/* chip's register is reset to 0 when rc send reset
	 * mbx command. Return -EIO if in this state, others
	 * fw == hw->mbx.fw_req means no new msg.

Should rnpgbe_read_stats() reject all-ones reads, and take a new snapshot
when it sees that a counter has been reset?

> +
> +	for (i = 0; i < mucse->num_rx_queues; i++) {
> +		u32 index = mucse->rx_ring[i]->rnpgbe_queue_idx;
> +
> +		value = mucse_hw_rd32(hw, RNPGBE_DMA_RX_MULTICAST(index));
> +		stats->multicast += (u32)(value - stats->multicast_prev[i]);
> +		stats->multicast_prev[i] = value;

The same question applies to the per-ring multicast counters here.

> +	}
> +}

[ ... ]

> +void rnpgbe_get_strings(struct net_device *netdev, u32 sset, u8 *data)
> +{
> +	struct mucse *mucse = netdev_priv(netdev);
> +	int i, j;
> +
> +	if (sset != ETH_SS_STATS)
> +		return;
> +
> +	for (i = 0; i < mucse->num_tx_queues; i++)
> +		for (j = 0; j < ARRAY_SIZE(rnpgbe_tx_queue_stats); j++)
> +			ethtool_sprintf(&data, "tx_queue_%u_%s", i,
> +					rnpgbe_tx_queue_stats[j].name);

[Severity: Low]
This isn't a bug, but i is a signed int and it is passed to a %u format
in ethtool_sprintf(). The output is correct because i is never negative,
but -Wformat-signedness would flag it. Declaring i as unsigned int, or
using %d, would make the type and format match.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/FB722C2F25FE9005%2B20261106112420.43050-1-dong100%40mucse.com