Re: [PATCH v2 2/5] pmdomain: core: Allow a non-CPU device in a CPU PM domain to do power on

[Ulf Hansson]

On Fri, Sep 4, 2026 at 6:26 PM Dhruva G <goledhruva@xxxxxxxxx> wrote:
>
> On 01-09-2026 16:44, Ulf Hansson wrote:
> > A driver for a non-CPU device that is attached to a CPU PM domain (the
> > genpd has the GENPD_FLAG_CPU_DOMAIN configuration set), is currently not
> > able to power on the PM domain. More precisely, to power on a CPU PM domain
> > one of its corresponding CPUs needs to be woken up if they are idle.
> >
> > The current support for a non-CPU device is that its driver can only
> > prevent an already powered on CPU PM domain from being powered off. This
> > leads to problems for a driver while probing its device or when it needs to
> > call pm_runtime_get_sync() to turn on the power for it. From the driver
> > point of view it looks like it all works fine, but when accessing the
> > device it may end up with various errors as the device may not be fully
> > powered on.
> >
> > To fix the behavior for these types of devices, let's adjust the behaviour
> > in genpd_power_on() to wake up an idle CPU that belongs to it, in cases
> > when it's needed.
> >
> > Link: https://lore.kernel.org/all/CAPx+jO-sCierYj8jnoKQHckJG16dOBxnNrsZVYO=38R2cLV8nw@xxxxxxxxxxxxxx/
> > Reviewed-by: Abel Vesa <abel.vesa@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> >       - Rename a function according to Abel's suggestion.
> >
> > ---
> >  drivers/pmdomain/core.c | 80 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 75 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 6ac1ce18fda3..97273ed2f825 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/pm_runtime.h>
> > @@ -19,11 +20,14 @@
> >  #include <linux/slab.h>
> >  #include <linux/err.h>
> >  #include <linux/sched.h>
> > +#include <linux/smp.h>
> >  #include <linux/suspend.h>
> >  #include <linux/export.h>
> >  #include <linux/cpu.h>
> >  #include <linux/debugfs.h>
> >
> > +#include <trace/events/ipi.h>
> > +
> >  /* Provides a unique ID for each genpd device */
> >  static DEFINE_IDA(genpd_ida);
> >
> > @@ -32,7 +36,9 @@ static const struct bus_type genpd_provider_bus_type = {
> >       .name           = "genpd_provider",
> >  };
> >
> > -#define GENPD_RETRY_MAX_MS   250             /* Approximate */
> > +#define GENPD_RETRY_MAX_MS           250             /* Approximate */
> > +#define GENPD_CPU_ON_POLL_PERIOD_US  100             /* 100us */
> > +#define GENPD_CPU_ON_TIMEOUT_US              5000000         /* 5s */
> >
> >  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)               \
> >  ({                                                           \
> > @@ -1027,15 +1033,75 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> >       }
> >  }
> >
> > +static bool genpd_status_on(struct generic_pm_domain *genpd)
> > +{
> > +     bool is_on;
> > +
> > +     genpd_lock(genpd);
> > +     is_on = genpd_status_on_unlocked(genpd);
> > +     genpd_unlock(genpd);
> > +
> > +     return is_on;
> > +}
> > +
> > +static int genpd_wakeup_cpu(struct generic_pm_domain *genpd)
> > +{
> > +     unsigned int cpu;
> > +     bool is_on;
> > +     int ret;
> > +
> > +     /* Find the first online CPU in the genpd's cpumask. */
> > +     cpu = cpumask_first_and(genpd->cpus, cpu_online_mask);
> > +     if (cpu >= nr_cpu_ids)
> > +             return -EAGAIN;
> > +
> > +     genpd_unlock(genpd);
> > +
> > +     /* Send a IPI to wakeup the selected CPU. */
> > +     smp_send_reschedule(cpu);
> > +
> > +     /* Poll to wait for it to complete the power on sequence. */
> > +     ret = readx_poll_timeout(genpd_status_on, genpd, is_on, is_on,
> > +                              GENPD_CPU_ON_POLL_PERIOD_US,
> > +                              GENPD_CPU_ON_TIMEOUT_US);
>
> How is sleeping here made safe for IRQ-safe consumers and child domains?
>
> For a hypothetical example, consider an IRQ-safe SPI controller in an IRQ-safe child domain D,
> whose parent P is a CPU domain. CPU A is outside P, and CPU B belongs to P. Both domains are initially off.
>
> A runtime-resume request on CPU A follows this path in drivers/pmdomain/core.c:
>
> genpd_runtime_resume(SPI device)
> -> lock D
> -> genpd_power_on(D)
> -> lock parent P
> -> genpd_power_on(P)
> -> genpd_wakeup_cpu(P)
>
> genpd_wakeup_cpu() drops P's lock and sends an IPI to CPU B, but D's spinlock remains held. readx_poll_timeout()
> can then reach usleep_range() while D’s spinlock is still held.
>
> There is also a problem without the child domain: for an IRQ-safe device attached directly to P, __rpm_callback()
> leaves interrupts disabled. Dropping P’s lock restores the already-disabled interrupt state, so readx_poll_timeout()
> still cannot be used with a nonzero sleep interval and timeout.
>
> I have not reproduced this on a board; this is a hypothetical configuration illustrating the paths.
>
> Is there a restriction elsewhere that prevents either configuration? Otherwise, this wait cannot sleep,
> and the parent lock needs to be reacquired with the same nesting depth used by genpd_power_on().

Good point and thanks for catching this!

As this path is only for CPU PM domains (has GENPD_FLAG_IRQ_SAFE and
GENPD_FLAG_CPU_DOMAIN bits set), this is solved by converting to the
atomic polling helpers. I am cooking a new version!

[...]

Kind regards
Uffe