Received-SPF: pass (google.com: domain of linux-kernel+bounces-199258-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240527142557.321610-1-ulf.hansson@linaro.org>
 <20240527142557.321610-2-ulf.hansson@linaro.org> <52dce8d3-acfa-4f2c-92d0-c25aa59d6526@quicinc.com>
 <CAPDyKFq0V9ke30zYAGzS1aU0yq49DdfDkfYzxkLBZuaVGsyGDA@mail.gmail.com> <24570028-42cf-43b1-95f2-b6f48233bef9@quicinc.com>
In-Reply-To: <24570028-42cf-43b1-95f2-b6f48233bef9@quicinc.com>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Mon, 3 Jun 2024 15:58:47 +0200
Message-ID: <CAPDyKFoOxa1tmzxY4jrbPET5RKF6s-OAfYV=eZY9sPrJMR6jaQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains
 on PREEMPT_RT
To: Nikunj Kela <quic_nkela@quicinc.com>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, linux-pm@vger.kernel.org, 
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>, Nikunj Kela <nkela@quicinc.com>, 
	Prasad Sodagudi <psodagud@quicinc.com>, Maulik Shah <quic_mkshah@quicinc.com>, 
	Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, linux-rt-users@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Thu, 30 May 2024 at 16:23, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>
>
> On 5/30/2024 1:15 AM, Ulf Hansson wrote:
> > On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@quicinc.com> wrote:
> >>
> >> On 5/27/2024 7:25 AM, Ulf Hansson wrote:
> >>> To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
> >>> during s2idle on a PREEMPT_RT based configuration, we can't use the regular
> >>> spinlock, as they are turned into sleepable locks on PREEMPT_RT.
> >>>
> >>> To address this problem, let's convert into using the raw spinlock, but
> >>> only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
> >>> this way, the lock can still be acquired/released in atomic context, which
> >>> is needed in the idle-path for PREEMPT_RT.
> >>>
> >>> Do note that the genpd power-on/off notifiers may also be fired during
> >>> s2idle, but these are already prepared for PREEMPT_RT as they are based on
> >>> the raw notifiers. However, consumers of them may need to adopt accordingly
> >>> to work properly on PREEMPT_RT.
> >>>
> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>       - None.
> >>>
> >>> ---
> >>>  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
> >>>  include/linux/pm_domain.h |  5 ++++-
> >>>  2 files changed, 50 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >>> index 623d15b68707..072e6bdb6ee6 100644
> >>> --- a/drivers/pmdomain/core.c
> >>> +++ b/drivers/pmdomain/core.c
> >>> @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> >>>       .unlock = genpd_unlock_spin,
> >>>  };
> >>>
> >>> +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +}
> >>> +
> >>> +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
> >>> +                                     int depth)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +}
> >>> +
> >>> +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __releases(&genpd->raw_slock)
> >>> +{
> >>> +     raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
> >>> +}
> >>> +
> >>> +static const struct genpd_lock_ops genpd_raw_spin_ops = {
> >>> +     .lock = genpd_lock_raw_spin,
> >>> +     .lock_nested = genpd_lock_nested_raw_spin,
> >>> +     .lock_interruptible = genpd_lock_interruptible_raw_spin,
> >>> +     .unlock = genpd_unlock_raw_spin,
> >>> +};
> >>> +
> >>>  #define genpd_lock(p)                        p->lock_ops->lock(p)
> >>>  #define genpd_lock_nested(p, d)              p->lock_ops->lock_nested(p, d)
> >>>  #define genpd_lock_interruptible(p)  p->lock_ops->lock_interruptible(p)
> >>> @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
> >>>
> >>>  static void genpd_lock_init(struct generic_pm_domain *genpd)
> >>>  {
> >>> -     if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> >>> +     if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
> >>> +             raw_spin_lock_init(&genpd->raw_slock);
> >>> +             genpd->lock_ops = &genpd_raw_spin_ops;
> >>> +     } else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> >> Hi Ulf, though you are targeting only CPU domains for now, I wonder if
> >> FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
> >> it is safe for atomic context which won't be the case for PREEMPT_RT?
> > You have a point!
> >
> > However, we also need to limit the use of raw spinlocks, from
> > PREEMPT_RT point of view. In other words, just because a genpd
> > provider is capable of executing its callbacks in atomic context,
> > doesn't always mean that it should use raw spinlocks too.
>
> Got it! Thanks. Maybe in future, if there is a need, a new GENPD FLAG
> for RT, something like GENPD_FLAG_IRQ_SAFE_RT, can be added to address this.

Yes, I agree, something along those lines would make sense.

BTW, did you manage to get some time to test the series on your end?

Kind regards
Uffe