Hi Simon, For the review generated by AI, I had done changes on that line previously but Jakub Kicinski <[email protected]> pointed out that such checks are redundant https://lore.kernel.org/lkml/[email protected]/ .
Kindly let me know according to whom I've to make the final changes. Thanks, Kshitiz From: Simon Horman <[email protected]> To: <[email protected]> Cc: "Simon Horman"<[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]> Date: Mon, 15 Jun 2026 17:28:06 +0530 Subject: Re: [Intel-wired-lan] [PATCH net-next v4] ixgbe: implement get_queue_stats_rx > 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/ > --- > ixgbe: implement get_queue_stats_rx > > This patch wires ixgbe into the netdev_stat_ops interface so that per-RX > queue counters (bytes, packets, alloc_fail, csum_bad) are exported > through the netdev generic netlink API. > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index bc16e4c93fd4..67844e25af23 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -9759,6 +9759,30 @@ static void ixgbe_get_stats64(struct net_device > > *netdev, > > stats->rx_missed_errors = netdev->stats.rx_missed_errors; > > } > > > > +static void ixgbe_get_queue_stats_rx(struct net_device *dev, int idx, > > + struct netdev_queue_stats_rx *stats) > > +{ > > + struct ixgbe_adapter *adapter = ixgbe_from_netdev(dev); > > + struct ixgbe_ring *ring = adapter->rx_ring[idx]; > > + > > [High] > Can adapter->rx_ring[idx] be observed NULL here? > > Elsewhere in the same driver, this slot is treated as nullable. The > writer side uses WRITE_ONCE() in both directions, for example in > ixgbe_free_q_vector(): > > WRITE_ONCE(adapter->rx_ring[ring->queue_index], NULL); > > and the existing readers in ixgbe_main.c pair that with READ_ONCE() and > a NULL guard, e.g. ixgbe_get_stats64(): > > struct ixgbe_ring *ring = READ_ONCE(adapter->rx_ring[i]); > if (ring) { > ... > } > > and ixgbe_update_stats(): > > struct ixgbe_ring *rx_ring = READ_ONCE(adapter->rx_ring[i]); > if (!rx_ring) > continue; > > If a userspace caller issues a per-queue qstats request while the driver > is reconfiguring queues (e.g. ixgbe_clear_interrupt_scheme() / > ixgbe_init_interrupt_scheme() during reset), would a plain dereference > here oops? > > Should this callback follow the same READ_ONCE() plus NULL-skip pattern > used in ixgbe_get_stats64() and ixgbe_update_stats()? > > > + stats->bytes = ring->stats.bytes; > > + stats->packets = ring->stats.packets; > > [Medium] > Should these 64-bit reads be wrapped in a u64_stats_sync read section? > > The writer side, ixgbe_update_rx_ring_stats(), updates these counters > under ring->syncp: > > u64_stats_update_begin(&rx_ring->syncp); > rx_ring->stats.packets += total_packets; > rx_ring->stats.bytes += total_bytes; > u64_stats_update_end(&rx_ring->syncp); > > and the syncp is initialized at probe (u64_stats_init() on ring->syncp). > ixgbe_get_stats64() in this same file already mirrors that on the read > side: > > do { > start = u64_stats_fetch_begin(&ring->syncp); > packets = ring->stats.packets; > bytes = ring->stats.bytes; > } while (u64_stats_fetch_retry(&ring->syncp, start)); > > On 32-bit kernels a 64-bit load is not atomic, so without the fetch > loop can the values returned to userspace be torn (high half from one > update, low half from another)? > > > + stats->alloc_fail = ring->rx_stats.alloc_rx_page_failed + > > + ring->rx_stats.alloc_rx_buff_failed; > > + stats->csum_bad = ring->rx_stats.csum_err; > > +} > > + > > [ ... ] >
