[PATCHv2 0/2] dmaengine: idma64: descriptor allocation and length limit fixes

[PATCHv2 0/2] dmaengine: idma64: descriptor allocation and length limit fixes

[Rosen Penev]

This small series cleans up the idma64 descriptor allocation and fixes a
long-standing truncation bug in idma64_prep_slave_sg().

Patch 1 replaces the open-coded two-stage allocation in
idma64_alloc_desc() with kzalloc_flex() using a flexible array member
for the hardware descriptor list, annotated with __counted_by for extra
runtime bounds checking. The now-redundant helper is removed.

Patch 2 addresses the hardware limit. The iDMA 64-bit CTL_HI BLOCK_TS
field is only 17 bits (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff), so when a
scatterlist entry exceeds that size the driver would silently truncate
the transfer length. Use sg_nents_for_dma() to size the descriptor ring
after splitting oversized entries, and iterate the new per-chunk loop so
each hardware descriptor stays within the field.

Rosen Penev (2):

Rosen Penev (2):
  dmaengine: idma64: use kzalloc_flex
  dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor
    length limit

v2: add second patch

 drivers/dma/idma64.c | 70 ++++++++++++++++++++------------------------
 drivers/dma/idma64.h |  7 +++--
 2 files changed, 36 insertions(+), 41 deletions(-)

Changes since v1:
- (fill in changes here)

-- 
2.55.0

[PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

[Rosen Penev]

Simplifies allocations by using a flexible array member in this struct.

Remove idma64_alloc_desc. It now offers no readability advantages in
this single usage.

Add __counted_by to get extra runtime analysis.

Apply the exact same treatment to struct idma64_dma and devm_kzalloc.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/idma64.c | 30 ++++--------------------------
 drivers/dma/idma64.h |  4 ++--
 2 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index 5fcd1befc92d..d914f50ec309 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -192,23 +192,6 @@ static irqreturn_t idma64_irq(int irq, void *dev)
 
 /* ---------------------------------------------------------------------- */
 
-static struct idma64_desc *idma64_alloc_desc(unsigned int ndesc)
-{
-	struct idma64_desc *desc;
-
-	desc = kzalloc_obj(*desc, GFP_NOWAIT);
-	if (!desc)
-		return NULL;
-
-	desc->hw = kzalloc_objs(*desc->hw, ndesc, GFP_NOWAIT);
-	if (!desc->hw) {
-		kfree(desc);
-		return NULL;
-	}
-
-	return desc;
-}
-
 static void idma64_desc_free(struct idma64_chan *idma64c,
 		struct idma64_desc *desc)
 {
@@ -223,7 +206,6 @@ static void idma64_desc_free(struct idma64_chan *idma64c,
 		} while (i);
 	}
 
-	kfree(desc->hw);
 	kfree(desc);
 }
 
@@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 	struct scatterlist *sg;
 	unsigned int i;
 
-	desc = idma64_alloc_desc(sg_len);
+	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
+	desc->ndesc = sg_len;
+
 	for_each_sg(sgl, sg, sg_len, i) {
 		struct idma64_hw_desc *hw = &desc->hw[i];
 
@@ -326,7 +310,6 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 		hw->len = sg_dma_len(sg);
 	}
 
-	desc->ndesc = sg_len;
 	desc->direction = direction;
 	desc->status = DMA_IN_PROGRESS;
 
@@ -541,18 +524,13 @@ static int idma64_probe(struct idma64_chip *chip)
 	unsigned short i;
 	int ret;
 
-	idma64 = devm_kzalloc(chip->dev, sizeof(*idma64), GFP_KERNEL);
+	idma64 = devm_kzalloc(chip->dev, struct_size(idma64, chan, nr_chan), GFP_KERNEL);
 	if (!idma64)
 		return -ENOMEM;
 
 	idma64->regs = chip->regs;
 	chip->idma64 = idma64;
 
-	idma64->chan = devm_kcalloc(chip->dev, nr_chan, sizeof(*idma64->chan),
-				    GFP_KERNEL);
-	if (!idma64->chan)
-		return -ENOMEM;
-
 	idma64->all_chan_mask = (1 << nr_chan) - 1;
 
 	/* Turn off iDMA controller */
