Re: [PATCH net-next v7 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] Deadlock risk due to using plain `spin_lock()` on `sw_lock` in both process and softirq contexts.
--

commit cc6b09f42ef1f85f9c4a542f2826c373a91c2f46
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: lan9645x: add port statistics

Add statistics support for the port counters. Chip registers are 32 bit,
so this unit is responsible maintaining a 64bit software cache, and
updating it frequently to handle overflows in hardware.

> 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
[ ... ]
> +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]
Does this code risk a deadlock? This lock is acquired here in process context
(the delayed workqueue worker) using a plain spin_lock().

However, lan9645x_stats_get_stats64() is called in atomic context (such as
softirq) and also acquires sw_lock. If a softirq preempts the process-context
thread while it holds sw_lock, and the softirq then attempts to read
statistics, the softirq could spin forever trying to acquire the same sw_lock.

Should the process-context paths use spin_lock_bh() instead to disable
bottom halves and prevent this race?

> +		__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]
Is it possible for a softirq to preempt here as well? Like the update function
above, this is in process context and uses a plain spin_lock(), which carries
the same deadlock risk with the atomic context in lan9645x_stats_get_stats64().

The same pattern is repeated across all the other ethtool callbacks in this
file.

> +	for (i = 0; i < ARRAY_SIZE(lan9645x_port_ethtool_stats); i++)
> +		*data++ = c[lan9645x_port_ethtool_stats[i].idx];
> +	spin_unlock(&stats->sw_lock);
> +}
[ ... ]
> +/* Called in atomic context */
> +void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
> +				struct rtnl_link_stats64 *stats)
> +{
> +	struct lan9645x_stats *s = lan9645x->stats;
> +	u64 *c;
> +
> +	c = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> +
> +	spin_lock(&s->sw_lock);

[Severity: High]
Could the deadlock mentioned above occur here if the lock is acquired by a
preempting softirq, since this is explicitly called in atomic context?

> +
> +	stats->rx_bytes = c[SCNT_RX_OCT] + c[SCNT_RX_PMAC_OCT];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-dsa_lan9645x_switch_driver_base-v7-0-b2f90e676707@microchip.com?part=9