Hi Laurent
On Sat, Aug 31, 2024 at 02:04:49PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2024 at 09:40:18AM +0200, Jacopo Mondi wrote:
> > The pisp_be driver uses and depends on runtime_pm.
> >
> > 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, without going
> > through the pm_runtime helpers, but later 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 resuming the interface using the pm runtime helpers instead
> > of calling the resume function directly.
> >
> > Fixes: 12187bd5d4f8 ("media: raspberrypi: Add support for PiSP BE")
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> >
> > ---
> > v2->v3:
> > - Mark pispbe_runtime_resume() as __maybe_unused as reported by
> > the kernel test robot <lkp@xxxxxxxxx>
> > - Add fixes tag
> > ---
> > drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> > index f42541bb4827..7b62585d7510 100644
> > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> > @@ -1638,7 +1638,7 @@ static int pispbe_runtime_suspend(struct device *dev)
> > return 0;
> > }
> >
> > -static int pispbe_runtime_resume(struct device *dev)
> > +static int __maybe_unused pispbe_runtime_resume(struct device *dev)
>
> The preferred way to handle this is to use RUNTIME_PM_OPS() instead of
> SET_RUNTIME_PM_OPS(), not use __maybe_unused, and set the driver's .pm
> field with pm_ptr(). That's probably a candidate for a separate patch,
> before this one.
Ah thanks, didn't consider that
>
> > {
> > struct pispbe_dev *pispbe = dev_get_drvdata(dev);
> > int ret;
> > @@ -1741,7 +1741,7 @@ static int pispbe_probe(struct platform_device *pdev)
> > pm_runtime_use_autosuspend(pispbe->dev);
> > pm_runtime_enable(pispbe->dev);
> >
> > - ret = pispbe_runtime_resume(pispbe->dev);
> > + ret = pm_runtime_resume_and_get(pispbe->dev);
>
> There's clearly a bug in the way runtime PM is used in this driver.
> Here's the full initialization sequence at probe time:
>
> 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);
resume_and_get() ?
> if (ret)
> goto pm_runtime_disable_err;
>
> ...
>
> pm_runtime_mark_last_busy(pispbe->dev);
> pm_runtime_put_autosuspend(pispbe->dev);
>
> I'm actually surprised there's no warning for PM use count underflow, as
> the last line decreases the use count while it hasn't been increased
> before.
Doesn't
ret = pm_runtime_resume_and_get(pispbe->dev);
increase the reference count ??
>
> The way you fix it in this patch makes the driver depend on CONFIG_PM
I would have sworn the driver depends on CONFIG_PM, sorry I haven't
check
> for proper operation. Without CONFIG_PM, the PM runtime calls are
> no-ops, and so the device will never be powered on. I'm fine with that,
> assuming that there's no use case for running a !CONFIG_PM kernel on a
> Raspberry Pi 5. In this case, you should however add a dependency on
> CONFIG_PM in Kconfig to make this clear.
>
That would be my prefernce
> There are other related issues. The error paths in probe() call
> pispbe_runtime_suspend(), and remove() does the same. Additionally,
> remove() should not call pispbe_runtime_suspend(), as the device should
> be suspended by the time remove() is called. These need to be fixed too.
Are you suggesting these two should go through runtime_pm() ?
>
> If we want to keep !CONFIG_PM support, the direct call to
Should we ? What's your and other's opinion, RPi's one in particular ?
> pispbe_runtime_resume() is fine, but the rest of the runtime PM
> initialization isn't. What you should instead do is
>
> ret = pispbe_runtime_resume(pispbe->dev);
> if (ret)
> goto pm_runtime_disable_err;
>
> ...
>
> 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);
>
> When CONFIG_PM is enabled, the pm_runtime_set_active() call tells
> runtime PM that the device is powered on, and pm_runtime_idle() then
> powers it off without decrementing the use count, which has never been
> incremented in the first place. When CONFIG_PM is disabled, all the
> pm_runtime_*() calls are no-ops, and the device will remaining powered
> until remove().
>
> The corresponding implementation in remove() would be
>
> pm_runtime_dont_use_autosuspend(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);
>
> > if (ret)
> > goto pm_runtime_disable_err;
> >
>
> --
> Regards,
>
> Laurent Pinchart
