* [PATCH v23 01/14] dmaengine: constify struct dma_descriptor_metadata_ops
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
` (12 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 01/14] dmaengine: constify struct dma_descriptor_metadata_ops Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:48 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
` (11 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path
2026-07-21 13:33 ` [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
@ 2026-07-21 13:48 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:48 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:
Pre-existing issues:
- [High] Unclocked Hardware Access in `bam_dma_remove()`
- [High] Unclocked Register Access in IRQ Handler
- [High] Tasklets are killed too late in `bam_dma_remove()`, causing a race condition with device teardown
- [High] Requesting the IRQ before PM runtime is fully enabled can trigger a system-hanging IRQ storm
- [Medium] Unbalanced `clk_disable_unprepare()` in `bam_dma_remove()` triggers clock framework warning
- [Medium] PM Runtime usage counter leak on `pm_runtime_get_sync()` failures
--
--- Patch [2]: [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path ---
commit 9cb6e1d095c67c46c54a187984f1dac9efb8fda4
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
dmaengine: qcom: bam_dma: free interrupt before the clock in error path
This commit stops using devres for the BAM interrupt, freeing it manually
in the probe error path and remove functions. This prevents the interrupt
from firing after the clock is disabled.
Link: 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
> index 1bb26af0405f3..fc155e0d1870c 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);
[Severity: High]
This isn't a bug introduced by this patch, but does requesting this
interrupt before PM runtime is fully enabled risk a system-hanging IRQ
storm?
In bam_dma_probe(), the global BAM interrupts are unmasked before this call,
but pm_runtime_enable() is not called until the end of the probe function.
If an interrupt fires immediately, bam_dma_irq() will call
pm_runtime_get_sync(). Since PM runtime isn't enabled yet, it returns
-EACCES, and the handler returns IRQ_NONE without clearing the interrupt.
Because the interrupt is level-sensitive (IRQF_TRIGGER_HIGH), could the
hardware instantly re-assert it and trap the CPU in an infinite loop?
[ ... ]
> @@ -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: High]
This is a pre-existing issue, but does this write result in unclocked
hardware access?
When pm_runtime_force_suspend() is called just above, it invokes
bam_dma_runtime_suspend() which explicitly disables the device clock via
clk_disable().
Immediately afterward, writel_relaxed() attempts to mask interrupts on the
now-unclocked hardware. Could this access trigger an external bus abort and
crash the kernel during device removal?
> - 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);
[Severity: High]
This is a pre-existing issue, but does bam_dma_terminate_all() also access
hardware registers while the clock is disabled?
Similar to the interrupt masking above, this invokes channel initialization
and reset logic. Could accessing unclocked registers here cause a synchronous
bus abort?
> tasklet_kill(&bdev->channels[i].vc.task);
[Severity: High]
This isn't a bug introduced by this patch, but are these channel tasklets
and the global DMA tasklet terminated too late?
Since pm_runtime_disable() is never called in this remove function, if a
tasklet was scheduled prior to free_irq(), it might execute concurrently
with this channel teardown.
The tasklet calls pm_runtime_get_sync(), which could successfully wake the
device and initiate DMA transfers while the main thread is unregistering
channels and freeing FIFO memory. Could this lead to a use-after-free and
hardware state corruption?
I also noticed a few other pre-existing issues in this driver outside the
immediate diff context that might need attention:
drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
clk_disable_unprepare(bdev->bamclk);
}
[Severity: Medium]
This is a pre-existing issue, but does calling clk_disable_unprepare() here
cause a clock framework warning?
In probe, the clock is prepared and enabled exactly once. During removal,
pm_runtime_force_suspend() guarantees the device is in a suspended state,
meaning bam_dma_runtime_suspend() has already decremented the clock enable
count to 0.
Calling clk_disable_unprepare() here attempts to disable the clock a second
time, which triggers a WARN splat in clk_core_disable() and corrupts the
state. Should only clk_unprepare() be used here instead?
drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
...
srcs |= process_channel_irqs(bdev);
...
}
[Severity: High]
This is a pre-existing issue, but can this lead to unclocked register access
in the IRQ handler?
In bam_dma_irq(), process_channel_irqs() is called before
pm_runtime_get_sync(). process_channel_irqs() immediately reads the
BAM_IRQ_SRCS_EE hardware register.
If a spurious or delayed interrupt is delivered while the device is in a
runtime-suspended state (and the clock is disabled), will this unclocked
read trigger an external bus abort and crash the system?
drivers/dma/qcom/bam_dma.c:bam_start_dma() {
...
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return;
...
}
[Severity: Medium]
This is a pre-existing issue, but does this leak the PM runtime usage
counter when pm_runtime_get_sync() fails?
pm_runtime_get_sync() increments the runtime PM usage counter
unconditionally. If it returns an error, the caller must decrement it via
pm_runtime_put_noidle().
Checking for a negative return value and returning immediately leaves the
counter incremented. This same pattern appears to exist in bam_pause(),
bam_resume(), bam_free_chan(), and bam_dma_irq(). Will this prevent the BAM
DMA controller from ever entering runtime suspend again?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 01/14] dmaengine: constify struct dma_descriptor_metadata_ops Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:52 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data Bartosz Golaszewski
` (10 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
2026-07-21 13:33 ` [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
@ 2026-07-21 13:52 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:52 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Frank.Li, vkoul, dmaengine
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Teardown race condition between background work and device removal
- [High] Hardware registers accessed before verifying Runtime PM state in IRQ handler
- [High] Resource leak on pm_runtime_get_sync() failure
- [High] Spurious IRQ storm and permanent device disablement due to early IRQ request
--
--- Patch [3]: [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue ---
commit b043c7508eb0576f9b0b8a168ece5940655bca10
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue
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.
> 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 is a pre-existing issue, but does process_channel_irqs() read hardware
registers before pm_runtime_get_sync() verifies the device is active?
If the device is runtime-suspended (clocks disabled) when a spurious or
unhandled interrupt occurs, could this unclocked read cause a bus fault?
[ ... ]
> @@ -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)
[Severity: High]
This is a pre-existing issue, but looking at bam_start_dma() which is called
by this work queue:
drivers/dma/qcom/bam_dma.c:bam_start_dma() {
...
vd = vchan_next_desc(&bchan->vc);
if (!vd)
return;
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return;
If pm_runtime_get_sync() fails, it returns without dropping the usage counter
and without cleaning up the popped virt_dma descriptor.
Does this permanently leak the descriptor, hang the client waiting for
completion, and permanently prevent device suspend?
The same early return pattern appears in bam_free_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);
[Severity: High]
This is a pre-existing issue, but in bam_dma_probe(), the interrupt is
registered before pm_runtime_enable() is called:
drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
...
ret = request_irq(bdev->irq, bam_dma_irq, ...);
...
pm_runtime_enable(&pdev->dev);
...
}
If an error interrupt fires in this window, will pm_runtime_get_sync() in the
IRQ handler return -EACCES because runtime PM is not yet enabled?
If so, could this cause the handler to return IRQ_NONE without clearing the
hardware interrupt (skipping BAM_IRQ_CLR), leading to an infinite IRQ loop?
[ ... ]
> @@ -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: High]
This is a pre-existing issue, but is the work queue canceled after the device
is forcefully suspended?
Looking at the code in bam_dma_remove():
pm_runtime_force_suspend(&pdev->dev);
...
cancel_work_sync(&bdev->work);
If bam_dma_work() executes concurrently with bam_dma_remove(), it calls
bam_start_dma(), which assumes the hardware is active.
If pm_runtime_force_suspend() forcefully disables the device clock before the
work queue is canceled, could the background work attempt unclocked hardware
register access and cause a synchronous external abort?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (2 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 03/14] dmaengine: qcom: bam_dma: convert tasklet to a BH workqueue Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support Bartosz Golaszewski
` (9 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (3 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 04/14] dmaengine: qcom: bam_dma: Extend the driver's device match data Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:45 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
` (8 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support
2026-07-21 13:33 ` [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support Bartosz Golaszewski
@ 2026-07-21 13:45 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:45 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] Unclocked BAM register access in bam_dma_remove() causes system crash.
--
--- Patch [5]: [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support ---
commit 52e9e906437f52a7f85afb39e2221eefc5535d38
Author: Bartosz Golaszewski <brgl@kernel.org>
dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support
This commit extends the device match data with a flag indicating whether
the IP supports the BAM lock/unlock feature. It sets this flag to true
on BAM IP versions 1.4.0 and above.
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 8ce0fe085c5fe..f3e713a5259c2 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential system
crash during the device unbind or module unload path in this file:
drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
pm_runtime_force_suspend(&pdev->dev);
of_dma_controller_free(pdev->dev.of_node);
dma_async_device_unregister(&bdev->common);
/* mask all interrupts for this execution environment */
writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
for (i = 0; i < bdev->num_channels; i++) {
bam_dma_terminate_all(&bdev->channels[i].vc.chan);
...
}
Does this sequence trigger a synchronous external abort on ARM architectures?
The call to pm_runtime_force_suspend() shuts off the BAM device clock, but
the function then proceeds to execute unclocked hardware register writes via
writel_relaxed() and indirectly through bam_dma_terminate_all().
Can this hardware safely handle register accesses after its clock has been
disabled, or will this crash the system during teardown?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=5
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (4 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 05/14] dmaengine: qcom: bam_dma: Add pipe_lock_supported flag support Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:48 ` sashiko-bot
2026-07-21 18:07 ` Mukesh Savaliya
2026-07-21 13:33 ` [PATCH v23 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
` (7 subsequent siblings)
13 siblings, 2 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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 a TX
channel (DMA_MEM_TO_DEV) - 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 slave
config.
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/dma/qcom/bam_dma.c | 173 +++++++++++++++++++++++++++++++++++++--
include/linux/dma/qcom_bam_dma.h | 15 ++++
2 files changed, 182 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index f3e713a5259c2c7c24cfdcec094814eb1202971a..411c60c4433e21e202847aa9182163a56df2d97f 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,10 @@ struct bam_chan {
struct list_head desc_list;
struct list_head node;
+
+ /* BAM locking infrastructure */
+ u32 lock_scratchpad_addr;
+ bool bam_locked;
};
static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
@@ -638,8 +651,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);
@@ -676,13 +691,26 @@ static void bam_free_chan(struct dma_chan *chan)
static int bam_slave_config(struct dma_chan *chan,
struct dma_slave_config *cfg)
{
+ struct bam_config *peripheral_cfg = cfg->peripheral_config;
struct bam_chan *bchan = to_bam_chan(chan);
+ const struct bam_device_data *bdata = bchan->bdev->dev_data;
guard(spinlock_irqsave)(&bchan->vc.lock);
memcpy(&bchan->slave, cfg, sizeof(*cfg));
bchan->reconfigure = 1;
+ /*
+ * This is required to setup the pipe locking and must be done even
+ * before the first call to bam_start_dma().
+ */
+ if (bdata->pipe_lock_supported && peripheral_cfg) {
+ if (cfg->direction != DMA_MEM_TO_DEV)
+ return -EINVAL;
+
+ bchan->lock_scratchpad_addr = peripheral_cfg->lock_scratchpad_addr;
+ }
+
return 0;
}
@@ -802,6 +830,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 +888,16 @@ 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;
+ struct bam_chan *bchan = to_bam_chan(chan);
+
+ dma_unmap_sg(bchan->bdev->dev, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
+ kfree(async_desc);
+}
+
/**
* process_channel_irqs - processes the channel interrupts
* @bdev: bam controller
@@ -870,6 +909,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 +959,19 @@ 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;
+
+ 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 +1092,100 @@ 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 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->lock_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(bchan->bdev->dev, &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 = cpu_to_le32(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->lock_scratchpad_addr)
+ 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 +1197,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 +1206,24 @@ 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) {
+ ret = bam_setup_pipe_lock(bchan);
+ if (ret) {
+ dev_err_ratelimited(bdev->dev,
+ "failed to setup the pipe lock, deferring transfer: %d\n",
+ ret);
+ queue_work(system_bh_highpri_wq, &bdev->work);
+ break;
+ }
+ if (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 +1285,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 +1347,12 @@ 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;
+ struct bam_chan *bchan = to_bam_chan(chan);
+
+ if (async_desc->is_lock_desc)
+ dma_unmap_sg(bchan->bdev->dev, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
kfree(async_desc);
}
@@ -1384,6 +1544,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..188f667cad9935b338489e41e6dd03e58166ef9d 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,19 @@ enum bam_command_type {
BAM_READ_COMMAND,
};
+/**
+ * struct bam_config - BAM DMA peripheral config.
+ *
+ * @lock_scratchpad_addr: Peripheral-local register address to use for dummy
+ * write operations when queuing command descriptors
+ * with LOCK/UNLOCK bits set. This is not a system
+ * physical address: BAM command descriptors only
+ * encode a 24-bit address relative to the peripheral.
+ */
+struct bam_config {
+ u32 lock_scratchpad_addr;
+};
+
/*
* 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] 29+ messages in thread* Re: [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-21 13:33 ` [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
@ 2026-07-21 13:48 ` sashiko-bot
2026-07-21 18:07 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:48 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, dmaengine, Frank.Li
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] State leak causing hardware misconfiguration on channel reuse due to uncleared `lock_scratchpad_addr` and hardcoded direction.
- [High] Channel state corruption when `bam_slave_config` fails validation.
- [High] Infinite BH workqueue loop and unratelimited log flood on memory allocation failure.
- [High] DMA API violation: `lock_ce` buffer shares a cacheline with frequently CPU-modified struct members.
- [Low] Unused assignment of `hdesc` in IRQ handler.
--
--- Patch [6]: [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
commit 8b44c5e4935cbb61832adb3262df00b3b606a283
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 when starting DMA on a TX
channel. It uses a peripheral-local scratchpad address communicated via slave
config for the dummy writes.
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2..411c60c4433e2 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;
> +
> /* list node for the desc in the bam_chan list of descriptors */
> struct list_head desc_node;
[Severity: High]
Does placing lock_ce immediately adjacent to desc_node risk a DMA API
violation? lock_ce is mapped for DMA via dma_map_sg(), but desc_node is
actively modified by the CPU via list_add_tail() during bam_start_dma()
while the mapping is active. On non-coherent architectures, could this
cause cacheline tearing or stale data writebacks if they share the same
cacheline?
[ ... ]
> @@ -676,13 +691,26 @@ static void bam_free_chan(struct dma_chan *chan)
> static int bam_slave_config(struct dma_chan *chan,
> struct dma_slave_config *cfg)
> {
> + struct bam_config *peripheral_cfg = cfg->peripheral_config;
> struct bam_chan *bchan = to_bam_chan(chan);
> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
>
> guard(spinlock_irqsave)(&bchan->vc.lock);
>
> memcpy(&bchan->slave, cfg, sizeof(*cfg));
> bchan->reconfigure = 1;
>
[Severity: High]
Is it safe to overwrite bchan->slave and set bchan->reconfigure = 1 before
validating the configuration direction below? If cfg->direction !=
DMA_MEM_TO_DEV, this returns -EINVAL to the caller, but the channel
retains the rejected parameters and will apply them on the next transfer
because bchan->reconfigure remains set.
> + /*
> + * This is required to setup the pipe locking and must be done even
> + * before the first call to bam_start_dma().
> + */
> + if (bdata->pipe_lock_supported && peripheral_cfg) {
> + if (cfg->direction != DMA_MEM_TO_DEV)
> + return -EINVAL;
> +
> + bchan->lock_scratchpad_addr = peripheral_cfg->lock_scratchpad_addr;
> + }
[Severity: High]
If bam_slave_config() is called without peripheral_cfg, should
bchan->lock_scratchpad_addr be reset to 0 to avoid leaking state from a
prior configuration? If the previous lock_scratchpad_addr is retained,
bam_start_dma() might erroneously inject dummy lock descriptors on new
transfers.
[ ... ]
> @@ -919,13 +959,19 @@ 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;
> +
[Severity: Low]
Is the assignment to hdesc used anywhere? It appears to be assigned here but
is never read.
[ ... ]
> @@ -1046,13 +1092,100 @@ 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 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);
> + }
[Severity: High]
Could this unratelimited dev_err() cause a log flood? If kzalloc_flex()
fails here, bam_start_dma() requeues the work to system_bh_highpri_wq,
which runs in softirq context immediately without sleeping. This creates a
tight spin loop that will retry the exact same GFP_NOWAIT allocation, fail
again, and rapidly flood the console.
> +
> + 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;
[Severity: High]
Does hardcoding async_desc->dir to DMA_MEM_TO_DEV risk hardware
misconfiguration? If channel reuse retains lock_scratchpad_addr (as
mentioned in the bam_slave_config() review), a DEV_TO_MEM stream might
erroneously inject this dummy descriptor. This would cause
bam_apply_new_config() to execute with the wrong direction and incorrectly
apply dst_maxburst instead of src_maxburst, while clearing the reconfigure
flag for the actual data transfer.
[ ... ]
> @@ -1064,6 +1197,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 +1206,24 @@ 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) {
> + ret = bam_setup_pipe_lock(bchan);
> + if (ret) {
> + dev_err_ratelimited(bdev->dev,
> + "failed to setup the pipe lock, deferring transfer: %d\n",
> + ret);
> + queue_work(system_bh_highpri_wq, &bdev->work);
> + break;
> + }
[Severity: High]
Does requeuing to a high-priority BH workqueue upon ENOMEM cause CPU
starvation? Because system_bh_highpri_wq executes in softirq context,
requeuing immediately without sleeping prevents the memory reclaim needed
to resolve the GFP_NOWAIT failure, leading to an infinite spin loop.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
2026-07-21 13:33 ` [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
2026-07-21 13:48 ` sashiko-bot
@ 2026-07-21 18:07 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: Mukesh Savaliya @ 2026-07-21 18:07 UTC (permalink / raw)
To: Bartosz Golaszewski, 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, Vignesh Raghavendra
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski
Hi Bartosz,
On 7/21/2026 7:03 PM, Bartosz Golaszewski wrote:
[...]
> @@ -676,13 +691,26 @@ static void bam_free_chan(struct dma_chan *chan)
> static int bam_slave_config(struct dma_chan *chan,
> struct dma_slave_config *cfg)
> {
> + struct bam_config *peripheral_cfg = cfg->peripheral_config;
> struct bam_chan *bchan = to_bam_chan(chan);
> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
>
> guard(spinlock_irqsave)(&bchan->vc.lock);
>
> memcpy(&bchan->slave, cfg, sizeof(*cfg));
> bchan->reconfigure = 1;
>
> + /*
> + * This is required to setup the pipe locking and must be done even
> + * before the first call to bam_start_dma().
> + */
> + if (bdata->pipe_lock_supported && peripheral_cfg) {
> + if (cfg->direction != DMA_MEM_TO_DEV)
> + return -EINVAL;
> +
> + bchan->lock_scratchpad_addr = peripheral_cfg->lock_scratchpad_addr;
IIUC, peripheral_cfg will be filled by the client driver of BAM right ?
if not, who actually passes it ?
> + }
> +
> return 0;
> }
>
> @@ -802,6 +830,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
> }
[...]
> 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 +1197,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 +1206,24 @@ 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) {
> + ret = bam_setup_pipe_lock(bchan);
Hence, overall you meant to lock bam pipe once during the starting of
the bam dma engine ? is it ?
> + if (ret) {
> + dev_err_ratelimited(bdev->dev,
> + "failed to setup the pipe lock, deferring transfer: %d\n",
> + ret);
> + queue_work(system_bh_highpri_wq, &bdev->work);
> + break;
> + }
> + if (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 +1285,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;
> }
>
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 07/14] crypto: qce - Cancel work on device detach
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (5 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 06/14] dmaengine: qcom: bam_dma: add support for BAM locking Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:47 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 08/14] crypto: qce - Include algapi.h in the core.h header Bartosz Golaszewski
` (6 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 07/14] crypto: qce - Cancel work on device detach
2026-07-21 13:33 ` [PATCH v23 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
@ 2026-07-21 13:47 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:47 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: dmaengine, Frank.Li, vkoul
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Incorrect devres registration order causes Use-After-Free and Use-After-Destroy vulnerabilities during driver teardown.
Pre-existing issues:
- [High] The driver acquires a sleeping lock (`mutex`) in a crypto enqueue path that can be called from atomic contexts.
--
--- Patch [7]: [PATCH v23 07/14] crypto: qce - Cancel work on device detach ---
commit c537ce289434b17e2b6ebcb9c83403711f835f30
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
crypto: qce - Cancel work on device detach
The patch adds a devres action to cancel the done_work workqueue on device
detach or probe error. It moves the work initialization earlier and ensures
DMA transfers are terminated during DMA release.
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);
> + if (ret)
> + return ret;
> +
[Severity: High]
Will registering the devres action for work cancellation here cause a
use-after-free and use-after-destroy during driver teardown?
Because devres actions are executed in reverse order (LIFO), registering
the work cancellation before DMA and mutex initialization means the work
will be cancelled last during unbind or probe failure.
The teardown sequence would be:
1. mutex_destroy(&qce->lock)
2. qce_dma_release() (terminates DMA, releases channels, frees dma->result_buf)
3. qce_cancel_work()
If qce_req_done_work() is already scheduled or executing concurrently,
it will attempt to acquire the destroyed qce->lock, then dequeue
pending requests and submit new operations using the freed DMA channels
and result_buf.
> 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;
[Severity: High]
This is a pre-existing issue, but could qce_async_request_enqueue() trigger
a kernel warning by acquiring a sleeping lock in an atomic context?
The crypto API allows asynchronous algorithms to have their enqueue functions
invoked from atomic contexts, such as a softirq. However,
qce_async_request_enqueue() calls qce_handle_queue(), which attempts to
acquire a mutex:
drivers/crypto/qce/core.c:qce_handle_queue() {
...
scoped_guard(mutex, &qce->lock) {
...
}
...
}
Can this result in a "BUG: sleeping function called from invalid context"
when processing network traffic like IPsec in softirq?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 08/14] crypto: qce - Include algapi.h in the core.h header
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (6 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 07/14] crypto: qce - Cancel work on device detach Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
` (5 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (7 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 08/14] crypto: qce - Include algapi.h in the core.h header Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:46 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() Bartosz Golaszewski
` (4 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf
2026-07-21 13:33 ` [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
@ 2026-07-21 13:46 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:46 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] The DMA channels and `result_buf` are released before the asynchronous request workqueue (`done_work`) is cancelled, leading to a Use-After-Free of DMA channels and buffer during driver unbind.
--
--- Patch [9]: [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf ---
commit c6ae497c31e39eb0af424aa124dcc9493834ad11
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Remove unused ignore_buf
This commit removes the unused ignore_buf field from the qce_dma_data
structure. It also cleans up the associated offset calculation in the
DMA request initialization.
> 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]
This is a pre-existing issue, but registering qce_dma_release() here creates
a teardown order problem with the asynchronous request workqueue.
In drivers/crypto/qce/core.c, qce_crypto_probe() registers the cleanup action
for qce->done_work before this DMA request:
qce_crypto_probe() {
INIT_WORK(&qce->done_work, qce_req_done_work);
devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
...
devm_qce_dma_request(qce->dev, &qce->dma);
}
Because devres uses a LIFO execution order during unbind, qce_dma_release()
will execute before the workqueue is canceled.
If a user unbinds the driver while crypto operations are ongoing,
qce_dma_release() will free the DMA channels and dma->result_buf. If a DMA
completion interrupt had already scheduled qce->done_work, that work sits
pending.
When qce_cancel_work() finally runs, cancel_work_sync() will flush the work,
allowing qce_req_done_work() to call qce_handle_queue() and attempt to submit
the next request using the already-freed dma->result_buf and DMA channels.
Could the workqueue cleanup be registered after the DMA request to ensure
done_work is explicitly canceled before the DMA channels and buffers are
released?
>
> error_nomem:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=9
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request()
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (8 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 09/14] crypto: qce - Remove unused ignore_buf Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:44 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
` (3 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request()
2026-07-21 13:33 ` [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-21 13:44 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:44 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, dmaengine, Frank.Li
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Device unbind sequence incorrectly releases DMA channels and frees memory before the background work item is cancelled, allowing the work item to trigger a Use-After-Free on these resources.
--
--- Patch [10]: [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() ---
commit 6680b197a5f4cdb8f7389167e1c7991cd29ab241
Author: Bartosz Golaszewski <brgl@kernel.org>
Subject: crypto: qce - Simplify arguments of devm_qce_dma_request()
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.
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index dd860435d2c47..aa4a0b1774908 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);
[Severity: High]
Does this devres teardown order create a use-after-free during device unbind?
In qce_crypto_probe(), devm_add_action_or_reset() is called for qce_cancel_work
before calling devm_qce_dma_request():
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);
...
}
Since devres actions execute in reverse order of registration, the DMA resources
freed in qce_dma_release() (which is registered inside devm_qce_dma_request)
will be released before the background work is cancelled.
If the device is unbound while a crypto request is pending, qce_dma_release()
will free the DMA channels and the heap-allocated dma->result_buf.
The background work qce->done_work could then run, dequeue the next request,
and submit it to the freed DMA channels via qce_dma_prep_sgs(), writing to the
freed dma->result_buf.
A later fix in commit "crypto: qce - Cancel work on device detach" resolves
this inverted teardown order by grouping termination and freeing together.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=10
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (9 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 10/14] crypto: qce - Simplify arguments of devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:47 ` sashiko-bot
2026-07-21 18:16 ` Mukesh Savaliya
2026-07-21 13:33 ` [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
` (2 subsequent siblings)
13 siblings, 2 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()
2026-07-21 13:33 ` [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-21 13:47 ` sashiko-bot
2026-07-21 18:16 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:47 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] Race condition during device unbind leads to Use-After-Free of destroyed mutex, released DMA channels, and freed DMA buffers due to incorrect devres registration order.
--
--- Patch [11]: [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() ---
commit 4763bdc65678beffe23d93e69645b76ed7ebda22
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
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -12,49 +12,34 @@
[ ... ]
> int devm_qce_dma_request(struct qce_device *qce)
> {
> struct qce_dma_data *dma = &qce->dma;
> struct device *dev = qce->dev;
>
> - 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 isn't a bug introduced by this patch, but does the devres registration
order create a race condition during device unbind?
In drivers/crypto/qce/core.c:qce_crypto_probe(), the qce_cancel_work action
is registered before devm_qce_dma_request() and devm_mutex_init():
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;
...
ret = devm_mutex_init(qce->dev, &qce->lock);
Since devres unwinds in reverse (LIFO) order, during unbind the mutex is
destroyed and DMA resources are freed before qce_cancel_work() can synchronize
qce->done_work.
If a concurrent completion schedules qce->done_work, could qce_req_done_work()
execute after these resources are destroyed?
drivers/crypto/qce/core.c:qce_req_done_work()
...
scoped_guard(mutex, &qce->lock) {
req = qce->req;
qce->req = NULL;
}
Would this lead to a use-after-free on the destroyed mutex and released
DMA resources?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=11
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()
2026-07-21 13:33 ` [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
2026-07-21 13:47 ` sashiko-bot
@ 2026-07-21 18:16 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: Mukesh Savaliya @ 2026-07-21 18:16 UTC (permalink / raw)
To: Bartosz Golaszewski, 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, Vignesh Raghavendra
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Konrad Dybcio
On 7/21/2026 7:03 PM, Bartosz Golaszewski wrote:
> 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>
> ---
Acked-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (10 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:48 ` sashiko-bot
2026-07-21 18:20 ` Mukesh Savaliya
2026-07-21 13:33 ` [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
2026-07-21 13:33 ` [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
13 siblings, 2 replies; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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] 29+ messages in thread* Re: [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA
2026-07-21 13:33 ` [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
@ 2026-07-21 13:48 ` sashiko-bot
2026-07-21 18:20 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:48 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] Devres LIFO teardown ordering causes DMA channel Use-After-Free and premature IOMMU unmapping.
--
--- Patch [12]: [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA ---
commit e48bbe9c29cbad1f2dade29fb3a850939710cf45
Author: Bartosz Golaszewski <brgl@kernel.org>
crypto: qce - Map crypto memory for DMA
This commit maps the crypto memory range for DMA as a first step in
converting the driver to use DMA for register I/O.
> 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);
[Severity: High]
Does this devres registration order create a teardown race condition?
Because devres actions execute in reverse (LIFO) order, registering
qce_crypto_unmap_dma here means it will execute first during driver unbind.
Earlier in qce_crypto_probe(), we register the devres to cancel done_work:
INIT_WORK(&qce->done_work, qce_req_done_work);
ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
And we request the DMA channels:
ret = devm_qce_dma_request(qce);
If the driver is unbound while crypto requests are active, the unmapping of
base_dma happens first. Then the DMA channels are released.
Finally, the done_work is cancelled.
If qce->done_work is pending and executes after the channels are released but
before cancel_work_sync(), will it access the freed DMA channels and trigger
a use-after-free, or cause BAM DMA faults because the IOMMU mapping was
already removed?
> + if (ret)
> + return ret;
> +
> return devm_qce_register_algs(qce);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=12
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA
2026-07-21 13:33 ` [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
2026-07-21 13:48 ` sashiko-bot
@ 2026-07-21 18:20 ` Mukesh Savaliya
1 sibling, 0 replies; 29+ messages in thread
From: Mukesh Savaliya @ 2026-07-21 18:20 UTC (permalink / raw)
To: Bartosz Golaszewski, 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, Vignesh Raghavendra
Cc: dmaengine, linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski
On 7/21/2026 7:03 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
[...]
> 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))
Would you like to add any debug/error log ?
> 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)
Would you like to add any debug/error log ?
> + return ret;
> +
> + ret = devm_add_action_or_reset(qce->dev, qce_crypto_unmap_dma, qce);
> + if (ret)
same here
> + return ret;
> +
> return devm_qce_register_algs(qce);
> }
>
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (11 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 12/14] crypto: qce - Map crypto memory for DMA Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:50 ` sashiko-bot
2026-07-21 13:33 ` [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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 | 122 ++++++++++++++++++++++++++++++++++++++++--
drivers/crypto/qce/dma.h | 5 ++
drivers/crypto/qce/sha.c | 10 ++--
drivers/crypto/qce/skcipher.c | 10 ++--
6 files changed, 146 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..88d253d9147cfc9ed275653f7fd248538378194a 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,98 @@
#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_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;
+}
+
+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 +133,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 +202,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] 29+ messages in thread* Re: [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O
2026-07-21 13:33 ` [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
@ 2026-07-21 13:50 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:50 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:
- [High] BAM DMA command descriptors are populated with native-endian values instead of little-endian values, breaking Big-Endian architectures.
- [Medium] Extremely inefficient scatterlist generation isolates every command into its own scatterlist entry instead of batching them.
- [Medium] Early return in `qce_dma_terminate_all` bypasses DMA unmapping and TX channel termination on failure.
- [Low] Unnecessary dynamic allocation and unused struct `qce_desc_info`.
--
--- Patch [13]: [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O ---
commit 5eea68044298bd3c177ad5e3c2972d5e1761d259
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/dma.c b/drivers/crypto/qce/dma.c
> index 26347e9fc078a..88d253d9147cf 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);
[Severity: Low]
Is the separate dynamic allocation and usage of struct qce_desc_info
necessary?
The dma_desc field appears to be used merely to temporarily hold the
dma_desc pointer right before passing it directly to dmaengine_submit().
Additionally, the dir field of struct qce_desc_info is never used. Removing
this could avoid unnecessary code complexity and memory allocation overhead.
[ ... ]
> +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: High]
Can this cause endianness issues on big-endian architectures?
When qce_write_dma() is called, the host-endian u32 variable val is passed
by reference. Casting the pointer with *((__le32 *)buf) merely reinterprets
the memory without applying the required cpu_to_le32() byte swap. As a
result, the native bytes are assigned directly to bam_ce->data, discarding
the endianness handling that the prior writel() implementation provided.
> +
> + 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;
> +}
[Severity: Medium]
Does this logic unintentionally isolate every single command into its own
scatterlist entry?
Because pre_bam_ce_idx is immediately synchronized with bam_ce_idx at the
end of this per-write wrapper, the bam_ce_size calculation will always equal
the size of a single element. This appears to defeat the contiguous bam_ce
array batching, resulting in around 80 discrete hardware descriptors per
operation, which may degrade performance and waste DMA ring capacity.
[ ... ]
> 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);
> }
[Severity: Medium]
Could this early return cause a memory leak and leave the TX channel
orphaned?
If dmaengine_terminate_all(dma->rxchan) fails during an error rollback
(e.g., from the error_terminate path), returning early completely abandons
the unmapping of bam_txn->wr_sgl and fails to terminate dma->txchan. Should
the teardown sequence proceed to release the remaining resources even if
this step fails?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=13
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine
2026-07-21 13:33 [PATCH v23 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O Bartosz Golaszewski
` (12 preceding siblings ...)
2026-07-21 13:33 ` [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O Bartosz Golaszewski
@ 2026-07-21 13:33 ` Bartosz Golaszewski
2026-07-21 13:55 ` sashiko-bot
13 siblings, 1 reply; 29+ messages in thread
From: Bartosz Golaszewski @ 2026-07-21 13:33 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,
Mukesh Kumar Savaliya, Peter Ujfalusi, Michal Simek, Frank Li,
Andy Gross, Neil Armstrong, Vignesh Raghavendra
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 the dedicated BAM peripheral config structure and pass it to
the DMA driver using dmaengine_slave_config().
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/crypto/qce/core.c | 6 ++++--
drivers/crypto/qce/dma.c | 13 +++++++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 4031b4516d6519fc5024bbbcc439500a7b3314b2..50fdc8d808d0fe1d9d430ec601ab2ccf2ddf6e91 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -246,6 +246,9 @@ static int qce_crypto_probe(struct platform_device *pdev)
if (ret)
return ret;
+ qce->base_phys = res->start;
+ qce->dma_size = resource_size(res);
+
ret = devm_qce_dma_request(qce);
if (ret)
return ret;
@@ -263,10 +266,9 @@ 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;
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 88d253d9147cfc9ed275653f7fd248538378194a..09699d96575b105e2feaee08fad2731792f77ea2 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
@@ -116,8 +117,16 @@ static void qce_dma_terminate(void *data)
int devm_qce_dma_request(struct qce_device *qce)
{
+ struct bam_config peripheral_cfg = {
+ .lock_scratchpad_addr = (u32)qce->base_phys + REG_VERSION,
+ };
+ struct dma_slave_config cfg = {
+ .direction = DMA_MEM_TO_DEV,
+ .peripheral_config = &peripheral_cfg,
+ };
struct qce_dma_data *dma = &qce->dma;
struct device *dev = qce->dev;
+ int ret;
dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
if (!dma->result_buf)
@@ -133,6 +142,10 @@ 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");
+ ret = dmaengine_slave_config(dma->rxchan, &cfg);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to configure the RX DMA channel\n");
+
dma->bam_txn = devm_kzalloc(dev, sizeof(*dma->bam_txn), GFP_KERNEL);
if (!dma->bam_txn)
return -ENOMEM;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine
2026-07-21 13:33 ` [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine Bartosz Golaszewski
@ 2026-07-21 13:55 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-07-21 13:55 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unintended hardware misconfiguration due to partially initialized `dma_slave_config`.
- [Medium] Potential memory leak in qce_submit_cmd_desc() error path because dmaengine_desc_free() refuses to free non-reusable descriptors.
--
--- Patch [14]: [PATCH v23 14/14] crypto: qce - Communicate the base physical address to the dmaengine ---
commit 6c1591ac036f81ee86f8eab63457a3de06d13cef
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
crypto: qce - Communicate the base physical address to the dmaengine
This patch configures the BAM DMA engine with the base physical address
to be used as a scratchpad for dummy writes related to BAM pipe locking.
It passes the peripheral configuration structure to the DMA driver via
dmaengine_slave_config().
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 88d253d9147cf..09699d96575b1 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -116,8 +117,16 @@ static void qce_dma_terminate(void *data)
>
> int devm_qce_dma_request(struct qce_device *qce)
> {
> + struct bam_config peripheral_cfg = {
> + .lock_scratchpad_addr = (u32)qce->base_phys + REG_VERSION,
> + };
> + struct dma_slave_config cfg = {
> + .direction = DMA_MEM_TO_DEV,
> + .peripheral_config = &peripheral_cfg,
> + };
[Severity: Medium]
Does this partially initialized dma_slave_config cause an unintended
hardware misconfiguration?
By only setting the direction and peripheral_config, the designated
initializer silently zeroes out src_maxburst and dst_maxburst.
When dmaengine_slave_config() is called, the BAM DMA driver copies this
configuration. Later, if the engine is not remotely controlled, it will
write this zeroed maxburst value to the hardware threshold register:
drivers/dma/qcom/bam_dma.c:bam_apply_new_config() {
...
if (dir == DMA_DEV_TO_MEM)
maxburst = bchan->slave.src_maxburst;
else
maxburst = bchan->slave.dst_maxburst;
writel_relaxed(maxburst,
bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
...
}
This alters the hardware configuration from its default (4) and might
break DMA interrupt generation or cause an interrupt storm on systems
where the BAM is natively configured.
[ ... ]
Also, there is a second issue I'd like to ask about regarding
qce_submit_cmd_desc():
[Severity: Medium]
Can this error path leak the DMA descriptor?
In qce_submit_cmd_desc(), the code calls dmaengine_prep_slave_sg() with
attrs = DMA_PREP_CMD but lacks the DMA_CTRL_REUSE flag. If
dma_submit_error() detects an error, it branches to err_free_desc:
drivers/crypto/qce/dma.c:qce_submit_cmd_desc() {
...
err_free_desc:
dmaengine_desc_free(dma_desc);
...
}
Because the descriptor was not prepared with DMA_CTRL_REUSE,
dmaengine_desc_free() will immediately return -EPERM without freeing the
descriptor.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=14
^ permalink raw reply [flat|nested] 29+ messages in thread