[PATCHv2 0/4] dma: fsl_raid: fix sparse warnings and simplify probing

[PATCHv2 0/4] dma: fsl_raid: fix sparse warnings and simplify probing

[Rosen Penev]

This series cleans up the fsl_raid driver to address issues reported by
sparse and to simplify its MMIO handling.

Patch 1 fixes the endianness of in-memory descriptor stores. The
descriptor structs are handed to the device as big-endian, but the driver
stored CPU-endian values directly, which is both incorrect and flagged by
sparse as a base-type mismatch. The stores are wrapped in cpu_to_be32()
and the final-frame bit is now passed as an argument rather than
read-modify-written.

Patch 2 keeps the MMIO bases (re_regs and jrregs) as void __iomem *
instead of typed register struct pointers, eliminating "different address
spaces" sparse warnings on every register access. Each accessor derives a
local __iomem-qualified typed pointer.

Patch 3 replaces the open-coded platform_get_resource() +
devm_ioremap() sequence with devm_platform_ioremap_resource().

v2: split up first patch and simplify __iomem patch.

Rosen Penev (4):
  dma: fsl_raid: convert descriptor stores to big-endian
  dma: fsl_raid: set final bit via fill_cfd_frame() argument
  dma: fsl_raid: keep MMIO bases as void __iomem and cast at access
  dma: fsl_raid: use devm_platform_ioremap_resource

 drivers/dma/fsl_raid.c | 58 ++++++++++++++++++++----------------------
 drivers/dma/fsl_raid.h |  4 +--
 2 files changed, 30 insertions(+), 32 deletions(-)

-- 
2.55.0

[PATCHv2 1/4] dma: fsl_raid: convert descriptor stores to big-endian

[Rosen Penev]

The descriptor structs (fsl_re_cmpnd_frame / fsl_re_hw_desc) are
in-memory but their fields are __be32, because the structures are handed
to the device as big-endian. The driver stored CPU-endian u32 values
into them directly, which is both wrong (the engine would see
byte-swapped lengths/addresses) and flagged by sparse as a base-type
mismatch.

Wrap those stores in cpu_to_be32() so the values are little->big
converted.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/fsl_raid.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 99945845d8b5..2778d07a05e8 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -242,9 +242,9 @@ static void fill_cfd_frame(struct fsl_re_cmpnd_frame *cf, u8 index,
 	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;
 
 	efrl |= final << FSL_RE_CF_FINAL_SHIFT;
-	cf[index].efrl32 = efrl;
-	cf[index].addr_high = upper_32_bits(addr);
-	cf[index].addr_low = lower_32_bits(addr);
+	cf[index].efrl32 = cpu_to_be32(efrl);
+	cf[index].addr_high = cpu_to_be32(upper_32_bits(addr));
+	cf[index].addr_low = cpu_to_be32(lower_32_bits(addr));
 }
 
 static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
@@ -256,9 +256,10 @@ static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
 	dma_async_tx_descriptor_init(&desc->async_tx, &re_chan->chan);
 	INIT_LIST_HEAD(&desc->node);
 
-	desc->hwdesc.fmt32 = FSL_RE_FRAME_FORMAT << FSL_RE_HWDESC_FMT_SHIFT;
-	desc->hwdesc.lbea32 = upper_32_bits(paddr);
-	desc->hwdesc.addr_low = lower_32_bits(paddr);
+	desc->hwdesc.fmt32 = cpu_to_be32(FSL_RE_FRAME_FORMAT <<
+					  FSL_RE_HWDESC_FMT_SHIFT);
+	desc->hwdesc.lbea32 = cpu_to_be32(upper_32_bits(paddr));
+	desc->hwdesc.addr_low = cpu_to_be32(lower_32_bits(paddr));
 	desc->cf_addr = cf;
 	desc->cf_paddr = paddr;
 
-- 
2.55.0

[PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument

[Rosen Penev]

The final-frame bit is now passed as the "final" argument of
fill_cfd_frame() (as fsl_re_prep_dma_memcpy already did) and set in CPU
order before the single cpu_to_be32() store, replacing the previous
read-modify-write of the __be32 efrl32 field.

Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/fsl_raid.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 2778d07a05e8..888f55b672a5 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -375,11 +375,11 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
 	for (i = 2, j = 0; j < save_src_cnt; i++, j++)
 		fill_cfd_frame(cf, i, len, src[j], 0);
 
+	/* Fill the last frame and mark it final */
 	if (cont_q)
-		fill_cfd_frame(cf, i++, len, dest, 0);
-
-	/* Setting the final bit in the last source buffer frame in CFD */
-	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
+		fill_cfd_frame(cf, i, len, dest, 1);
+	else
+		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);
 
 	return &desc->async_tx;
 }
