Re: [PATCH v1 3/5] dmaengine: switchtec-dma: Add config sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] Concurrent sysfs writes to different configuration attributes cause lost updates due to an unlocked read-modify-write sequence.
- [Medium] The configuration store function silently ignores writes when the channel is in use by returning success instead of an error code.
- [Medium] Lockless check of `chan->client_count` allows modifying channel configuration while the channel is concurrently allocated.

Pre-existing issues:
- [High] Memory leak of DMA channel structures on initialization failure.
- [High] switchtec_dma_alloc_chan_resources leaks DMA memory and descriptors on error paths.
--

--- Patch [3]: [PATCH v1 3/5] dmaengine: switchtec-dma: Add config sysfs attributes ---
commit 8760f8c3c1480cd451ac42e9181aa92c4b359981
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: Add config sysfs attributes

Add sysfs configuration options for switchtec-dma devices.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..4841134bd7b83 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1027,6 +1027,145 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	return SWITCHTEC_DMA_SQ_SIZE;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed that
switchtec_dma_alloc_chan_resources() allocates coherent DMA memory and
descriptor rings by calling switchtec_dma_alloc_desc(). If any subsequent
hardware initialization step fails, it returns directly with an error code:

	rc = switchtec_dma_alloc_desc(swdma_chan);
	if (rc)
		return rc;

	rc = enable_channel(swdma_chan);
	if (rc)
		return rc;

Since the dmaengine core handles allocation failures by dropping the module
reference without calling device_free_chan_resources(), does this permanently
leak the allocated descriptors and 32KB coherent DMA memory blocks?

> +static __always_inline ssize_t perf_cfg_store(struct dma_chan *chan,
> +		const char *page, size_t count, unsigned int mask)
> +{
> +	struct switchtec_dma_chan *swdma_chan =
> +		container_of(chan, struct switchtec_dma_chan, dma_chan);
> +	struct chan_fw_regs __iomem *chan_fw = swdma_chan->mmio_chan_fw;
> +	ssize_t ret = count;
> +	u32 perf_cfg;
> +	int value;
> +
> +	if (kstrtoint(page, 0, &value) < 0)
> +		return -EINVAL;
> +
> +	if (value < 0 || value > field_max(mask))
> +		return -EINVAL;
> +
> +	rcu_read_lock();
> +	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	if (chan->client_count)
> +		goto err_unlock;

[Severity: Medium]
If the channel is in use (chan->client_count is non-zero), the code jumps to
err_unlock but returns ret (which is still initialized to count). Will this
falsely indicate to userspace that the write succeeded instead of returning an
error code like -EBUSY?

[Severity: Medium]
Also, this lockless check of chan->client_count appears to allow a TOCTOU
race. Could a concurrent thread allocate the channel via
dma_request_chan() immediately after this check passes, causing the sysfs
thread to modify the hardware configuration out from under an active DMA
client?

> +
> +	perf_cfg = readl(&chan_fw->perf_cfg);
> +	perf_cfg = (perf_cfg & ~mask) | field_prep(mask, value);
> +	writel(perf_cfg, &chan_fw->perf_cfg);

[Severity: Medium]
This read-modify-write sequence on the shared MMIO register
chan_fw->perf_cfg appears to be unprotected by hw_ctrl_lock or any other
lock. If there are concurrent sysfs writes to different attributes (like
burst_scale_store and mrrs_store), could they interleave and cause one of
the updates to be lost?

> +
> +err_unlock:
> +	rcu_read_unlock();
> +	return ret;
> +}

[ ... ]

> @@ -1286,6 +1425,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  		goto err_chans_release_exit;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but if
dma_async_device_register() fails during probe (which jumps to
err_chans_release_exit just above here), does it leak the dynamically
allocated swdma_chan structures?

The error path calls switchtec_dma_chans_release(), which stops hardware
and frees IRQs, but never calls kfree() on the channels or the
swdma_dev->swdma_chans array itself.

> +	dma_chan_kobject_add(dma, &switchtec_ktype, "switchtec");
> +
>  	pci_dbg(pdev, "Channel count: %d\n", chan_cnt);

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