Re: [PATCH] dma: jz4780: handle clk_prepare_enable() failure in probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown ordering in `jz4780_dma_remove()`: `clk_disable_unprepare()` is called before `free_irq()`, which can lead to a bus fault.
- [High] The driver mixes devm-managed dmaengine registration with manual teardown in `remove()`, causing the dmaengine to be unregistered after hardware is disabled.
- [High] When descriptor setup fails, the driver erroneously frees the currently active descriptor instead of the newly allocated descriptor, causing memory leaks and a NULL pointer dereference.
- [High] Checking the status of a submitted but unissued transaction while the channel is idle causes a NULL pointer dereference.
--

--- Patch [1]: [PATCH] dma: jz4780: handle clk_prepare_enable() failure in probe ---
commit 15ac33e185449fe2ef9ed06622c4c9792fca2767
Author: jiawen <1298662399@qq.com>

dma: jz4780: handle clk_prepare_enable() failure in probe

jz4780_dma_probe() ignores the return value of clk_prepare_enable(). If
the clock fails to enable, the driver continues and may later attempt to
disable an unprepared clock on error paths, leading to unbalanced clock
operations. Check the return value and propagate the error immediately,
avoiding the erroneous cleanup.

> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 6070dfdb7114c..2547fa229a835 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -903,7 +903,9 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	clk_prepare_enable(jzdma->clk);
> +	ret = clk_prepare_enable(jzdma->clk);
> +	if (ret)
> +		return ret;
>  
>  	/* Property is optional, if it doesn't exist the value will remain 0. */
>  	of_property_read_u32_index(dev->of_node, "ingenic,reserved-channels",

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the probe
error handling, I noticed an issue with devm-managed dmaengine registration
mixed with manual teardown in jz4780_dma_remove().

probe() uses dmaenginem_async_device_register(), which defers unregistering
the DMA engine to the devres cleanup phase, occurring after remove() returns.

However, in jz4780_dma_remove(), the driver manually kills tasklets, frees the
IRQ, and disables the clock:

jz4780_dma_remove() {
    ...
	clk_disable_unprepare(jzdma->clk);
	free_irq(jzdma->irq, jzdma);

	for (i = 0; i < jzdma->soc_data->nb_channels; i++)
		tasklet_kill(&jzdma->chan[i].vchan.task);
    ...
}

Does this create a critical window where the DMA controller is still
registered and accessible to dmaengine clients, but its hardware is
shut down? If a client accesses the controller (e.g., via
dmaengine_terminate_all()) during this window, could it access
unclocked registers and cause a bus fault?

[Severity: High]
This is a pre-existing issue, but I also noticed a teardown ordering
problem in jz4780_dma_remove().

clk_disable_unprepare() is called before free_irq():

jz4780_dma_remove() {
    ...
	clk_disable_unprepare(jzdma->clk);
	free_irq(jzdma->irq, jzdma);
    ...
}

If a DMA interrupt fires or is pending after the driver calls
clk_disable_unprepare() but before free_irq() completes, the
jz4780_dma_irq_handler will execute and attempt to read the DMA
controller's registers. Could accessing these registers with the clock
disabled cause an imprecise abort or bus timeout?

[Severity: High]
This isn't a bug introduced by this patch, but I found a potential issue
when descriptor setup fails in jz4780_dma_prep_slave_sg() and
jz4780_dma_prep_dma_cyclic().

The error path frees the channel's active descriptor instead of the newly
allocated local desc:

jz4780_dma_prep_slave_sg() {
    ...
		if (err < 0) {
			jz4780_dma_desc_free(&jzchan->desc->vdesc);
			return NULL;
		}
    ...
}

jzchan->desc points to the currently executing transfer, not the newly
allocated desc. If the channel is idle, jzchan->desc is NULL, wouldn't
this lead to an immediate NULL pointer dereference? If active, could it
free a running descriptor (causing a use-after-free and DMA pool
corruption) while the newly allocated desc is leaked entirely?

[Severity: High]
This is a pre-existing issue, but checking the status of a submitted but
unissued transaction while the channel is idle appears to cause a NULL
pointer dereference in jz4780_dma_tx_status().

If a client driver submits a DMA transaction but has not yet called
dma_async_issue_pending(), the descriptor sits on vchan->desc_submitted.

If the client queries the status via dmaengine_tx_status(),
vchan_find_desc() returns NULL because it only searches the desc_issued
list. The code then evaluates:

jz4780_dma_tx_status() {
    ...
	} else if (cookie == jzchan->desc->vdesc.tx.cookie) {
    ...
}

If the channel is idle, jzchan->desc is NULL. Could this fallback check
dereference jzchan->desc without verifying it is not NULL, leading to a
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_BE00B17EBF2A95EADD8B80203B745B9C7609@qq.com?part=1