Re: [PATCH net-next v14 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] net: dsa: lan9645x: spin_lock deadlock between process context and ndo_get_stats64
--

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

net: dsa: lan9645x: add port statistics

This commit adds support for port statistics counters. It handles overflows
in 32-bit hardware registers by maintaining a 64-bit software cache.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c

[ ... ]

> @@ -321,11 +321,11 @@ 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 this cause a deadlock? The periodic stats update workqueue and ethtool
queries run in process context and acquire s->sw_lock here without disabling
softirqs. Process-context lock acquisitions must use spin_lock_bh() to
prevent a deadlock if a softirq tries to take the lock on the same CPU.

>  		__lan9645x_stats_view_idx_transfer(lan9645x, vtype, idx);
>  		spin_unlock(&s->sw_lock);
>  	}
>  }

[ ... ]

> @@ -536,11 +536,11 @@ void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
>  				struct rtnl_link_stats64 *stats)
>  {
>  	struct lan9645x_stats *s = lan9645x->stats;
>  	u64 *c;
>  
>  	c = lan9645x_stat_counters(lan9645x, LAN9645X_STAT_PORTS, port);
>  
>  	/* ndo_get_stats64 may run in non-sleepable context (under
>  	 * rcu_read_lock, or with a callers spinlock held as in bonding), so
>  	 * unlike the ethtool paths we must not take the hw_lock mutex or touch
>  	 * hardware here.
>  	 * sw_lock is never taken from softirq/IRQ context by any path, so a
>  	 * plain spin_lock is sufficient, as in ocelot and ksz.
>  	 */
>  	spin_lock(&s->sw_lock);

[Severity: High]
Could this lead to a deadlock on s->sw_lock?

The comment indicates the lock is never taken from softirq/IRQ context, but
ndo_get_stats64 can be called from atomic/softirq context (for example, when
queried by bonding or netfilter). 

If a softirq fires on the same CPU while the preempted process-context thread
holds the lock (from __lan9645x_stats_view_idx_update), it will spin forever
trying to acquire s->sw_lock.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213@microchip.com?part=9