Re: [PATCH net-next v9 9/9] net: dsa: lan9645x: add port statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential spinlock deadlock due to using `spin_lock()` instead of `spin_lock_bh()` for `sw_lock`, which is shared between process context and SoftIRQ context.
--

--- Patch [9]: [PATCH net-next v9 9/9] net: dsa: lan9645x: add port statistics ---
commit 3c87269564235dbc14761981f5b167363d8b2671
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
net: dsa: lan9645x: add port statistics

Add statistics support for the port counters. The driver maintains a 64-bit
software cache and updates it frequently from a delayed work item to handle
hardware overflows.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> new file mode 100644
> index 0000000000000..e38f510eb8f66
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> @@ -0,0 +1,929 @@
[ ... ]
> +static void __lan9645x_stats_view_idx_update(struct lan9645x *lan9645x,
> +					     enum lan9645x_view_stat_type vtype,
> +					     int idx)
> +{
> +	struct lan9645x_stats *s = lan9645x->stats;
> +
> +	lockdep_assert_held(&s->hw_lock);
> +
> +	if (!__lan9645x_stats_view_idx_hw_read(lan9645x, vtype, idx)) {
> +		spin_lock(&s->sw_lock);

[Severity: High]
Could this lead to a deadlock if a softirq interrupts the process context
while the lock is held?

In __lan9645x_stats_view_idx_update() this lock is acquired from the
background workqueue context using a plain spin_lock().

However, lan9645x_stats_get_stats64() is called from atomic/softirq
context and also tries to acquire s->sw_lock:

/* Called in atomic context */
void lan9645x_stats_get_stats64(...)
{
        ...
        spin_lock(&s->sw_lock);
        ...
}

If a network softirq fires on the same CPU while the background workqueue
holds sw_lock, it seems the softirq would spin forever waiting for the lock.
Should the process-context callers use spin_lock_bh() instead?

> +		__lan9645x_stats_view_idx_transfer(lan9645x, vtype, idx);
> +		spin_unlock(&s->sw_lock);
> +	}
> +}
[ ... ]
> +void lan9645x_stats_get_ethtool_stats(struct lan9645x *lan9645x, int port,
> +				      u64 *data)
> +{
> +	struct lan9645x_stats *stats = lan9645x->stats;
> +	u64 *c;
> +	int i;
> +
> +	c = lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> +
> +	spin_lock(&stats->sw_lock);

[Severity: High]
Similar to the background updater above, should this ethtool callback
also use spin_lock_bh() to protect against the softirq reader?

> +	for (i = 0; i < ARRAY_SIZE(lan9645x_port_ethtool_stats); i++)
> +		*data++ = c[lan9645x_port_ethtool_stats[i].idx];
> +	spin_unlock(&stats->sw_lock);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-dsa_lan9645x_switch_driver_base-v9-0-0d1512a326d7@microchip.com?part=9