Re: [PATCH net-next v6 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] Update process context `sw_lock` acquisitions to use `spin_lock_bh()` to resolve a softirq deadlock.
- [Medium] Remove `tx_drop` from `lan9645x_port_ethtool_stats` to fix standard uAPI duplication.
--

commit 1372a97790e46033dbc8d913e95a8b88e1650d02
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
Subject: net: dsa: lan9645x: add port statistics

This commit adds statistics support for port counters on the lan9645x switch.
It introduces a 64-bit software cache to handle 32-bit hardware register
overflows, updating them periodically from a workqueue, and implements both
standard and ethtool statistics callbacks.

> 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 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_green_prio_6",    SCNT_RX_GREEN_PRIO_6 },
> +	{ "rx_green_prio_7",    SCNT_RX_GREEN_PRIO_7 },
> +	{ "tx_drop",            SCNT_TX_DROP },

[Severity: Medium]
Is this statistic duplicating a standard kernel uAPI? 

It looks like tx_drop maps to SCNT_TX_DROP, which is already reported via
tx_dropped in the rtnl_link_stats64 struct provided by 
lan9645x_stats_get_stats64(). The networking guidelines recommend only using
the ethtool -S interface for driver-private statistics that have no standard
representation, to avoid duplicating counters and adding maintenance burden.

> +	{ "tx_yellow_prio_0",   SCNT_TX_YELLOW_PRIO_0 },
> +	{ "tx_yellow_prio_1",   SCNT_TX_YELLOW_PRIO_1 },
[ ... ]
> +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]
Does this lock acquisition need to disable bottom halves?

Since lan9645x_stats_get_stats64() can be called in softirq context by the
networking core and acquires sw_lock, taking sw_lock here in process context
using a plain spin_lock() could lead to a deadlock. If a softirq interrupts
this thread while the lock is held and attempts to take the same lock, the
system will hang.

Could spin_lock_bh() be used for all process-context sw_lock acquisitions
(such as in the ethtool callbacks and workqueue contexts) to prevent this?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-dsa_lan9645x_switch_driver_base-v6-0-4d409ae64f3c@microchip.com?part=9