Re: [PATCH] dmaengine: fsl_raid: replace in_be32/out_be32 with ioread32be/iowrite32be

[sashiko-bot@kernel.org]

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 <rosenp@gmail.com>

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.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..e559c636bf552 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
[ ... ]
> @@ -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?

>  	while (count--) {
>  		found = 0;
>  		hwdesc = &re_chan->oub_ring_virt_addr[re_chan->oub_count];
[ ... ]
> @@ -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);

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

> +	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);
[ ... ]
> @@ -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?

>  
>  	/* 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/20260718033122.2372985-1-rosenp@gmail.com?part=1