On Sat, Aug 31, 2024 at 06:20:30PM GMT, Laurent Pinchart wrote:
> On Sat, Aug 31, 2024 at 05:05:47PM +0200, Jacopo Mondi wrote:
> > 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() ?
>
> No, I'm talking about the current code.
>
> > > 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 ??
>
> Re-read the code above.
>
If you're talking about the current code, there indeed is a PM use
count underflow as reported in the commit message.
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!
Aren't we getting each other ?
> > > 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
>
> It would likely be simpler. I'll let RPi decide, I don't know if
> !CONFIG_PM is something that should be supported.
>
> > > 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 you replace pispbe_runtime_resume() with pm_runtime_resume_and_get()
> in probe(), you should replace pispbe_runtime_suspend() too.
>
> > > 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
>