@@ -505,16 +505,16 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
 			p[save_src_cnt + 2] = 1;
 			fill_cfd_frame(cf, i++, len, dest[0], 0);
 			fill_cfd_frame(cf, i++, len, dest[1], 0);
-			fill_cfd_frame(cf, i++, len, dest[1], 0);
+			fill_cfd_frame(cf, i++, len, dest[1], 1);
 		} else {
 			dev_err(re_chan->dev, "PQ tx continuation error!\n");
 			return NULL;
 		}
+	} else {
+		/* Mark the last source buffer frame final */
+		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);
 	}
 
-	/* Setting the final bit in the last source buffer frame in CFD */
-	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
-
 	return &desc->async_tx;
 }
 
-- 
2.55.0

[PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

[Rosen Penev]

The fsl_re_ctrl and fsl_re_chan_cfg structures describe memory-mapped
RAID Engine registers accessed only via ioread32be()/iowrite32be(), yet
the pointers to them (re_regs in struct fsl_re_drv_private, and jrregs
in struct fsl_re_chan) were not __iomem-qualified, so sparse emitted
"different address spaces" warnings for every register access.

Store both MMIO bases as a plain void __iomem * and derive jrregs with
void __iomem * arithmetic from re_regs, rather than carrying typed
register struct pointers through the driver. Each function that touches
the registers introduces a local typed pointer (struct fsl_re_ctrl
__iomem *ctrl) and uses ->field, which is the idiomatic kernel pattern
 and keeps the registers' __iomem qualification intact.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/fsl_raid.c | 19 ++++++++++---------
 drivers/dma/fsl_raid.h |  4 ++--
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 888f55b672a5..524f83faf3da 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -657,8 +657,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 		goto err_free;
 	}
 
-	chan->jrregs = (struct fsl_re_chan_cfg *)((u8 *)re_priv->re_regs +
-			off + ptr);
+	chan->jrregs = re_priv->base + off + ptr;
 
 	/* read irq property from dts */
 	chan->irq = irq_of_parse_and_map(np, 0);
@@ -746,6 +745,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 /* Probe function for RAID Engine */
 static int fsl_re_probe(struct platform_device *ofdev)
 {
+	struct fsl_re_ctrl __iomem *re_regs;
 	struct fsl_re_drv_private *re_priv;
 	struct device_node *child;
 	u32 off;
@@ -764,20 +764,21 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		return -ENODEV;
 
 	/* IOMAP the entire RAID Engine region */
-	re_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
-	if (!re_priv->re_regs)
+	re_regs = devm_ioremap(dev, res->start, resource_size(res));
+	if (!re_regs)
 		return -EBUSY;
+	re_priv->base = re_regs;
 
 	/* Program the RE mode */
-	out_be32(&re_priv->re_regs->global_config, FSL_RE_NON_DPAA_MODE);
+	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);
 
 	/* Program Galois Field polynomial */
-	out_be32(&re_priv->re_regs->galois_field_config, FSL_RE_GFM_POLY);
+	out_be32(&re_regs->galois_field_config, FSL_RE_GFM_POLY);
 
 	dev_info(dev, "version %x, mode %x, gfp %x\n",
-		 in_be32(&re_priv->re_regs->re_version_id),
-		 in_be32(&re_priv->re_regs->global_config),
-		 in_be32(&re_priv->re_regs->galois_field_config));
+		 in_be32(&re_regs->re_version_id),
+		 in_be32(&re_regs->global_config),
+		 in_be32(&re_regs->galois_field_config));
 
 	dma_dev = &re_priv->dma_dev;
 	dma_dev->dev = dev;
