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@xxxxxxxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxxx>
> }
>
> static const struct dev_pm_ops pispbe_pm_ops = {
--
Regards,
Laurent Pinchart
