* [PATCH v21 01/14] dmaengine: constify struct dma_descriptor_metadata_ops
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
` (12 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski,
Radhey Shyam Pandey
There's no reason for the instances of this struct to be modifiable.
Constify the pointer in struct dma_async_tx_descriptor and all drivers
currently using it.
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/ti/k3-udma.c | 2 +-
drivers/dma/xilinx/xilinx_dma.c | 2 +-
include/linux/dmaengine.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 1cf158eb7bdb541c4e7f4f79f65ab70be4311fad..fb21e0df5ab7b20e4e16777b5ff7f61d2ae67b2b 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -3408,7 +3408,7 @@ static int udma_set_metadata_len(struct dma_async_tx_descriptor *desc,
return 0;
}
-static struct dma_descriptor_metadata_ops metadata_ops = {
+static const struct dma_descriptor_metadata_ops metadata_ops = {
.attach = udma_attach_metadata,
.get_ptr = udma_get_metadata_ptr,
.set_len = udma_set_metadata_len,
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 404235c1735384635597e88edc25c67c7d250647..165b11a7c776abc6a8d66d631e19da669644577d 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -653,7 +653,7 @@ static void *xilinx_dma_get_metadata_ptr(struct dma_async_tx_descriptor *tx,
return seg->hw.app;
}
-static struct dma_descriptor_metadata_ops xilinx_dma_metadata_ops = {
+static const struct dma_descriptor_metadata_ops xilinx_dma_metadata_ops = {
.get_ptr = xilinx_dma_get_metadata_ptr,
};
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b3d251c9734e95e1b75cf6763d4d2c3a1c6a9910..5244edb90e7e7510bf4460b6a74ee2a7f91c1ccc 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -623,7 +623,7 @@ struct dma_async_tx_descriptor {
void *callback_param;
struct dmaengine_unmap_data *unmap;
enum dma_desc_metadata_mode desc_metadata_mode;
- struct dma_descriptor_metadata_ops *metadata_ops;
+ const struct dma_descriptor_metadata_ops *metadata_ops;
#ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
struct dma_async_tx_descriptor *next;
struct dma_async_tx_descriptor *parent;
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 01/14] dmaengine: constify struct dma_descriptor_metadata_ops Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:14 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
` (11 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
The BAM interrupt is requested with a devres helper and so on error it's
freed after probe() returns. We disable the clock before freeing or
masking it so it may still fire and we may end up reading BAM registers
with clock disabled.
Stop using devres for interrupts as we free it in remove() manually
anyway. Add an appropriate label and free the interrupt before disabling
the clock in error path and in remove().
Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/qcom/bam_dma.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 1bb26af0405f3a16f97e0d4b86c945c252d97f57..fc155e0d1870cbb7e099a2c4280f9f8fbdf6cf15 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1332,8 +1332,7 @@ static int bam_dma_probe(struct platform_device *pdev)
for (i = 0; i < bdev->num_channels; i++)
bam_channel_init(bdev, &bdev->channels[i], i);
- ret = devm_request_irq(bdev->dev, bdev->irq, bam_dma_irq,
- IRQF_TRIGGER_HIGH, "bam_dma", bdev);
+ ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH, "bam_dma", bdev);
if (ret)
goto err_bam_channel_exit;
@@ -1366,7 +1365,7 @@ static int bam_dma_probe(struct platform_device *pdev)
ret = dma_async_device_register(&bdev->common);
if (ret) {
dev_err(bdev->dev, "failed to register dma async device\n");
- goto err_bam_channel_exit;
+ goto err_free_irq;
}
ret = of_dma_controller_register(pdev->dev.of_node, bam_dma_xlate,
@@ -1385,6 +1384,8 @@ static int bam_dma_probe(struct platform_device *pdev)
err_unregister_dma:
dma_async_device_unregister(&bdev->common);
+err_free_irq:
+ free_irq(bdev->irq, bdev);
err_bam_channel_exit:
for (i = 0; i < bdev->num_channels; i++)
tasklet_kill(&bdev->channels[i].vc.task);
@@ -1401,6 +1402,8 @@ static void bam_dma_remove(struct platform_device *pdev)
struct bam_device *bdev = platform_get_drvdata(pdev);
u32 i;
+ free_irq(bdev->irq, bdev);
+
pm_runtime_force_suspend(&pdev->dev);
of_dma_controller_free(pdev->dev.of_node);
@@ -1409,8 +1412,6 @@ static void bam_dma_remove(struct platform_device *pdev)
/* mask all interrupts for this execution environment */
writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
- devm_free_irq(bdev->dev, bdev->irq, bdev);
-
for (i = 0; i < bdev->num_channels; i++) {
bam_dma_terminate_all(&bdev->channels[i].vc.chan);
tasklet_kill(&bdev->channels[i].vc.task);
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path
2026-07-13 13:01 ` [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
@ 2026-07-13 13:14 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:14 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: dmaengine, vkoul, Frank.Li
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Unclocked hardware register access in `bam_dma_remove()` leading to a synchronous external abort (system crash).
- [High] Unsafe hardware access in the IRQ handler before acquiring a Runtime PM reference.
- [High] Concurrency race allowing tasklets to execute unclocked hardware accesses during device removal.
- [High] Clock enable count underflow during device removal.
- [High] Runtime PM usage counter leak in the IRQ handler on failure path.
--
--- Patch [2]: [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path ---
commit 9ca369a6aae9bb2fa96d12a2fd87eaad7821b59b
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
dmaengine: qcom: bam_dma: free interrupt before the clock in error path
The BAM interrupt is requested with a devres helper and so on error it's
freed after probe() returns. We disable the clock before freeing or
masking it so it may still fire and we may end up reading BAM registers
with clock disabled. Stop using devres for interrupts as we free it in
remove() manually anyway.
Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1332,8 +1332,7 @@ static int bam_dma_probe(struct platform_device *pdev)
> for (i = 0; i < bdev->num_channels; i++)
> bam_channel_init(bdev, &bdev->channels[i], i);
>
> - ret = devm_request_irq(bdev->dev, bdev->irq, bam_dma_irq,
> - IRQF_TRIGGER_HIGH, "bam_dma", bdev);
> + ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH, "bam_dma", bdev);
[Severity: High]
This is a pre-existing issue, but I have a question about the interrupt
handler bam_dma_irq() being requested here.
Does the interrupt handler read hardware registers before guaranteeing
the clock is enabled?
In drivers/dma/qcom/bam_dma.c:bam_dma_irq():
srcs |= process_channel_irqs(bdev);
...
ret = pm_runtime_get_sync(bdev->dev);
process_channel_irqs() executes a readl_relaxed() on BAM_IRQ_SRCS_EE.
If a spurious interrupt fires or races with an auto-suspend transition,
could this read occur while the device is suspended and its clock is
off, causing a synchronous external abort?
[Severity: High]
This is also a pre-existing issue, but looking further into the error
path of bam_dma_irq():
In drivers/dma/qcom/bam_dma.c:bam_dma_irq():
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return IRQ_NONE;
If pm_runtime_get_sync() fails, it returns without dropping the reference.
Does this leak the usage counter and permanently prevent the device from
entering lower power states?
> if (ret)
> goto err_bam_channel_exit;
[ ... ]
> @@ -1401,6 +1402,8 @@ static void bam_dma_remove(struct platform_device *pdev)
> struct bam_device *bdev = platform_get_drvdata(pdev);
> u32 i;
>
> + free_irq(bdev->irq, bdev);
> +
> pm_runtime_force_suspend(&pdev->dev);
>
> of_dma_controller_free(pdev->dev.of_node);
> @@ -1409,8 +1412,6 @@ static void bam_dma_remove(struct platform_device *pdev)
> /* mask all interrupts for this execution environment */
> writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
[Severity: Critical]
This isn't a bug introduced by this patch, but does this sequence cause
unclocked hardware register accesses?
In drivers/dma/qcom/bam_dma.c:bam_dma_remove(), the clock is forced off
unconditionally by pm_runtime_force_suspend(&pdev->dev). Immediately
afterward, the code writes to BAM registers:
pm_runtime_force_suspend(&pdev->dev);
...
writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
It then calls bam_dma_terminate_all(), which performs more register
accesses in bam_chan_init_hw(). Can accessing these registers with the
clock off lead to a system crash (SError)?
> - devm_free_irq(bdev->dev, bdev->irq, bdev);
> -
> for (i = 0; i < bdev->num_channels; i++) {
> bam_dma_terminate_all(&bdev->channels[i].vc.chan);
> tasklet_kill(&bdev->channels[i].vc.task);
[Severity: High]
This is a pre-existing issue, but does killing the tasklets after
pm_runtime_force_suspend() leave a race window?
If a scheduled tasklet is concurrently executing bam_start_dma() (having
already incremented the usage count via pm_runtime_get_sync()),
pm_runtime_force_suspend() will ignore the usage counter and disable
the clock anyway.
Could the racing tasklet then access BAM registers with the clock off,
causing a system crash?
[Severity: High]
This isn't a bug introduced by this patch, but does bam_dma_remove()
underflow the clock enable count?
pm_runtime_force_suspend() invokes the runtime suspend callback which
calls clk_disable(). Later in bam_dma_remove(), the driver calls
clk_disable_unprepare(bdev->bamclk) unconditionally.
Does this result in an unbalanced second call to clk_disable() that
underflows the enable count?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 01/14] dmaengine: constify struct dma_descriptor_metadata_ops Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:17 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data Bartosz Golaszewski
` (10 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski,
Dmitry Baryshkov
BH workqueues are a modern mechanism, aiming to replace legacy tasklets.
Let's convert the BAM DMA driver to using the high-priority variant of
the BH workqueue.
[Vinod: suggested using the BG workqueue instead of the regular one
running in process context]
Suggested-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
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 fc155e0d1870cbb7e099a2c4280f9f8fbdf6cf15..ea3df28e777f99c0532761b6aee6807ab23ab4ca 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"
@@ -426,8 +427,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;
};
/**
@@ -892,7 +893,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);
@@ -922,9 +923,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);
+ queue_work(system_bh_highpri_wq, &bdev->work);
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
@@ -1120,14 +1121,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;
@@ -1140,14 +1141,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)
{
@@ -1316,14 +1316,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 */
@@ -1389,8 +1389,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);
@@ -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);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
2026-07-13 13:01 ` [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
@ 2026-07-13 13:17 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:17 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Driver removal forcefully suspends the device before hardware teardown and workqueue cancellation, causing system crashes and Use-After-Free.
- [High] The IRQ handler accesses hardware registers before ensuring the device is active, leading to a potential system crash on spurious interrupts.
- [High] Error paths for `pm_runtime_get_sync()` fail to decrement the PM usage counter, permanently preventing the device from runtime suspending.
- [High] DMA controller is exposed to clients before runtime PM is enabled, dropping transactions.
- [Medium] Missing pm_runtime_disable() in driver removal.
--
--- Patch [3]: [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue ---
commit 69bb6ec401a63dcf8f8b7e2ec728a8a983774e1a
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Subject: dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
This patch replaces the legacy tasklet implementation in the BAM DMA driver
with a high-priority BH workqueue. It updates the probe, remove, irq, and
execution paths to use workqueue APIs.
> 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);
>
> - /* 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);
[Severity: High]
This isn't a bug introduced by this patch, but does process_channel_irqs()
read hardware registers before pm_runtime_get_sync() ensures the clock is
enabled?
If a spurious interrupt arrives while the controller is in runtime suspend
(clocks disabled), could reading the register cause a synchronous external
abort and crash the system?
> if (ret < 0)
> - return IRQ_NONE;
[Severity: High]
This is a pre-existing issue, but if pm_runtime_get_sync() fails, it still
increments the usage counter.
Does this early return fail to drop the PM usage counter (e.g., via
pm_runtime_put_noidle()), permanently skewing the reference count and
preventing the hardware from entering low-power states?
It looks like this same error handling pattern is also present in
bam_start_dma() and bam_resume().
[ ... ]
> @@ -1316,14 +1316,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;
> }
[Severity: High]
This is a pre-existing issue, but it looks like bam_dma_probe() registers
the DMA controller with the subsystem before runtime PM is enabled.
If a consumer device instantly requests a channel and submits a transaction,
could pm_runtime_get_sync() return -EACCES and silently abort the transfer
because PM runtime is not yet active?
[ ... ]
> @@ -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);
[Severity: Critical]
This is a pre-existing issue, but does workqueue cancellation happen after
channel FIFOs have been freed by dma_free_wc() earlier in this function?
Could a concurrently running bam_dma_work execute bam_start_dma() and
access the freed fifo_virt memory, leading to a use-after-free?
[Severity: Critical]
This is also a pre-existing issue, but bam_dma_remove() calls
pm_runtime_force_suspend() early on, which disables the hardware clocks.
Does the driver then perform unclocked register accesses (e.g., by writing
to the BAM_IRQ_SRCS_MSK_EE register and calling bam_dma_terminate_all())
before reaching this point, potentially causing a kernel panic?
>
> clk_disable_unprepare(bdev->bamclk);
> }
[Severity: Medium]
This is a pre-existing issue, but the function exits without calling
pm_runtime_disable().
Does this leave the PM core's runtime PM state active for this device after
the driver is unbound?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (2 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support Bartosz Golaszewski
` (9 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
In preparation for supporting the pipe locking feature flag, extend the
amount of information we can carry in device match data: create a
separate structure and make the register information one of its fields.
This way, in subsequent patches, it will be just a matter of adding a
new field to the device data.
Reviewed-by: Dmitry Baryshkov <lumag@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/qcom/bam_dma.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index ea3df28e777f99c0532761b6aee6807ab23ab4ca..8ce0fe085c5fea6cc614edd692b5cfd264b94d5a 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -113,6 +113,10 @@ struct reg_offset_data {
unsigned int pipe_mult, evnt_mult, ee_mult;
};
+struct bam_device_data {
+ const struct reg_offset_data *reg_info;
+};
+
static const struct reg_offset_data bam_v1_3_reg_info[] = {
[BAM_CTRL] = { 0x0F80, 0x00, 0x00, 0x00 },
[BAM_REVISION] = { 0x0F84, 0x00, 0x00, 0x00 },
@@ -142,6 +146,10 @@ static const struct reg_offset_data bam_v1_3_reg_info[] = {
[BAM_P_FIFO_SIZES] = { 0x1020, 0x00, 0x40, 0x00 },
};
+static const struct bam_device_data bam_v1_3_data = {
+ .reg_info = bam_v1_3_reg_info,
+};
+
static const struct reg_offset_data bam_v1_4_reg_info[] = {
[BAM_CTRL] = { 0x0000, 0x00, 0x00, 0x00 },
[BAM_REVISION] = { 0x0004, 0x00, 0x00, 0x00 },
@@ -171,6 +179,10 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
[BAM_P_FIFO_SIZES] = { 0x1820, 0x00, 0x1000, 0x00 },
};
+static const struct bam_device_data bam_v1_4_data = {
+ .reg_info = bam_v1_4_reg_info,
+};
+
static const struct reg_offset_data bam_v1_7_reg_info[] = {
[BAM_CTRL] = { 0x00000, 0x00, 0x00, 0x00 },
[BAM_REVISION] = { 0x01000, 0x00, 0x00, 0x00 },
@@ -200,6 +212,10 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
[BAM_P_FIFO_SIZES] = { 0x13820, 0x00, 0x1000, 0x00 },
};
+static const struct bam_device_data bam_v1_7_data = {
+ .reg_info = bam_v1_7_reg_info,
+};
+
static const struct reg_offset_data bam_v2_0_reg_info[] = {
[BAM_CTRL] = { 0x0000, 0x00, 0x00, 0x00 },
[BAM_REVISION] = { 0x1000, 0x00, 0x00, 0x00 },
@@ -229,6 +245,10 @@ static const struct reg_offset_data bam_v2_0_reg_info[] = {
[BAM_P_FIFO_SIZES] = { 0xC820, 0x00, 0x1000, 0x00 },
};
+static const struct bam_device_data bam_v2_0_data = {
+ .reg_info = bam_v2_0_reg_info,
+};
+
/* BAM CTRL */
#define BAM_SW_RST BIT(0)
#define BAM_EN BIT(1)
@@ -422,7 +442,7 @@ struct bam_device {
bool powered_remotely;
u32 active_channels;
- const struct reg_offset_data *layout;
+ const struct bam_device_data *dev_data;
struct clk *bamclk;
int irq;
@@ -440,7 +460,7 @@ struct bam_device {
static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe,
enum bam_reg reg)
{
- const struct reg_offset_data r = bdev->layout[reg];
+ const struct reg_offset_data r = bdev->dev_data->reg_info[reg];
return bdev->regs + r.base_offset +
r.pipe_mult * pipe +
@@ -1234,10 +1254,10 @@ static void bam_channel_init(struct bam_device *bdev, struct bam_chan *bchan,
}
static const struct of_device_id bam_of_match[] = {
- { .compatible = "qcom,bam-v1.3.0", .data = &bam_v1_3_reg_info },
- { .compatible = "qcom,bam-v1.4.0", .data = &bam_v1_4_reg_info },
- { .compatible = "qcom,bam-v1.7.0", .data = &bam_v1_7_reg_info },
- { .compatible = "qcom,bam-v2.0.0", .data = &bam_v2_0_reg_info },
+ { .compatible = "qcom,bam-v1.3.0", .data = &bam_v1_3_data },
+ { .compatible = "qcom,bam-v1.4.0", .data = &bam_v1_4_data },
+ { .compatible = "qcom,bam-v1.7.0", .data = &bam_v1_7_data },
+ { .compatible = "qcom,bam-v2.0.0", .data = &bam_v2_0_data },
{}
};
@@ -1261,7 +1281,7 @@ static int bam_dma_probe(struct platform_device *pdev)
return -ENODEV;
}
- bdev->layout = match->data;
+ bdev->dev_data = match->data;
bdev->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(bdev->regs))
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v21 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (3 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
` (8 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski,
Dmitry Baryshkov
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Extend the device match data with a flag indicating whether the IP
supports the BAM lock/unlock feature. Set it to true on BAM IP versions
1.4.0 and above.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/qcom/bam_dma.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 8ce0fe085c5fea6cc614edd692b5cfd264b94d5a..f3e713a5259c2c7c24cfdcec094814eb1202971a 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -115,6 +115,7 @@ struct reg_offset_data {
struct bam_device_data {
const struct reg_offset_data *reg_info;
+ bool pipe_lock_supported;
};
static const struct reg_offset_data bam_v1_3_reg_info[] = {
@@ -181,6 +182,7 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
static const struct bam_device_data bam_v1_4_data = {
.reg_info = bam_v1_4_reg_info,
+ .pipe_lock_supported = true,
};
static const struct reg_offset_data bam_v1_7_reg_info[] = {
@@ -214,6 +216,7 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
static const struct bam_device_data bam_v1_7_data = {
.reg_info = bam_v1_7_reg_info,
+ .pipe_lock_supported = true,
};
static const struct reg_offset_data bam_v2_0_reg_info[] = {
@@ -247,6 +250,7 @@ static const struct reg_offset_data bam_v2_0_reg_info[] = {
static const struct bam_device_data bam_v2_0_data = {
.reg_info = bam_v2_0_reg_info,
+ .pipe_lock_supported = true,
};
/* BAM CTRL */
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (4 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:17 ` sashiko-bot
2026-07-14 9:49 ` Stephan Gerhold
2026-07-13 13:01 ` [PATCH v21 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
` (7 subsequent siblings)
13 siblings, 2 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
Add support for BAM pipe locking. To that end: when starting DMA on an RX
channel - prepend the existing queue of issued descriptors with an
additional "dummy" command descriptor with the LOCK bit set. Once the
transaction is done (no more issued descriptors), issue one more dummy
descriptor with the UNLOCK bit.
We *must* wait until the transaction is signalled as done because we
must not perform any writes into config registers while the engine is
busy.
The dummy writes must be issued into a scratchpad register of the client
so provide a mechanism to communicate the right address via descriptor
metadata.
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/qcom/bam_dma.c | 191 +++++++++++++++++++++++++++++++++++++--
include/linux/dma/qcom_bam_dma.h | 14 +++
2 files changed, 198 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index f3e713a5259c2c7c24cfdcec094814eb1202971a..f08549ee3872eece85884606d6ee9e540ee688ca 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -28,11 +28,13 @@
#include <linux/clk.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
+#include <linux/dma/qcom_bam_dma.h>
#include <linux/dmaengine.h>
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/lockdep.h>
#include <linux/module.h>
#include <linux/of_address.h>
#include <linux/of_dma.h>
@@ -60,6 +62,8 @@ struct bam_desc_hw {
#define DESC_FLAG_EOB BIT(13)
#define DESC_FLAG_NWD BIT(12)
#define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)
struct bam_async_desc {
struct virt_dma_desc vd;
@@ -72,6 +76,11 @@ struct bam_async_desc {
struct bam_desc_hw *curr_desc;
+ /* BAM locking infrastructure */
+ bool is_lock_desc;
+ struct scatterlist lock_sg;
+ struct bam_cmd_element lock_ce;
+
/* list node for the desc in the bam_chan list of descriptors */
struct list_head desc_node;
enum dma_transfer_direction dir;
@@ -425,6 +434,11 @@ struct bam_chan {
struct list_head desc_list;
struct list_head node;
+
+ /* BAM locking infrastructure */
+ phys_addr_t scratchpad_addr;
+ enum dma_transfer_direction direction;
+ bool bam_locked;
};
static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
@@ -638,8 +652,10 @@ static void bam_free_chan(struct dma_chan *chan)
goto err;
}
- scoped_guard(spinlock_irqsave, &bchan->vc.lock)
+ scoped_guard(spinlock_irqsave, &bchan->vc.lock) {
bam_reset_channel(bchan);
+ bchan->bam_locked = false;
+ }
dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE, bchan->fifo_virt,
bchan->fifo_phys);
@@ -686,6 +702,35 @@ static int bam_slave_config(struct dma_chan *chan,
return 0;
}
+static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
+{
+ struct bam_chan *bchan = to_bam_chan(desc->chan);
+ const struct bam_device_data *bdata = bchan->bdev->dev_data;
+ struct bam_desc_metadata *metadata = data;
+
+ if (!data)
+ return -EINVAL;
+
+ if (!bdata->pipe_lock_supported)
+ /*
+ * The client wants to use locking but this BAM version doesn't
+ * support it. Don't return an error here as this will stop the
+ * client from using DMA at all for no reason.
+ */
+ return 0;
+
+ guard(spinlock_irqsave)(&bchan->vc.lock);
+
+ bchan->scratchpad_addr = metadata->scratchpad_addr;
+ bchan->direction = metadata->direction;
+
+ return 0;
+}
+
+static const struct dma_descriptor_metadata_ops bam_metadata_ops = {
+ .attach = bam_metadata_attach,
+};
+
/**
* bam_prep_slave_sg - Prep slave sg transaction
*
@@ -702,6 +747,7 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
void *context)
{
struct bam_chan *bchan = to_bam_chan(chan);
+ struct dma_async_tx_descriptor *tx_desc;
struct bam_device *bdev = bchan->bdev;
struct bam_async_desc *async_desc;
struct scatterlist *sg;
@@ -757,7 +803,10 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
} while (remainder > 0);
}
- return vchan_tx_prep(&bchan->vc, &async_desc->vd, flags);
+ tx_desc = vchan_tx_prep(&bchan->vc, &async_desc->vd, flags);
+ tx_desc->metadata_ops = &bam_metadata_ops;
+
+ return tx_desc;
}
/**
@@ -802,6 +851,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
}
vchan_get_all_descriptors(&bchan->vc, &head);
+ bchan->bam_locked = false;
}
vchan_dma_desc_free_list(&bchan->vc, &head);
@@ -859,6 +909,15 @@ static int bam_resume(struct dma_chan *chan)
return 0;
}
+static void bam_dma_free_lock_desc(struct virt_dma_desc *vd)
+{
+ struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
+ struct dma_chan *chan = vd->tx.chan;
+
+ dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
+ kfree(async_desc);
+}
+
/**
* process_channel_irqs - processes the channel interrupts
* @bdev: bam controller
@@ -870,6 +929,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
{
u32 i, srcs, pipe_stts, offset, avail;
struct bam_async_desc *async_desc, *tmp;
+ struct bam_desc_hw *hdesc;
srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
@@ -919,13 +979,20 @@ static u32 process_channel_irqs(struct bam_device *bdev)
* push back to front of desc_issued so that
* it gets restarted by the work queue.
*/
+
+ list_del(&async_desc->desc_node);
if (!async_desc->num_desc) {
- vchan_cookie_complete(&async_desc->vd);
+ hdesc = async_desc->desc;
+ u16 flags = le16_to_cpu(hdesc->flags);
+
+ if (async_desc->is_lock_desc)
+ bam_dma_free_lock_desc(&async_desc->vd);
+ else
+ vchan_cookie_complete(&async_desc->vd);
} else {
list_add(&async_desc->vd.node,
&bchan->vc.desc_issued);
}
- list_del(&async_desc->desc_node);
}
}
@@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
bchan->reconfigure = 0;
}
+static struct bam_async_desc *
+bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
+{
+ struct dma_chan *chan = &bchan->vc.chan;
+ struct bam_async_desc *async_desc;
+ struct bam_desc_hw *desc;
+ struct virt_dma_desc *vd;
+ struct virt_dma_chan *vc;
+ unsigned int mapped;
+
+ async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
+ if (!async_desc) {
+ dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ sg_init_table(&async_desc->lock_sg, 1);
+
+ async_desc->num_desc = 1;
+ async_desc->curr_desc = async_desc->desc;
+ async_desc->dir = DMA_MEM_TO_DEV;
+ async_desc->is_lock_desc = true;
+
+ desc = async_desc->desc;
+
+ bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
+ sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
+
+ mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
+ if (!mapped) {
+ kfree(async_desc);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
+ desc->addr = sg_dma_address(&async_desc->lock_sg);
+ desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));
+
+ vc = &bchan->vc;
+ vd = &async_desc->vd;
+
+ dma_async_tx_descriptor_init(&vd->tx, &vc->chan);
+ vd->tx.flags = DMA_PREP_CMD;
+ vd->tx_result.result = DMA_TRANS_NOERROR;
+ vd->tx_result.residue = 0;
+
+ return async_desc;
+}
+
+static int bam_setup_pipe_lock(struct bam_chan *bchan)
+{
+ const struct bam_device_data *bdata = bchan->bdev->dev_data;
+ struct bam_async_desc *lock_desc, *unlock_desc;
+
+ lockdep_assert_held(&bchan->vc.lock);
+
+ if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
+ bchan->direction != DMA_MEM_TO_DEV)
+ return 0;
+
+ /*
+ * Allocate both the LOCK and the UNLOCK descriptors up-front so the
+ * operation is all-or-nothing: if either allocation fails we free both
+ * and run the sequence unlocked rather than leave the pipe locked with
+ * no matching UNLOCK.
+ *
+ * Both are queued in-band around the currently issued work: the LOCK is
+ * prepended so it enters the FIFO first, the UNLOCK is appended so it is
+ * the last descriptor of the sequence. They are loaded together with the
+ * payload in a single operation so the engine executes LOCK, the work
+ * and UNLOCK as one ordered batch.
+ */
+ lock_desc = bam_make_lock_desc(bchan, DESC_FLAG_LOCK);
+ if (IS_ERR(lock_desc))
+ return PTR_ERR(lock_desc);
+
+ unlock_desc = bam_make_lock_desc(bchan, DESC_FLAG_UNLOCK);
+ if (IS_ERR(unlock_desc)) {
+ bam_dma_free_lock_desc(&lock_desc->vd);
+ return PTR_ERR(unlock_desc);
+ }
+
+ list_add(&lock_desc->vd.node, &bchan->vc.desc_issued);
+ list_add_tail(&unlock_desc->vd.node, &bchan->vc.desc_issued);
+ bchan->bam_locked = true;
+
+ return 0;
+}
+
/**
* bam_start_dma - start next transaction
* @bchan: bam dma channel
*/
static void bam_start_dma(struct bam_chan *bchan)
{
- struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
+ struct virt_dma_desc *vd;
struct bam_device *bdev = bchan->bdev;
struct bam_async_desc *async_desc = NULL;
struct bam_desc_hw *desc;
@@ -1064,6 +1220,7 @@ static void bam_start_dma(struct bam_chan *bchan)
lockdep_assert_held(&bchan->vc.lock);
+ vd = vchan_next_desc(&bchan->vc);
if (!vd)
return;
@@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
return;
while (vd && !IS_BUSY(bchan)) {
+ /*
+ * Open a LOCK/UNLOCK bracket around each fresh sequence.
+ * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
+ * already have bam_locked set and must not trigger a second pair.
+ */
+ if (!bchan->bam_locked &&
+ !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
+ ret = bam_setup_pipe_lock(bchan);
+ if (ret == 0 && bchan->bam_locked)
+ vd = vchan_next_desc(&bchan->vc);
+ }
+
list_del(&vd->node);
async_desc = container_of(vd, struct bam_async_desc, vd);
@@ -1133,6 +1302,10 @@ static void bam_start_dma(struct bam_chan *bchan)
bchan->tail += async_desc->xfer_len;
bchan->tail %= MAX_DESCRIPTORS;
list_add_tail(&async_desc->desc_node, &bchan->desc_list);
+
+ if (async_desc->is_lock_desc &&
+ (le16_to_cpu(async_desc->desc->flags) & DESC_FLAG_UNLOCK))
+ bchan->bam_locked = false;
}
/* ensure descriptor writes and dma start not reordered */
@@ -1191,8 +1364,11 @@ static void bam_issue_pending(struct dma_chan *chan)
*/
static void bam_dma_free_desc(struct virt_dma_desc *vd)
{
- struct bam_async_desc *async_desc = container_of(vd,
- struct bam_async_desc, vd);
+ struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
+ struct dma_chan *chan = vd->tx.chan;
+
+ if (async_desc->is_lock_desc)
+ dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
kfree(async_desc);
}
@@ -1384,6 +1560,7 @@ static int bam_dma_probe(struct platform_device *pdev)
bdev->common.device_terminate_all = bam_dma_terminate_all;
bdev->common.device_issue_pending = bam_issue_pending;
bdev->common.device_tx_status = bam_tx_status;
+ bdev->common.desc_metadata_modes = DESC_METADATA_CLIENT;
bdev->common.dev = bdev->dev;
ret = dma_async_device_register(&bdev->common);
diff --git a/include/linux/dma/qcom_bam_dma.h b/include/linux/dma/qcom_bam_dma.h
index 68fc0e643b1b97fe4520d5878daa322b81f4f559..a2594264b0f58c4b2b1c85e243cad0d5669c26dc 100644
--- a/include/linux/dma/qcom_bam_dma.h
+++ b/include/linux/dma/qcom_bam_dma.h
@@ -6,6 +6,8 @@
#ifndef _QCOM_BAM_DMA_H
#define _QCOM_BAM_DMA_H
+#include <linux/dmaengine.h>
+
#include <asm/byteorder.h>
/*
@@ -34,6 +36,18 @@ enum bam_command_type {
BAM_READ_COMMAND,
};
+/**
+ * struct bam_desc_metadata - DMA descriptor metadata specific to the BAM driver.
+ *
+ * @scratchpad_addr: Physical address to use for dummy write operations when
+ * queuing command descriptors with LOCK/UNLOCK bits set.
+ * @direction: Transfer direction of this channel.
+ */
+struct bam_desc_metadata {
+ phys_addr_t scratchpad_addr;
+ enum dma_transfer_direction direction;
+};
+
/*
* prep_bam_ce_le32 - Wrapper function to prepare a single BAM command
* element with the data already in le32 format.
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-13 13:01 ` [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
@ 2026-07-13 13:17 ` sashiko-bot
2026-07-14 9:49 ` Stephan Gerhold
1 sibling, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:17 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] `dma_map_sg()` and `dma_unmap_sg()` incorrectly use the client device (`chan->slave`) instead of the BAM DMA controller device.
- [High] Descriptor-specific metadata is incorrectly stored in the global channel state, causing a state leak and potential memory corruption.
- [High] Missing endianness conversion (`cpu_to_le32`) when assigning the physical address to the BAM hardware descriptor.
- [High] Silent fallback to unlocked operation if memory allocation fails for pipe lock descriptors.
- [Medium] Mapping an embedded structure member (`lock_ce`) for DMA violates DMA API cacheline alignment rules.
- [Medium] `bam_metadata_attach` casts and reads the user-provided `data` buffer without validating `len`.
- [Medium] Silent truncation of a physical address to a 24-bit offset.
--
--- Patch [6]: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
commit 6c206bd6717df7ed7061a889f4998d1b7849b74d
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
dmaengine: qcom: bam_dma: add support for BAM locking
This commit adds support for BAM pipe locking by prepending and appending
dummy command descriptors with LOCK/UNLOCK bits. It uses a client-provided
scratchpad address passed via descriptor metadata.
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2..f08549ee3872e 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -72,6 +76,11 @@ struct bam_async_desc {
>
> struct bam_desc_hw *curr_desc;
>
> + /* BAM locking infrastructure */
> + bool is_lock_desc;
> + struct scatterlist lock_sg;
> + struct bam_cmd_element lock_ce;
[Severity: Medium]
Is it safe to map lock_ce for DMA here without cacheline alignment?
Since lock_ce is embedded inside the dynamically allocated
struct bam_async_desc, it shares a cacheline with heavily accessed CPU members
like desc_node.
Would this violate DMA API cacheline alignment rules and risk memory
corruption during cache maintenance operations on non-coherent architectures?
[ ... ]
> @@ -686,6 +702,35 @@ static int bam_slave_config(struct dma_chan *chan,
> return 0;
> }
>
> +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> + struct bam_chan *bchan = to_bam_chan(desc->chan);
> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
> + struct bam_desc_metadata *metadata = data;
> +
> + if (!data)
> + return -EINVAL;
[Severity: Medium]
Does this function need to validate the length of the data buffer?
If a caller accidentally provides a short metadata buffer, could accessing
metadata->scratchpad_addr result in an out-of-bounds memory read?
> +
> + if (!bdata->pipe_lock_supported)
> + /*
> + * The client wants to use locking but this BAM version doesn't
> + * support it. Don't return an error here as this will stop the
> + * client from using DMA at all for no reason.
> + */
> + return 0;
> +
> + guard(spinlock_irqsave)(&bchan->vc.lock);
> +
> + bchan->scratchpad_addr = metadata->scratchpad_addr;
> + bchan->direction = metadata->direction;
[Severity: High]
Is it intentional to write transaction-specific metadata directly into the
global channel state here?
If a client queues multiple descriptors with different metadata, won't the
latest call overwrite the state for all pending descriptors?
Also, since bchan->scratchpad_addr is not cleared in bam_free_chan(), if a
new client acquires the channel, could bam_setup_pipe_lock() read a stale
scratchpad_addr and silently inject dummy locks writing to the previous
client's memory?
> +
> + return 0;
> +}
[ ... ]
> @@ -859,6 +909,15 @@ static int bam_resume(struct dma_chan *chan)
> return 0;
> }
>
> +static void bam_dma_free_lock_desc(struct virt_dma_desc *vd)
> +{
> + struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> + struct dma_chan *chan = vd->tx.chan;
> +
> + dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
[Severity: High]
Should this unmap use the BAM DMA controller device instead of chan->slave?
Because the BAM DMA controller is the bus master reading the command element,
using the client device's domain might result in an invalid IOVA or IOMMU
translation faults.
> + kfree(async_desc);
> +}
[ ... ]
> @@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
> bchan->reconfigure = 0;
> }
>
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> + struct dma_chan *chan = &bchan->vc.chan;
> + struct bam_async_desc *async_desc;
> + struct bam_desc_hw *desc;
> + struct virt_dma_desc *vd;
> + struct virt_dma_chan *vc;
> + unsigned int mapped;
> +
> + async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> + if (!async_desc) {
> + dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + sg_init_table(&async_desc->lock_sg, 1);
> +
> + async_desc->num_desc = 1;
> + async_desc->curr_desc = async_desc->desc;
> + async_desc->dir = DMA_MEM_TO_DEV;
> + async_desc->is_lock_desc = true;
> +
> + desc = async_desc->desc;
> +
> + bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
[Severity: Medium]
Does bam_prep_ce_le32() safely handle a full phys_addr_t?
The struct defines scratchpad_addr as a phys_addr_t, but bam_prep_ce_le32()
appears to mask the address with 0xffffff, discarding bits above the lowest
24 bits. If a client provides a 32-bit or 64-bit physical address, will the
dummy write target the wrong address?
> + sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> + mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
[Severity: High]
Similar to the unmap path, should this mapping use the BAM DMA controller
device rather than the client device (chan->slave)?
> + if (!mapped) {
> + kfree(async_desc);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> + desc->addr = sg_dma_address(&async_desc->lock_sg);
[Severity: High]
Is a cpu_to_le32() conversion missing here when assigning to desc->addr?
Since sg_dma_address() returns a dma_addr_t in host CPU byte order, writing
this directly to the __le32 addr field could result in a byte-swapped address
on Big-Endian systems, causing the BAM DMA engine to read from an invalid
IOVA.
> + desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));
[ ... ]
> @@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
> return;
>
> while (vd && !IS_BUSY(bchan)) {
> + /*
> + * Open a LOCK/UNLOCK bracket around each fresh sequence.
> + * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
> + * already have bam_locked set and must not trigger a second pair.
> + */
> + if (!bchan->bam_locked &&
> + !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
> + ret = bam_setup_pipe_lock(bchan);
> + if (ret == 0 && bchan->bam_locked)
> + vd = vchan_next_desc(&bchan->vc);
> + }
[Severity: High]
If bam_setup_pipe_lock() fails and returns an error code, is it safe to
proceed to queue the DMA transaction unlocked?
Since the commit message states the lock is strictly required to prevent
config writes while the engine is busy, could silently ignoring the failure
allow unpredictable hardware behavior?
> +
> list_del(&vd->node);
[ ... ]
> @@ -1191,8 +1364,11 @@ static void bam_issue_pending(struct dma_chan *chan)
> */
> static void bam_dma_free_desc(struct virt_dma_desc *vd)
> {
> - struct bam_async_desc *async_desc = container_of(vd,
> - struct bam_async_desc, vd);
> + struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> + struct dma_chan *chan = vd->tx.chan;
> +
> + if (async_desc->is_lock_desc)
> + dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
[Severity: High]
As noted earlier, should this unmap use the BAM DMA controller device instead
of chan->slave?
>
> kfree(async_desc);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-13 13:01 ` [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
2026-07-13 13:17 ` sashiko-bot
@ 2026-07-14 9:49 ` Stephan Gerhold
1 sibling, 0 replies; 26+ messages in thread
From: Stephan Gerhold @ 2026-07-14 9:49 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Bjorn Andersson, Peter Ujfalusi,
Michal Simek, Frank Li, Andy Gross, Neil Armstrong, dmaengine,
linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski
On Mon, Jul 13, 2026 at 03:01:07PM +0200, Bartosz Golaszewski wrote:
> Add support for BAM pipe locking. To that end: when starting DMA on an RX
> channel - prepend the existing queue of issued descriptors with an
> additional "dummy" command descriptor with the LOCK bit set. Once the
> transaction is done (no more issued descriptors), issue one more dummy
> descriptor with the UNLOCK bit.
>
> We *must* wait until the transaction is signalled as done because we
> must not perform any writes into config registers while the engine is
> busy.
>
> The dummy writes must be issued into a scratchpad register of the client
> so provide a mechanism to communicate the right address via descriptor
> metadata.
>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Thanks for the fixes. The lock/unlock sequence looks good to me now,
I commented on a couple of minor things below that would be good to fix
(some of them are also reported by Sashiko).
> ---
> drivers/dma/qcom/bam_dma.c | 191 +++++++++++++++++++++++++++++++++++++--
> include/linux/dma/qcom_bam_dma.h | 14 +++
> 2 files changed, 198 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2c7c24cfdcec094814eb1202971a..f08549ee3872eece85884606d6ee9e540ee688ca 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> [...]
> @@ -686,6 +702,35 @@ static int bam_slave_config(struct dma_chan *chan,
> return 0;
> }
>
> +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> + struct bam_chan *bchan = to_bam_chan(desc->chan);
> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
> + struct bam_desc_metadata *metadata = data;
> +
> + if (!data)
Doesn't really matter much, but since the parameter exists you might as
well add
&& len == sizeof(*metadata)
here to be sure.
> + return -EINVAL;
> +
> + if (!bdata->pipe_lock_supported)
> + /*
> + * The client wants to use locking but this BAM version doesn't
> + * support it. Don't return an error here as this will stop the
> + * client from using DMA at all for no reason.
> + */
> + return 0;
> +
> + guard(spinlock_irqsave)(&bchan->vc.lock);
> +
> + bchan->scratchpad_addr = metadata->scratchpad_addr;
> + bchan->direction = metadata->direction;
> +
> + return 0;
> +}
> +
> +static const struct dma_descriptor_metadata_ops bam_metadata_ops = {
> + .attach = bam_metadata_attach,
> +};
I'm not sure if we have discussed this before, but could we avoid
re-programming the scratchpad_addr all the time by placing it into
struct dma_slave_config -> peripheral_config? It still feels awkward to
me to place a global constant configuration value into per-descriptor
metadata.
> +
> /**
> * bam_prep_slave_sg - Prep slave sg transaction
> *
> [...]
> @@ -802,6 +851,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
> }
>
> vchan_get_all_descriptors(&bchan->vc, &head);
> + bchan->bam_locked = false;
I wonder about the implications of this. If the LOCK descriptor was
already processed, will we cause a deadlock if we never submit the
UNLOCK descriptor? Or I guess bam_reset_channel() might reset the lock
as well?
> }
>
> vchan_dma_desc_free_list(&bchan->vc, &head);
> [...]
> @@ -870,6 +929,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
> {
> u32 i, srcs, pipe_stts, offset, avail;
> struct bam_async_desc *async_desc, *tmp;
> + struct bam_desc_hw *hdesc;
>
> srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
>
> @@ -919,13 +979,20 @@ static u32 process_channel_irqs(struct bam_device *bdev)
> * push back to front of desc_issued so that
> * it gets restarted by the work queue.
> */
> +
> + list_del(&async_desc->desc_node);
> if (!async_desc->num_desc) {
> - vchan_cookie_complete(&async_desc->vd);
> + hdesc = async_desc->desc;
> + u16 flags = le16_to_cpu(hdesc->flags);
Is this unused? Also a bit odd to have hdesc declared outside of the
loop and flags declared inside.
> +
> + if (async_desc->is_lock_desc)
> + bam_dma_free_lock_desc(&async_desc->vd);
> + else
> + vchan_cookie_complete(&async_desc->vd);
> } else {
> list_add(&async_desc->vd.node,
> &bchan->vc.desc_issued);
> }
> - list_del(&async_desc->desc_node);
> }
> }
>
> @@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
> bchan->reconfigure = 0;
> }
>
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> + struct dma_chan *chan = &bchan->vc.chan;
> + struct bam_async_desc *async_desc;
> + struct bam_desc_hw *desc;
> + struct virt_dma_desc *vd;
> + struct virt_dma_chan *vc;
> + unsigned int mapped;
> +
> + async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> + if (!async_desc) {
> + dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + sg_init_table(&async_desc->lock_sg, 1);
> +
> + async_desc->num_desc = 1;
> + async_desc->curr_desc = async_desc->desc;
> + async_desc->dir = DMA_MEM_TO_DEV;
> + async_desc->is_lock_desc = true;
> +
> + desc = async_desc->desc;
> +
> + bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
> + sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> + mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
I agree with Sashiko that mapping using the BAM device
(bchan->bdev->dev) would be more precise, since the BAM will be reading
the descriptor. (It doesn't matter in practice since both BAM and
consumer usually have the same IOMMUs defined.)
> + if (!mapped) {
> + kfree(async_desc);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> + desc->addr = sg_dma_address(&async_desc->lock_sg);
cpu_to_le32()
> + desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));
> +
> + vc = &bchan->vc;
> + vd = &async_desc->vd;
> +
> + dma_async_tx_descriptor_init(&vd->tx, &vc->chan);
> + vd->tx.flags = DMA_PREP_CMD;
> + vd->tx_result.result = DMA_TRANS_NOERROR;
> + vd->tx_result.residue = 0;
> +
> + return async_desc;
> +}
> +
> [...]
> @@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
> return;
>
> while (vd && !IS_BUSY(bchan)) {
> + /*
> + * Open a LOCK/UNLOCK bracket around each fresh sequence.
> + * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
> + * already have bam_locked set and must not trigger a second pair.
> + */
> + if (!bchan->bam_locked &&
> + !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
Do we need the ->is_lock_desc check here? Looks redundant to me.
> + ret = bam_setup_pipe_lock(bchan);
> + if (ret == 0 && bchan->bam_locked)
> + vd = vchan_next_desc(&bchan->vc);
Do we want some error handling here? If there is an error, this will
silently continue queuing everything without any locking.
Thanks,
Stephan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 07/14] crypto: qce - Cancel work on device detach
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (5 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:15 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header Bartosz Golaszewski
` (6 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
The workqueue is setup in probe() but never cancelled on error or in
remove(). Set up a devres action to clean it up. We need to move the
initialization earlier as we don't want to cancel the work before any
outstanding DMA transfer is terminated. Make sure we do terminate all
transfers in qce_dma_release() devres action.
Fixes: eb7986e5e14d ("crypto: qce - convert tasklet to workqueue")
Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=7
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/core.c | 13 ++++++++++++-
drivers/crypto/qce/dma.c | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index ac74f69914d6175b39ccde43f16269570fbcf715..b52a26ffff5ee733adcf4e8cf8bef75018dfa63e 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -185,6 +185,13 @@ static int qce_check_version(struct qce_device *qce)
return 0;
}
+static void qce_cancel_work(void *data)
+{
+ struct work_struct *work = data;
+
+ cancel_work_sync(work);
+}
+
static int qce_crypto_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -226,6 +233,11 @@ static int qce_crypto_probe(struct platform_device *pdev)
if (ret)
return ret;
+ INIT_WORK(&qce->done_work, qce_req_done_work);
+ ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
+ if (ret)
+ return ret;
+
ret = devm_qce_dma_request(qce->dev, &qce->dma);
if (ret)
return ret;
@@ -238,7 +250,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
if (ret)
return ret;
- INIT_WORK(&qce->done_work, qce_req_done_work);
crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
qce->async_req_enqueue = qce_async_request_enqueue;
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 68cafd4741ad3d91906d39e817fc7873b028d498..7ec9d72fd690fb17e03ade7efe3cc522fb47e1ac 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -13,6 +13,8 @@ static void qce_dma_release(void *data)
{
struct qce_dma_data *dma = data;
+ dmaengine_terminate_sync(dma->txchan);
+ dmaengine_terminate_sync(dma->rxchan);
dma_release_channel(dma->txchan);
dma_release_channel(dma->rxchan);
kfree(dma->result_buf);
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 07/14] crypto: qce - Cancel work on device detach
2026-07-13 13:01 ` [PATCH v21 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
@ 2026-07-13 13:15 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:15 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, vkoul, dmaengine
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Devres ordering introduces Use-After-Free and Mutex Use-After-Destroy vulnerabilities because the workqueue is canceled after its required resources (mutex and DMA channels) are freed.
- [High] Terminating DMA and cancelling the workqueue during unbind without explicitly completing pending crypto requests causes callers to hang indefinitely.
--
--- Patch [7]: [PATCH v21 07/14] crypto: qce - Cancel work on device detach ---
commit 4cbda8145d4c325fc75ff4841164bf36bd06014c
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
crypto: qce - Cancel work on device detach
This commit introduces a devres action to clean up the workqueue on
device detach. It moves the initialization earlier to ensure outstanding
DMA transfers are terminated before the work is canceled.
Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=7
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index ac74f69914d61..b52a26ffff5ee 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
[ ... ]
> @@ -226,6 +233,11 @@ static int qce_crypto_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + INIT_WORK(&qce->done_work, qce_req_done_work);
> + ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
[Severity: High]
Could this devres ordering introduce a use-after-free and mutex
use-after-destroy during driver unbind?
Because devres cleanup executes in LIFO order, registering qce_cancel_work()
here before mutex and DMA initialization means it will execute last during
cleanup.
When the driver unbinds, qce_dma_release() and mutex destruction will occur
before cancel_work_sync() runs. If the worker thread qce_req_done_work() is
scheduled concurrently, could it attempt to lock the destroyed mutex and use
the already freed DMA channels and result buffers?
> + if (ret)
> + return ret;
> +
> ret = devm_qce_dma_request(qce->dev, &qce->dma);
[ ... ]
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 68cafd4741ad3..7ec9d72fd690f 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -13,6 +13,8 @@ static void qce_dma_release(void *data)
> {
> struct qce_dma_data *dma = data;
>
> + dmaengine_terminate_sync(dma->txchan);
> + dmaengine_terminate_sync(dma->rxchan);
[Severity: High]
Will terminating the DMA transfers here cause callers to hang indefinitely?
By aborting the active DMA transfer during unbind, the normal completion
callback won't fire, so qce->done_work is not scheduled. The driver then
cancels the workqueue via qce_cancel_work(), but it does not appear to
explicitly complete the active request (qce->req) or drain qce->queue.
Will callers waiting on these requests block forever when the device is
unbound?
> dma_release_channel(dma->txchan);
> dma_release_channel(dma->rxchan);
> kfree(dma->result_buf);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (6 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:18 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
` (5 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
The header defines a struct embedding struct crypto_queue whose size
needs to be known and which is defined in crypto/algapi.h. Move the
inclusion from core.c to core.h.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/core.c | 1 -
drivers/crypto/qce/core.h | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index b52a26ffff5ee733adcf4e8cf8bef75018dfa63e..dd860435d2c47a608c82cc2686583a44ff96c889 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -12,7 +12,6 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/types.h>
-#include <crypto/algapi.h>
#include <crypto/internal/hash.h>
#include "core.h"
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index eb6fa7a8b64a81daf9ad5304a3ae4e5e597a70b8..f092ce2d3b04a936a37805c20ac5ba78d8fdd2df 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -8,6 +8,7 @@
#include <linux/mutex.h>
#include <linux/workqueue.h>
+#include <crypto/algapi.h>
#include "dma.h"
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header
2026-07-13 13:01 ` [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header Bartosz Golaszewski
@ 2026-07-13 13:18 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:18 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, dmaengine, vkoul
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] bam_make_lock_desc maps the command descriptor using the slave device pointer instead of the DMA controller device, which will cause IOMMU faults or unresolvable physical addresses.
- [High] LIFO devres execution order in qce_crypto_probe causes the DMA channels to be freed before the workqueue is cancelled, leading to use-after-free if the workqueue executes concurrently.
- [High] BAM pipe locking is restricted to TX channels (Memory-to-Device), which directly contradicts the commit message stating it applies to RX channels.
Pre-existing issues:
- [High] Writing to BAM registers after forcefully suspending the device causes an unclocked access crash.
--
--- Patch [8]: [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header ---
commit 2564fb6b194c2eaa4eed81579eda988d138343a5
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Include algapi.h in the core.h header
The header defines a struct embedding struct crypto_queue whose size
needs to be known and which is defined in crypto/algapi.h. Move the
inclusion from core.c to core.h.
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index ac74f69914d61..dd860435d2c47 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
[ ... ]
> @@ -226,6 +232,11 @@ static int qce_crypto_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + INIT_WORK(&qce->done_work, qce_req_done_work);
> + ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
[Severity: High]
Does registering the workqueue cancellation here cause a use-after-free
during device teardown?
Since devres actions are executed in reverse order (LIFO),
devm_qce_dma_request() registers qce_dma_release() which will run first
and release the DMA channels.
Only after that does qce_cancel_work() execute, meaning if
qce_req_done_work() runs concurrently during this window, it could
access the already freed DMA channels.
> + if (ret)
> + return ret;
> +
> ret = devm_qce_dma_request(qce->dev, &qce->dma);
> if (ret)
> return ret;
[ ... ]
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1bb26af0405f3..f08549ee3872e 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
[ ... ]
> @@ -1021,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
> bchan->reconfigure = 0;
> }
>
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> + struct dma_chan *chan = &bchan->vc.chan;
> + struct bam_async_desc *async_desc;
> + struct bam_desc_hw *desc;
> + struct virt_dma_desc *vd;
> + struct virt_dma_chan *vc;
> + unsigned int mapped;
> +
> + async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> + if (!async_desc) {
> + dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + sg_init_table(&async_desc->lock_sg, 1);
> +
> + async_desc->num_desc = 1;
> + async_desc->curr_desc = async_desc->desc;
> + async_desc->dir = DMA_MEM_TO_DEV;
> + async_desc->is_lock_desc = true;
> +
> + desc = async_desc->desc;
> +
> + bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
> + sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> + mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
[Severity: High]
Does mapping the command descriptor using chan->slave instead of the DMA
controller device cause IOMMU faults?
The memory is mapped into the peripheral's IOMMU domain rather than the
DMA controller's domain, which could lead to DMA faults when the controller
tries to read the descriptor.
> + if (!mapped) {
> + kfree(async_desc);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> + desc->addr = sg_dma_address(&async_desc->lock_sg);
> + desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));
[ ... ]
> +static int bam_setup_pipe_lock(struct bam_chan *bchan)
> +{
> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
> + struct bam_async_desc *lock_desc, *unlock_desc;
> +
> + lockdep_assert_held(&bchan->vc.lock);
> +
> + if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
> + bchan->direction != DMA_MEM_TO_DEV)
[Severity: High]
Does restricting this hardware locking workaround to DMA_MEM_TO_DEV (TX)
channels contradict the intent to apply it to RX channels?
The commit message states "when starting DMA on an RX channel - prepend
the existing queue...", but this check prevents the locking from applying
to DMA_DEV_TO_MEM (RX) channels.
> + return 0;
> +
> + /*
> + * Allocate both the LOCK and the UNLOCK descriptors up-front so the
> + * operation is all-or-nothing: if either allocation fails we free both
[ ... ]
> @@ -1401,6 +1603,8 @@ static void bam_dma_remove(struct platform_device *pdev)
> struct bam_device *bdev = platform_get_drvdata(pdev);
> u32 i;
>
> + free_irq(bdev->irq, bdev);
> +
> pm_runtime_force_suspend(&pdev->dev);
>
> of_dma_controller_free(pdev->dev.of_node);
> @@ -1409,8 +1613,6 @@ static void bam_dma_remove(struct platform_device *pdev)
> /* mask all interrupts for this execution environment */
> writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
[Severity: High]
This is a pre-existing issue, but does writing to BAM_IRQ_SRCS_MSK_EE after
pm_runtime_force_suspend() has disabled the clock cause a synchronous
external abort?
Since the clock is turned off by the suspend callback, this unclocked access
can trigger a fatal bus error.
> - devm_free_irq(bdev->dev, bdev->irq, bdev);
> -
> for (i = 0; i < bdev->num_channels; i++) {
> bam_dma_terminate_all(&bdev->channels[i].vc.chan);
> tasklet_kill(&bdev->channels[i].vc.task);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=8
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (7 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:14 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() Bartosz Golaszewski
` (4 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
It's unclear what the purpose of this field is. It has been here since
the initial commit but without any explanation. The driver works fine
without it. We still keep allocating more space in the result buffer, we
just don't need to store its address. While at it: move the
QCE_IGNORE_BUF_SZ definition into dma.c as it's not used outside of this
compilation unit.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/dma.c | 4 ++--
drivers/crypto/qce/dma.h | 2 --
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 7ec9d72fd690fb17e03ade7efe3cc522fb47e1ac..d1daa229361aa74da5d3d7bfe1bc8ab189761e38 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -9,6 +9,8 @@
#include "dma.h"
+#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
+
static void qce_dma_release(void *data)
{
struct qce_dma_data *dma = data;
@@ -43,8 +45,6 @@ int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
goto error_nomem;
}
- dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
-
return devm_add_action_or_reset(dev, qce_dma_release, dma);
error_nomem:
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 31629185000e12242fa07c2cc08b95fcbd5d4b8c..fc337c435cd14917bdfb99febcf9119275afdeba 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -23,7 +23,6 @@ struct qce_result_dump {
u32 status2;
};
-#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
#define QCE_RESULT_BUF_SZ \
ALIGN(sizeof(struct qce_result_dump), QCE_BAM_BURST_SIZE)
@@ -31,7 +30,6 @@ struct qce_dma_data {
struct dma_chan *txchan;
struct dma_chan *rxchan;
struct qce_result_dump *result_buf;
- void *ignore_buf;
};
int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf
2026-07-13 13:01 ` [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
@ 2026-07-13 13:14 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:14 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, vkoul, dmaengine
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-After-Free of DMA resources during device teardown due to incorrect devres ordering.
--
--- Patch [9]: [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf ---
commit 65a70a3ae148fcccff0fe900df224577ddcc1190
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Remove unused ignore_buf
This commit removes the unused ignore_buf field from qce_dma_data and
relocates the QCE_IGNORE_BUF_SZ macro since it is only used within a
single compilation unit.
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 7ec9d72fd690f..d1daa229361aa 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> @@ -43,8 +45,6 @@ int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
> goto error_nomem;
> }
>
> - dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
> -
> return devm_add_action_or_reset(dev, qce_dma_release, dma);
[Severity: High]
Does this devres registration order create a use-after-free risk for
dma->result_buf and the DMA channels during device unbind?
In drivers/crypto/qce/core.c:qce_crypto_probe(), the devres action to cancel
the background workqueue (qce_cancel_work) is registered before the DMA
resources are requested here.
Because devres executes actions in reverse order, qce_dma_release() will run
before the workqueue is actually canceled.
During device unbind, qce_dma_release() frees the DMA resources:
drivers/crypto/qce/dma.c:qce_dma_release() {
...
dma_release_channel(dma->txchan);
dma_release_channel(dma->rxchan);
kfree(dma->result_buf);
}
If the workqueue is actively processing requests or starts running before
qce_cancel_work() executes, it will process the next crypto request in the
queue and call qce_skcipher_async_req_handle().
This function will then map the freed result_buf and attempt to use the already
released DMA channels, which can lead to memory corruption or a crash.
>
> error_nomem:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=9
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request()
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (8 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
` (3 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
This function can extract all the information it needs from struct
qce_device alone so simplify its arguments. This is done in preparation
for adding support for register I/O over DMA which will require
accessing even more fields from struct qce_device.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/core.c | 2 +-
drivers/crypto/qce/dma.c | 5 ++++-
drivers/crypto/qce/dma.h | 4 +++-
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index dd860435d2c47a608c82cc2686583a44ff96c889..aa4a0b17749081f1ad653424bc265ee81e348e15 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -237,7 +237,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = devm_qce_dma_request(qce->dev, &qce->dma);
+ ret = devm_qce_dma_request(qce);
if (ret)
return ret;
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index d1daa229361aa74da5d3d7bfe1bc8ab189761e38..d60efb5c26d88f8b0259b1dccc8724d0f75571c6 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -7,6 +7,7 @@
#include <linux/dmaengine.h>
#include <crypto/scatterwalk.h>
+#include "core.h"
#include "dma.h"
#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
@@ -22,8 +23,10 @@ static void qce_dma_release(void *data)
kfree(dma->result_buf);
}
-int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
+int devm_qce_dma_request(struct qce_device *qce)
{
+ struct qce_dma_data *dma = &qce->dma;
+ struct device *dev = qce->dev;
int ret;
dma->txchan = dma_request_chan(dev, "tx");
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index fc337c435cd14917bdfb99febcf9119275afdeba..483789d9fa98e79d1283de8297bf2fc2a773f3a7 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -8,6 +8,8 @@
#include <linux/dmaengine.h>
+struct qce_device;
+
/* maximum data transfer block size between BAM and CE */
#define QCE_BAM_BURST_SIZE 64
@@ -32,7 +34,7 @@ struct qce_dma_data {
struct qce_result_dump *result_buf;
};
-int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);
+int devm_qce_dma_request(struct qce_device *qce);
int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *sg_in,
int in_ents, struct scatterlist *sg_out, int out_ents,
dma_async_tx_callback cb, void *cb_param);
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (9 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:16 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
` (2 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski,
Konrad Dybcio
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Switch to devm_kmalloc() and devm_dma_alloc_chan() in
devm_qce_dma_request(). This allows us to drop two labels and shrink the
function.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/dma.c | 37 +++++++++++--------------------------
1 file changed, 11 insertions(+), 26 deletions(-)
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index d60efb5c26d88f8b0259b1dccc8724d0f75571c6..26347e9fc078adede712722107e74958538accdf 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -12,49 +12,34 @@
#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
-static void qce_dma_release(void *data)
+static void qce_dma_terminate(void *data)
{
struct qce_dma_data *dma = data;
dmaengine_terminate_sync(dma->txchan);
dmaengine_terminate_sync(dma->rxchan);
- dma_release_channel(dma->txchan);
- dma_release_channel(dma->rxchan);
- kfree(dma->result_buf);
}
int devm_qce_dma_request(struct qce_device *qce)
{
struct qce_dma_data *dma = &qce->dma;
struct device *dev = qce->dev;
- int ret;
- dma->txchan = dma_request_chan(dev, "tx");
+ dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
+ if (!dma->result_buf)
+ return -ENOMEM;
+
+ dma->txchan = devm_dma_request_chan(dev, "tx");
if (IS_ERR(dma->txchan))
return dev_err_probe(dev, PTR_ERR(dma->txchan),
"Failed to get TX DMA channel\n");
- dma->rxchan = dma_request_chan(dev, "rx");
- if (IS_ERR(dma->rxchan)) {
- ret = dev_err_probe(dev, PTR_ERR(dma->rxchan),
- "Failed to get RX DMA channel\n");
- goto error_rx;
- }
-
- dma->result_buf = kmalloc(QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ,
- GFP_KERNEL);
- if (!dma->result_buf) {
- ret = -ENOMEM;
- goto error_nomem;
- }
-
- return devm_add_action_or_reset(dev, qce_dma_release, dma);
+ dma->rxchan = devm_dma_request_chan(dev, "rx");
+ if (IS_ERR(dma->rxchan))
+ return dev_err_probe(dev, PTR_ERR(dma->rxchan),
+ "Failed to get RX DMA channel\n");
-error_nomem:
- dma_release_channel(dma->rxchan);
-error_rx:
- dma_release_channel(dma->txchan);
- return ret;
+ return devm_add_action_or_reset(dev, qce_dma_terminate, dma);
}
struct scatterlist *
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()
2026-07-13 13:01 ` [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-13 13:16 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:16 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, dmaengine, vkoul
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The crypto request workqueue can execute after DMA channels and the result buffer have been freed, leading to a Use-After-Free during device unbind or probe failure.
--
--- Patch [11]: [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() ---
commit ae30a428f07757ca3220181d917faca67619bbbd
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Use existing devres APIs in devm_qce_dma_request()
Switch to devm_kmalloc() and devm_dma_alloc_chan() in
devm_qce_dma_request(). This allows us to drop two labels and shrink the
function.
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index d60efb5c26d88..26347e9fc078a 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> @@ -12,49 +12,34 @@
>
> #define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
>
> -static void qce_dma_release(void *data)
> +static void qce_dma_terminate(void *data)
> {
> struct qce_dma_data *dma = data;
>
> dmaengine_terminate_sync(dma->txchan);
> dmaengine_terminate_sync(dma->rxchan);
> - dma_release_channel(dma->txchan);
> - dma_release_channel(dma->rxchan);
> - kfree(dma->result_buf);
> }
>
> int devm_qce_dma_request(struct qce_device *qce)
> {
> struct qce_dma_data *dma = &qce->dma;
> struct device *dev = qce->dev;
> - int ret;
>
> - dma->txchan = dma_request_chan(dev, "tx");
> + dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
> + if (!dma->result_buf)
> + return -ENOMEM;
> +
> + dma->txchan = devm_dma_request_chan(dev, "tx");
> if (IS_ERR(dma->txchan))
> return dev_err_probe(dev, PTR_ERR(dma->txchan),
> "Failed to get TX DMA channel\n");
>
> - dma->rxchan = dma_request_chan(dev, "rx");
> - if (IS_ERR(dma->rxchan)) {
> - ret = dev_err_probe(dev, PTR_ERR(dma->rxchan),
> - "Failed to get RX DMA channel\n");
> - goto error_rx;
> - }
> -
> - dma->result_buf = kmalloc(QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ,
> - GFP_KERNEL);
> - if (!dma->result_buf) {
> - ret = -ENOMEM;
> - goto error_nomem;
> - }
> -
> - return devm_add_action_or_reset(dev, qce_dma_release, dma);
> + dma->rxchan = devm_dma_request_chan(dev, "rx");
> + if (IS_ERR(dma->rxchan))
> + return dev_err_probe(dev, PTR_ERR(dma->rxchan),
> + "Failed to get RX DMA channel\n");
>
> -error_nomem:
> - dma_release_channel(dma->rxchan);
> -error_rx:
> - dma_release_channel(dma->txchan);
> - return ret;
> + return devm_add_action_or_reset(dev, qce_dma_terminate, dma);
[Severity: High]
This is a pre-existing issue, but does this devres registration order leave a
use-after-free window during device unbind or probe failure?
In qce_crypto_probe() in drivers/crypto/qce/core.c, qce_cancel_work is
registered before calling devm_qce_dma_request():
INIT_WORK(&qce->done_work, qce_req_done_work);
ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
if (ret)
return ret;
ret = devm_qce_dma_request(qce);
Because devres actions execute in reverse order of registration, during unbind
the DMA resources and result_buf registered here in devm_qce_dma_request()
will be freed first. Then, qce_cancel_work() will finally be executed to cancel
the workqueue.
If a crypto request completes and schedules qce->done_work just before or
during teardown, could the worker execute concurrently and access the already
freed DMA channels and result_buf inside qce_handle_queue()?
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=11
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (10 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:22 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
2026-07-13 13:01 ` [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
As the first step in converting the driver to using DMA for register
I/O, let's map the crypto memory range.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/core.c | 23 ++++++++++++++++++++++-
drivers/crypto/qce/core.h | 6 ++++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index aa4a0b17749081f1ad653424bc265ee81e348e15..4031b4516d6519fc5024bbbcc439500a7b3314b2 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -191,10 +191,19 @@ static void qce_cancel_work(void *data)
cancel_work_sync(work);
}
+static void qce_crypto_unmap_dma(void *data)
+{
+ struct qce_device *qce = data;
+
+ dma_unmap_resource(qce->dev, qce->base_dma, qce->dma_size,
+ DMA_BIDIRECTIONAL, 0);
+}
+
static int qce_crypto_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct qce_device *qce;
+ struct resource *res;
int ret;
qce = devm_kzalloc(dev, sizeof(*qce), GFP_KERNEL);
@@ -204,7 +213,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
qce->dev = dev;
platform_set_drvdata(pdev, qce);
- qce->base = devm_platform_ioremap_resource(pdev, 0);
+ qce->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(qce->base))
return PTR_ERR(qce->base);
@@ -254,6 +263,18 @@ static int qce_crypto_probe(struct platform_device *pdev)
qce->async_req_enqueue = qce_async_request_enqueue;
qce->async_req_done = qce_async_request_done;
+ qce->dma_size = resource_size(res);
+ qce->base_dma = dma_map_resource(dev, res->start, qce->dma_size,
+ DMA_BIDIRECTIONAL, 0);
+ qce->base_phys = res->start;
+ ret = dma_mapping_error(dev, qce->base_dma);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(qce->dev, qce_crypto_unmap_dma, qce);
+ if (ret)
+ return ret;
+
return devm_qce_register_algs(qce);
}
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index f092ce2d3b04a936a37805c20ac5ba78d8fdd2df..a80e12eac6c87e5321cce16c56a4bf5003474ef0 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -27,6 +27,9 @@
* @dma: pointer to dma data
* @burst_size: the crypto burst size
* @pipe_pair_id: which pipe pair id the device using
+ * @base_dma: base DMA address
+ * @base_phys: base physical address
+ * @dma_size: size of memory mapped for DMA
* @async_req_enqueue: invoked by every algorithm to enqueue a request
* @async_req_done: invoked by every algorithm to finish its request
*/
@@ -43,6 +46,9 @@ struct qce_device {
struct qce_dma_data dma;
int burst_size;
unsigned int pipe_pair_id;
+ dma_addr_t base_dma;
+ phys_addr_t base_phys;
+ size_t dma_size;
int (*async_req_enqueue)(struct qce_device *qce,
struct crypto_async_request *req);
void (*async_req_done)(struct qce_device *qce, int ret);
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA
2026-07-13 13:01 ` [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
@ 2026-07-13 13:22 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:22 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Devres teardown ordering leads to Use-After-Free of DMA channels.
--
--- Patch [12]: [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA ---
commit b1c15310873afc3b915460ae4afcab5b35bb8ea3
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Map crypto memory for DMA
As the first step in converting the driver to using DMA for register
I/O, let's map the crypto memory range.
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index aa4a0b1774908..4031b4516d651 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -254,6 +263,18 @@ static int qce_crypto_probe(struct platform_device *pdev)
> qce->async_req_enqueue = qce_async_request_enqueue;
> qce->async_req_done = qce_async_request_done;
>
> + qce->dma_size = resource_size(res);
> + qce->base_dma = dma_map_resource(dev, res->start, qce->dma_size,
> + DMA_BIDIRECTIONAL, 0);
> + qce->base_phys = res->start;
> + ret = dma_mapping_error(dev, qce->base_dma);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(qce->dev, qce_crypto_unmap_dma, qce);
> + if (ret)
> + return ret;
> +
[Severity: High]
This is a pre-existing issue, but looking at qce_crypto_probe() earlier in
the function, could devres teardown ordering lead to a use-after-free of
the DMA channels?
Currently, workqueue cancellation is registered before the DMA channels are
requested:
drivers/crypto/qce/core.c:qce_crypto_probe() {
...
INIT_WORK(&qce->done_work, qce_req_done_work);
ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
if (ret)
return ret;
ret = devm_qce_dma_request(qce);
if (ret)
return ret;
...
}
Because devres actions execute in LIFO order during device unbind or late
probe failure, the devm-managed DMA channels will be freed before
qce_cancel_work() runs.
If done_work is scheduled but executes during or after the DMA channels are
freed, it will dequeue the next request and attempt to map and submit it
using the now-freed txchan and rxchan.
Should the workqueue cancellation be registered after the DMA channels are
requested to ensure any pending work is stopped before the channels are
released?
> return devm_qce_register_algs(qce);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=12
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (11 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:24 ` sashiko-bot
2026-07-13 13:01 ` [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Switch to using BAM DMA for register I/O in addition to passing data. To
that end: provide the necessary infrastructure in the driver, modify the
ordering of operations as required and replace all direct register writes
with wrappers queueing DMA command descriptors.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/aead.c | 10 ++--
drivers/crypto/qce/common.c | 20 ++++---
drivers/crypto/qce/dma.c | 120 ++++++++++++++++++++++++++++++++++++++++--
drivers/crypto/qce/dma.h | 5 ++
drivers/crypto/qce/sha.c | 10 ++--
drivers/crypto/qce/skcipher.c | 10 ++--
6 files changed, 144 insertions(+), 31 deletions(-)
diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
index 1461a08e6c58b00e60aa35515f3392c096726f6a..544a3cf8709248a5f3eb2b669e30b09183d3a69d 100644
--- a/drivers/crypto/qce/aead.c
+++ b/drivers/crypto/qce/aead.c
@@ -463,17 +463,17 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
src_nents = dst_nents - 1;
}
- ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
- qce_aead_done, async_req);
+ ret = qce_start(async_req, tmpl->crypto_alg_type);
if (ret)
goto error_unmap_src;
- qce_dma_issue_pending(&qce->dma);
-
- ret = qce_start(async_req, tmpl->crypto_alg_type);
+ ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
+ qce_aead_done, async_req);
if (ret)
goto error_terminate;
+ qce_dma_issue_pending(&qce->dma);
+
return 0;
error_terminate:
diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
index 54a78a57f63028f01870a3edeb8e390f523bb190..37bb6f03244d317a887aeb0aa10cefe327b4ce05 100644
--- a/drivers/crypto/qce/common.c
+++ b/drivers/crypto/qce/common.c
@@ -25,7 +25,7 @@ static inline u32 qce_read(struct qce_device *qce, u32 offset)
static inline void qce_write(struct qce_device *qce, u32 offset, u32 val)
{
- writel(val, qce->base + offset);
+ qce_write_dma(qce, offset, val);
}
static inline void qce_write_array(struct qce_device *qce, u32 offset,
@@ -82,6 +82,8 @@ static void qce_setup_config(struct qce_device *qce)
{
u32 config;
+ qce_clear_bam_transaction(qce);
+
/* get big endianness */
config = qce_config_reg(qce, 0);
@@ -90,12 +92,14 @@ static void qce_setup_config(struct qce_device *qce)
qce_write(qce, REG_CONFIG, config);
}
-static inline void qce_crypto_go(struct qce_device *qce, bool result_dump)
+static inline int qce_crypto_go(struct qce_device *qce, bool result_dump)
{
if (result_dump)
qce_write(qce, REG_GOPROC, BIT(GO_SHIFT) | BIT(RESULTS_DUMP_SHIFT));
else
qce_write(qce, REG_GOPROC, BIT(GO_SHIFT));
+
+ return qce_submit_cmd_desc(qce);
}
#if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD)
@@ -223,9 +227,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req)
config = qce_config_reg(qce, 1);
qce_write(qce, REG_CONFIG, config);
- qce_crypto_go(qce, true);
-
- return 0;
+ return qce_crypto_go(qce, true);
}
#endif
@@ -386,9 +388,7 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req)
config = qce_config_reg(qce, 1);
qce_write(qce, REG_CONFIG, config);
- qce_crypto_go(qce, true);
-
- return 0;
+ return qce_crypto_go(qce, true);
}
#endif
@@ -535,9 +535,7 @@ static int qce_setup_regs_aead(struct crypto_async_request *async_req)
qce_write(qce, REG_CONFIG, config);
/* Start the process */
- qce_crypto_go(qce, !IS_CCM(flags));
-
- return 0;
+ return qce_crypto_go(qce, !IS_CCM(flags));
}
#endif
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 26347e9fc078adede712722107e74958538accdf..1b43c56503334154be4b8000e5a9330b2005cb64 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -4,6 +4,8 @@
*/
#include <linux/device.h>
+#include <linux/dma/qcom_bam_dma.h>
+#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
#include <crypto/scatterwalk.h>
@@ -11,6 +13,96 @@
#include "dma.h"
#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
+#define QCE_BAM_CMD_SGL_SIZE 128
+#define QCE_BAM_CMD_ELEMENT_SIZE 128
+
+struct qce_desc_info {
+ struct dma_async_tx_descriptor *dma_desc;
+ enum dma_data_direction dir;
+};
+
+struct qce_bam_transaction {
+ struct bam_cmd_element bam_ce[QCE_BAM_CMD_ELEMENT_SIZE];
+ struct scatterlist wr_sgl[QCE_BAM_CMD_SGL_SIZE];
+ struct qce_desc_info *desc;
+ u32 bam_ce_idx;
+ u32 pre_bam_ce_idx;
+ u32 wr_sgl_cnt;
+};
+
+void qce_clear_bam_transaction(struct qce_device *qce)
+{
+ struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
+
+ bam_txn->bam_ce_idx = 0;
+ bam_txn->wr_sgl_cnt = 0;
+ bam_txn->pre_bam_ce_idx = 0;
+}
+
+int qce_submit_cmd_desc(struct qce_device *qce)
+{
+ struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
+ struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct dma_chan *chan = qce->dma.rxchan;
+ unsigned long attrs = DMA_PREP_CMD;
+ dma_cookie_t cookie;
+ unsigned int mapped;
+ int ret;
+
+ mapped = dma_map_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
+ if (!mapped)
+ return -ENOMEM;
+
+ dma_desc = dmaengine_prep_slave_sg(chan, bam_txn->wr_sgl, mapped, DMA_MEM_TO_DEV, attrs);
+ if (!dma_desc) {
+ ret = -ENOMEM;
+ goto err_unmap_sg;
+ }
+
+ qce_desc->dma_desc = dma_desc;
+ cookie = dmaengine_submit(qce_desc->dma_desc);
+
+ ret = dma_submit_error(cookie);
+ if (ret)
+ goto err_unmap_sg;
+
+ return 0;
+
+err_unmap_sg:
+ dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
+ return ret;
+}
+
+static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
+ unsigned int addr, void *buf)
+{
+ struct qce_bam_transaction *bam_txn = dma->bam_txn;
+ struct bam_cmd_element *bam_ce_buf;
+ int bam_ce_size, cnt, idx;
+
+ idx = bam_txn->bam_ce_idx;
+ bam_ce_buf = &bam_txn->bam_ce[idx];
+ bam_prep_ce_le32(bam_ce_buf, addr, BAM_WRITE_COMMAND, *((__le32 *)buf));
+
+ bam_ce_buf = &bam_txn->bam_ce[bam_txn->pre_bam_ce_idx];
+ bam_txn->bam_ce_idx++;
+ bam_ce_size = (bam_txn->bam_ce_idx - bam_txn->pre_bam_ce_idx) * sizeof(*bam_ce_buf);
+
+ cnt = bam_txn->wr_sgl_cnt;
+
+ sg_set_buf(&bam_txn->wr_sgl[cnt], bam_ce_buf, bam_ce_size);
+
+ ++bam_txn->wr_sgl_cnt;
+ bam_txn->pre_bam_ce_idx = bam_txn->bam_ce_idx;
+}
+
+void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val)
+{
+ unsigned int reg_addr = ((unsigned int)(qce->base_phys) + offset);
+
+ qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);
+}
static void qce_dma_terminate(void *data)
{
@@ -39,6 +131,16 @@ int devm_qce_dma_request(struct qce_device *qce)
return dev_err_probe(dev, PTR_ERR(dma->rxchan),
"Failed to get RX DMA channel\n");
+ dma->bam_txn = devm_kzalloc(dev, sizeof(*dma->bam_txn), GFP_KERNEL);
+ if (!dma->bam_txn)
+ return -ENOMEM;
+
+ dma->bam_txn->desc = devm_kzalloc(dev, sizeof(*dma->bam_txn->desc), GFP_KERNEL);
+ if (!dma->bam_txn->desc)
+ return -ENOMEM;
+
+ sg_init_table(dma->bam_txn->wr_sgl, QCE_BAM_CMD_SGL_SIZE);
+
return devm_add_action_or_reset(dev, qce_dma_terminate, dma);
}
@@ -98,28 +200,36 @@ int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *rx_sg,
{
struct dma_chan *rxchan = dma->rxchan;
struct dma_chan *txchan = dma->txchan;
- unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+ unsigned long txflags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+ unsigned long rxflags = txflags | DMA_PREP_FENCE;
int ret;
- ret = qce_dma_prep_sg(rxchan, rx_sg, rx_nents, flags, DMA_MEM_TO_DEV,
+ ret = qce_dma_prep_sg(rxchan, rx_sg, rx_nents, rxflags, DMA_MEM_TO_DEV,
NULL, NULL);
if (ret)
return ret;
- return qce_dma_prep_sg(txchan, tx_sg, tx_nents, flags, DMA_DEV_TO_MEM,
+ return qce_dma_prep_sg(txchan, tx_sg, tx_nents, txflags, DMA_DEV_TO_MEM,
cb, cb_param);
}
void qce_dma_issue_pending(struct qce_dma_data *dma)
{
- dma_async_issue_pending(dma->rxchan);
dma_async_issue_pending(dma->txchan);
+ dma_async_issue_pending(dma->rxchan);
}
int qce_dma_terminate_all(struct qce_dma_data *dma)
{
+ struct qce_device *qce = container_of(dma, struct qce_device, dma);
+ struct qce_bam_transaction *bam_txn = dma->bam_txn;
int ret;
ret = dmaengine_terminate_all(dma->rxchan);
- return ret ?: dmaengine_terminate_all(dma->txchan);
+ if (ret)
+ return ret;
+
+ dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
+
+ return dmaengine_terminate_all(dma->txchan);
}
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 483789d9fa98e79d1283de8297bf2fc2a773f3a7..f05dfa9e6b25bd60e32f45079a8bc7e6a4cf81f9 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -8,6 +8,7 @@
#include <linux/dmaengine.h>
+struct qce_bam_transaction;
struct qce_device;
/* maximum data transfer block size between BAM and CE */
@@ -32,6 +33,7 @@ struct qce_dma_data {
struct dma_chan *txchan;
struct dma_chan *rxchan;
struct qce_result_dump *result_buf;
+ struct qce_bam_transaction *bam_txn;
};
int devm_qce_dma_request(struct qce_device *qce);
@@ -43,5 +45,8 @@ int qce_dma_terminate_all(struct qce_dma_data *dma);
struct scatterlist *
qce_sgtable_add(struct sg_table *sgt, struct scatterlist *sg_add,
unsigned int max_len);
+void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val);
+int qce_submit_cmd_desc(struct qce_device *qce);
+void qce_clear_bam_transaction(struct qce_device *qce);
#endif /* _DMA_H_ */
diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index 5476d4d30fae7eb72bbcbcdd7d8be7a76f6732c2..5cfd769a59a791a79da42e2a5b0554ad974f7631 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -109,17 +109,17 @@ static int qce_ahash_async_req_handle(struct crypto_async_request *async_req)
goto error_unmap_src;
}
- ret = qce_dma_prep_sgs(&qce->dma, req->src, rctx->src_nents,
- &rctx->result_sg, 1, qce_ahash_done, async_req);
+ ret = qce_start(async_req, tmpl->crypto_alg_type);
if (ret)
goto error_unmap_dst;
- qce_dma_issue_pending(&qce->dma);
-
- ret = qce_start(async_req, tmpl->crypto_alg_type);
+ ret = qce_dma_prep_sgs(&qce->dma, req->src, rctx->src_nents,
+ &rctx->result_sg, 1, qce_ahash_done, async_req);
if (ret)
goto error_terminate;
+ qce_dma_issue_pending(&qce->dma);
+
return 0;
error_terminate:
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index a9b59e68df4b6837805d45391f5a5fe43fd47709..b4ef3748fbb4dde542b0307f32d4c871b7c33ac2 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -142,18 +142,18 @@ qce_skcipher_async_req_handle(struct crypto_async_request *async_req)
src_nents = dst_nents - 1;
}
+ ret = qce_start(async_req, tmpl->crypto_alg_type);
+ if (ret)
+ goto error_unmap_src;
+
ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents,
rctx->dst_sg, dst_nents,
qce_skcipher_done, async_req);
if (ret)
- goto error_unmap_src;
+ goto error_terminate;
qce_dma_issue_pending(&qce->dma);
- ret = qce_start(async_req, tmpl->crypto_alg_type);
- if (ret)
- goto error_terminate;
-
return 0;
error_terminate:
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O
2026-07-13 13:01 ` [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
@ 2026-07-13 13:24 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:24 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, vkoul, dmaengine
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Medium] BAM DMA command descriptors are written with native host-endian `u32` values, causing byte-swapping data corruption on big-endian platforms.
- [Medium] Early return in `qce_dma_terminate_all()` leaks DMA mappings and skips terminating the TX channel if RX channel termination fails.
- [Medium] Leaked DMA descriptor in error path due to incorrect use of `dmaengine_desc_free` on a non-reusable descriptor.
Pre-existing issues:
- [High] Memory leak of `rctx->adata` buffer in CCM requests with associated data on both success and error paths.
- [High] The error path in `qce_aead_async_req_handle` unmaps the wrong scatterlist for CCM requests, leading to a DMA mapping leak and invalid unmap.
- [High] Failure of `dma_map_sg` for the source scatterlist is bypassed due to an incorrect error check, leading to an invalid unmap.
--
--- Patch [13]: [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O ---
commit 07a714be1f94a3c7b4d4c5c79104848b7570dad5
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Add BAM DMA support for crypto register I/O
Switch to using BAM DMA for register I/O in addition to passing data. To
that end: provide the necessary infrastructure in the driver, modify the
ordering of operations as required and replace all direct register writes
with wrappers queueing DMA command descriptors.
> diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
> index 92d84941d3dbf..48ea7fb9a91e6 100644
> --- a/drivers/crypto/qce/aead.c
> +++ b/drivers/crypto/qce/aead.c
[ ... ]
> @@ -461,17 +461,17 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
> src_nents = dst_nents - 1;
> }
>
> - ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
> - qce_aead_done, async_req);
> + ret = qce_start(async_req, tmpl->crypto_alg_type);
> if (ret)
> goto error_unmap_src;
>
> - qce_dma_issue_pending(&qce->dma);
> -
> - ret = qce_start(async_req, tmpl->crypto_alg_type);
> + ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
> + qce_aead_done, async_req);
> if (ret)
> goto error_terminate;
>
> + qce_dma_issue_pending(&qce->dma);
> +
> return 0;
>
> error_terminate:
[Severity: High]
This is a pre-existing issue, but does this function leak memory allocated
for CCM requests with associated data?
qce_aead_async_req_handle() calls qce_aead_ccm_prepare_buf() which
allocates:
rctx->adata = kzalloc(...)
The buffer allocated for rctx->adata is never freed anywhere in the driver,
neither in the success path (qce_aead_done) nor in the error paths. Will
this reliably exhaust kernel memory for every CCM request?
[Severity: High]
This is a pre-existing issue, but is the wrong scatterlist being unmapped
in the error path?
If an error occurs, the error handler incorrectly unmaps the original user
scatterlist:
error_unmap_src:
if (diff_dst)
dma_unmap_sg(qce->dev, req->src, rctx->src_nents, dir_src);
For CCM requests with associated data, the dynamically constructed
scatterlist rctx->src_sg is mapped. Does unmapping req->src instead cause a
permanent DMA mapping leak and trigger IOMMU faults?
[Severity: High]
This is a pre-existing issue, but does this function incorrectly check the
return value of dma_map_sg()?
Earlier in this function:
src_nents = dma_map_sg(qce->dev, rctx->src_sg, rctx->src_nents,
dir_src);
if (src_nents < 0) {
dma_map_sg() returns 0 on failure, not a negative value. Will this bypass
the error check, pass a count of 0 to qce_dma_prep_sgs(), and eventually
lead to an invalid unmap?
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 26347e9fc078a..1b43c56503334 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> +int qce_submit_cmd_desc(struct qce_device *qce)
> +{
> + struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
> + struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
> + struct dma_async_tx_descriptor *dma_desc;
> + struct dma_chan *chan = qce->dma.rxchan;
> + unsigned long attrs = DMA_PREP_CMD;
> + dma_cookie_t cookie;
> + unsigned int mapped;
> + int ret;
> +
> + mapped = dma_map_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> + if (!mapped)
> + return -ENOMEM;
> +
> + dma_desc = dmaengine_prep_slave_sg(chan, bam_txn->wr_sgl, mapped, DMA_MEM_TO_DEV, attrs);
> + if (!dma_desc) {
> + ret = -ENOMEM;
> + goto err_unmap_sg;
> + }
> +
> + qce_desc->dma_desc = dma_desc;
> + cookie = dmaengine_submit(qce_desc->dma_desc);
> +
> + ret = dma_submit_error(cookie);
> + if (ret)
> + goto err_unmap_sg;
> +
> + return 0;
> +
> +err_unmap_sg:
> + dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> + return ret;
> +}
[Severity: Medium]
Is there a leaked DMA descriptor on this error path?
If dmaengine_desc_free(dma_desc) is ever used on error paths here (e.g., if
metadata attachment or submission fails):
err_free_desc:
dmaengine_desc_free(dma_desc);
Is it safe to call this? This API does not free non-reusable descriptors,
and returns -EPERM instead, leaving the descriptor permanently leaked in
the unsubmitted pool.
> +static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
> + unsigned int addr, void *buf)
> +{
> + struct qce_bam_transaction *bam_txn = dma->bam_txn;
> + struct bam_cmd_element *bam_ce_buf;
> + int bam_ce_size, cnt, idx;
> +
> + idx = bam_txn->bam_ce_idx;
> + bam_ce_buf = &bam_txn->bam_ce[idx];
> + bam_prep_ce_le32(bam_ce_buf, addr, BAM_WRITE_COMMAND, *((__le32 *)buf));
[Severity: Medium]
Will this cause endianness issues on big-endian platforms?
In qce_write_dma(), a native host-endian u32 value is passed by reference:
qce_write_dma()
unsigned int reg_addr = ...
qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);
Here, the pointer is cast to __le32 * and dereferenced without using
cpu_to_le32(). Won't the BAM DMA hardware receive incorrectly byte-swapped
values?
[ ... ]
> @@ -98,28 +200,36 @@ int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *rx_sg,
[ ... ]
> int qce_dma_terminate_all(struct qce_dma_data *dma)
> {
> + struct qce_device *qce = container_of(dma, struct qce_device, dma);
> + struct qce_bam_transaction *bam_txn = dma->bam_txn;
> int ret;
>
> ret = dmaengine_terminate_all(dma->rxchan);
> - return ret ?: dmaengine_terminate_all(dma->txchan);
> + if (ret)
> + return ret;
[Severity: Medium]
If dmaengine_terminate_all(dma->rxchan) returns an error, the function
returns immediately. Does this early return leak the DMA mappings for
bam_txn->wr_sgl and skip terminating the TX channel entirely?
> +
> + dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> +
> + return dmaengine_terminate_all(dma->txchan);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=13
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine
2026-07-13 13:01 [PATCH v21 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (12 preceding siblings ...)
2026-07-13 13:01 ` [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
@ 2026-07-13 13:01 ` Bartosz Golaszewski
2026-07-13 13:26 ` sashiko-bot
13 siblings, 1 reply; 26+ messages in thread
From: Bartosz Golaszewski @ 2026-07-13 13:01 UTC (permalink / raw)
To: Vinod Koul, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Stephan Gerhold, Bjorn Andersson,
Peter Ujfalusi, Michal Simek, Frank Li, Andy Gross,
Neil Armstrong
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Bartosz Golaszewski
In order to communicate to the BAM DMA engine which address should be
used as a scratchpad for dummy writes related to BAM pipe locking,
fill out and attach the provided metadata struct to the descriptor.
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/dma.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 1b43c56503334154be4b8000e5a9330b2005cb64..6410f8dc5bcf517223c768a3e8f87af245076c84 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -11,6 +11,7 @@
#include "core.h"
#include "dma.h"
+#include "regs-v5.h"
#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE)
#define QCE_BAM_CMD_SGL_SIZE 128
@@ -41,6 +42,10 @@ void qce_clear_bam_transaction(struct qce_device *qce)
int qce_submit_cmd_desc(struct qce_device *qce)
{
+ struct bam_desc_metadata meta = {
+ .scratchpad_addr = qce->base_phys + REG_VERSION,
+ .direction = DMA_MEM_TO_DEV,
+ };
struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
struct dma_async_tx_descriptor *dma_desc;
@@ -60,15 +65,21 @@ int qce_submit_cmd_desc(struct qce_device *qce)
goto err_unmap_sg;
}
+ ret = dmaengine_desc_attach_metadata(dma_desc, &meta, sizeof(meta));
+ if (ret)
+ goto err_free_desc;
+
qce_desc->dma_desc = dma_desc;
cookie = dmaengine_submit(qce_desc->dma_desc);
ret = dma_submit_error(cookie);
if (ret)
- goto err_unmap_sg;
+ goto err_free_desc;
return 0;
+err_free_desc:
+ dmaengine_desc_free(dma_desc);
err_unmap_sg:
dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
return ret;
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine
2026-07-13 13:01 ` [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
@ 2026-07-13 13:26 ` sashiko-bot
0 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-13 13:26 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The error path in `qce_submit_cmd_desc()` attempts to free a non-reusable DMA descriptor using `dmaengine_desc_free()`, which will fail and leak the descriptor.
--
--- Patch [14]: [PATCH v21 14/14] crypto: qce - Communicate the base physical address to the dmaengine ---
commit abb736f89a48907c45dba4ccf37ffa56752bda22
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
crypto: qce - Communicate the base physical address to the dmaengine
This commit attaches metadata containing the scratchpad address and direction
to the BAM DMA descriptor. It also adds an error handling path to free the
descriptor if metadata attachment or submission fails.
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 1b43c56503334..6410f8dc5bcf5 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> @@ -60,15 +65,21 @@ int qce_submit_cmd_desc(struct qce_device *qce)
> goto err_unmap_sg;
> }
>
> + ret = dmaengine_desc_attach_metadata(dma_desc, &meta, sizeof(meta));
> + if (ret)
> + goto err_free_desc;
> +
> qce_desc->dma_desc = dma_desc;
> cookie = dmaengine_submit(qce_desc->dma_desc);
>
> ret = dma_submit_error(cookie);
> if (ret)
> - goto err_unmap_sg;
> + goto err_free_desc;
>
> return 0;
>
> +err_free_desc:
> + dmaengine_desc_free(dma_desc);
[Severity: Medium]
Does this error path leak the DMA descriptor?
Looking at dmaengine_desc_free() in include/linux/dmaengine.h, it checks if
the descriptor is reusable and returns -EPERM if it is not:
static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc)
{
if (!dmaengine_desc_test_reuse(desc))
return -EPERM;
return desc->desc_free(desc);
}
Since the descriptor allocated earlier in qce_submit_cmd_desc() by
dmaengine_prep_slave_sg() is a standard descriptor and not marked as
reusable, it appears dmaengine_desc_free() will fail without actually
freeing the memory if either dmaengine_desc_attach_metadata() or
dma_submit_error() triggers the goto.
Is there a different method intended for cleaning up non-reusable
descriptors in this error path, or should the descriptor be allowed to
complete with a failure instead?
> err_unmap_sg:
> dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=14
^ permalink raw reply [flat|nested] 26+ messages in thread