| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <rrw7q7cmkaykng5mnyqk5oxsjednptx3yvjilh3tf5uub4nxzh@p5a4sbgbaha2>
Date: Wed, 12 Nov 2025 10:19:46 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Vinod Koul <vkoul@...nel.org>, linux-arm-msm@...r.kernel.org,
dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 3/3] dmaengine: qcom: bam_dma: convert tasklet to a
workqueue
On Thu, Nov 06, 2025 at 04:44:52PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> There is nothing in the interrupt handling that requires us to run in
> atomic context so convert the tasklet to a workqueue.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Reviewed-by: Bjorn Andersson <andersson@...nel.org>
I like the patch, getting off the tasklet is nice. But reading the
patch/driver spawned some additional questions (not (necessarily)
feedback to this patch).
> ---
> drivers/dma/qcom/bam_dma.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index bcd8de9a9a12621a36b49c31bff96f474468be06..40ad4179177fb7a074776db05b834da012f6a35f 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -42,6 +42,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/scatterlist.h>
> #include <linux/slab.h>
> +#include <linux/workqueue.h>
>
> #include "../dmaengine.h"
> #include "../virt-dma.h"
> @@ -397,8 +398,8 @@ struct bam_device {
> struct clk *bamclk;
> int irq;
>
> - /* dma start transaction tasklet */
> - struct tasklet_struct task;
> + /* dma start transaction workqueue */
> + struct work_struct work;
> };
>
> /**
> @@ -869,7 +870,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
> /*
> * if complete, process cookie. Otherwise
> * push back to front of desc_issued so that
> - * it gets restarted by the tasklet
> + * it gets restarted by the work queue.
> */
> if (!async_desc->num_desc) {
> vchan_cookie_complete(&async_desc->vd);
> @@ -899,9 +900,9 @@ static irqreturn_t bam_dma_irq(int irq, void *data)
>
> srcs |= process_channel_irqs(bdev);
>
> - /* 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);
> + schedule_work(&bdev->work);
I'm not entirely familiar with the BAM driver, but wouldn't it be
preferable to make the interrupt handler threaded and just kick off the
next set of transactions directly from here? To reduce the downtime
between transactions when more are ready queued.
It seems this might be of concern when we have queued more transfers
than can fit in the hardware, but I don't have any data indicating how
often this happens.
>
> ret = pm_runtime_get_sync(bdev->dev);
The "bus" clock is tied to the PM runtime state, so I presume this is
here in order to ensure the block is clocked for the following register
accesses(?)
But process_channel_irqs() was just all over the same register space.
Also noteworthy is that none of the pm_runtime_get_sync() in this driver
calls have adequate error handling.
Regards,
Bjorn
> if (ret < 0)
> @@ -1097,14 +1098,14 @@ static void bam_start_dma(struct bam_chan *bchan)
> }
>
> /**
> - * dma_tasklet - DMA IRQ tasklet
> - * @t: tasklet argument (bam controller structure)
> + * bam_dma_work() - DMA interrupt work queue callback
> + * @work: work queue struct embedded in the BAM controller device struct
> *
> * Sets up next DMA operation and then processes all completed transactions
> */
> -static void dma_tasklet(struct tasklet_struct *t)
> +static void bam_dma_work(struct work_struct *work)
> {
> - struct bam_device *bdev = from_tasklet(bdev, t, task);
> + struct bam_device *bdev = from_work(bdev, work, work);
> struct bam_chan *bchan;
> unsigned int i;
>
> @@ -1117,14 +1118,13 @@ static void dma_tasklet(struct tasklet_struct *t)
> if (!list_empty(&bchan->vc.desc_issued) && !IS_BUSY(bchan))
> bam_start_dma(bchan);
> }
> -
> }
>
> /**
> * bam_issue_pending - starts pending transactions
> * @chan: dma channel
> *
> - * Calls tasklet directly which in turn starts any pending transactions
> + * Calls work queue directly which in turn starts any pending transactions
> */
> static void bam_issue_pending(struct dma_chan *chan)
> {
> @@ -1292,14 +1292,14 @@ static int bam_dma_probe(struct platform_device *pdev)
> if (ret)
> goto err_disable_clk;
>
> - tasklet_setup(&bdev->task, dma_tasklet);
> + INIT_WORK(&bdev->work, bam_dma_work);
>
> bdev->channels = devm_kcalloc(bdev->dev, bdev->num_channels,
> sizeof(*bdev->channels), GFP_KERNEL);
>
> if (!bdev->channels) {
> ret = -ENOMEM;
> - goto err_tasklet_kill;
> + goto err_workqueue_cancel;
> }
>
> /* allocate and initialize channels */
> @@ -1364,8 +1364,8 @@ static int bam_dma_probe(struct platform_device *pdev)
> err_bam_channel_exit:
> for (i = 0; i < bdev->num_channels; i++)
> tasklet_kill(&bdev->channels[i].vc.task);
> -err_tasklet_kill:
> - tasklet_kill(&bdev->task);
> +err_workqueue_cancel:
> + cancel_work_sync(&bdev->work);
> err_disable_clk:
> clk_disable_unprepare(bdev->bamclk);
>
> @@ -1399,7 +1399,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);
> }
>
> --
> 2.51.0
>
>
Powered by blists - more mailing lists