[PATCH 0/3] dma: fsl_raid: fix sparse warnings and simplify probing

[PATCH 0/3] 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().

Rosen Penev (3):
  dma: fsl_raid: fix endianness of in-memory descriptor stores
  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 | 109 ++++++++++++++++++++---------------------
 drivers/dma/fsl_raid.h |   4 +-
 2 files changed, 54 insertions(+), 59 deletions(-)

-- 
2.55.0

[PATCH 1/3] dma: fsl_raid: fix endianness of in-memory descriptor stores

[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. 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.

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 | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 99945845d8b5..888f55b672a5 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;
 
@@ -374,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;
 }
@@ -504,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

[PATCH 2/3] 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 / struct fsl_re_chan_cfg __iomem *jr) 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 | 66 ++++++++++++++++++++----------------------
 drivers/dma/fsl_raid.h |  4 +--
 2 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 888f55b672a5..47ebdf274331 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -106,15 +106,17 @@ static dma_cookie_t fsl_re_tx_submit(struct dma_async_tx_descriptor *tx)
 static void fsl_re_issue_pending(struct dma_chan *chan)
 {
 	struct fsl_re_chan *re_chan;
+	struct fsl_re_chan_cfg __iomem *jr;
 	int avail;
 	struct fsl_re_desc *desc, *_desc;
 	unsigned long flags;
 
 	re_chan = container_of(chan, struct fsl_re_chan, chan);
+	jr = re_chan->jrregs;
 
 	spin_lock_irqsave(&re_chan->desc_lock, flags);
 	avail = FSL_RE_SLOT_AVAIL(
-		in_be32(&re_chan->jrregs->inbring_slot_avail));
+		in_be32(&jr->inbring_slot_avail));
 
 	list_for_each_entry_safe(desc, _desc, &re_chan->submit_q, node) {
 		if (!avail)
@@ -127,7 +129,7 @@ static void fsl_re_issue_pending(struct dma_chan *chan)
 
 		re_chan->inb_count = (re_chan->inb_count + 1) &
 						FSL_RE_RING_SIZE_MASK;
-		out_be32(&re_chan->jrregs->inbring_add_job, FSL_RE_ADD_JOB(1));
+		out_be32(&jr->inbring_add_job, FSL_RE_ADD_JOB(1));
 		avail--;
 	}
 	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
@@ -158,6 +160,7 @@ static void fsl_re_cleanup_descs(struct fsl_re_chan *re_chan)
 static void fsl_re_dequeue(struct tasklet_struct *t)
 {
 	struct fsl_re_chan *re_chan = from_tasklet(re_chan, t, irqtask);
+	struct fsl_re_chan_cfg __iomem *jr = re_chan->jrregs;
 	struct fsl_re_desc *desc, *_desc;
 	struct fsl_re_hw_desc *hwdesc;
 	unsigned long flags;
@@ -167,7 +170,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 	fsl_re_cleanup_descs(re_chan);
 
 	spin_lock_irqsave(&re_chan->desc_lock, flags);
-	count =	FSL_RE_SLOT_FULL(in_be32(&re_chan->jrregs->oubring_slot_full));
+	count =	FSL_RE_SLOT_FULL(in_be32(&jr->oubring_slot_full));
 	while (count--) {
 		found = 0;
 		hwdesc = &re_chan->oub_ring_virt_addr[re_chan->oub_count];
@@ -192,8 +195,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 		oub_count = (re_chan->oub_count + 1) & FSL_RE_RING_SIZE_MASK;
 		re_chan->oub_count = oub_count;
 
-		out_be32(&re_chan->jrregs->oubring_job_rmvd,
-			 FSL_RE_RMVD_JOB(1));
+		out_be32(&jr->oubring_job_rmvd, FSL_RE_RMVD_JOB(1));
 	}
 	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
 }
