Re: [PATCH v2 03/18] arm64: dts: ti: k3-j721s2-main: Add multiple channels for CSI2RX DMA

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Leakage of allocated DMA channels during driver probe error paths due to flawed PM runtime status check.
- [High] Missing `pm_runtime_disable()` in the `ti_csi2rx_probe` error path.
- [High] Leakage of successfully requested DMA channels on partial `runtime_resume` failures.
- [High] Unreachable stream validation error check silently ignores missing streams.
--

--- Patch [3]: [PATCH v2 03/18] arm64: dts: ti: k3-j721s2-main: Add multiple channels for CSI2RX DMA ---
commit aa226bddd4872f73702e34f1bec61a88480652a0
Author: Vaishnav Achath <vaishnav.a@ti.com>

arm64: dts: ti: k3-j721s2-main: Add multiple channels for CSI2RX DMA

This commit adds additional DMA channels to the J721E CSI2RX SHIM layer
device tree node. It enables multistream support by expanding the dmas
and dma-names properties to handle up to 8 channels per node.

> diff --git a/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
> index 5cac119e4292a..f4b8713873b17 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
> @@ -1239,8 +1239,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 isn't a bug introduced by this patch, but scaling up the number of
channels in the device tree makes a DMA channel leak regression worse on
probe errors in the driver.

If ti_csi2rx_probe() in drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
fails midway through initializing these new contexts, it cleans up here:

err_ctx:
        while (i--)
                ti_csi2rx_cleanup_ctx(&csi->ctx[i]);

However, ti_csi2rx_cleanup_ctx() has this check:

        if (!pm_runtime_status_suspended(ctx->csi->dev))
                dma_release_channel(ctx->dma.chan);

Since pm_runtime_set_active() hasn't been called yet during early probe,
the device is still in its default RPM_SUSPENDED state. Will this cause
the DMA channels to be permanently leaked when probe fails?

[Severity: High]
This is another pre-existing issue, but the addition of multiple DMA channels
also increases the impact of a leak during runtime resume:

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 channel (e.g., rx4), the function
returns immediately without an error unwind path. Do the channels
successfully requested in earlier iterations get stranded indefinitely since
runtime_suspend won't be called?

[Severity: High]
This is a pre-existing issue in the probe error path that leaves a dangling
PM configuration:

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]);
        ti_csi2rx_cleanup_v4l2(csi);
err_dma_chan:
        dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
                          csi->drain.paddr);
        return ret;

This path executes after pm_runtime_enable(csi->dev) has been called. Should
pm_runtime_disable() be called here before returning to unroll the PM
enablement?

[Severity: High]
This is a pre-existing issue, but it breaks stream validation for multistream
setups.

In ti_csi2rx_get_vc_and_dt(), the loop searches for a matching stream:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_get_vc_and_dt()
        for (j = 0; j < fd.num_entries; j++) {
                ...
                        /* Return error if no matching stream found */
                        if (j == fd.num_entries)
                                return -EINVAL;
        }

Since the if statement is completely inside the for-loop where
j < fd.num_entries, it is mathematically impossible for the condition to
evaluate to true. Could the driver silently proceed with incorrect
hardware configurations if a requested stream is missing?

>  		power-domains = <&k3_pds 38 TI_SCI_PD_EXCLUSIVE>;
>  		status = "disabled";

[ ... ]

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