diff --git a/drivers/dma/fsl_raid.h b/drivers/dma/fsl_raid.h
index 69d743c04973..adbfede330a7 100644
--- a/drivers/dma/fsl_raid.h
+++ b/drivers/dma/fsl_raid.h
@@ -256,7 +256,7 @@ struct fsl_re_hw_desc {
 struct fsl_re_drv_private {
 	u8 total_chans;
 	struct dma_device dma_dev;
-	struct fsl_re_ctrl *re_regs;
+	void __iomem *base;
 	struct fsl_re_chan *re_jrs[FSL_RE_MAX_CHANS];
 	struct dma_pool *cf_desc_pool;
 	struct dma_pool *hw_desc_pool;
@@ -273,7 +273,7 @@ struct fsl_re_chan {
 	struct device *dev;
 	struct fsl_re_drv_private *re_dev;
 	struct dma_chan chan;
-	struct fsl_re_chan_cfg *jrregs;
+	struct fsl_re_chan_cfg __iomem *jrregs;
 	int irq;
 	struct tasklet_struct irqtask;
 	u32 alloc_count;
-- 
2.55.0

[PATCHv2 4/4] dma: fsl_raid: use devm_platform_ioremap_resource

[Rosen Penev]

Replace the open-coded platform_get_resource() plus devm_ioremap()
sequence with devm_platform_ioremap_resource(), which fetches the
resource, requests the region and maps it in one call. Switch the error
check to IS_ERR()/PTR_ERR() and drop the now-unused struct resource
pointer.

The raideng node has a single reg region (0x320000, 0x10000); the
job-queue/ring children are separate OF devices probed independently, so
the region reservation added by devm_ioremap_resource() is exclusive and
does not introduce overlap failures.

Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/fsl_raid.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 524f83faf3da..2d86f61105e5 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -751,22 +751,18 @@ static int fsl_re_probe(struct platform_device *ofdev)
 	u32 off;
 	u8 ridx = 0;
 	struct dma_device *dma_dev;
-	struct resource *res;
 	int rc;
 	struct device *dev = &ofdev->dev;
 
+	/* IOMAP the entire RAID Engine region */
+	re_regs = devm_platform_ioremap_resource(ofdev, 0);
+	if (IS_ERR(re_regs))
+		return PTR_ERR(re_regs);
+
 	re_priv = devm_kzalloc(dev, sizeof(*re_priv), GFP_KERNEL);
 	if (!re_priv)
 		return -ENOMEM;
 
-	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
-
-	/* IOMAP the entire RAID Engine region */
-	re_regs = devm_ioremap(dev, res->start, resource_size(res));
-	if (!re_regs)
-		return -EBUSY;
 	re_priv->base = re_regs;
 
 	/* Program the RE mode */
-- 
2.55.0

Re: [PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check when parsing device tree Job Ring nodes.
- [High] `fsl_re_probe()` ignores the return value of `fsl_re_chan_probe()` and unconditionally increments `total_chans`, leading to a NULL pointer dereference.
- [High] DMA channels are never registered with the parent DMA device framework.
- [High] Missing cleanup for channel IRQs and platform devices in both error paths and driver removal, plus incorrect teardown order.
- [High] Potential NULL pointer dereference in `fsl_re_isr` due to premature IRQ registration.
--

--- Patch [1]: [PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access ---
commit d9bcf292f2ff70b568c4dd2362d8ec21ba56f6cd
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

The commit modifies the fsl_re_drv_private and fsl_re_chan structures to
store MMIO bases as void __iomem * instead of typed struct pointers. This
resolves sparse warnings about mismatched address spaces by ensuring typed
pointers are only introduced locally in functions that touch the registers.

Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..5bc5bd499ba27 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -656,8 +656,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  		goto err_free;
>  	}
>  
> -	chan->jrregs = (struct fsl_re_chan_cfg *)((u8 *)re_priv->re_regs +
> -			off + ptr);
> +	chan->jrregs = re_priv->base + off + ptr;
>  
>  	/* read irq property from dts */
>  	chan->irq = irq_of_parse_and_map(np, 0);

[Severity: High]
This isn't a bug introduced by this patch, but the driver context seems to
be attached to the device too late.

In fsl_re_chan_probe(), request_irq() is called before dev_set_drvdata()
populates the driver data:

    ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
    dev_set_drvdata(chandev, chan);

If an interrupt triggers immediately, fsl_re_isr() retrieves a NULL pointer
and dereferences it:

    re_chan = dev_get_drvdata((struct device *)data);
    irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);

Is there a vulnerable window here where an incoming interrupt could lead to
a NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but it appears the DMA channels are never
registered with the parent DMA device framework.

In fsl_re_chan_probe(), the channels are initialized, but the required list
addition to attach them to dma_dev->channels is omitted:

    re_priv->re_jrs[q] = chan;
    chan->chan.device = dma_dev;
    ...

Without adding the channel to dma_dev->channels, will
dma_async_device_register() end up registering 0 channels and leave the
driver functionally dead?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed potential resource
leaks and incorrect teardown ordering.

On error paths inside fsl_re_chan_probe(), resources like requested IRQs
and platform devices are not freed:

    err_free:
        return ret;

Additionally, in fsl_re_remove_chan(), free_irq() or
of_platform_device_destroy() are not called.

This leaves an active unmanaged IRQ pointing to freed driver memory.

Also, in fsl_re_remove(), the driver frees ring buffers before unregistering
the async DMA device:

    for (i = 0; i < re_priv->total_chans; i++)
        fsl_re_remove_chan(re_priv->re_jrs[i]);

    /* Unregister the driver */
    dma_async_device_unregister(&re_priv->dma_dev);

Could this cause a use-after-free if the IRQ fires, and does the teardown
order violate teardown safety expectations?

[ ... ]

> @@ -763,20 +763,21 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		return -ENODEV;
>  
>  	/* IOMAP the entire RAID Engine region */
> -	re_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!re_priv->re_regs)
> +	re_regs = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!re_regs)
>  		return -EBUSY;
> +	re_priv->base = re_regs;
>  
>  	/* Program the RE mode */
> -	out_be32(&re_priv->re_regs->global_config, FSL_RE_NON_DPAA_MODE);
> +	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);