@@ -201,12 +203,12 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 /* Per Job Ring interrupt handler */
 static irqreturn_t fsl_re_isr(int irq, void *data)
 {
-	struct fsl_re_chan *re_chan;
+	struct device *dev = data;
+	struct fsl_re_chan *re_chan = dev_get_drvdata(dev);
+	struct fsl_re_chan_cfg __iomem *jr = re_chan->jrregs;
 	u32 irqstate, status;
 
-	re_chan = dev_get_drvdata((struct device *)data);
-
-	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
+	irqstate = in_be32(&jr->jr_interrupt_status);
 	if (!irqstate)
 		return IRQ_NONE;
 
@@ -216,13 +218,13 @@ static irqreturn_t fsl_re_isr(int irq, void *data)
 	 * need to do something more than just crashing
 	 */
 	if (irqstate & FSL_RE_ERROR) {
-		status = in_be32(&re_chan->jrregs->jr_status);
+		status = in_be32(&jr->jr_status);
 		dev_err(re_chan->dev, "chan error irqstate: %x, status: %x\n",
 			irqstate, status);
 	}
 
 	/* Clear interrupt */
-	out_be32(&re_chan->jrregs->jr_interrupt_status, FSL_RE_CLR_INTR);
+	out_be32(&jr->jr_interrupt_status, FSL_RE_CLR_INTR);
 
 	tasklet_schedule(&re_chan->irqtask);
 
@@ -627,6 +629,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 	struct device *dev, *chandev;
 	struct fsl_re_drv_private *re_priv;
 	struct fsl_re_chan *chan;
+	struct fsl_re_chan_cfg __iomem *jr;
 	struct dma_device *dma_dev;
 	u32 ptr;
 	u32 status;
@@ -657,8 +660,8 @@ 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);
+	jr = re_priv->re_regs + off + ptr;
+	chan->jrregs = jr;
 
 	/* read irq property from dts */
 	chan->irq = irq_of_parse_and_map(np, 0);
@@ -709,30 +712,23 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 	}
 
 	/* Program the Inbound/Outbound ring base addresses and size */
