Received-SPF: pass (google.com: domain of linux-kernel+bounces-192989-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <52dce8d3-acfa-4f2c-92d0-c25aa59d6526@quicinc.com>
Date: Tue, 28 May 2024 12:55:58 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains
 on PREEMPT_RT
To: Ulf Hansson <ulf.hansson@linaro.org>,
        "Rafael J . Wysocki"
	<rafael@kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>, <linux-pm@vger.kernel.org>
CC: 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>
References: <20240527142557.321610-1-ulf.hansson@linaro.org>
 <20240527142557.321610-2-ulf.hansson@linaro.org>
Content-Language: en-US
From: Nikunj Kela <quic_nkela@quicinc.com>
In-Reply-To: <20240527142557.321610-2-ulf.hansson@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit


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? 


>  		spin_lock_init(&genpd->slock);
>  		genpd->lock_ops = &genpd_spin_ops;
>  	} else {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f24546a3d3db..0d7fc47de3bc 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -194,8 +194,11 @@ struct generic_pm_domain {
>  			spinlock_t slock;
>  			unsigned long lock_flags;
>  		};
> +		struct {
> +			raw_spinlock_t raw_slock;
> +			unsigned long raw_lock_flags;
> +		};
>  	};
> -
>  };
>  
>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)