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

On Tue, 2026-07-14 at 14:43 +0000, [email protected] wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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 <[email protected]>
> 
> 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.
> 

I think this is a false positive, since .ndo_get_stats64 is not called from softirq
context, so the deadlock can not occur. Ocelot and ksz also use a straight spin_lock
for the same purpose here.

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