[PATCH] bus: simple-pm-bus: fix forced runtime PM use

[PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Johan Hovold]

The simple-pm-bus driver only enables runtime PM for some buses
('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
pm_runtime_force_resume() during system suspend unconditionally.

This currently works, but that is not obvious and depends on
implementation details which may change at some point.

Add dedicated system sleep ops and only call pm_runtime_force_suspend()
and pm_runtime_force_resume() for buses that use runtime PM to avoid any
future surprises.

Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")
Cc: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/bus/simple-pm-bus.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index 5dea31769f9a..d8e029e7e53f 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int simple_pm_bus_suspend(struct device *dev)
+{
+	struct simple_pm_bus *bus = dev_get_drvdata(dev);
+
+	if (!bus)
+		return 0;
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int simple_pm_bus_resume(struct device *dev)
+{
+	struct simple_pm_bus *bus = dev_get_drvdata(dev);
+
+	if (!bus)
+		return 0;
+
+	return pm_runtime_force_resume(dev);
+}
+
 static const struct dev_pm_ops simple_pm_bus_pm_ops = {
 	RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
 };
 
 #define ONLY_BUS	((void *) 1) /* Match if the device is only a bus. */
-- 
2.45.3

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Ulf Hansson]

On Fri, 14 Feb 2025 at 11:21, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The simple-pm-bus driver only enables runtime PM for some buses
> ('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
> pm_runtime_force_resume() during system suspend unconditionally.
>
> This currently works, but that is not obvious and depends on
> implementation details which may change at some point.
>
> Add dedicated system sleep ops and only call pm_runtime_force_suspend()
> and pm_runtime_force_resume() for buses that use runtime PM to avoid any
> future surprises.
>
> Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")

This doesn't look like it is needed to me. It isn't broken, right?

> Cc: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Seems reasonable to me, but I think we need an ack from Geert here too.

Anyway, feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/bus/simple-pm-bus.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 5dea31769f9a..d8e029e7e53f 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> +static int simple_pm_bus_suspend(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_suspend(dev);
> +}
> +
> +static int simple_pm_bus_resume(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_resume(dev);
> +}
> +
>  static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>         RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
> -       NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +       NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
>  };
>
>  #define ONLY_BUS       ((void *) 1) /* Match if the device is only a bus. */
> --
> 2.45.3
>

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Johan Hovold]

On Fri, Feb 14, 2025 at 01:36:48PM +0100, Ulf Hansson wrote:
> On Fri, 14 Feb 2025 at 11:21, Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > The simple-pm-bus driver only enables runtime PM for some buses
> > ('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
> > pm_runtime_force_resume() during system suspend unconditionally.
> >
> > This currently works, but that is not obvious and depends on
> > implementation details which may change at some point.
> >
> > Add dedicated system sleep ops and only call pm_runtime_force_suspend()
> > and pm_runtime_force_resume() for buses that use runtime PM to avoid any
> > future surprises.
> >
> > Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")
> 
> This doesn't look like it is needed to me. It isn't broken, right?

I didn't add a CC stable tag since this currently works, but I still
consider it a bug to call these helpers unconditionally when not using
runtime PM.

[ And during rc1 these callbacks were suddenly called and triggered a
NULL-pointer dereference as you know. [1] ]

Johan

[1] https://lore.kernel.org/lkml/Z6YcjFBWAVVVANf2@hovoldconsulting.com/

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Geert Uytterhoeven]

Hi Johan,

On Fri, 14 Feb 2025 at 13:53, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Feb 14, 2025 at 01:36:48PM +0100, Ulf Hansson wrote:
> > On Fri, 14 Feb 2025 at 11:21, Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > The simple-pm-bus driver only enables runtime PM for some buses
> > > ('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
> > > pm_runtime_force_resume() during system suspend unconditionally.
> > >
> > > This currently works, but that is not obvious and depends on
> > > implementation details which may change at some point.
> > >
> > > Add dedicated system sleep ops and only call pm_runtime_force_suspend()
> > > and pm_runtime_force_resume() for buses that use runtime PM to avoid any
> > > future surprises.

Thanks for your patch!

> > > Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")
> >
> > This doesn't look like it is needed to me. It isn't broken, right?
>
> I didn't add a CC stable tag since this currently works, but I still
> consider it a bug to call these helpers unconditionally when not using
> runtime PM.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> [ And during rc1 these callbacks were suddenly called and triggered a
> NULL-pointer dereference as you know. [1] ]
>
> Johan
>
> [1] https://lore.kernel.org/lkml/Z6YcjFBWAVVVANf2@hovoldconsulting.com/

Thanks, that was the context I needed to raise review priority ;)

