| Message ID | 20240827074018.534354-5-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: pisp-be: Split jobs creation and scheduling | expand |
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@ideasonboard.com> > > --- > v2->v3: > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > the kernel test robot <lkp@intel.com> > - 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; >
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@ideasonboard.com> > > > > --- > > v2->v3: > > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > > the kernel test robot <lkp@intel.com> > > - 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
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@ideasonboard.com> > > > > > > --- > > > v2->v3: > > > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > > > the kernel test robot <lkp@intel.com> > > > - 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; > > >
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@ideasonboard.com> > > > > > > > > --- > > > > v2->v3: > > > > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > > > > the kernel test robot <lkp@intel.com> > > > > - 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 >
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@ideasonboard.com> > > > > > > > > > > --- > > > > > v2->v3: > > > > > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > > > > > the kernel test robot <lkp@intel.com> > > > > > - 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; > > > > >
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) { 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); if (ret) goto pm_runtime_disable_err;
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@ideasonboard.com> --- v2->v3: - Mark pispbe_runtime_resume() as __maybe_unused as reported by the kernel test robot <lkp@intel.com> - Add fixes tag --- drivers/media/platform/raspberrypi/pisp_be/pisp_be.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.45.2