[ ... ]

[Severity: High]
This is a pre-existing issue, but there seems to be a missing bounds check
when parsing device tree Job Ring nodes.

In fsl_re_probe(), the ridx counter is incremented indefinitely and passed
directly to fsl_re_chan_probe() without being bounded against
FSL_RE_MAX_CHANS (4):

    fsl_re_chan_probe(ofdev, child, ridx++, off);
    re_priv->total_chans++;

Then in fsl_re_chan_probe(), the unbounded index q is directly used to write
into the statically sized array:

    re_priv->re_jrs[q] = chan;

If a malformed device tree contains more than 4 job ring child nodes,
would this cause an out-of-bounds write and memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but fsl_re_probe() appears to
ignore the return value of fsl_re_chan_probe() and unconditionally
increments total_chans.

    fsl_re_chan_probe(ofdev, child, ridx++, off);
    re_priv->total_chans++;

If fsl_re_chan_probe() fails and returns an error, fsl_re_probe() ignores
the error but still increments re_priv->total_chans.

Subsequent calls to fsl_re_remove_chan() on driver remove or error rollback
will then dereference a NULL pointer:

    static void fsl_re_remove_chan(struct fsl_re_chan *chan)
    {
        tasklet_kill(&chan->irqtask);
        ...

Might this result in a kernel panic from a NULL pointer dereference during
teardown?

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

Re: [PATCHv2 1/4] dma: fsl_raid: convert descriptor stores to big-endian

[sashiko-bot]

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

New issues:
- [High] Bitwise OR operation on a __be32 field using a host-endian constant introduces data corruption.

Pre-existing issues:
- [High] Incomplete endianness conversion for command descriptor blocks leaves the driver broken on little-endian platforms.
--

--- Patch [1]: [PATCHv2 1/4] dma: fsl_raid: convert descriptor stores to big-endian ---
commit 4b4dfb0a314d465959f6268650dc8a30d7c69bd2
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: convert descriptor stores to big-endian

The descriptor structs (fsl_re_cmpnd_frame / fsl_re_hw_desc) are
in-memory but their fields are __be32, because the structures are handed
to the device as big-endian. The driver stored CPU-endian u32 values
into them directly, which is both wrong (the engine would see
byte-swapped lengths/addresses) and flagged by sparse as a base-type
mismatch. Wrap those stores in cpu_to_be32().

Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..2778d07a05e86 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -242,9 +242,9 @@ static void fill_cfd_frame(struct fsl_re_cmpnd_frame *cf, u8 index,
>  	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;
>  
>  	efrl |= final << FSL_RE_CF_FINAL_SHIFT;
> -	cf[index].efrl32 = efrl;
> -	cf[index].addr_high = upper_32_bits(addr);
> -	cf[index].addr_low = lower_32_bits(addr);
> +	cf[index].efrl32 = cpu_to_be32(efrl);

[Severity: High]
Since this function now writes a big-endian value to efrl32, doesn't this
cause a regression later when a host-endian bitwise OR is applied?

In fsl_re_prep_dma_genq():

	/* Setting the final bit in the last source buffer frame in CFD */
	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;

Since efrl32 now contains a byte-swapped value on little-endian architectures,
won't this host-endian bitwise OR place the final flag in the wrong byte and
corrupt the lower bits of the length field? 

The same pattern also occurs in fsl_re_prep_dma_pq().

[Severity: High]
This is a pre-existing issue, but while you are fixing endianness, should the
cdb32 field in the command descriptor blocks also be converted?

For example, in fsl_re_prep_dma_genq():

	xor = desc->cdb_addr;
	xor->cdb32 = cdb;

And similarly in fsl_re_prep_dma_pq() and fsl_re_prep_dma_memcpy(), a
host-endian u32 is assigned directly to the __be32 cdb32 field. Doesn't this
leave the driver broken on little-endian platforms?

> +	cf[index].addr_high = cpu_to_be32(upper_32_bits(addr));
> +	cf[index].addr_low = cpu_to_be32(lower_32_bits(addr));
>  }

[ ... ]

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

Re: [PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument

[sashiko-bot]

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

New issues:
- [High] Integer underflow leading to wild pointer dereference when src_cnt == 0.
- [Medium] The commit message claims to fix an endianness issue by setting a bit before a 'single cpu_to_be32() store', but this byte-swap is completely missing from the modified code.

Pre-existing issues:
- [High] Multiple severe out-of-bounds writes in `fsl_re_prep_dma_pq()` corrupt the Command Descriptor Block (CDB) when `DMA_PREP_CONTINUE` is set.
- [High] Missing IRQ cleanup leaves the interrupt handler active after module unload.
- [High] Incorrect teardown sequence frees DMA channel resources before unregistering the DMA device.
- [High] Ignored probe failures lead to uninitialized or partially initialized channels being torn down, causing NULL pointer dereferences or double frees.
- [High] Recursive locking deadlock when invoking DMA client callback.
- [High] Race condition due to prematurely enabling interrupts before initialization is complete.
- [High] DMA transfers of exactly 1MB will be silently truncated to 0 bytes due to `FSL_RE_CF_LENGTH_MASK` mismatch.
--

--- Patch [1]: [PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument ---
commit 62a6bad1aef1e87fcc87f1d390c2ed3be7152732
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: set final bit via fill_cfd_frame() argument

The final-frame bit is now passed as the "final" argument of
fill_cfd_frame() (as fsl_re_prep_dma_memcpy already did) and set in CPU
order before the single cpu_to_be32() store, replacing the previous
read-modify-write of the __be32 efrl32 field.

[Severity: Medium]
The commit message claims to fix an endianness issue by setting a bit before
a single cpu_to_be32() store, but this byte-swap appears to be missing from
the modified code.  Looking at fill_cfd_frame() in drivers/dma/fsl_raid.c,
the native integer is assigned directly to the __be32 field:

    cf[index].efrl32 = efrl;

Could this leave the type mismatch unresolved and the endianness bug in
place on little-endian hardware?

> ---
>  drivers/dma/fsl_raid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..a4feaa9a1c628 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -374,11 +374,11 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
>  	for (i = 2, j = 0; j < save_src_cnt; i++, j++)
>  		fill_cfd_frame(cf, i, len, src[j], 0);
>  
> +	/* Fill the last frame and mark it final */
>  	if (cont_q)
> -		fill_cfd_frame(cf, i++, len, dest, 0);
> -
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> +		fill_cfd_frame(cf, i, len, dest, 1);
> +	else
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);

[Severity: High]
Does this code cause an integer underflow when fsl_re_prep_dma_genq() is
called with src_cnt == 0?

If the loop initializing cf frames is completely skipped, j remains 0
(unsigned), causing j - 1 to underflow to UINT_MAX.  This could lead to a
wild pointer dereference when accessing src[j - 1].

>  	return &desc->async_tx;
>  }

[ ... ]

> @@ -504,16 +504,16 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
>  			p[save_src_cnt + 2] = 1;
>  			fill_cfd_frame(cf, i++, len, dest[0], 0);
>  			fill_cfd_frame(cf, i++, len, dest[1], 0);
> -			fill_cfd_frame(cf, i++, len, dest[1], 0);
> +			fill_cfd_frame(cf, i++, len, dest[1], 1);

[Severity: High]
This is a pre-existing issue, but does this continuation path write out of
bounds?

When dmaengine_prep_dma_pq is called with DMA_PREP_CONTINUE and a
src_cnt of 14 to 16, the driver artificially increments src_cnt by 3 (up to
19).  This causes the loop initializing gfm_q1 (and subsequently scf copying)
to overflow the fixed-size 16-byte arrays.

Additionally, the continuation frames are written to cf[19], cf[20], and
cf[21], which overflows the 20-frame limit and can corrupt the adjacent
hardware Command Descriptor Block (CDB) structure.

>  		} else {
>  			dev_err(re_chan->dev, "PQ tx continuation error!\n");
>  			return NULL;
>  		}
> +	} else {
> +		/* Mark the last source buffer frame final */
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);

