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

[sashiko-bot@kernel.org]

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