dmaengine: fsl_raid: replace in_be32/out_be32 with ioread32be/iowrite32be

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

Pre-existing issues:

- [Critical] request_irq is used without devm_ and is never freed on error paths or during device removal, leading to Use-After-Free in the interrupt handler.
- [High] The driver configures a 40-bit DMA mask but incorrectly programs the 64-bit ring base addresses, truncating upper address bits and incorrectly placing the lowest 4 bits into the high address register, causing silent memory corruption for high memory allocations.
- [High] Deadlock in fsl_re_dequeue due to invoking DMA client callback with the channel spinlock held.
- [High] Unchecked return value of fsl_re_chan_probe causes a NULL pointer dereference during device removal.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: replace in_be32/out_be32 with ioread32be/iowrite32be --- commit 9a55b4c8c34b72fe5484852d6708a7ea677fc0df
Author: Rosen Penev <[email protected]>

dmaengine: fsl_raid: replace in_be32/out_be32 with ioread32be/iowrite32be

This commit replaces ppc4xx-specific accessors (in_be32/out_be32) with generic portable helpers (ioread32be/iowrite32be) in the fsl_raid driver. It also enables COMPILE_TEST for extra compile coverage.

Quoted 4 lines

drivers/dma/fsl_raid.c +0 -0

index 2d86f61105e52..e559c636bf552 100644

[ ... ]

drivers/dma/fsl_raid.c static void fsl_re_dequeue(struct tasklet_struct *t) · line 170

Quoted 6 lines

drivers/dma/fsl_raid.c +1 -1

@@ -167,7 +167,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(ioread32be(&re_chan->jrregs->oubring_slot_full));

[Severity: High]
This is a pre-existing issue, but can this lead to a deadlock?

Looking at fsl_re_dequeue(), it acquires the desc_lock spinlock using spin_lock_irqsave() and holds it while processing completed descriptors:

spin_lock_irqsave(&re_chan->desc_lock, flags);
...

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

The fsl_re_desc_done() function synchronously invokes the client callback via dmaengine_desc_get_callback_invoke().

If the DMA client callback submits a new request, would it call fsl_re_tx_submit() and attempt to reacquire the exact same desc_lock, resulting in a deadlock?

Quoted 3 lines

drivers/dma/fsl_raid.c +0 -0

      while (count--) {
              found = 0;
              hwdesc = &re_chan->oub_ring_virt_addr[re_chan->oub_count];

[ ... ]

Quoted 10 lines

drivers/dma/fsl_raid.c +0 -6

@@ -708,30 +707,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);

drivers/dma/fsl_raid.c static int fsl_re_chan_probe(struct platform_device *ofdev, · line 713

Quoted 10 lines

drivers/dma/fsl_raid.c +4 -6

-     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);
+     iowrite32be(chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK, &chan->jrregs->inbring_base_h);
+     iowrite32be(chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK, &chan->jrregs->oubring_base_h);
+     iowrite32be(chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT, &chan->jrregs->inbring_base_l);
+     iowrite32be(chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT, &chan->jrregs->oubring_base_l);

[Severity: High]
This is a pre-existing issue, but does this correctly program the 64-bit DMA base addresses?

The driver advertises 40-bit DMA address support in fsl_re_probe() by calling:

dma_set_mask(dev, DMA_BIT_MASK(40));

However, when writing the physical addresses here in fsl_re_chan_probe(), it shifts the address right by FSL_RE_ADDR_BIT_SHIFT into inbring_base_l, and extracts the lowest bits via FSL_RE_ADDR_BIT_MASK into inbring_base_h.

If a DMA buffer is allocated above the 64GB boundary, will this drop the upper bits and extract the lowest bits into the high register, leading to the hardware reading or writing the wrong physical memory?

[Severity: Critical]
This is a pre-existing issue, but is there a missing IRQ cleanup here?

Earlier in fsl_re_chan_probe(), the driver requests an interrupt without using the managed devm_ wrapper:

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

If fsl_re_chan_probe() fails later, or if the driver is unbound and fsl_re_remove_chan() is called, free_irq() is never called to unregister the handler.

Since the chan structure is allocated with devm_kzalloc, will a dangling interrupt firing after the device is removed cause fsl_re_isr() to dereference the freed chan pointer and crash?

Quoted 2 lines

drivers/dma/fsl_raid.c +2 -0

+     iowrite32be(FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT, &chan->jrregs->inbring_size);
+     iowrite32be(FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT, &chan->jrregs->oubring_size);

[ ... ]

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 761

Quoted 6 lines

drivers/dma/fsl_raid.c +1 -1

@@ -766,15 +758,15 @@ static int fsl_re_probe(struct platform_device *ofdev)
      re_priv->base = re_regs;
 
      /* Program the RE mode */
-     out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);
+     iowrite32be(FSL_RE_NON_DPAA_MODE, &re_regs->global_config);

[Severity: High]
This is a pre-existing issue, but are we missing an error check on the return value of fsl_re_chan_probe()?

Later in fsl_re_probe(), the driver iterates over child nodes and calls fsl_re_chan_probe(), but ignores the return value and unconditionally increments total_chans:

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

If fsl_re_chan_probe() fails (for example, failing to map an IRQ), the array element re_priv->re_jrs[ridx] remains uninitialized.

During module unload, fsl_re_remove_chan() unconditionally dereferences the pointer when calling tasklet_kill(&chan->irqtask).

Will this result in a NULL pointer dereference on cleanup if any channel failed to probe?

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 764

Quoted 4 lines

