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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: dsa: lan9645x: deadlock due to sw_lock taken in process and softirq contexts
- [Medium] net: dsa: lan9645x: duplication of queue and standard stats in private ethtool -S
--

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

net: dsa: lan9645x: add port statistics

This commit adds port statistics support for the LAN9645x switch. It
introduces a 64-bit software cache to handle 32-bit hardware counter
overflows via a periodic polling workqueue. It also implements callbacks
for ethtool and netdev statistics to expose these counters to userspace.

> 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

[ ... ]

> +static const struct lan9645x_ethtool_stat lan9645x_port_ethtool_stats[] = {
> +	{ "rx_cat_drop",        SCNT_RX_CAT_DROP },
> +	{ "rx_red_prio_0",      SCNT_RX_RED_PRIO_0 },
> +	{ "rx_red_prio_1",      SCNT_RX_RED_PRIO_1 },
> +	{ "rx_red_prio_2",      SCNT_RX_RED_PRIO_2 },

[Severity: Medium]
Should these priority queue counters be exposed through the standard netdev
per-queue statistics interface instead?

Adding hardware counters to the private ethtool -S list when a standard uAPI
already exists (like struct netdev_queue_stats_rx and netdev_queue_stats_tx)
duplicates effort and creates confusion. The tx_drop counter listed further
down in this array also appears to map directly to the standard
rtnl_link_stats64.tx_dropped counter.

[ ... ]

> +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);
> +		__lan9645x_stats_view_idx_transfer(lan9645x, vtype, idx);
> +		spin_unlock(&s->sw_lock);
> +	}
> +}

[Severity: High]
Since __lan9645x_stats_view_idx_update is called from a workqueue running in
process context, can this cause a deadlock?

If a softirq interrupts the CPU while this lock is held, and the softirq
attempts to read network statistics via dev_get_stats() to ndo_get_stats64(),
it will attempt to acquire sw_lock in lan9645x_stats_get_stats64() and spin
forever. Do these process-context paths need to use spin_lock_bh() instead?

[ ... ]

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

[Severity: High]
Similarly, since ethtool queries run in process context, does this need
to be spin_lock_bh() to protect against preemption by softirqs that call
lan9645x_stats_get_stats64()?

[ ... ]

> +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]
The comment notes that sw_lock is never taken from softirq/IRQ context,
but lan9645x_stats_get_stats64() itself can be called from softirq context.
For example, when traffic control qdiscs or bonding query network statistics.

Is it possible for this to deadlock against the plain spin_lock() calls in
the ethtool and workqueue paths mentioned above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2@microchip.com?part=9