diff --git a/drivers/dma/idma64.h b/drivers/dma/idma64.h
index d013b54356aa..1a67dbb24db5 100644
--- a/drivers/dma/idma64.h
+++ b/drivers/dma/idma64.h
@@ -113,10 +113,10 @@ struct idma64_hw_desc {
 struct idma64_desc {
 	struct virt_dma_desc vdesc;
 	enum dma_transfer_direction direction;
-	struct idma64_hw_desc *hw;
 	unsigned int ndesc;
 	size_t length;
 	enum dma_status status;
+	struct idma64_hw_desc hw[] __counted_by(ndesc);
 };
 
 static inline struct idma64_desc *to_idma64_desc(struct virt_dma_desc *vdesc)
@@ -187,7 +187,7 @@ struct idma64 {
 
 	/* channels */
 	unsigned short all_chan_mask;
-	struct idma64_chan *chan;
+	struct idma64_chan chan[];
 };
 
 static inline struct idma64 *to_idma64(struct dma_device *ddev)
-- 
2.55.0

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If `idma64_prep_slave_sg()` is called with `sg_len == 0`, it eventually calls `idma64_desc_fill()` which accesses `desc->hw[UINT_MAX]`, causing a kernel panic.
--

--- Patch [1]: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex ---
commit 54aec29a1bbe69497782adf915ba6169fd73e345
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: idma64: use kzalloc_flex

This commit refactors the idma64_desc and idma64 structures to use flexible
array members. It simplifies allocations by using kzalloc_flex and adds
__counted_by annotations for runtime bounds checking.

> diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
> index 5fcd1befc92da..d914f50ec3096 100644
> --- a/drivers/dma/idma64.c
> +++ b/drivers/dma/idma64.c
[ ... ]
> @@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
>  	struct scatterlist *sg;
>  	unsigned int i;
>  
> -	desc = idma64_alloc_desc(sg_len);
> +	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
>  	if (!desc)
>  		return NULL;
>  
> +	desc->ndesc = sg_len;
> +

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds access
if idma64_prep_slave_sg() is called with sg_len == 0?

If sg_len is 0, desc->ndesc is set to 0 and the for_each_sg() loop is
bypassed. The function then calls idma64_desc_fill() which does the
following:

drivers/dma/idma64.c:idma64_desc_fill() {
    ...
    unsigned int i = desc->ndesc;
    struct idma64_hw_desc *hw = &desc->hw[i - 1];
    ...
}

Since i is 0, i - 1 underflows to UINT_MAX, causing an out-of-bounds
access on the hw flex array. With the new __counted_by bounds check,
this will likely trigger a panic immediately.

>  	for_each_sg(sgl, sg, sg_len, i) {
>  		struct idma64_hw_desc *hw = &desc->hw[i];
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712220039.924958-2-rosenp@gmail.com?part=1

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

[Rosen Penev]

On Sun, Jul 12, 2026 at 3:11 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] If `idma64_prep_slave_sg()` is called with `sg_len == 0`, it eventually calls `idma64_desc_fill()` which accesses `desc->hw[UINT_MAX]`, causing a kernel panic.
Fixed in the second patch. This is just sashiko being broken with v2 patches.
> --
>
> --- Patch [1]: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex ---
> commit 54aec29a1bbe69497782adf915ba6169fd73e345
> Author: Rosen Penev <rosenp@gmail.com>
>
> dmaengine: idma64: use kzalloc_flex
>
> This commit refactors the idma64_desc and idma64 structures to use flexible
> array members. It simplifies allocations by using kzalloc_flex and adds
> __counted_by annotations for runtime bounds checking.
>
> > diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
> > index 5fcd1befc92da..d914f50ec3096 100644
> > --- a/drivers/dma/idma64.c
> > +++ b/drivers/dma/idma64.c
> [ ... ]
> > @@ -307,10 +289,12 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
> >       struct scatterlist *sg;
> >       unsigned int i;
> >
> > -     desc = idma64_alloc_desc(sg_len);
> > +     desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
> >       if (!desc)
> >               return NULL;
> >
> > +     desc->ndesc = sg_len;
> > +
>
> [Severity: High]
> This is a pre-existing issue, but can this lead to an out-of-bounds access
> if idma64_prep_slave_sg() is called with sg_len == 0?
>
> If sg_len is 0, desc->ndesc is set to 0 and the for_each_sg() loop is
> bypassed. The function then calls idma64_desc_fill() which does the
> following:
>
> drivers/dma/idma64.c:idma64_desc_fill() {
>     ...
>     unsigned int i = desc->ndesc;
>     struct idma64_hw_desc *hw = &desc->hw[i - 1];
>     ...
> }
>
> Since i is 0, i - 1 underflows to UINT_MAX, causing an out-of-bounds
> access on the hw flex array. With the new __counted_by bounds check,
> this will likely trigger a panic immediately.
>
> >       for_each_sg(sgl, sg, sg_len, i) {
> >               struct idma64_hw_desc *hw = &desc->hw[i];
> >
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260712220039.924958-2-rosenp@gmail.com?part=1

Re: [PATCHv2 1/2] dmaengine: idma64: use kzalloc_flex

[Andy Shevchenko]

On Sun, Jul 12, 2026 at 03:00:38PM -0700, Rosen Penev wrote:
> Simplifies allocations by using a flexible array member in this struct.
> 
> Remove idma64_alloc_desc. It now offers no readability advantages in
> this single usage.
> 
> Add __counted_by to get extra runtime analysis.
> 
> Apply the exact same treatment to struct idma64_dma and devm_kzalloc.


...

> static struct dma_async_tx_descriptor *idma64_prep_slave_sg(

>  	struct scatterlist *sg;
>  	unsigned int i;
>  
> -	desc = idma64_alloc_desc(sg_len);
> +	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
>  	if (!desc)
>  		return NULL;
>  
> +	desc->ndesc = sg_len;

There are two places where this is updated. Are you sure the code become
correct after this change? Perhaps idma64_desc_free() needs additional care?

>  	for_each_sg(sgl, sg, sg_len, i) {
>  		struct idma64_hw_desc *hw = &desc->hw[i];

>  		hw->len = sg_dma_len(sg);
>  	}
>  
> -	desc->ndesc = sg_len;
>  	desc->direction = direction;
>  	desc->status = DMA_IN_PROGRESS;

In case the above is okay to do, move all three up to keep this block of
assignments together.

-- 
With Best Regards,
Andy Shevchenko

[PATCHv2 2/2] dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor length limit

[Rosen Penev]

The iDMA 64-bit hardware has a 17-bit block transfer size field in the
CTL_HI register (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff). When a
scatterlist entry exceeds this limit, the driver would silently
truncate the length, transferring fewer bytes than intended.

Use sg_nents_for_dma() to compute the number of hardware descriptors
needed after splitting large SG entries into chunks that fit within
the hardware limit. Split the loop to iterate over each chunk.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/idma64.c | 44 ++++++++++++++++++++++++++++++--------------
 drivers/dma/idma64.h |  3 ++-
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index d914f50ec309..6954ec2cdeae 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -287,27 +287,43 @@ static struct dma_async_tx_descriptor *idma64_prep_slave_sg(
 	struct idma64_chan *idma64c = to_idma64_chan(chan);
 	struct idma64_desc *desc;
 	struct scatterlist *sg;
-	unsigned int i;
+	unsigned int i, nents;
+	int ndesc;
 
-	desc = kzalloc_flex(*desc, hw, sg_len, GFP_NOWAIT);
+	ndesc = sg_nents_for_dma(sgl, sg_len, IDMA64C_CTLH_BLOCK_TS_MASK);
+	if (ndesc <= 0)
+		return NULL;
+
+	desc = kzalloc_flex(*desc, hw, ndesc, GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
-	desc->ndesc = sg_len;
+	desc->ndesc = ndesc;
 
+	nents = 0;
 	for_each_sg(sgl, sg, sg_len, i) {
-		struct idma64_hw_desc *hw = &desc->hw[i];
-
-		/* Allocate DMA capable memory for hardware descriptor */
-		hw->lli = dma_pool_alloc(idma64c->pool, GFP_NOWAIT, &hw->llp);
-		if (!hw->lli) {
-			desc->ndesc = i;
-			idma64_desc_free(idma64c, desc);
-			return NULL;
+		dma_addr_t addr = sg_dma_address(sg);
+		unsigned int len = sg_dma_len(sg);
+
+		while (len) {
+			struct idma64_hw_desc *hwdesc = &desc->hw[nents++];
+			unsigned int chunk = min(len, IDMA64C_CTLH_BLOCK_TS_MASK);
+
+			hwdesc->lli = dma_pool_alloc(idma64c->pool, GFP_NOWAIT,
+						     &hwdesc->llp);
+			if (!hwdesc->lli) {
+				/* nents was already incremented by ++ above */
+				desc->ndesc = nents - 1;
+				idma64_desc_free(idma64c, desc);
+				return NULL;
+			}
+
+			hwdesc->phys = addr;
+			hwdesc->len = chunk;
+
+			addr += chunk;
+			len -= chunk;
 		}
-
-		hw->phys = sg_dma_address(sg);
-		hw->len = sg_dma_len(sg);
 	}
 
 	desc->direction = direction;
diff --git a/drivers/dma/idma64.h b/drivers/dma/idma64.h
index 1a67dbb24db5..297a91594b31 100644
--- a/drivers/dma/idma64.h
+++ b/drivers/dma/idma64.h
@@ -8,6 +8,7 @@
 #ifndef __DMA_IDMA64_H__
 #define __DMA_IDMA64_H__
 
+#include <linux/bits.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>
@@ -51,7 +52,7 @@
 #define IDMA64C_CTLL_LLP_S_EN		(1 << 28)	/* src block chain */
 
 /* Bitfields in CTL_HI */
-#define IDMA64C_CTLH_BLOCK_TS_MASK	((1 << 17) - 1)
+#define IDMA64C_CTLH_BLOCK_TS_MASK	GENMASK_U32(16, 0)
 #define IDMA64C_CTLH_BLOCK_TS(x)	((x) & IDMA64C_CTLH_BLOCK_TS_MASK)
 #define IDMA64C_CTLH_DONE		(1 << 17)
 
-- 
2.55.0

Re: [PATCHv2 2/2] dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor length limit

[Andy Shevchenko]

On Sun, Jul 12, 2026 at 03:00:39PM -0700, Rosen Penev wrote:
> The iDMA 64-bit hardware has a 17-bit block transfer size field in the
> CTL_HI register (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff). When a
> scatterlist entry exceeds this limit, the driver would silently
> truncate the length, transferring fewer bytes than intended.
> 
> Use sg_nents_for_dma() to compute the number of hardware descriptors
> needed after splitting large SG entries into chunks that fit within
> the hardware limit. Split the loop to iterate over each chunk.

I appreciate the intention, but... I have issues with the implementation.

> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/dma/idma64.c | 44 ++++++++++++++++++++++++++++++--------------
>  drivers/dma/idma64.h |  3 ++-
>  2 files changed, 32 insertions(+), 15 deletions(-)

First of all, the statistics and code readability. Next is the requirement
for drivers to do that. This should be done on the DMAengine core level
for all, this is software resplit and we just need a driver agreement of
getting a such to be done before handing over to the driver's callback.

OTOH, there is an API to get DMA maximum segment size (note, that your split
is incorrect since the BLOCK_TS is in "bus width" units, it may be up to 4
or 8 bytes and it depends on the alignment: so, in this form this patch is
no go). The consumer drivers should actually call it before preparing SG
list to make sure that resplit is not needed and the SG list is compatible
with what HW capable of.

See dma_set_max_seg_size() and dma_get_max_seg_size().

...

>  #ifndef __DMA_IDMA64_H__
>  #define __DMA_IDMA64_H__
>  
> +#include <linux/bits.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/spinlock.h>

Yeah, the inclusions should be revisit as many changes happened after the
driver introduction in the area of how we split headers.

...

>  /* Bitfields in CTL_HI */
> -#define IDMA64C_CTLH_BLOCK_TS_MASK	((1 << 17) - 1)
> +#define IDMA64C_CTLH_BLOCK_TS_MASK	GENMASK_U32(16, 0)

Why? I think this becomes inconsistent. If you want to switch to bits.h, make
it in a separate patch for all eligible definitions.

-- 
With Best Regards,
Andy Shevchenko