Re: [PATCH] media: microchip-csi2dc: Power off on probe failure and remove

[Eugen Hristev]

On 9/11/26 01:48, Myeonghun Pak wrote:
> csi2dc_probe() leaves runtime PM enabled and the controller powered on
> if subdevice registration fails. Removal also leaves an active controller
> powered on, since pm_runtime_disable() does not suspend it.
> 
> Disable runtime PM and power off the controller on both paths, then mark
> it suspended. Check pm_runtime_status_suspended() after disabling runtime
> PM to avoid turning off clocks that runtime suspend has already disabled.
> 
> This issue was identified during our ongoing static-analysis research while
> reviewing kernel code.
> 
> Fixes: 2de0b3c0f678 ("media: atmel: introduce microchip csi2dc driver")
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
>  drivers/media/platform/microchip/microchip-csi2dc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-csi2dc.c b/drivers/media/platform/microchip/microchip-csi2dc.c
> index e69292f3b2a909be6a044db6ad086a7b0e082802..56cccb02bc562febe5e6a36e69a9ae014e005ccc 100644
> --- a/drivers/media/platform/microchip/microchip-csi2dc.c
> +++ b/drivers/media/platform/microchip/microchip-csi2dc.c
> @@ -727,13 +727,18 @@ static int csi2dc_probe(struct platform_device *pdev)
>  	ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>  	if (ret) {
>  		dev_err(csi2dc->dev, "failed to register the subdevice\n");
> -		goto csi2dc_probe_cleanup_notifier;
> +		goto csi2dc_probe_disable_pm;
>  	}
>  
>  	dev_info(dev, "Microchip CSI2DC version %x\n", ver);
>  
>  	return 0;
>  
> +csi2dc_probe_disable_pm:
> +	pm_runtime_disable(dev);
> +	if (!pm_runtime_status_suspended(dev))
> +		csi2dc_power(csi2dc, false);
> +	pm_runtime_set_suspended(dev);

Hello Myeonghun,

Thank you for your patch. I have added Balakrishnan for review and help
testing.

I guess the proper way would be to not leave the device powered on at
the end of the probe, since all further operations will use resume on
the device.
So if probe fails, yes, disable runtime PM, but regardless of probe
success/fail, at the end, the device should be powered off.

>  csi2dc_probe_cleanup_notifier:
>  	v4l2_async_nf_cleanup(&csi2dc->notifier);
>  csi2dc_probe_cleanup_entity:
> @@ -746,12 +751,15 @@ static void csi2dc_remove(struct platform_device *pdev)
>  {
>  	struct csi2dc_device *csi2dc = platform_get_drvdata(pdev);
>  
> -	pm_runtime_disable(&pdev->dev);
> -
>  	v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>  	v4l2_async_nf_unregister(&csi2dc->notifier);
>  	v4l2_async_nf_cleanup(&csi2dc->notifier);
>  	media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		csi2dc_power(csi2dc, false);

I suppose the device should be powered off, and when runtime pm is
disabled, there should not be any requirement to manually power off. So
something is odd here.

Eugen

> +	pm_runtime_set_suspended(&pdev->dev);
>  }
>  
>  static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
> 
> base-commit: 50d05c7c76c96b90462f24debacca971d2e86713