Re: [PATCH net-next v10 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] Process-context functions acquire `sw_lock` using `spin_lock()` instead of `spin_lock_bh()`, risking a deadlock with softirq contexts.
--

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

net: dsa: lan9645x: add port statistics

This commit adds support for port statistics counters. It introduces a 64-bit
software cache to handle 32-bit hardware register overflows and exposes standard
as well as private ethtool statistics.

> 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]
Can a softirq preempt this process context and cause a deadlock?

Since this is called from a delayed workqueue context, it runs with softirqs
enabled. If a network receive softirq fires on the same CPU while this lock
is held, it will call lan9645x_stats_get_stats64(), which attempts to
acquire this same spin_lock, leading to a permanent hang.

Should this and other process-context lock acquisitions use spin_lock_bh()
to prevent softirq preemption?

[ ... ]

> +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 this another location where the lock should be taken with spin_lock_bh()?

Ethtool callbacks run in process context and are also vulnerable to the same
softirq preemption race as the workqueue.

[ ... ]

> +/* 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);
> +
> +	stats->rx_bytes = c[SCNT_RX_OCT] + c[SCNT_RX_PMAC_OCT];

[Severity: High]
This is the softirq-callable path mentioned above that completes the deadlock
scenario if it preempts the workqueue or ethtool operations holding sw_lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-dsa_lan9645x_switch_driver_base-v10-0-a4886a08fb15@microchip.com?part=9