Re: [PATCH v20 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A modulo math mismatch between the ring buffer size and the CIRC_SPACE calculation allows the tail pointer to jump over the head pointer.
- [High] Hardware registers are accessed after device clock is disabled during removal, due to pm_runtime_force_suspend() being called too early.
- [High] Hardware registers accessed in IRQ handler before device is confirmed active, risking a bus fault.
- [High] Missing usage counter decrement when pm_runtime_get_sync() fails, leading to PM runtime refcount leak.
--

commit 0c2081a4425fb055ae1074f9c27f82d0875546e3
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Subject: dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue

This commit converts the BAM DMA driver to use the high-priority variant
of the BH workqueue instead of legacy tasklets for starting DMA transactions.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index fc155e0d1870c..ea3df28e777f9 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c

[ ... ]

> @@ -922,9 +923,9 @@ static irqreturn_t bam_dma_irq(int irq, void *data)
>  
>  	srcs |= process_channel_irqs(bdev);

[Severity: High]
This is a pre-existing issue, but does process_channel_irqs() read from
hardware registers before the device is confirmed active? If a spurious
interrupt occurs while the device is suspended, could this unprotected
hardware access trigger an unclocked access exception and crash the system?

>  
> -	/* kick off tasklet to start next dma transfer */
> +	/* kick off the work queue to start next dma transfer */
>  	if (srcs & P_IRQ)
> -		tasklet_schedule(&bdev->task);
> +		queue_work(system_bh_highpri_wq, &bdev->work);
>  
>  	ret = pm_runtime_get_sync(bdev->dev);
>  	if (ret < 0)

[Severity: High]
This isn't a bug introduced by this patch, but if pm_runtime_get_sync()
fails here, should we call pm_runtime_put_noidle() before returning?
Without it, the PM runtime usage counter might leak and prevent the device
from entering runtime suspend again.

[ ... ]

> @@ -1120,14 +1121,14 @@ static void bam_start_dma(struct bam_chan *bchan)
>  }

[Severity: Critical]
This is a pre-existing issue, but there appears to be a modulo math mismatch
in bam_start_dma() when evaluating available buffer space:

avail = CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1);
...
bchan->tail += async_desc->xfer_len;
bchan->tail %= MAX_DESCRIPTORS;

Could this mismatch between CIRC_SPACE assuming a capacity of 4096 and the
explicit modulo using 4095 allow the tail pointer to jump over the head
pointer? When the buffer is full (tail reaches 4094 and wraps to 0), does
CIRC_SPACE calculate the space as 4095, falsely indicating an empty buffer
and allowing the hardware to overwrite active descriptors?

[Severity: High]
Also in bam_start_dma(), as a pre-existing issue, if pm_runtime_get_sync()
fails:

ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
        return;

Does this early return need a call to pm_runtime_put_noidle() to avoid
leaking the PM runtime usage counter?

[ ... ]

> @@ -1424,7 +1424,7 @@ static void bam_dma_remove(struct platform_device *pdev)
>  			    bdev->channels[i].fifo_phys);
>  	}
>  
> -	tasklet_kill(&bdev->task);
> +	cancel_work_sync(&bdev->work);
>  
>  	clk_disable_unprepare(bdev->bamclk);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does bam_dma_remove()
access hardware registers after the device clock is disabled?

pm_runtime_force_suspend(&pdev->dev);
...
writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

Since pm_runtime_force_suspend() disables the BAM clock, could accessing
hardware registers immediately after trigger a bus fault and crash during
driver unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com?part=3