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.
> > 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
