Re: [PATCH 21/26] dmaengine: sun4i-dma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing tasklet_kill() for virtual DMA channels in device teardown (remove and probe error paths) leads to Use-After-Free.
- [High] Ongoing DMA hardware transfers are not gracefully stopped in sun4i_dma_remove() before clock disable.
- [High] Race condition between sun4i_dma_interrupt() and sun4i_dma_terminate_all() leads to NULL pointer dereference and Use-After-Free.
- [Medium] Incorrect DMA residue calculation in sun4i_dma_tx_status() for queued (unstarted) contracts.
- [Low] sun4i_dma_of_xlate() strictly validates against hardcoded max DRQ type limits rather than the SoC-specific config.
--

--- Patch [21]: [PATCH 21/26] dmaengine: sun4i-dma: Remove redundant dev_err()/dev_err_probe() ---
commit 0e16200d5dd839cadb8ffc88ed67f75051cff988
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: sun4i-dma: Remove redundant dev_err()/dev_err_probe()

The devm_request_irq() now automatically logs detailed error messages on
failure. This eliminates the need for driver-specific dev_err() and
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
> index d2321f7287d20..303d71ba0ab61 100644
> --- a/drivers/dma/sun4i-dma.c
> +++ b/drivers/dma/sun4i-dma.c
> @@ -1337,7 +1337,7 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt,
>  			       0, dev_name(&pdev->dev), priv);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret, "Cannot request IRQ\n");
> +		return ret;
>  
>  	ret = dmaenginem_async_device_register(&priv->slave);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but does driver unbind safely tear down
the virtual channels?

drivers/dma/sun4i-dma.c:sun4i_dma_remove() {
    struct sun4i_dma_dev *priv = platform_get_drvdata(pdev);

    /* Disable IRQ so no more work is scheduled */
    disable_irq(priv->irq);

    of_dma_controller_free(pdev->dev.of_node);
}

If the driver is unbound while an interrupt has scheduled a tasklet,
dma_async_device_unregister() called by devres will free the DMA channels
and the associated priv->vchans memory.

Since there is no tasklet_kill(&vchan->vc.task) call to cancel pending
tasklets, could the scheduled tasklet execute after the memory is freed
and cause a use-after-free?


[Severity: High]
This is another pre-existing issue in the same function, but does this
safely stop active DMA transfers?

drivers/dma/sun4i-dma.c:sun4i_dma_remove() {
    struct sun4i_dma_dev *priv = platform_get_drvdata(pdev);

    /* Disable IRQ so no more work is scheduled */
    disable_irq(priv->irq);

    of_dma_controller_free(pdev->dev.of_node);
}

If the driver is unbound while a DMA transfer is actively executing, the
hardware configuration registers are not cleared to halt the channels.

When devres subsequently disables the DMA controller's clock mid-transaction,
can this forcefully lock up the AXI/AHB bus or corrupt memory?


[Severity: High]
This is a pre-existing issue, but is there a race condition between the
interrupt handler and channel termination?

drivers/dma/sun4i-dma.c:sun4i_dma_interrupt() {
    ...
    contract = vchan->contract;

    /*
     * Disable the IRQ and free the pchan if it's an end
     * interrupt (odd bit)
     */
    if (bit & 1) {
        spin_lock(&vchan->vc.lock);

        /*
         * Move the promise into the completed list now that
         * we're done with it
         */
        list_move_tail(&vchan->processing->list,
                       &contract->completed_demands);
    ...
}

The contract pointer is read before acquiring vchan->vc.lock. Concurrently,
sun4i_dma_terminate_all() on another CPU could acquire the lock, set
vchan->processing to NULL, free the active contract, and release the lock.

When sun4i_dma_interrupt() subsequently acquires the lock, it performs a
list_move_tail() using vchan->processing->list.

Could this cause a guaranteed NULL pointer dereference since
vchan->processing would be NULL, as well as a use-after-free when accessing
the freed contract structure?


[Severity: Medium]
This is a pre-existing issue, but does this calculate the residue correctly
for queued contracts?

drivers/dma/sun4i-dma.c:sun4i_dma_tx_status() {
    ...
    promise = list_first_entry_or_null(&contract->demands,
                                       struct sun4i_dma_promise, list);
    if (promise && pchan) {
        bytes -= promise->len;
        if (pchan->is_dedicated)
            bytes += readl(pchan->base + SUN4I_DDMA_BYTE_COUNT_REG);
        else
            bytes += readl(pchan->base + SUN4I_NDMA_BYTE_COUNT_REG);
    }
    ...
}

If vchan_find_desc() returns a pending, unstarted contract, and the virtual
channel is currently executing a different contract on a physical channel,
this code unconditionally assumes the hardware byte count belongs to the
queried contract.

By subtracting the queried contract's promise length and adding the executing
contract's hardware residue, can this return corrupted residue values for
queued transfers?


[Severity: Low]
This is a pre-existing issue, but does the validation correctly restrict
endpoints on all SoC variants?

drivers/dma/sun4i-dma.c:sun4i_dma_of_xlate() {
    ...
    /* Make sure the endpoint looks sane */
    if ((is_dedicated && endpoint >= SUN4I_DDMA_DRQ_TYPE_LIMIT) ||
        (!is_dedicated && endpoint >= SUN4I_NDMA_DRQ_TYPE_LIMIT))
        return NULL;
    ...
}

The validation uses hardcoded limits like SUN4I_DDMA_DRQ_TYPE_LIMIT (32)
regardless of the SoC variant.

If a device tree requests an endpoint of 20 on a suniv_f1c100s device, which
has a limit of 10, would this bypass the check and program out-of-bounds
DRQ indices into the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=21