Re: [PATCH 03/26] dmaengine: loongson-loongson1-apb-dma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup of devm-managed IRQ on error paths inside ls1x_dma_alloc_chan_resources() leading to persistent leaks.
- [High] Unsafe IRQF_SHARED handler lacks hardware status verification, risking data corruption.
- [High] API violation: non-atomic readl_poll_timeout macro used in atomic context risks hard lockups.
- [High] Incorrect use of the logical sysfs channel device for DMA API operations and devres allocations.
- [High] Bypassing the devres unregister mechanism causes a Use-After-Free and resource leaks.
- [High] Memory for the DMA descriptor is leaked if scatterlist allocation fails in ls1x_dma_prep_dma_cyclic().
--

--- Patch [3]: [PATCH 03/26] dmaengine: loongson-loongson1-apb-dma: Remove redundant dev_err()/dev_err_probe() ---
commit cc0e2dcbd30ac2febb8a1c6ef2b34cbde416aa33
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: loongson-loongson1-apb-dma: Remove redundant dev_err()/dev_err_probe()

This commit removes redundant dev_err() calls when devm_request_irq() fails in
ls1x_dma_alloc_chan_resources(). Since devm_request_irq() logs its own detailed
error messages, the driver-specific messages are no longer needed.

> diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
> index 89786cbd20ab5..812bd7c2068f9 100644
> --- a/drivers/dma/loongson/loongson1-apb-dma.c
> +++ b/drivers/dma/loongson/loongson1-apb-dma.c
> @@ -162,10 +162,8 @@ static int ls1x_dma_alloc_chan_resources(struct dma_chan *dchan)
>  
>  	ret = devm_request_irq(dev, chan->irq, ls1x_dma_irq_handler,
>  			       IRQF_SHARED, dma_chan_name(dchan), chan);
> -	if (ret) {
> -		dev_err(dev, "failed to request IRQ %d\n", chan->irq);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	chan->lli_pool = dma_pool_create(dma_chan_name(dchan), dev,
>  					 sizeof(struct ls1x_dma_lli),
>  					 __alignof__(struct ls1x_dma_lli), 0);
>  	if (!chan->lli_pool)
>  		return -ENOMEM;
>  
>  	/* allocate memory for querying the current lli */
>  	dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>  	chan->curr_lli = dma_alloc_coherent(dev, sizeof(struct ls1x_dma_lli),
>  					    &phys, GFP_KERNEL);
>  	if (!chan->curr_lli) {
>  		dma_pool_destroy(chan->lli_pool);
>  		return -ENOMEM;
>  	}

[Severity: High]
This is a pre-existing issue, but is the devm-managed IRQ leaked on this
error path?

If dma_pool_create() or dma_alloc_coherent() fails, we return -ENOMEM without
freeing the IRQ requested via devm_request_irq(). 

The devm-managed cleanup won't happen because the channel's sysfs device is not
destroyed, leading to local clients hitting -EBUSY on subsequent allocation
attempts.

[ ... ]

> static inline struct device *chan2dev(struct dma_chan *chan)
> {
> 	return &chan->dev->device;
> }

[Severity: High]
This isn't a bug introduced by this patch, but should this helper be returning
the hardware platform device instead?

This macro returns the logical sysfs channel device instead of the hardware
platform device (dchan->device->dev). Passing this logical device to DMA API
operations like dma_alloc_coherent() or dma_pool_create() can cause allocation
failures or incorrect mappings on platforms with strict DMA tracking or an
IOMMU.

[ ... ]

> static inline int ls1x_dma_start(struct ls1x_dma_chan *chan,
> 				 dma_addr_t *lli_phys)
> {
[ ... ]
> 	ret = readl_poll_timeout(chan->reg_base + LS1X_DMA_CTRL, val,
> 				 !(val & LS1X_DMA_START), 0, 1000);

[Severity: High]
This is a pre-existing issue, but does using readl_poll_timeout() here risk
hard lockups?

ls1x_dma_start() is called from ls1x_dma_resume() and ls1x_dma_issue_pending()
with the channel's lock held and interrupts disabled:

ls1x_dma_resume()
    guard(spinlock_irqsave)(&chan->vc.lock);
    return ls1x_dma_start(chan, &chan->curr_lli->phys);

Using the non-atomic readl_poll_timeout() macro in an atomic context is unsafe
because timekeeping might be suspended, causing the internal ktime_get() to
stall or return invalid values.

[ ... ]

> static struct dma_async_tx_descriptor *
> ls1x_dma_prep_dma_cyclic(struct dma_chan *dchan, dma_addr_t buf_addr,
[ ... ]
> 	desc = ls1x_dma_alloc_desc();
> 	if (!desc)
> 		return NULL;
> 
> 	/* allocate the scatterlist */
> 	sg_len = buf_len / period_len;
> 	sgl = kmalloc_objs(*sgl, sg_len, GFP_NOWAIT);
> 	if (!sgl)
> 		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but what happens to the allocated
descriptor if kmalloc_objs() fails?

If we fail to allocate the scatterlist, the function returns NULL without
freeing the desc allocated just above it, resulting in a memory leak.

[ ... ]

> static irqreturn_t ls1x_dma_irq_handler(int irq, void *data)
> {
[ ... ]
> 	scoped_guard(spinlock, &chan->vc.lock) {
> 		vd = vchan_next_desc(&chan->vc);
> 		if (!vd) {
> 			dev_warn(dev,
> 				 "IRQ %d with no active desc on channel %d\n",
> 				 irq, dchan->chan_id);
> 			return IRQ_NONE;
> 		}

[Severity: High]
This is a pre-existing issue, but is it safe for a shared IRQ handler to
assume ownership of the interrupt without checking hardware status?

The handler is registered with IRQF_SHARED, but it processes active descriptors
purely based on whether one is queued in software. If another device sharing
the IRQ line triggers an interrupt, this could prematurely complete the current
in-flight DMA transfer and cause data corruption.

[ ... ]

> static void ls1x_dma_chan_remove(struct ls1x_dma *dma)
> {
> 	int id;
> 
> 	for (id = 0; id < dma->nr_chans; id++) {
> 		struct ls1x_dma_chan *chan = &dma->chan[id];
> 
> 		if (chan->vc.chan.device == &dma->ddev) {
> 			list_del(&chan->vc.chan.device_node);
> 			tasklet_kill(&chan->vc.task);
> 		}
> 	}
> }

[Severity: High]
This isn't a bug introduced by this patch, but will deleting the channel from
the list here bypass the devres unregister mechanism?

During module removal or probe failure, manually calling list_del() orphans
the channel from the dmaengine core. When devres later runs
dma_async_device_unregister(), it iterates over an empty list and fails to
unregister the channels.

This permanently leaks the sysfs channel devices and percpu memory, and will
trigger a Use-After-Free when the underlying struct ls1x_dma is freed by
devm_kzalloc cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=3