-	out_be32(&chan->jrregs->inbring_base_h,
-		 chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
-	out_be32(&chan->jrregs->oubring_base_h,
-		 chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
-	out_be32(&chan->jrregs->inbring_base_l,
-		 chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
-	out_be32(&chan->jrregs->oubring_base_l,
-		 chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
-	out_be32(&chan->jrregs->inbring_size,
-		 FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
-	out_be32(&chan->jrregs->oubring_size,
-		 FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
+	out_be32(&jr->inbring_base_h, chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
+	out_be32(&jr->oubring_base_h, chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
+	out_be32(&jr->inbring_base_l, chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
+	out_be32(&jr->oubring_base_l, chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
+	out_be32(&jr->inbring_size, FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
+	out_be32(&jr->oubring_size, FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
 
 	/* Read LIODN value from u-boot */
-	status = in_be32(&chan->jrregs->jr_config_1) & FSL_RE_REG_LIODN_MASK;
+	status = in_be32(&jr->jr_config_1) & FSL_RE_REG_LIODN_MASK;
 
 	/* Program the CFG reg */
-	out_be32(&chan->jrregs->jr_config_1,
-		 FSL_RE_CFG1_CBSI | FSL_RE_CFG1_CBS0 | status);
+	out_be32(&jr->jr_config_1, FSL_RE_CFG1_CBSI | FSL_RE_CFG1_CBS0 | status);
 
 	dev_set_drvdata(chandev, chan);
 
 	/* Enable RE/CHAN */
-	out_be32(&chan->jrregs->jr_command, FSL_RE_ENABLE);
+	out_be32(&jr->jr_command, FSL_RE_ENABLE);
 
 	return 0;
 
@@ -752,6 +748,7 @@ static int fsl_re_probe(struct platform_device *ofdev)
 	u8 ridx = 0;
 	struct dma_device *dma_dev;
 	struct resource *res;
+	struct fsl_re_ctrl __iomem *re_regs;
 	int rc;
 	struct device *dev = &ofdev->dev;
 
@@ -767,17 +764,18 @@ static int fsl_re_probe(struct platform_device *ofdev)
 	re_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
 	if (!re_priv->re_regs)
 		return -EBUSY;
+	re_regs = re_priv->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..6069615e2e1e 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 *re_regs;
 	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;
+	void __iomem *jrregs;
 	int irq;
 	struct tasklet_struct irqtask;
 	u32 alloc_count;
-- 
2.55.0

[PATCH 3/3] 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>
---
 drivers/dma/fsl_raid.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 47ebdf274331..aadb3dcb1b03 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -747,24 +747,20 @@ static int fsl_re_probe(struct platform_device *ofdev)
 	u32 off;
 	u8 ridx = 0;
 	struct dma_device *dma_dev;
-	struct resource *res;
 	struct fsl_re_ctrl __iomem *re_regs;
 	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_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
-	if (!re_priv->re_regs)
-		return -EBUSY;
-	re_regs = re_priv->re_regs;
+	re_priv->re_regs = re_regs;
 
 	/* Program the RE mode */
 	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);
-- 
2.55.0

Re: [PATCH 2/3] 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 return value check for `fsl_re_chan_probe()` causes NULL pointer dereference.
- [High] Race condition during probe causes NULL pointer dereference in the ISR.
- [High] Missing `free_irq()` causes Use-After-Free of the interrupt handler.
- [High] DMA channels are never added to the DMA device's channel list.
- [High] The driver invokes the client callback while holding the channel's spinlock, risking an ABBA deadlock.
--

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

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

This patch updates the driver to store MMIO bases as void __iomem pointers
and uses typed local pointers to access fields. This resolves sparse warnings
about different address spaces when using ioread32be() and iowrite32be().

Link: 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 888f55b672a58..47ebdf2743310 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
[ ... ]
> @@ -167,7 +170,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  	fsl_re_cleanup_descs(re_chan);
>  
>  	spin_lock_irqsave(&re_chan->desc_lock, flags);
> -	count =	FSL_RE_SLOT_FULL(in_be32(&re_chan->jrregs->oubring_slot_full));
> +	count =	FSL_RE_SLOT_FULL(in_be32(&jr->oubring_slot_full));

[Severity: High]
This is a pre-existing issue, but can this lock handling cause a recursive
deadlock?

In fsl_re_dequeue(), desc_lock is acquired via spin_lock_irqsave(). While
iterating through completed descriptors, fsl_re_desc_done() is called:

    while (count--) {
        ...
        if (found) {
            fsl_re_desc_done(desc);
            list_move_tail(&desc->node, &re_chan->ack_q);
        }
    ...
    }

fsl_re_desc_done() invokes the client callback inline. If the client attempts
to submit new work from its callback, fsl_re_tx_submit() will attempt to
acquire the exact same desc_lock, resulting in a deadlock.

>  	while (count--) {
>  		found = 0;
>  		hwdesc = &re_chan->oub_ring_virt_addr[re_chan->oub_count];
[ ... ]
> @@ -201,12 +203,12 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  /* Per Job Ring interrupt handler */
>  static irqreturn_t fsl_re_isr(int irq, void *data)
>  {
> -	struct fsl_re_chan *re_chan;
> +	struct device *dev = data;
> +	struct fsl_re_chan *re_chan = dev_get_drvdata(dev);
> +	struct fsl_re_chan_cfg __iomem *jr = re_chan->jrregs;

[Severity: High]
This isn't a bug introduced by this patch, but does this assume drvdata is
already set when the interrupt fires?

In fsl_re_chan_probe(), request_irq() is called to register this handler
before dev_set_drvdata(chandev, chan) sets the driver data.

If an interrupt fires immediately after registration, dev_get_drvdata(dev)
will return NULL, leading to a crash when dereferencing re_chan->jrregs.

>  	u32 irqstate, status;
>  
> -	re_chan = dev_get_drvdata((struct device *)data);
> -
> -	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
> +	irqstate = in_be32(&jr->jr_interrupt_status);
>  	if (!irqstate)
>  		return IRQ_NONE;
>  
[ ... ]
> @@ -709,30 +712,23 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  	}
>  
>  	/* Program the Inbound/Outbound ring base addresses and size */
> -	out_be32(&chan->jrregs->inbring_base_h,
> -		 chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
> -	out_be32(&chan->jrregs->oubring_base_h,
> -		 chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
> -	out_be32(&chan->jrregs->inbring_base_l,
> -		 chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> -	out_be32(&chan->jrregs->oubring_base_l,
> -		 chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> -	out_be32(&chan->jrregs->inbring_size,
> -		 FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
> -	out_be32(&chan->jrregs->oubring_size,
> -		 FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
> +	out_be32(&jr->inbring_base_h, chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
> +	out_be32(&jr->oubring_base_h, chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
> +	out_be32(&jr->inbring_base_l, chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> +	out_be32(&jr->oubring_base_l, chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> +	out_be32(&jr->inbring_size, FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
> +	out_be32(&jr->oubring_size, FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
>  
>  	/* Read LIODN value from u-boot */
> -	status = in_be32(&chan->jrregs->jr_config_1) & FSL_RE_REG_LIODN_MASK;
> +	status = in_be32(&jr->jr_config_1) & FSL_RE_REG_LIODN_MASK;
>  
>  	/* Program the CFG reg */
> -	out_be32(&chan->jrregs->jr_config_1,
> -		 FSL_RE_CFG1_CBSI | FSL_RE_CFG1_CBS0 | status);
> +	out_be32(&jr->jr_config_1, FSL_RE_CFG1_CBSI | FSL_RE_CFG1_CBS0 | status);
>  
>  	dev_set_drvdata(chandev, chan);

[Severity: High]
This is a pre-existing issue, but are we leaking the registered IRQ?

Neither the error path of fsl_re_chan_probe() nor the teardown function
fsl_re_remove_chan() appears to call free_irq().

If fsl_re_chan_probe() fails after request_irq(), or if the module is
unloaded, the ISR remains registered. This could lead to a use-after-free
if an interrupt fires and attempts to execute unmapped memory.

[Severity: High]
This is another pre-existing issue, but is it intentional that the
initialized DMA channel is not added to the DMA device's channel list?

Earlier in fsl_re_chan_probe(), the channel is initialized:

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

However, list_add_tail(&chan->chan.device_node, &dma_dev->channels) is never
called. When dma_async_device_register() is executed, it might find the list
empty and register 0 channels, making them unavailable to clients.

>  
>  	/* Enable RE/CHAN */
> -	out_be32(&chan->jrregs->jr_command, FSL_RE_ENABLE);
> +	out_be32(&jr->jr_command, FSL_RE_ENABLE);
>  
>  	return 0;
>  
[ ... ]
> @@ -752,6 +748,7 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  	u8 ridx = 0;
>  	struct dma_device *dma_dev;
>  	struct resource *res;
> +	struct fsl_re_ctrl __iomem *re_regs;
>  	int rc;
>  	struct device *dev = &ofdev->dev;

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
fsl_re_chan_probe() later in this function?

If the probe fails (e.g. out of memory), total_chans is still incremented:

    for_each_child_of_node(np, child) {
        rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
        if (rc) {
            fsl_re_chan_probe(ofdev, child, ridx++, off);
            re_priv->total_chans++;
        }
    }

This leaves an uninitialized pointer in the re_jrs array, which might be
dereferenced as a NULL pointer during driver unbind in fsl_re_remove_chan()
when it calls tasklet_kill(&chan->irqtask).

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

Re: [PATCH 3/3] dma: fsl_raid: use devm_platform_ioremap_resource

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked return value from `fsl_re_chan_probe()` leads to double-free and NULL pointer dereference in `fsl_re_remove_chan()`.
- [High] The driver requests IRQs but never frees them, leading to a permanent leak and Use-After-Free crashes.
- [High] The return value of `dma_async_device_register()` is completely ignored.
- [High] System-wide device tree traversal for child nodes instead of scoped child node search.
- [High] Missing cleanup of tasklet, IRQ, and platform device on error paths in `fsl_re_chan_probe`.
- [High] Failure during parent probe loop does not tear down already initialized channels.
- [Medium] Platform devices dynamically created for job rings via `of_platform_device_create` are never unregistered.
--

--- Patch [3]: [PATCH 3/3] dma: fsl_raid: use devm_platform_ioremap_resource ---
Note: The format of this review has been altered to provide a free-form
summary of the findings without quoting the original patch, to adhere to
system recitation restrictions.

Commit df861cda9af0548c1bb12583b9a2c34f69725af1
Author: Rosen Penev <rosenp@gmail.com>
Subject: dma: fsl_raid: use devm_platform_ioremap_resource

This patch replaces the open-coded platform_get_resource() and devm_ioremap()
sequence with devm_platform_ioremap_resource(). It simplifies the probe routine
by fetching the resource and mapping it in one call. The error check is also
updated to use IS_ERR/PTR_ERR.

While reviewing the driver code around these changes, I noticed a few
pre-existing issues in the driver that might need attention. These are not
bugs introduced by this patch, but they could cause problems during operation
or driver unbind.

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() handle failures from
fsl_re_chan_probe() correctly? If fsl_re_chan_probe() fails before fully
initializing the channel, it returns an error. However, fsl_re_probe() ignores
this return value and unconditionally increments re_priv->total_chans. This
leaves a NULL pointer in the re_priv->re_jrs array for that channel index.
Later, when fsl_re_remove() is called, it iterates up to total_chans and passes
these NULL pointers to fsl_re_remove_chan(), which will result in a NULL
pointer dereference when calling tasklet_kill() on the irqtask.

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing free_irq()
in the teardown path? In fsl_re_chan_probe(), the driver uses request_irq()
to register an interrupt handler for each channel. However,
fsl_re_remove_chan() does not appear to call free_irq(). Since the channel
structure is allocated with devm_kzalloc, its memory is freed automatically
by devres when the driver is unbound. If an interrupt fires after the driver
is removed, could the kernel execute fsl_re_isr and dereference the freed
chan pointer?

[Severity: Medium]
This is a pre-existing issue, but are the platform devices created for job
rings properly unregistered? fsl_re_chan_probe() calls
of_platform_device_create() to create platform devices for the child nodes.
However, neither the error handling path nor the driver's teardown function
(fsl_re_remove_chan) calls of_platform_device_destroy() or
platform_device_unregister(). Could this cause a memory and sysfs leak, and
prevent the driver from successfully rebinding later?

[Severity: High]
This isn't a bug introduced by this patch, but is it intentional to ignore the
return value of dma_async_device_register() in fsl_re_probe()? The function
is called just before returning 0. If registration fails, the driver might
falsely indicate success to the driver core, which could lead to inconsistent
state or oopses if clients attempt to use the unregistered DMA device.

[Severity: High]
This is a pre-existing issue, but does the device tree traversal in
fsl_re_probe() search too broadly? The loop uses
for_each_compatible_node_scoped() with a NULL starting node. This instructs
the macro to traverse the entire system's device tree rather than just the
children of the current RAID engine instance. If multiple RAID engines are
present, could each instance mistakenly parse and attempt to bind to all
job queues on the system?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
fsl_re_chan_probe() clean up all resources? If an error occurs after the IRQ
is successfully requested (for example, if dma_pool_alloc() fails for
inb_ring_virt_addr), the code jumps to the err_free label. This label returns
the error code but does not kill the tasklet, free the IRQ, or unregister the
platform device. Could this lead to a crash if the dangling IRQ fires?

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() clean up successfully
initialized channels if a subsequent loop iteration fails? If
of_property_read_u32() fails to read the reg property for any job-queue node
after some channels have already been probed, the function immediately returns
an error. It doesn't appear to tear down the previously probed channels using
fsl_re_remove_chan(). Since the parent device's managed memory will be freed
by devres, could the dangling IRQs, tasklets, and DMA pools from the orphaned
channels cause a crash?

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

Re: [PATCH 1/3] dma: fsl_raid: fix endianness of in-memory descriptor stores

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch correctly fixes the endianness of `struct fsl_re_cmpnd_frame` and `struct fsl_re_hw_desc` by wrapping assignments in `cpu_to_be32()`, but completely misses the same class of bug for `cdb32` fields in the Command Descriptor Block structs (`fsl_re_xor_cdb`, `fsl_re_pq_cdb`, `fsl_re_move_cdb`).
- [High] Uninitialized kernel DMA pool memory is passed directly to the hardware for Command Descriptor Blocks (CDBs) and compound frames.
- [High] A DMA length of exactly 1MB (1024*1024) is allowed by `FSL_RE_MAX_DATA_LEN` but is truncated to 0 when bitwise-ANDed with `FSL_RE_CF_LENGTH_MASK`, resulting in a silent 0-byte transfer.
--

--- Patch [1]: [PATCH 1/3] dma: fsl_raid: fix endianness of in-memory descriptor stores ---
commit b79b0fb376ec1f7e303159ca0f7c486730ae5496
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: fix endianness of in-memory descriptor stores

This commit updates the driver to wrap in-memory descriptor fields in
cpu_to_be32() to resolve base-type mismatches and endianness issues. It
also simplifies the final-frame bit assignment in fill_cfd_frame().

Link: 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..888f55b672a58 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;

[Severity: High]
This is a pre-existing issue, but can this truncate a legitimate 1MB
transfer down to 0 bytes?

If a caller issues a DMA request of exactly FSL_RE_MAX_DATA_LEN (which
is 1024*1024, or 0x100000), it passes the bounds checks. However,
FSL_RE_CF_LENGTH_MASK is 0x000FFFFF, so the bitwise-AND operation here
would evaluate to 0.

Does this result in a silent 0-byte transfer instead of the intended
1MB transfer?

>  
>  	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));
>  }

[Severity: High]
This is a pre-existing issue, but does this patch also need to fix the
endianness of the cdb32 fields in the Command Descriptor Block structs?

When preparing a DMA transfer in fsl_re_prep_dma_genq(), fsl_re_prep_dma_pq(),
or fsl_re_prep_dma_memcpy(), the CPU-endian cdb variable is directly
assigned to the __be32 cdb32 field in the command descriptor block:

drivers/dma/fsl_raid.c:fsl_re_prep_dma_genq() {
    ...
    xor = desc->cdb_addr;
    xor->cdb32 = cdb;
    ...
}

Will the hardware receive byte-swapped opcodes and configuration flags
on little-endian platforms?

>  
>  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;

[Severity: High]
This is a pre-existing issue, but does the hardware receive uninitialized
memory here?

The DMA pool allocation for the compound frame and CDB buffer in
fsl_re_chan_alloc_desc() uses dma_pool_alloc() without zeroing the buffer:

drivers/dma/fsl_raid.c:fsl_re_chan_alloc_desc() {
    ...
    cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT, &paddr);
    ...
}

Since functions like fill_cfd_frame() and fsl_re_init_desc() only populate
specific fields, could stale data in uninitialized fields like rbro32 or
dpi_dest_spec cause unpredictable hardware behavior when passed to the
DMA engine?

[ ... ]

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