Hi Laurent,
On Tue, Sep 03, 2024 at 02:13:16AM GMT, Laurent Pinchart wrote:
> 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.
Oh crabs, how did I missed that ?
>
> With those issues fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks
>
> > }
> >
> > static const struct dev_pm_ops pispbe_pm_ops = {
>
> --
> Regards,
>
> Laurent Pinchart
