Re: [PATCH v2 04/18] arm64: dts: ti: k3-j784s4-j742s2-main-common: Add multiple channels for CSI2RX DMA

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] DMA channel leak on probe failure due to incorrect PM status check.
- [High] DMA channels are leaked on module removal if the device is active.
- [High] Missing pm_runtime_disable() on probe error paths.
- [High] DMA channel leak on error path in ti_csi2rx_runtime_resume().
- [High] DMA drain transaction is instantly aborted during system suspend.
--

--- Patch [4]: [PATCH v2 04/18] arm64: dts: ti: k3-j784s4-j742s2-main-common: Add multiple channels for CSI2RX DMA ---
commit ebceec6da92de871c7ab51be87cc009f8c4d7c16
Author: Vaishnav Achath <vaishnav.a@ti.com>

arm64: dts: ti: k3-j784s4-j742s2-main-common: Add multiple channels for CSI2RX DMA

This commit expands the CSI2RX SHIM layer configuration in the device tree
to support up to 8 DMA channel contexts. This change enables multistream
support for CSI2RX by adding the necessary DMA mappings.

> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi
> index c2636e624f18b..4b4545a5af192 100644
> --- a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi
> @@ -811,8 +811,11 @@ ti_csi2rx0: ticsi2rx@4500000 {
>  		ranges;
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> -		dmas = <&main_bcdma_csi 0 0x4940 0>;
> -		dma-names = "rx0";
> +		dmas = <&main_bcdma_csi 0 0x4940 0>, <&main_bcdma_csi 0 0x4941 0>,
> +		       <&main_bcdma_csi 0 0x4942 0>, <&main_bcdma_csi 0 0x4943 0>,
> +		       <&main_bcdma_csi 0 0x4944 0>, <&main_bcdma_csi 0 0x4945 0>,
> +		       <&main_bcdma_csi 0 0x4946 0>, <&main_bcdma_csi 0 0x4947 0>;
> +		dma-names = "rx0", "rx1", "rx2", "rx3", "rx4", "rx5", "rx6", "rx7";

[Severity: High]
This is a pre-existing issue, but expanding the channel count makes a 
multi-iteration failure during initialization more likely.

If ti_csi2rx_init_ctx() fails for a later context in ti_csi2rx_probe(), it 
jumps to err_ctx to clean up previously successful contexts. However, 
pm_runtime_set_active() has not been called yet, so the device is 
considered suspended:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_cleanup_ctx() {
    ...
    if (!pm_runtime_status_suspended(ctx->csi->dev))
        dma_release_channel(ctx->dma.chan);
    ...
}

Will this skip the DMA release and leak the successfully requested channels?

[Severity: High]
This is a pre-existing issue, but does a partial failure during runtime 
resume also cause a permanent leak of previously allocated DMA channels?

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_runtime_resume() {
    ...
    for (unsigned int i = 0; i < csi->num_ctx; i++) {
        ret = ti_csi2rx_init_dma(&csi->ctx[i]);
        if (ret)
            return ret;
    }
    ...
}

If ti_csi2rx_init_dma() fails for a later context, it returns immediately 
without freeing the channels allocated for previous contexts. Since the next 
resume attempt will re-allocate new channels, do the old channels become 
permanently leaked?

[Severity: High]
This is a pre-existing issue, but does module removal also leak all 8 
channels if the device is active?

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_remove() {
    ...
    if (!pm_runtime_status_suspended(&pdev->dev))
        pm_runtime_set_suspended(&pdev->dev);
    ...
}

This forcefully manipulates the internal PM state to suspended without 
invoking the runtime_suspend callback. When ti_csi2rx_cleanup_ctx() is 
called immediately afterward, it sees the newly set suspended state and 
skips calling dma_release_channel(). Can this be avoided so channels are 
properly released?

[Severity: High]
This is a pre-existing issue, but do the probe error paths leave the PM 
state unbalanced?

pm_runtime_enable() is called earlier in probe, but pm_runtime_disable() 
appears to be missing on the error paths:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_probe() {
    ...
err_notifier:
    ti_csi2rx_cleanup_notifier(csi);
err_ctx:
    while (i--)
        ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
    ...
}

If ti_csi2rx_notifier_register() or subsequent initialization steps fail, 
the driver returns an error but leaves the device improperly registered with 
the PM framework. Should a pm_runtime_disable() be added here?

[Severity: High]
This is a pre-existing issue, but is the asynchronous DMA drain transaction 
instantly aborted during system suspend?

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_suspend() {
    ...
    /* Drain DMA */
    ti_csi2rx_drain_dma(ctx);

    /* Terminate DMA */
    ret = dmaengine_terminate_sync(ctx->dma.chan);
    ...
}

The driver submits an asynchronous DMA drain transaction via 
ti_csi2rx_drain_dma(), but immediately calls dmaengine_terminate_sync() 
without waiting for it to complete. Does this synchronously terminate the 
drain transfer before it can execute, potentially leaving stale data in the 
hardware FIFO?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713113423.2310443-1-y-abhilashchandra@ti.com?part=4