Re: [PATCH net-next 4/4] net: dsa: hms: Add ethtool statistics support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Stack-allocated buffers are passed directly to SPI transfer functions, violating DMA memory safety rules.
- [High] 64-bit statistic values and 32-bit SPI command headers are exchanged with the hardware without byte-order conversion.
- [Medium] The driver adds driver-private ethtool -S strings for standard device statistics, violating networking subsystem guidelines.
--

commit 61954301ddb9081ce56697d8d4fd8e465791f9ac
Author: Minghuan Lian <minghuan.lian@nxp.com>

net: dsa: hms: Add ethtool statistics support
    
Add ethtool statistics support for the HMS switch. This provides
per-port statistics including RX/TX counters, error counters, and
per-queue statistics retrieved from the RT1180 MCU via SPI.

> diff --git a/drivers/net/dsa/hms/hms_ethtool.c b/drivers/net/dsa/hms/hms_ethtool.c
> new file mode 100644
> index 0000000000000..00c8406b12862
> --- /dev/null
> +++ b/drivers/net/dsa/hms/hms_ethtool.c

[ ... ]

> +static const char hms_stat_name[][ETH_GSTRING_LEN] = {
> +	/* RX stats */
> +	[HMS_STAT_RX_BYTES] = "in-bytes",
> +	[HMS_STAT_RX_VALID_BYTES] = "in-valid-bytes",
> +	[HMS_STAT_RX_PAUSE_FRAMES] = "in-pause-frames",
> +	[HMS_STAT_RX_VALID_FRAMES] = "in-valid-frames",

[ ... ]

Does this add driver-private ethtool strings for standard device statistics? 

The networking subsystem guidelines specify that driver patches must not add
new counters to ethtool -S for values that have a standard uapi. Standard
statistics like in-bytes, in-pause-frames, and per-queue frames should be
exposed using standardized interfaces such as ndo_get_stats64, the ethtool
pause api, or standard netlink per-queue statistics. Duplicating these in
private string lists bypasses standard tooling.

Could these standard metrics be migrated to the appropriate uapi?

[ ... ]

> +void hms_get_ethtool_stats(struct dsa_switch *ds, int port, u64 *data)
> +{
> +	struct hms_private *priv = ds->priv;
> +	struct hms_cmd_port_ethtool_stats stats;
> +	int rc;
> +	enum hms_stat_index i;
> +
> +	rc = hms_xfer_get_cmd(priv, HMS_CMD_PORT_ETHTOOL_STATS_GET,
> +			      port, &stats, sizeof(stats));

Is it safe to pass a stack-allocated buffer (stats) to the spi transfer
functions? 

If the underlying spi layer maps this buffer for dma and vmap stacks
are enabled, stack addresses are not physically contiguous and will cause
the underlying dma_map_single() to fail or panic. Even without vmap stacks,
unaligned stack buffers risk memory corruption of adjacent variables due to
dma cache invalidation.

Should this buffer be dynamically allocated using kzalloc() instead?

> +
> +	if (rc) {
> +		memset(data, 0, sizeof(u64) * HMS_STAT_NUM);
> +		dev_err(ds->dev, "Failed to get port %d stats\n", port);
> +		return;
> +	}
> +
> +	for (i = 0; i < HMS_STAT_NUM; i++)
> +		data[i] = stats.values[i];
> +}

Does this code need byte-order conversion?

Since the statistics are retrieved from the hardware over a spi byte stream,
reading directly into host-order u64 values means the transmission byte
order depends on the cpu architecture's endianness. If the host cpu has a
different endianness than the switch mcu, the multi-byte values will be
mangled.

Could this struct use explicit __le64 or __be64 types and apply the
corresponding conversions like le64_to_cpu()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509090632.2959553-1-minghuan.lian@nxp.com?part=4