drivers/dma/fsl_raid.c +1 -1

      /* Program Galois Field polynomial */
-     out_be32(&re_regs->galois_field_config, FSL_RE_GFM_POLY);
+     iowrite32be(FSL_RE_GFM_POLY, &re_regs->galois_field_config);

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Mechanical conversion of the ppc4xx-specific accessors to the generic portable helpers.

As a result, enable COMPILE_TEST for extra compile coverage.

Assisted-by opencode:big-pickle

Signed-off-by Rosen Penev <[email protected]>

---

 drivers/dma/Kconfig    |  2 +-
 drivers/dma/fsl_raid.c | 50 ++++++++++++++++++------------------------
 2 files changed, 22 insertions(+), 30 deletions(-)

drivers/dma/Kconfig +1 -1

index cb92687294c3..dbdb35f6610a 100644
@@ -242,7 +242,7 @@ config FSL_QDMA
 
 config FSL_RAID
 	tristate "Freescale RAID engine Support"
-	depends on FSL_SOC && !ASYNC_TX_ENABLE_CHANNEL_SWITCH
+	depends on (FSL_SOC && !ASYNC_TX_ENABLE_CHANNEL_SWITCH) || COMPILE_TEST
 	select DMA_ENGINE
 	select DMA_ENGINE_RAID
 	help

drivers/dma/fsl_raid.c +21 -30

index 2d86f61105e5..e559c636bf55 100644
@@ -114,7 +114,7 @@ static void fsl_re_issue_pending(struct dma_chan *chan)
 
 	spin_lock_irqsave(&re_chan->desc_lock, flags);
 	avail = FSL_RE_SLOT_AVAIL(
-		in_be32(&re_chan->jrregs->inbring_slot_avail));
+		ioread32be(&re_chan->jrregs->inbring_slot_avail));
 
 	list_for_each_entry_safe(desc, _desc, &re_chan->submit_q, node) {
 		if (!avail)

@@ -127,7 +127,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));
+		iowrite32be(FSL_RE_ADD_JOB(1), &re_chan->jrregs->inbring_add_job);
 		avail--;
 	}
 	spin_unlock_irqrestore(&re_chan->desc_lock, flags);

@@ -167,7 +167,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(ioread32be(&re_chan->jrregs->oubring_slot_full));
 	while (count--) {
 		found = 0;
 		hwdesc = &re_chan->oub_ring_virt_addr[re_chan->oub_count];

@@ -192,8 +192,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));
+		iowrite32be(FSL_RE_RMVD_JOB(1), &re_chan->jrregs->oubring_job_rmvd);
 	}
 	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
 }

@@ -206,7 +205,7 @@ static irqreturn_t fsl_re_isr(int irq, void *data)
 
 	re_chan = dev_get_drvdata((struct device *)data);
 
-	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
+	irqstate = ioread32be(&re_chan->jrregs->jr_interrupt_status);
 	if (!irqstate)
 		return IRQ_NONE;
 

@@ -216,13 +215,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 = ioread32be(&re_chan->jrregs->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);
+	iowrite32be(FSL_RE_CLR_INTR, &re_chan->jrregs->jr_interrupt_status);
 
 	tasklet_schedule(&re_chan->irqtask);
 

@@ -708,30 +707,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);
+	iowrite32be(chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK, &chan->jrregs->inbring_base_h);
+	iowrite32be(chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK, &chan->jrregs->oubring_base_h);
+	iowrite32be(chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT, &chan->jrregs->inbring_base_l);
+	iowrite32be(chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT, &chan->jrregs->oubring_base_l);
+	iowrite32be(FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT, &chan->jrregs->inbring_size);
+	iowrite32be(FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT, &chan->jrregs->oubring_size);
 
 	/* Read LIODN value from u-boot */
-	status = in_be32(&chan->jrregs->jr_config_1) & FSL_RE_REG_LIODN_MASK;
+	status = ioread32be(&chan->jrregs->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);
+	iowrite32be(FSL_RE_CFG1_CBSI | FSL_RE_CFG1_CBS0 | status, &chan->jrregs->jr_config_1);
 
 	dev_set_drvdata(chandev, chan);
 
 	/* Enable RE/CHAN */
-	out_be32(&chan->jrregs->jr_command, FSL_RE_ENABLE);
+	iowrite32be(FSL_RE_ENABLE, &chan->jrregs->jr_command);
 
 	return 0;
 

@@ -766,15 +758,15 @@ static int fsl_re_probe(struct platform_device *ofdev)
 	re_priv->base = re_regs;
 
 	/* Program the RE mode */
-	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);
+	iowrite32be(FSL_RE_NON_DPAA_MODE, &re_regs->global_config);
 
 	/* Program Galois Field polynomial */
-	out_be32(&re_regs->galois_field_config, FSL_RE_GFM_POLY);
+	iowrite32be(FSL_RE_GFM_POLY, &re_regs->galois_field_config);
 
 	dev_info(dev, "version %x, mode %x, gfp %x\n",
-		 in_be32(&re_regs->re_version_id),
-		 in_be32(&re_regs->global_config),
-		 in_be32(&re_regs->galois_field_config));
+		 ioread32be(&re_regs->re_version_id),
+		 ioread32be(&re_regs->global_config),
+		 ioread32be(&re_regs->galois_field_config));
 
 	dma_dev = &re_priv->dma_dev;
 	dma_dev->dev = dev;
--

2.55.0

Mechanical conversion of the ppc4xx-specific accessors to the generic portable helpers.

As a result, enable COMPILE_TEST for extra compile coverage.