On Sat, Aug 31, 2024 at 05:29:44PM +0200, Jacopo Mondi wrote:
> 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:
> > > On Sat, Aug 31, 2024 at 02:04:49PM GMT, Laurent Pinchart wrote:
> > > > 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 ??
It does but it's not called currently.
> > 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 ?
Ah, I had missed that from the commit message. Makes sense now :-)
> > > > 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