[Severity: High]
Similar to the issue in fsl_re_prep_dma_genq(), does this access cause an
integer underflow if fsl_re_prep_dma_pq() is called with src_cnt == 0?

>  	}
>  
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> -
>  	return &desc->async_tx;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove_chan() leak the
interrupt handler after module unload?

The function kills the tasklet and frees memory, but fails to call
free_irq(chan->irq, chan->dev).  This could leave the ISR active after the
driver is unloaded, potentially causing a use-after-free on the next
hardware interrupt:

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
    ...
	tasklet_kill(&chan->irqtask);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove() free DMA channel
resources before unregistering the DMA device?

The teardown sequence iterates over channels to destroy their rings before
notifying the DMA core.  This creates a race condition where a concurrent
client might submit a new DMA request into already freed memory:

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	/* Cleanup chan related memory areas */
	for (i = 0; i < re_priv->total_chans; i++)
		fsl_re_remove_chan(re_priv->re_jrs[i]);
	
	/* Unregister the driver */
	dma_async_device_unregister(&re_priv->dma_dev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() ignore probe failures
and blindly increment the active channel count?

drivers/dma/fsl_raid.c:fsl_re_probe() {
    ...
		if (rc) {
			fsl_re_chan_probe(ofdev, child, ridx++, off);
			re_priv->total_chans++;
		}
    ...
}

If fsl_re_chan_probe() fails, re_jrs[i] remains NULL.  During unload, the
teardown loop unconditionally dereferences it:

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	for (i = 0; i < re_priv->total_chans; i++)
		fsl_re_remove_chan(re_priv->re_jrs[i]);
    ...
}

