Re: [PATCH v2 12/64] dmaengine: mv_xor: convert irq tasklet to dma_chan BH

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

Pre-existing issues:
- [High] The driver fails to explicitly cancel pending bottom-half work during module removal and initialization error paths, leading to a Use-After-Free.
- [High] Interrupts are requested and unmasked before the channel spinlock is initialized, creating a race condition that can lead to locking an uninitialized spinlock.
- [High] Recursive deadlock when submitting dependent operations from the completion handler.
--

--- Patch [12]: [PATCH v2 12/64] dmaengine: mv_xor: convert irq tasklet to dma_chan BH ---
commit a6fdd06d107411d5248196d2bc3b3e3a0282f10d
Author: Allen Pais <[email protected]>
Subject: dmaengine: mv_xor: convert irq tasklet to dma_chan BH

This patch replaces the per-channel tasklet with the shared dma_chan BH
helper. The handler continues to run in softirq context while dmaengine
owns the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..3a70d565e1ef0 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c

[ ... ]

> @@ -336,9 +336,9 @@ static void mv_chan_slot_cleanup(struct mv_xor_chan *mv_chan)
>  		mv_chan->dmachan.completed_cookie = cookie;
>  }
>  
> -static void mv_xor_tasklet(struct tasklet_struct *t)
> +static void mv_xor_tasklet(struct dma_chan *c)
>  {
> -	struct mv_xor_chan *chan = from_tasklet(chan, t, irq_tasklet);
> +	struct mv_xor_chan *chan = to_mv_xor_chan(c);
>  
>  	spin_lock(&chan->lock);
>  	mv_chan_slot_cleanup(chan);
>  	spin_unlock(&chan->lock);
>  }

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a recursive
deadlock? Looking at mv_xor_tasklet(), it holds the lock while running
cleanup:

mv_xor_tasklet()
    spin_lock(&chan->lock);
    mv_chan_slot_cleanup(chan);

If mv_chan_slot_cleanup() processes completed slots and invokes
dma_run_dependencies(), the dependent operations might be submitted to the
same channel:

mv_chan_slot_cleanup()
    mv_desc_run_tx_complete_actions()
        dma_run_dependencies()
            mv_xor_tx_submit()
                spin_lock_bh(&mv_chan->lock);

Would this recursive lock attempt cause a system hang, or is there something
preventing dependencies from being queued on the same channel?

[ ... ]

> @@ -1107,7 +1107,7 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  
>  	mv_chan->mmr_base = xordev->xor_base;
>  	mv_chan->mmr_high_base = xordev->xor_high_base;
> -	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);
> +	dma_chan_init_bh(&mv_chan->dmachan, mv_xor_tasklet);
>  
>  	/* clear errors before enabling interrupts */
>  	mv_chan_clear_err_status(mv_chan);

[Severity: High]
This is a pre-existing issue, but does the initialization error path properly
cancel the bottom half work before the channel structure is freed?

If a failure occurs later in mv_xor_channel_add() (like self-tests failing),
the code jumps to err_free_irq:

err_free_irq:
    free_irq(mv_chan->irq, mv_chan);
err_free_dma:
    ...

Should dma_chan_kill_bh() be called here and in mv_xor_channel_remove() to
ensure scheduled work doesn't execute after mv_chan is freed?

[Severity: High]
This is also a pre-existing issue, but is it possible for an interrupt to
fire before the channel spinlock is initialized?

In mv_xor_channel_add(), request_irq() is called and interrupts are unmasked
before the lock is initialized:

mv_xor_channel_add()
    request_irq(..., mv_xor_interrupt_handler, ...);
    mv_chan_unmask_interrupts(mv_chan);
    ...
    spin_lock_init(&mv_chan->lock);

If a spurious interrupt fires immediately and schedules the BH work, could
mv_xor_tasklet() attempt to acquire an uninitialized lock?

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