Re: [PATCH net-next v16 4/7] r8169: prepare for new interrupt mapping

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what the patch changes.

--

r8169: prepare for new interrupt mapping

This patch adds rtl8169_hw_enable_vec_mapping() and calls it from
rtl_hw_start_8125_common() when more than one IRQ vector is in use. It
also moves the RX counters in rtl_rx() and rtl8169_check_rx_desc_error()
from dev->stats into new per-ring u64_stats_sync protected counters in
struct rtl8169_rx_ring, and rtl8169_get_stats64() now sums them.

> r8169: prepare for new interrupt mapping
>
> This patch enables new interrupt mapping for RTL8127 and add error pkts
> counter per ring.

[Severity: Low]

This isn't a bug, but could the subject and body describe the stats
change more precisely?

The subject only mentions interrupt mapping. The patch also moves every
RX counter in rtl_rx() and rtl8169_check_rx_desc_error() out of
dev->stats and into ring->stats. Those counters are rx_errors,
rx_dropped, rx_length_errors, rx_crc_errors and multicast.

The phrase "add error pkts counter per ring" suggests new counters, but
these are existing counters being moved. Also, multicast is not an error
counter:

drivers/net/ethernet/realtek/r8169_main.c:rtl_rx() {
    ...
		if (skb->pkt_type == PACKET_MULTICAST) {
			u64_stats_update_begin(&ring->stats.syncp);
			ring->stats.multicast++;
			u64_stats_update_end(&ring->stats.syncp);
		}
    ...
}

The stats change takes effect right away on every chip the driver
supports. The vector mapping only runs when tp->irq_nvecs is greater
than 1:

drivers/net/ethernet/realtek/r8169_main.c:rtl_hw_start_8125_common() {
    ...
	if (tp->irq_nvecs > 1)
		rtl8169_hw_enable_vec_mapping(tp);
    ...
}

That can't happen until a later patch, because rtl_alloc_irq() still
requests only one vector.

With num_rx_rings == 1, the values reported by rtl8169_get_stats64()
seem unchanged. The only problem is that the description does not match
the diff.

Would it make sense to put the stats conversion in its own patch? If
not, could the subject and body say that RX statistics accounting
changes for all r8169 chips?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008065722.1026-1-javen_xu%40realsil.com.cn