Could this lead to a NULL pointer dereference in tasklet_kill() if a channel
failed to probe?

[Severity: High]
This is a pre-existing issue, but can fsl_re_dequeue() cause a recursive
locking deadlock if the client submits a new transaction from the callback?

fsl_re_dequeue() acquires desc_lock and holds it while executing the client
callback:

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
	...
		fsl_re_desc_done(desc);
    ...
}

If the callback implements request chaining, fsl_re_tx_submit() will attempt
to synchronously re-acquire the exact same desc_lock:

drivers/dma/fsl_raid.c:fsl_re_tx_submit() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fsl_re_chan_probe() enable interrupts
before initialization is complete?

The IRQ is requested before drvdata is persistently linked to the device:

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
	...
	dev_set_drvdata(chandev, chan);
    ...
}

If a pending or spurious interrupt fires immediately, fsl_re_isr() will call
dev_get_drvdata(), which returns NULL, and then dereference it:

drivers/dma/fsl_raid.c:fsl_re_isr() {
    ...
	re_chan = dev_get_drvdata((struct device *)data);

	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
    ...
}

[Severity: High]
This is a pre-existing issue, but does fill_cfd_frame() silently truncate
1MB transfers to 0 bytes?

drivers/dma/fsl_raid.c:fill_cfd_frame() {
    ...
	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;
    ...
}

FSL_RE_MAX_DATA_LEN is defined as 1024*1024 (0x100000), allowing exactly 1MB.
However, if FSL_RE_CF_LENGTH_MASK is 0x000FFFFF (20 bits), the bitwise-AND
operation will silently truncate a 1MB transfer to 0.

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

Re: [PATCHv2 1/4] dma: fsl_raid: convert descriptor stores to big-endian

[Frank Li]

