Message-ID: <CAJZ5v0j_nm_z4ma2AsRkjiZn-AJ2bK982+Mwa8+_PoUAveNATQ@mail.gmail.com>
Date: Mon, 16 Jun 2025 19:14:00 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, dakr@...nel.org, 
	len.brown@...el.com, pavel@...nel.org, ulf.hansson@...aro.org, 
	jic23@...nel.org, daniel.lezcano@...aro.org, dmitry.torokhov@...il.com, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, bhelgaas@...gle.com, 
	geert@...ux-m68k.org, linux-iio@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, fabrizio.castro.jz@...esas.com, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v4 1/2] PM: domains: Detach on device_unbind_cleanup()

On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The dev_pm_domain_attach() function is typically used in bus code alongside
> dev_pm_domain_detach(), often following patterns like:
>
> static int bus_probe(struct device *_dev)
> {
>     struct bus_driver *drv = to_bus_driver(dev->driver);
>     struct bus_device *dev = to_bus_device(_dev);
>     int ret;
>
>     // ...
>
>     ret = dev_pm_domain_attach(_dev, true);
>     if (ret)
>         return ret;
>
>     if (drv->probe)
>         ret = drv->probe(dev);
>
>     // ...
> }
>
> static void bus_remove(struct device *_dev)
> {
>     struct bus_driver *drv = to_bus_driver(dev->driver);
>     struct bus_device *dev = to_bus_device(_dev);
>
>     if (drv->remove)
>         drv->remove(dev);
>     dev_pm_domain_detach(_dev);
> }
>
> When the driver's probe function uses devres-managed resources that depend
> on the power domain state, those resources are released later during
> device_unbind_cleanup().
>
> Releasing devres-managed resources that depend on the power domain state
> after detaching the device from its PM domain can cause failures.
>
> For example, if the driver uses devm_pm_runtime_enable() in its probe
> function, and the device's clocks are managed by the PM domain, then
> during removal the runtime PM is disabled in device_unbind_cleanup() after
> the clocks have been removed from the PM domain. It may happen that the
> devm_pm_runtime_enable() action causes the device to be runtime-resumed.
> If the driver specific runtime PM APIs access registers directly, this
> will lead to accessing device registers without clocks being enabled.
> Similar issues may occur with other devres actions that access device
> registers.
>
> Add detach_power_off member to struct dev_pm_info, to be used later in
> device_unbind_cleanup() as the power_off argument for
> dev_pm_domain_detach(). This is a preparatory step toward removing
> dev_pm_domain_detach() calls from bus remove functions. Since the current
> PM domain detach functions (genpd_dev_pm_detach() and acpi_dev_pm_detach())
> already set dev->pm_domain = NULL, there should be no issues with bus
> drivers that still call dev_pm_domain_detach() in their remove functions.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> Changes in v4:
> - save dev->power.detach_power_off in dev_pm_domain_attach() and use
>   it in device_unbind_cleanup() when detaching
> - adjusted patch description
>
> Changes in v3:
> - dropped devm_pm_domain_detach_off(), devm_pm_domain_detach_on()
>   and use a single function devm_pm_domain_detach()
>
> Changes in v2:
> - none; this patch is new
>
>  drivers/base/dd.c           | 2 ++
>  drivers/base/power/common.c | 3 +++
>  include/linux/pm.h          | 1 +
>  3 files changed, 6 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b526e0e0f52d..13ab98e033ea 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -25,6 +25,7 @@
>  #include <linux/kthread.h>
>  #include <linux/wait.h>
>  #include <linux/async.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
>  #include <linux/slab.h>
> @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev)
>         dev->dma_range_map = NULL;
>         device_set_driver(dev, NULL);
>         dev_set_drvdata(dev, NULL);
> +       dev_pm_domain_detach(dev, dev->power.detach_power_off);
>         if (dev->pm_domain && dev->pm_domain->dismiss)
>                 dev->pm_domain->dismiss(dev);
>         pm_runtime_reinit(dev);
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 781968a128ff..a8f302ed27a5 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>         if (!ret)
>                 ret = genpd_dev_pm_attach(dev);
>
> +       if (dev->pm_domain)
> +               dev->power.detach_power_off = power_on;

I'm assuming that you have checked all of the users of
dev_pm_domain_attach() and verified that the "power off" value is the
same as the "power on" one for all of them.

> +
>         return ret < 0 ? ret : 0;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index f0bd8fbae4f2..dcbe2c1ef59b 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -720,6 +720,7 @@ struct dev_pm_info {
>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>         void (*set_latency_tolerance)(struct device *, s32);
>         struct dev_pm_qos       *qos;
> +       bool                    detach_power_off:1;

Please put the new flag under #ifdef CONFIG_PM after memalloc_noio and
comment it as "Owned by the driver core".

Otherwise LGTM.

>  };
>
>  extern int dev_pm_get_subsys_data(struct device *dev);
> --
> 2.43.0
>

Powered by blists - more mailing lists