Perhaps Reported-by: and Closes:?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Johan Hovold]

On Fri, Feb 14, 2025 at 02:16:14PM +0100, Geert Uytterhoeven wrote:
> On Fri, 14 Feb 2025 at 13:53, Johan Hovold <johan@kernel.org> wrote:

> > I didn't add a CC stable tag since this currently works, but I still
> > consider it a bug to call these helpers unconditionally when not using
> > runtime PM.
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > [ And during rc1 these callbacks were suddenly called and triggered a
> > NULL-pointer dereference as you know. [1] ]

> > [1] https://lore.kernel.org/lkml/Z6YcjFBWAVVVANf2@hovoldconsulting.com/
> 
> Thanks, that was the context I needed to raise review priority ;)
> 
> Perhaps Reported-by: and Closes:?

The glitch was only temporary this time and the immediate issue was
addressed by Rafael's partial revert in rc2:

	https://lore.kernel.org/lkml/6137505.lOV4Wx5bFT@rjwysocki.net/

(and which has these tags).

Johan

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Geert Uytterhoeven]

Hi Johan,

On Fri, 14 Feb 2025 at 14:27, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Feb 14, 2025 at 02:16:14PM +0100, Geert Uytterhoeven wrote:
> > On Fri, 14 Feb 2025 at 13:53, Johan Hovold <johan@kernel.org> wrote:
>
> > > I didn't add a CC stable tag since this currently works, but I still
> > > consider it a bug to call these helpers unconditionally when not using
> > > runtime PM.
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > [ And during rc1 these callbacks were suddenly called and triggered a
> > > NULL-pointer dereference as you know. [1] ]
>
> > > [1] https://lore.kernel.org/lkml/Z6YcjFBWAVVVANf2@hovoldconsulting.com/
> >
> > Thanks, that was the context I needed to raise review priority ;)
> >
> > Perhaps Reported-by: and Closes:?
>
> The glitch was only temporary this time and the immediate issue was
> addressed by Rafael's partial revert in rc2:
>
>         https://lore.kernel.org/lkml/6137505.lOV4Wx5bFT@rjwysocki.net/
>
> (and which has these tags).

OK.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Rafael J. Wysocki]

On Fri, Feb 14, 2025 at 11:21 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The simple-pm-bus driver only enables runtime PM for some buses
> ('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
> pm_runtime_force_resume() during system suspend unconditionally.
>
> This currently works, but that is not obvious and depends on
> implementation details which may change at some point.
>
> Add dedicated system sleep ops and only call pm_runtime_force_suspend()
> and pm_runtime_force_resume() for buses that use runtime PM to avoid any
> future surprises.
>
> Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")
> Cc: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/bus/simple-pm-bus.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 5dea31769f9a..d8e029e7e53f 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> +static int simple_pm_bus_suspend(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_suspend(dev);
> +}
> +
> +static int simple_pm_bus_resume(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_resume(dev);
> +}
> +
>  static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>         RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
> -       NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +       NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
>  };
>
>  #define ONLY_BUS       ((void *) 1) /* Match if the device is only a bus. */
> --

Re: [PATCH] bus: simple-pm-bus: fix forced runtime PM use

[Liu Ying]

On 02/14/2025, Johan Hovold wrote:
> The simple-pm-bus driver only enables runtime PM for some buses
> ('simple-pm-bus') yet has started calling pm_runtime_force_suspend() and
> pm_runtime_force_resume() during system suspend unconditionally.
> 
> This currently works, but that is not obvious and depends on
> implementation details which may change at some point.

Fair enough. It's not a good practice to depend on the PM core behaviour.

Acked-by: Liu Ying <victor.liu@nxp.com>

> 
> Add dedicated system sleep ops and only call pm_runtime_force_suspend()
> and pm_runtime_force_resume() for buses that use runtime PM to avoid any
> future surprises.
> 
> Fixes: c45839309c3d ("drivers: bus: simple-pm-bus: Use clocks")
> Cc: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/bus/simple-pm-bus.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 5dea31769f9a..d8e029e7e53f 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int simple_pm_bus_suspend(struct device *dev)
> +{
> +	struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +	if (!bus)
> +		return 0;
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int simple_pm_bus_resume(struct device *dev)
> +{
> +	struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +	if (!bus)
> +		return 0;
> +
> +	return pm_runtime_force_resume(dev);
> +}
> +
>  static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>  	RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
> -	NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
>  };
>  
>  #define ONLY_BUS	((void *) 1) /* Match if the device is only a bus. */

-- 
Regards,
Liu Ying