On Thu, Jul 16, 2026 at 01:29:46PM -0700, Rosen Penev wrote:
> The descriptor structs (fsl_re_cmpnd_frame / fsl_re_hw_desc) are
> in-memory but their fields are __be32, because the structures are handed
> to the device as big-endian. The driver stored CPU-endian u32 values
> into them directly, which is both wrong (the engine would see
> byte-swapped lengths/addresses) and flagged by sparse as a base-type
> mismatch.
>
> Wrap those stores in cpu_to_be32() so the values are little->big
> converted.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/fsl_raid.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5..2778d07a05e8 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -242,9 +242,9 @@ static void fill_cfd_frame(struct fsl_re_cmpnd_frame *cf, u8 index,
>  	u32 efrl = length & FSL_RE_CF_LENGTH_MASK;
>
>  	efrl |= final << FSL_RE_CF_FINAL_SHIFT;
> -	cf[index].efrl32 = efrl;
> -	cf[index].addr_high = upper_32_bits(addr);
> -	cf[index].addr_low = lower_32_bits(addr);
> +	cf[index].efrl32 = cpu_to_be32(efrl);
> +	cf[index].addr_high = cpu_to_be32(upper_32_bits(addr));
> +	cf[index].addr_low = cpu_to_be32(lower_32_bits(addr));
>  }
>
>  static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
> @@ -256,9 +256,10 @@ static struct fsl_re_desc *fsl_re_init_desc(struct fsl_re_chan *re_chan,
>  	dma_async_tx_descriptor_init(&desc->async_tx, &re_chan->chan);
>  	INIT_LIST_HEAD(&desc->node);
>
> -	desc->hwdesc.fmt32 = FSL_RE_FRAME_FORMAT << FSL_RE_HWDESC_FMT_SHIFT;
> -	desc->hwdesc.lbea32 = upper_32_bits(paddr);
> -	desc->hwdesc.addr_low = lower_32_bits(paddr);
> +	desc->hwdesc.fmt32 = cpu_to_be32(FSL_RE_FRAME_FORMAT <<
> +					  FSL_RE_HWDESC_FMT_SHIFT);
> +	desc->hwdesc.lbea32 = cpu_to_be32(upper_32_bits(paddr));
> +	desc->hwdesc.addr_low = cpu_to_be32(lower_32_bits(paddr));
>  	desc->cf_addr = cf;
>  	desc->cf_paddr = paddr;
>
> --
> 2.55.0
>

Re: [PATCHv2 2/4] dma: fsl_raid: set final bit via fill_cfd_frame() argument

[Frank Li]

On Thu, Jul 16, 2026 at 01:29:47PM -0700, Rosen Penev wrote:
> The final-frame bit is now passed as the "final" argument of
> fill_cfd_frame() (as fsl_re_prep_dma_memcpy already did) and set in CPU
> order before the single cpu_to_be32() store, replacing the previous
> read-modify-write of the __be32 efrl32 field.
>
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/fsl_raid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2778d07a05e8..888f55b672a5 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -375,11 +375,11 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_genq(
>  	for (i = 2, j = 0; j < save_src_cnt; i++, j++)
>  		fill_cfd_frame(cf, i, len, src[j], 0);
>
> +	/* Fill the last frame and mark it final */
>  	if (cont_q)
> -		fill_cfd_frame(cf, i++, len, dest, 0);
> -
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> +		fill_cfd_frame(cf, i, len, dest, 1);
> +	else
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);
>
>  	return &desc->async_tx;
>  }
> @@ -505,16 +505,16 @@ static struct dma_async_tx_descriptor *fsl_re_prep_dma_pq(
>  			p[save_src_cnt + 2] = 1;
>  			fill_cfd_frame(cf, i++, len, dest[0], 0);
>  			fill_cfd_frame(cf, i++, len, dest[1], 0);
> -			fill_cfd_frame(cf, i++, len, dest[1], 0);
> +			fill_cfd_frame(cf, i++, len, dest[1], 1);
>  		} else {
>  			dev_err(re_chan->dev, "PQ tx continuation error!\n");
>  			return NULL;
>  		}
> +	} else {
> +		/* Mark the last source buffer frame final */
> +		fill_cfd_frame(cf, i - 1, len, src[j - 1], 1);
>  	}
>
> -	/* Setting the final bit in the last source buffer frame in CFD */
> -	cf[i - 1].efrl32 |= 1 << FSL_RE_CF_FINAL_SHIFT;
> -
>  	return &desc->async_tx;
>  }
>
> --
> 2.55.0
>

