Re: [PATCH v4 4/4] dmaengine: switchtec-dma: add pmon sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] dmaengine: switchtec-dma: multi-line menu output in
  latency_selector_show()
--

commit a610541f39faa290edb39d9134f3a2df55bdae1a
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: add pmon sysfs attributes

This commit adds a "switchtec-pmon" sysfs attribute group to expose
Switchtec DMA performance monitoring registers per channel.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 1ebe17095c61a..774b418330ed7 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1215,8 +1222,274 @@ static const struct attribute_group switchtec_config_group = {
>  	.attrs = switchtec_config_attrs,
>  };
>  

[ ... ]

> +static ssize_t latency_selector_show(struct device *dev,
> +				     struct device_attribute *attr, char *page)
> +{
> +	struct switchtec_dma_chan *swdma_chan;
> +	struct chan_fw_regs __iomem *chan_fw;
> +	u32 lat = 0;
> +	int len;
> +
> +	CLASS(dmaengine_chan_from_dev, c)(dev);
> +
> +	if (!c)
> +		return -ENODEV;
> +
> +	swdma_chan = container_of(c, struct switchtec_dma_chan, dma_chan);
> +	chan_fw = swdma_chan->mmio_chan_fw;
> +
> +	rcu_read_lock();
> +	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
> +		rcu_read_unlock();
> +		return -ENODEV;
> +	}
> +
> +	lat = readl(&chan_fw->perf_latency_selector);
> +	rcu_read_unlock();
> +
> +	len = sysfs_emit(page,
> +			 "To select a latency type, write the type number\n");
> +	len += sysfs_emit_at(page, len, "(1 ~ 5) to latency_selector\n\n");
> +
> +	len += sysfs_emit_at(page, len, "Latency Types:\n");
> +	len += sysfs_emit_at(page, len, "(1) SE Fetch latency");
> +	if (lat & SWITCHTEC_LAT_SE_FETCH)
> +		len += sysfs_emit_at(page, len, " (*)\n");
> +	else
> +		len += sysfs_emit_at(page, len, "\n");
> +
> +	len += sysfs_emit_at(page, len, "(2) VDM latency");
> +	if (lat & SWITCHTEC_LAT_VDM)
> +		len += sysfs_emit_at(page, len, " (*)\n");
> +	else
> +		len += sysfs_emit_at(page, len, "\n");
> +
> +	len += sysfs_emit_at(page, len, "(3) Read Immediate latency");
> +	if (lat & SWITCHTEC_LAT_RD_IMM)
> +		len += sysfs_emit_at(page, len, " (*)\n");
> +	else
> +		len += sysfs_emit_at(page, len, "\n");
> +
> +	len += sysfs_emit_at(page, len, "(4) SE Processing latency");
> +	if (lat & SWITCHTEC_LAT_SE_PROCESS)
> +		len += sysfs_emit_at(page, len, " (*)\n");
> +	else
> +		len += sysfs_emit_at(page, len, "\n");
> +
> +	len += sysfs_emit_at(page, len, "(5) FW NP TLP latency");
> +	if (lat & SWITCHTEC_LAT_FW_NP)
> +		len += sysfs_emit_at(page, len, " (*)\n");
> +	else
> +		len += sysfs_emit_at(page, len, "\n");
> +
> +	return len;
> +}

[Severity: Low]
Does latency_selector_show() violate the sysfs one-value-per-file rule?

Reading latency_selector emits multi-line usage instructions and a
numbered menu marked with "(*)", while latency_selector_store() expects a
single integer from 1 to 5 via kstrtoint(), so the output cannot be
parsed as a single value or written back to the attribute.  Could
latency_selector expose a single value (or a single-line space-separated
list of tokens with brackets around the active selection, documented
under Documentation/ABI/), or move human-oriented menus to debugfs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110173137.623764-1-logang@deltatee.com?part=4