Re: [PATCH net-next v4 15/15] net: ethernet: cortina: Scope RX statistics updates

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · 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 commit fixes a real runtime bug but has no Fixes: trailer
  and no statement of how the bug was found, how it was confirmed, or…
- [Low] The commit message starts with "On 32-bit systems
  u64_stats_update_begin() disables preemption." That is wrong for…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but none of
  port->rx_stats_syncp, tx_stats_syncp or ir_stats_syncp is ever…

--

net: ethernet: cortina: Scope RX statistics updates

This commit narrows the rx_stats_syncp writer sections in the Gemini RX
path so they cover only the counter updates. Before this, one writer
section in gmac_napi_poll() spanned all of gmac_rx(). The stated reason is
that on 32-bit PREEMPT_RT the writer section disables preemption while
gmac_rx() takes spinlock_t locks and calls into GRO.

> On 32-bit systems u64_stats_update_begin() disables preemption. The RX
> poll currently holds that writer section across gmac_rx(), which takes
> spinlock_t locks and calls into GRO and fragment handling. On PREEMPT_RT,
> spinlock_t can sleep and must not be acquired with preemption disabled.

[Severity: Low]
Is the first sentence accurate for non-RT kernels? On 32-bit,
__u64_stats_update_begin() calls preempt_disable_nested(). That helper
only disables preemption when PREEMPT_RT is enabled:

include/linux/preempt.h:preempt_disable_nested() {
	if (IS_ENABLED(CONFIG_PREEMPT_RT))
		preempt_disable();
	else
		lockdep_assert_preemption_disabled();
}

So the affected configuration looks like 32-bit with PREEMPT_RT only.
Could the opening sentence say that?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index a61bace61ea4b..9f7946876d585 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1712,12 +1712,12 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
>  	unsigned int rx_status = word0.bits.status;
>  	struct sk_buff *skb = NULL;
>  
> -	port->rx_stats[rx_status]++;
> -	port->rx_csum_stats[rx_csum]++;
> -
>  	if (word0.bits.derr || word0.bits.perr ||
>  	    rx_status || frame_len < ETH_ZLEN ||
>  	    rx_csum >= RX_CHKSUM_IP_ERR_UNKNOWN) {
> +		u64_stats_update_begin(&port->rx_stats_syncp);

[Severity: Medium]
This isn't a bug introduced by this patch, but is rx_stats_syncp ever
initialized?

Nothing in drivers/net/ethernet/cortina/ calls u64_stats_init() for
rx_stats_syncp, tx_stats_syncp or ir_stats_syncp. The port struct is
zeroed memory from devm_alloc_etherdev_mqs(). gemini_ethernet_port_probe()
initializes the config lock:

	spin_lock_init(&port->config_lock);

but none of the three syncp fields.

On 32-bit kernels with CONFIG_DEBUG_LOCK_ALLOC, the seqcount's lockdep_map
has no key. The first writer would therefore go through:

gmac_skb_if_good_frame()
  u64_stats_update_begin()
    __u64_stats_update_begin()
      write_seqcount_begin()
        seqcount_acquire()
          assign_lock_key()

This prints "INFO: trying to register non-static key." and turns lockdep
off for the rest of the boot.

Would it make sense to add u64_stats_init() calls for the three syncp
fields in gemini_ethernet_port_probe(), perhaps as a separate patch?

> +		port->rx_stats[rx_status]++;
> +		port->rx_csum_stats[rx_csum]++;
>  		port->stats.rx_errors++;

[ ... ]

> @@ -1915,13 +1924,13 @@ static int gmac_napi_poll(struct napi_struct *napi, int budget)
>  	bool reschedule;
>  
>  	freeq_threshold = 1 << (geth->freeq_order - 1);
> -	u64_stats_update_begin(&port->rx_stats_syncp);
>  
>  	received = gmac_rx(napi->dev, budget, &freeq_consumed, &reschedule);

[Severity: Medium]
The u64_stats_update_begin() removed here, placed before gmac_rx(), dates
back to the original driver commit. On 32-bit PREEMPT_RT the path is:

gmac_napi_poll()
  u64_stats_update_begin()
    __u64_stats_update_begin()
      preempt_disable_nested()    <- preempt_disable() on PREEMPT_RT
  gmac_rx()
    spin_lock_irqsave(&geth->irq_lock, flags)

gmac_rx() also takes the free queue lock in geth_freeq_claim() and calls
napi_gro_frags() while preemption is disabled.

This patch fixes that "sleeping function called from invalid context"
problem. Should it carry this tag?

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Documentation/process/maintainer-netdev.rst also asks bug fixes to say how
the issue was found and confirmed, and how the fix was tested. For
hardware drivers it asks for the hardware the fix was tested on. The
commit message has none of these.

Other patches in this series carry Fixes: 4d5ae32f5e1e, for example "Keep
shared free queue parent-owned" and "Drain free queue IRQ before resize".
Others include a testing statement, for example "Account RX buffer size in
truesize", which mentions testing on a D-Link DIR-685. Could this patch get
the same tag and testing statement?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org