Re: [PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

[Frank Li]

On Thu, Jul 16, 2026 at 01:29:48PM -0700, Rosen Penev wrote:
> The fsl_re_ctrl and fsl_re_chan_cfg structures describe memory-mapped
> RAID Engine registers accessed only via ioread32be()/iowrite32be(), yet
> the pointers to them (re_regs in struct fsl_re_drv_private, and jrregs
> in struct fsl_re_chan) were not __iomem-qualified, so sparse emitted
> "different address spaces" warnings for every register access.
>
> Store both MMIO bases as a plain void __iomem * and derive jrregs with
> void __iomem * arithmetic from re_regs, rather than carrying typed
                                 ^ base

> register struct pointers through the driver. Each function that touches
> the registers introduces a local typed pointer (struct fsl_re_ctrl
> __iomem *ctrl) and uses ->field, which is the idiomatic kernel pattern
>  and keeps the registers' __iomem qualification intact.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/fsl_raid.c | 19 ++++++++++---------
>  drivers/dma/fsl_raid.h |  4 ++--
>  2 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 888f55b672a5..524f83faf3da 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -657,8 +657,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  		goto err_free;
>  	}
>
> -	chan->jrregs = (struct fsl_re_chan_cfg *)((u8 *)re_priv->re_regs +
> -			off + ptr);
> +	chan->jrregs = re_priv->base + off + ptr;
>
>  	/* read irq property from dts */
>  	chan->irq = irq_of_parse_and_map(np, 0);
> @@ -746,6 +745,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  /* Probe function for RAID Engine */
>  static int fsl_re_probe(struct platform_device *ofdev)
>  {
> +	struct fsl_re_ctrl __iomem *re_regs;
>  	struct fsl_re_drv_private *re_priv;
>  	struct device_node *child;
>  	u32 off;
> @@ -764,20 +764,21 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		return -ENODEV;
>
>  	/* IOMAP the entire RAID Engine region */
> -	re_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!re_priv->re_regs)
> +	re_regs = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!re_regs)
>  		return -EBUSY;
> +	re_priv->base = re_regs;
>
>  	/* Program the RE mode */
> -	out_be32(&re_priv->re_regs->global_config, FSL_RE_NON_DPAA_MODE);
> +	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);
>
>  	/* Program Galois Field polynomial */
> -	out_be32(&re_priv->re_regs->galois_field_config, FSL_RE_GFM_POLY);
> +	out_be32(&re_regs->galois_field_config, FSL_RE_GFM_POLY);
>
>  	dev_info(dev, "version %x, mode %x, gfp %x\n",
> -		 in_be32(&re_priv->re_regs->re_version_id),
> -		 in_be32(&re_priv->re_regs->global_config),
> -		 in_be32(&re_priv->re_regs->galois_field_config));
> +		 in_be32(&re_regs->re_version_id),
> +		 in_be32(&re_regs->global_config),
> +		 in_be32(&re_regs->galois_field_config));
>
>  	dma_dev = &re_priv->dma_dev;
>  	dma_dev->dev = dev;
> diff --git a/drivers/dma/fsl_raid.h b/drivers/dma/fsl_raid.h
> index 69d743c04973..adbfede330a7 100644
> --- a/drivers/dma/fsl_raid.h
> +++ b/drivers/dma/fsl_raid.h
> @@ -256,7 +256,7 @@ struct fsl_re_hw_desc {
>  struct fsl_re_drv_private {
>  	u8 total_chans;
>  	struct dma_device dma_dev;
> -	struct fsl_re_ctrl *re_regs;
> +	void __iomem *base;
>  	struct fsl_re_chan *re_jrs[FSL_RE_MAX_CHANS];
>  	struct dma_pool *cf_desc_pool;
>  	struct dma_pool *hw_desc_pool;
> @@ -273,7 +273,7 @@ struct fsl_re_chan {
>  	struct device *dev;
>  	struct fsl_re_drv_private *re_dev;
>  	struct dma_chan chan;
> -	struct fsl_re_chan_cfg *jrregs;
> +	struct fsl_re_chan_cfg __iomem *jrregs;
>  	int irq;
>  	struct tasklet_struct irqtask;
>  	u32 alloc_count;
> --
> 2.55.0
>