Re: [PATCH v13 15/17] media: cadence: csi2rx: Support runtime PM

[sashiko-bot@kernel.org]

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

New issues:
- [High] Asynchronous pm_runtime_put() combined with pm_runtime_disable() during device removal causes a hardware resource leak.
- [Low] Unconditional assignment of the dev_pm_ops structure and improper Kconfig PM dependency.

Pre-existing issues:
- [High] The interrupt handler accesses hardware registers without ensuring the device is active, risking bus faults.
--

commit a72fbe3bfad5203480d07b860f9bc4fef20bf3ce
Author: Changhuang Liang <changhuang.liang@starfivetech.com>
media: cadence: csi2rx: Support runtime PM

This commit introduces runtime power management hooks to save power when
the CSI-RX controller is inactive. It also shifts the error handling in
the csi2rx_enable_streams() function to use goto statements.

> diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
> index 1aa608c00dbce..ea85ef82760e6 100644
> --- a/drivers/media/platform/cadence/Kconfig
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -5,6 +5,7 @@ comment "Cadence media platform drivers"
>  config VIDEO_CADENCE_CSI2RX
>  	tristate "Cadence MIPI-CSI2 RX Controller"
>  	depends on VIDEO_DEV
> +	depends on PM

[Severity: Low]
This isn't a bug, but does this driver strictly require the PM dependency?

Using the pm_ptr() macro for the dev_pm_ops assignment below would allow
this Kconfig dependency to be dropped, avoiding dead code while still
compiling correctly when PM is disabled.

>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API

[ ... ]

> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 8931e824c597c..1ff2d8f78d5b4 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c

[ ... ]

> @@ -590,8 +559,10 @@ static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
>  	csi2rx->count--;
>  
>  	/* Let the last user turn off the lights. */
> -	if (!csi2rx->count)
> +	if (!csi2rx->count) {
>  		csi2rx_stop(csi2rx);
> +		pm_runtime_put(csi2rx->dev);

[Severity: High]
Could this asynchronous pm_runtime_put() cause a hardware resource leak?

If the device is unbound via module removal right after streaming stops,
csi2rx_remove() will run and call pm_runtime_disable(). This cancels any
pending asynchronous idle/suspend work, leaving the controller in the
RPM_ACTIVE state.

Does this leave the sys_clk, pixel_clk, and p_clk running indefinitely?
Would it be safer to use pm_runtime_put_sync() here to ensure the suspend
completes before removal?

> +	}
>  
>  	return 0;
>  }

[ ... ]

> @@ -1019,6 +990,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>  
> +	pm_runtime_enable(csi2rx->dev);

[Severity: High]
This is a pre-existing issue, but does csi2rx_irq_handler() access hardware
registers without ensuring the device is active?

With runtime PM introduced, the device's clocks will be gated when it enters
the RPM_SUSPENDED state. If a spurious or delayed interrupt triggers while
the clocks are disabled, accessing the hardware registers could result in a
synchronous external abort and a kernel panic.

Should the interrupt handler use pm_runtime_get_if_active() to check if
the hardware is powered before reading the registers?

>  	ret = v4l2_async_register_subdev(&csi2rx->subdev);
>  	if (ret < 0)

[ ... ]

> @@ -1068,6 +1104,7 @@ static struct platform_driver csi2rx_driver = {
>  	.driver	= {
>  		.name		= "cdns-csi2rx",
>  		.of_match_table	= csi2rx_of_table,
> +		.pm		= &csi2rx_pm_ops,

[Severity: Low]
This isn't a bug, but could the pm_ptr() macro be used here?

Wrapping &csi2rx_pm_ops with pm_ptr() would avoid dead code compilation
warnings when PM support is disabled, without needing the hard Kconfig
dependency mentioned earlier.

>  	},
>  };
>  module_platform_driver(csi2rx_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=15