Re: [PATCH v4 4/4] media: pisp_be: Fix pm_runtime underrun in probe

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 02, 2024 at 01:24:06PM +0200, Jacopo Mondi wrote:
> During the probe() routine, the driver needs to power up the interface
> in order to identify and initialize the hardware and it later suspends
> it at the end of probe().
> 
> The driver erroneously resumes the interface by calling the
> pispbe_runtime_resume() function directly but suspends it by
> calling pm_runtime_put_autosuspend().
> 
> This causes a PM usage count imbalance at probe time, notified by the
> runtime_pm framework with the below message in the system log:
> 
>  pispbe 1000880000.pisp_be: Runtime PM usage count underflow!
> 
> Fix this by suspending the interface using pm_runtime_idle() which
> doesn't decrease the pm_runtime usage count and inform the PM framework
> that the device is active by calling pm_runtime_set_active().
> 
> Adjust the pispbe_remove() function as well to disable
> the pm_runtime in the correct order,

s/,$/./

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> ---
> v3->v4:
> - Instead of using pm_runtime for resuming, suspend using
>   pm_runtime_idle() to support !CONFIG_PM
> 
> v2->v3:
> - Mark pispbe_runtime_resume() as __maybe_unused as reported by
>   the kernel test robot <lkp@intel.com>
> ---
>  .../platform/raspberrypi/pisp_be/pisp_be.c    | 26 +++++++++----------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> index d614f53f0f68..1c19ca946bd4 100644
> --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> @@ -1720,36 +1720,32 @@ static int pispbe_probe(struct platform_device *pdev)
>  				     "Failed to get clock");
>  
>  	/* Hardware initialisation */
> -	pm_runtime_set_autosuspend_delay(pispbe->dev, 200);
> -	pm_runtime_use_autosuspend(pispbe->dev);
> -	pm_runtime_enable(pispbe->dev);
> -
>  	ret = pispbe_runtime_resume(pispbe->dev);
>  	if (ret)
> -		goto pm_runtime_disable_err;
> +		return ret;
>  
>  	pispbe->hw_busy = false;
>  	spin_lock_init(&pispbe->hw_lock);
>  	ret = pispbe_hw_init(pispbe);
>  	if (ret)
> -		goto pm_runtime_suspend_err;
> +		goto runtime_suspend_err;
>  
>  	ret = pispbe_init_devices(pispbe);
>  	if (ret)
>  		goto disable_devs_err;
>  
> -	pm_runtime_mark_last_busy(pispbe->dev);
> -	pm_runtime_put_autosuspend(pispbe->dev);
> +	pm_runtime_set_autosuspend_delay(pispbe->dev, 200);
> +	pm_runtime_use_autosuspend(pispbe->dev);
> +	pm_runtime_set_active(pispbe->dev);
> +	pm_runtime_enable(pispbe->dev);
> +	pm_runtime_idle(pispbe->dev);
>  
>  	return 0;
>  
>  disable_devs_err:

It would be nice to rename the error labels to start with err_, like
commonly done (in another patch of course).

>  	pispbe_destroy_devices(pispbe);
> -pm_runtime_suspend_err:
> +runtime_suspend_err:
>  	pispbe_runtime_suspend(pispbe->dev);
> -pm_runtime_disable_err:
> -	pm_runtime_dont_use_autosuspend(pispbe->dev);
> -	pm_runtime_disable(pispbe->dev);
>  
>  	return ret;
>  }
> @@ -1760,9 +1756,11 @@ static void pispbe_remove(struct platform_device *pdev)
>  
>  	pispbe_destroy_devices(pispbe);
>  
> -	pispbe_runtime_suspend(pispbe->dev);
>  	pm_runtime_dont_use_autosuspend(pispbe->dev);
> -	pm_runtime_disable(pispbe->dev);
> +        pm_runtime_disable(pispbe->dev);
> +        if (!pm_runtime_status_suspended(pispbe->dev))
> +                pispbe_runtime_suspend(pispbe->dev);
> +        pm_runtime_set_suspended(pispbe->dev);

Wrong indentation, you're using spaces instead of tabs.

With those issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  static const struct dev_pm_ops pispbe_pm_ops = {

-- 
Regards,

Laurent Pinchart