From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.org>
---
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);
ret = pm_runtime_get_sync(bdev->dev);
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.0On 06-11-25, 16:44, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > There is nothing in the interrupt handling that requires us to run in > atomic context so convert the tasklet to a workqueue. The reason dmaengine drivers use tasklets is higher priority to handle dma interrupts... This is by design for this subsystem. There was an attempt in past to do conversion away from tasklets [1]: We need something similar apprach here. I can queue up first two though 1: https://lore.kernel.org/all/20240327160314.9982-1-apais@linux.microsoft.com/ -- ~Vinod
On Sat, Nov 22, 2025 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 06-11-25, 16:44, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > There is nothing in the interrupt handling that requires us to run in > > atomic context so convert the tasklet to a workqueue. > > The reason dmaengine drivers use tasklets is higher priority to handle > dma interrupts... This is by design for this subsystem. > > There was an attempt in past to do conversion away from tasklets [1]: > We need something similar apprach here. I can queue up first two though > > 1: https://lore.kernel.org/all/20240327160314.9982-1-apais@linux.microsoft.com/ > -- > ~Vinod Ah, I wasn't aware of this. I tested some simple use cases with QCE and it worked so I assumed it's fine. Yes, please queue the first two ones and I'll revisit this later. Bart
On Thu, Nov 06, 2025 at 04:44:52PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.org>
Reviewed-by: Bjorn Andersson <andersson@kernel.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
>
>
On Wed, Nov 12, 2025 at 5:15 PM Bjorn Andersson <andersson@kernel.org> wrote: > > On Thu, Nov 06, 2025 at 04:44:52PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.org> > > Reviewed-by: Bjorn Andersson <andersson@kernel.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). > Thanks, I can look into it once the locking and these changes are in next. Bart
On Thu, Nov 06, 2025 at 04:44:52PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.org> > --- > drivers/dma/qcom/bam_dma.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> -- With best wishes Dmitry
© 2016 - 2025 Red Hat, Inc.