Re: [PATCH net 2/8] net: bcmgenet: initialize u64 stats seq counter for all queues

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 has no Signed-off-by line and carries tool artifacts
  that must not reach upstream: `Change-Id:…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but writers to
  the same `tx_rings[i].stats64.syncp` are not mutually exclusive, which…

--

net: bcmgenet: initialize u64 stats seq counter for all queues

bcmgenet_probe() only initialized the u64_stats_sync seq counters up to
priv->hw_params->rx_queues and priv->hw_params->tx_queues, leaving the
seq counters for rings 1..4 uninitialized while ethtool -S still reads
them.  The patch initializes both rx_rings[] and tx_rings[] seq counters
for all GENET_MAX_MQ_CNT + 1 entries.

A couple of questions below, one about the trailer block and one about
the locking around the tx ring syncp that this loop initializes.

    Fixes: ffc2c8c4a714 ("net: bcmgenet: Initialize u64 stats seq counter")
    Assisted-by: LLM
    Co-authored-by: Cursor [off-list ref]
    Change-Id: I6c3debbd9fa5e7a151789fccf3d9a38db184f434

[Severity: Low]
The trailer block has no Signed-off-by line from the author or submitter,
which Documentation/process/submitting-patches.rst lists as mandatory for
the Developer's Certificate of Origin.  Was it dropped by accident?

The Gerrit Change-Id line is also still present, along with the
Assisted-by and Co-authored-by: Cursor trailers.  Should the Change-Id be
stripped before posting, since stable backport tooling parses this
Fixes:/Signed-off-by trailer block?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the tx
syncp being initialized here: are the writers to a given
tx_rings[i].stats64.syncp actually mutually exclusive on 32-bit?

The NAPI-side writer in __bcmgenet_tx_reclaim() runs with ring->lock
held, taken by bcmgenet_tx_poll() and bcmgenet_tx_reclaim():

	spin_lock_bh(&ring->lock);
	released = __bcmgenet_tx_reclaim(dev, ring);
	...
	u64_stats_update_begin(&stats->syncp);
	u64_stats_add(&stats->packets, pkts_compl);
	u64_stats_add(&stats->bytes, bytes_compl);
	u64_stats_update_end(&stats->syncp);

bcmgenet_timeout() writes the same syncp after bcmgenet_tx_reclaim() has
already dropped that lock:

	bcmgenet_tx_reclaim(dev, ring, true);
	...
	txq_trans_cond_update(txq);

	BCMGENET_STATS64_INC((&ring->stats64), errors);

The timeout callback is invoked from dev_watchdog() as:

	netif_freeze_queues(dev);
	dev->netdev_ops->ndo_tx_timeout(dev, i);
	netif_unfreeze_queues(dev);

which stops queueing but does not quiesce the TX NAPI, so
bcmgenet_tx_poll() for the same ring can be running on another CPU.

On 32-bit, __u64_stats_update_begin() is only:

	preempt_disable_nested();
	write_seqcount_begin(&syncp->seq);

so there is no cross-CPU exclusion and the sequence increment is a plain
read-modify-write.  Can interleaved writers here hand readers such as
bcmgenet_get_stats64() and bcmgenet_get_ethtool_stats() torn 64-bit
values, or leave the sequence permanently odd so that later readers spin
in read_seqcount_begin()?

Would moving the errors increment under ring->lock be appropriate?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com