* [PATCH 2/3] dmaengine: qcom: bam_dma: use lock guards
2025-11-06 15:44 [PATCH 0/3] dmengine: qcom: bam_dma: some driver refactoring Bartosz Golaszewski
2025-11-06 15:44 ` [PATCH 1/3] dmaengine: qcom: bam_dma: order includes alphabetically Bartosz Golaszewski
@ 2025-11-06 15:44 ` Bartosz Golaszewski
2025-11-06 15:44 ` [PATCH 3/3] dmaengine: qcom: bam_dma: convert tasklet to a workqueue Bartosz Golaszewski
2 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2025-11-06 15:44 UTC (permalink / raw)
To: Vinod Koul; +Cc: linux-arm-msm, dmaengine, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Simplify locking across the driver with lock guards from cleanup.h.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/dma/qcom/bam_dma.c | 124 ++++++++++++++++++++-------------------------
1 file changed, 55 insertions(+), 69 deletions(-)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 2f1f295d3e1ff910a5051c20dc09cb1e8077df82..bcd8de9a9a12621a36b49c31bff96f474468be06 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -24,6 +24,7 @@
*/
#include <linux/circ_buf.h>
+#include <linux/cleanup.h>
#include <linux/clk.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
@@ -570,7 +571,6 @@ static void bam_free_chan(struct dma_chan *chan)
struct bam_chan *bchan = to_bam_chan(chan);
struct bam_device *bdev = bchan->bdev;
u32 val;
- unsigned long flags;
int ret;
ret = pm_runtime_get_sync(bdev->dev);
@@ -584,9 +584,8 @@ static void bam_free_chan(struct dma_chan *chan)
goto err;
}
- spin_lock_irqsave(&bchan->vc.lock, flags);
- bam_reset_channel(bchan);
- spin_unlock_irqrestore(&bchan->vc.lock, flags);
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock)
+ bam_reset_channel(bchan);
dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE, bchan->fifo_virt,
bchan->fifo_phys);
@@ -624,12 +623,11 @@ static int bam_slave_config(struct dma_chan *chan,
struct dma_slave_config *cfg)
{
struct bam_chan *bchan = to_bam_chan(chan);
- unsigned long flag;
- spin_lock_irqsave(&bchan->vc.lock, flag);
+ guard(spinlock_irqsave)(&bchan->vc.lock);
+
memcpy(&bchan->slave, cfg, sizeof(*cfg));
bchan->reconfigure = 1;
- spin_unlock_irqrestore(&bchan->vc.lock, flag);
return 0;
}
@@ -726,38 +724,37 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
{
struct bam_chan *bchan = to_bam_chan(chan);
struct bam_async_desc *async_desc, *tmp;
- unsigned long flag;
LIST_HEAD(head);
/* remove all transactions, including active transaction */
- spin_lock_irqsave(&bchan->vc.lock, flag);
- /*
- * If we have transactions queued, then some might be committed to the
- * hardware in the desc fifo. The only way to reset the desc fifo is
- * to do a hardware reset (either by pipe or the entire block).
- * bam_chan_init_hw() will trigger a pipe reset, and also reinit the
- * pipe. If the pipe is left disabled (default state after pipe reset)
- * and is accessed by a connected hardware engine, a fatal error in
- * the BAM will occur. There is a small window where this could happen
- * with bam_chan_init_hw(), but it is assumed that the caller has
- * stopped activity on any attached hardware engine. Make sure to do
- * this first so that the BAM hardware doesn't cause memory corruption
- * by accessing freed resources.
- */
- if (!list_empty(&bchan->desc_list)) {
- async_desc = list_first_entry(&bchan->desc_list,
- struct bam_async_desc, desc_node);
- bam_chan_init_hw(bchan, async_desc->dir);
- }
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock) {
+ /*
+ * If we have transactions queued, then some might be committed to the
+ * hardware in the desc fifo. The only way to reset the desc fifo is
+ * to do a hardware reset (either by pipe or the entire block).
+ * bam_chan_init_hw() will trigger a pipe reset, and also reinit the
+ * pipe. If the pipe is left disabled (default state after pipe reset)
+ * and is accessed by a connected hardware engine, a fatal error in
+ * the BAM will occur. There is a small window where this could happen
+ * with bam_chan_init_hw(), but it is assumed that the caller has
+ * stopped activity on any attached hardware engine. Make sure to do
+ * this first so that the BAM hardware doesn't cause memory corruption
+ * by accessing freed resources.
+ */
+ if (!list_empty(&bchan->desc_list)) {
+ async_desc = list_first_entry(&bchan->desc_list,
+ struct bam_async_desc, desc_node);
+ bam_chan_init_hw(bchan, async_desc->dir);
+ }
- list_for_each_entry_safe(async_desc, tmp,
- &bchan->desc_list, desc_node) {
- list_add(&async_desc->vd.node, &bchan->vc.desc_issued);
- list_del(&async_desc->desc_node);
- }
+ list_for_each_entry_safe(async_desc, tmp,
+ &bchan->desc_list, desc_node) {
+ list_add(&async_desc->vd.node, &bchan->vc.desc_issued);
+ list_del(&async_desc->desc_node);
+ }
- vchan_get_all_descriptors(&bchan->vc, &head);
- spin_unlock_irqrestore(&bchan->vc.lock, flag);
+ vchan_get_all_descriptors(&bchan->vc, &head);
+ }
vchan_dma_desc_free_list(&bchan->vc, &head);
@@ -773,17 +770,16 @@ static int bam_pause(struct dma_chan *chan)
{
struct bam_chan *bchan = to_bam_chan(chan);
struct bam_device *bdev = bchan->bdev;
- unsigned long flag;
int ret;
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return ret;
- spin_lock_irqsave(&bchan->vc.lock, flag);
- writel_relaxed(1, bam_addr(bdev, bchan->id, BAM_P_HALT));
- bchan->paused = 1;
- spin_unlock_irqrestore(&bchan->vc.lock, flag);
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock) {
+ writel_relaxed(1, bam_addr(bdev, bchan->id, BAM_P_HALT));
+ bchan->paused = 1;
+ }
pm_runtime_mark_last_busy(bdev->dev);
pm_runtime_put_autosuspend(bdev->dev);
@@ -799,17 +795,16 @@ static int bam_resume(struct dma_chan *chan)
{
struct bam_chan *bchan = to_bam_chan(chan);
struct bam_device *bdev = bchan->bdev;
- unsigned long flag;
int ret;
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return ret;
- spin_lock_irqsave(&bchan->vc.lock, flag);
- writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
- bchan->paused = 0;
- spin_unlock_irqrestore(&bchan->vc.lock, flag);
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock) {
+ writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
+ bchan->paused = 0;
+ }
pm_runtime_mark_last_busy(bdev->dev);
pm_runtime_put_autosuspend(bdev->dev);
@@ -826,7 +821,6 @@ static int bam_resume(struct dma_chan *chan)
static u32 process_channel_irqs(struct bam_device *bdev)
{
u32 i, srcs, pipe_stts, offset, avail;
- unsigned long flags;
struct bam_async_desc *async_desc, *tmp;
srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
@@ -846,7 +840,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
writel_relaxed(pipe_stts, bam_addr(bdev, i, BAM_P_IRQ_CLR));
- spin_lock_irqsave(&bchan->vc.lock, flags);
+ guard(spinlock_irqsave)(&bchan->vc.lock);
offset = readl_relaxed(bam_addr(bdev, i, BAM_P_SW_OFSTS)) &
P_SW_OFSTS_MASK;
@@ -885,8 +879,6 @@ static u32 process_channel_irqs(struct bam_device *bdev)
}
list_del(&async_desc->desc_node);
}
-
- spin_unlock_irqrestore(&bchan->vc.lock, flags);
}
return srcs;
@@ -950,7 +942,6 @@ static enum dma_status bam_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
int ret;
size_t residue = 0;
unsigned int i;
- unsigned long flags;
ret = dma_cookie_status(chan, cookie, txstate);
if (ret == DMA_COMPLETE)
@@ -959,23 +950,22 @@ static enum dma_status bam_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
if (!txstate)
return bchan->paused ? DMA_PAUSED : ret;
- spin_lock_irqsave(&bchan->vc.lock, flags);
- vd = vchan_find_desc(&bchan->vc, cookie);
- if (vd) {
- residue = container_of(vd, struct bam_async_desc, vd)->length;
- } else {
- list_for_each_entry(async_desc, &bchan->desc_list, desc_node) {
- if (async_desc->vd.tx.cookie != cookie)
- continue;
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock) {
+ vd = vchan_find_desc(&bchan->vc, cookie);
+ if (vd) {
+ residue = container_of(vd, struct bam_async_desc, vd)->length;
+ } else {
+ list_for_each_entry(async_desc, &bchan->desc_list, desc_node) {
+ if (async_desc->vd.tx.cookie != cookie)
+ continue;
- for (i = 0; i < async_desc->num_desc; i++)
- residue += le16_to_cpu(
- async_desc->curr_desc[i].size);
+ for (i = 0; i < async_desc->num_desc; i++)
+ residue += le16_to_cpu(
+ async_desc->curr_desc[i].size);
+ }
}
}
- spin_unlock_irqrestore(&bchan->vc.lock, flags);
-
dma_set_residue(txstate, residue);
if (ret == DMA_IN_PROGRESS && bchan->paused)
@@ -1116,17 +1106,16 @@ static void dma_tasklet(struct tasklet_struct *t)
{
struct bam_device *bdev = from_tasklet(bdev, t, task);
struct bam_chan *bchan;
- unsigned long flags;
unsigned int i;
/* go through the channels and kick off transactions */
for (i = 0; i < bdev->num_channels; i++) {
bchan = &bdev->channels[i];
- spin_lock_irqsave(&bchan->vc.lock, flags);
+
+ guard(spinlock_irqsave)(&bchan->vc.lock);
if (!list_empty(&bchan->vc.desc_issued) && !IS_BUSY(bchan))
bam_start_dma(bchan);
- spin_unlock_irqrestore(&bchan->vc.lock, flags);
}
}
@@ -1140,15 +1129,12 @@ static void dma_tasklet(struct tasklet_struct *t)
static void bam_issue_pending(struct dma_chan *chan)
{
struct bam_chan *bchan = to_bam_chan(chan);
- unsigned long flags;
- spin_lock_irqsave(&bchan->vc.lock, flags);
+ guard(spinlock_irqsave)(&bchan->vc.lock);
/* if work pending and idle, start a transaction */
if (vchan_issue_pending(&bchan->vc) && !IS_BUSY(bchan))
bam_start_dma(bchan);
-
- spin_unlock_irqrestore(&bchan->vc.lock, flags);
}
/**
--
2.51.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 3/3] dmaengine: qcom: bam_dma: convert tasklet to a workqueue
2025-11-06 15:44 [PATCH 0/3] dmengine: qcom: bam_dma: some driver refactoring Bartosz Golaszewski
2025-11-06 15:44 ` [PATCH 1/3] dmaengine: qcom: bam_dma: order includes alphabetically Bartosz Golaszewski
2025-11-06 15:44 ` [PATCH 2/3] dmaengine: qcom: bam_dma: use lock guards Bartosz Golaszewski
@ 2025-11-06 15:44 ` Bartosz Golaszewski
2 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2025-11-06 15:44 UTC (permalink / raw)
To: Vinod Koul; +Cc: linux-arm-msm, dmaengine, linux-kernel, Bartosz Golaszewski
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.0
^ permalink raw reply related [flat|nested] 4+ messages in thread