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.
> {
> 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);
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.
The way you fix it in this patch makes the driver depend on CONFIG_PM
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.
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.
If we want to keep !CONFIG_PM support, the direct call to
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
