Re: [PATCH v23 13/14] crypto: qce - Add BAM DMA support for crypto register I/O

[sashiko-bot@kernel.org]

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