linux-kernel - Re: [PATCH RFC 1/2] PM: domains: Add GENPD_FLAG_RT_SAFE for PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a1433373-20c5-91a4-bc94-1583b103bd58@linaro.org>
Date:   Fri, 16 Dec 2022 16:04:42 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Cc:     Adrien Thierry <athierry@...hat.com>,
        Brian Masney <bmasney@...hat.com>
Subject: Re: [PATCH RFC 1/2] PM: domains: Add GENPD_FLAG_RT_SAFE for
 PREEMPT_RT

On 16/12/2022 15:58, Krzysztof Kozlowski wrote:
> Realtime kernels with PREEMPT_RT must use raw_spinlock_t, so add a flag
> allowing a power domain provider to indicate it is RT safe.  The flag is
> supposed to be used with existing GENPD_FLAG_IRQ_SAFE.
> 
> Cc: Adrien Thierry <athierry@...hat.com>
> Cc: Brian Masney <bmasney@...hat.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> 
> ---
> 
> Independently from Adrien, I encountered the same problem around genpd
> when using PREEMPT_RT kernel.
> 
> Previous patch by Adrien:
> https://lore.kernel.org/all/20220615203605.1068453-1-athierry@redhat.com/
> ---
>  drivers/base/power/domain.c | 51 +++++++++++++++++++++++++++++++++++--
>  include/linux/pm_domain.h   | 13 ++++++++++
>  2 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a539d6bc6fe0..478862ae478a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -119,6 +119,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>  	.unlock = genpd_unlock_spin,
>  };
>  
> +static void genpd_lock_rawspin(struct generic_pm_domain *genpd)
> +	__acquires(&genpd->rslock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&genpd->rslock, flags);
> +	genpd->rlock_flags = flags;
> +}
> +
> +static void genpd_lock_nested_rawspin(struct generic_pm_domain *genpd,
> +					int depth)
> +	__acquires(&genpd->rslock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave_nested(&genpd->rslock, flags, depth);
> +	genpd->rlock_flags = flags;
> +}
> +
> +static int genpd_lock_interruptible_rawspin(struct generic_pm_domain *genpd)
> +	__acquires(&genpd->rslock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&genpd->rslock, flags);
> +	genpd->rlock_flags = flags;
> +	return 0;
> +}
> +
> +static void genpd_unlock_rawspin(struct generic_pm_domain *genpd)
> +	__releases(&genpd->rslock)
> +{
> +	raw_spin_unlock_irqrestore(&genpd->rslock, genpd->rlock_flags);
> +}
> +
> +static const struct genpd_lock_ops genpd_rawspin_ops = {
> +	.lock = genpd_lock_rawspin,
> +	.lock_nested = genpd_lock_nested_rawspin,
> +	.lock_interruptible = genpd_lock_interruptible_rawspin,
> +	.unlock = genpd_unlock_rawspin,
> +};
> +
>  #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)
> @@ -2048,8 +2090,13 @@ 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) {
> -		spin_lock_init(&genpd->slock);
> -		genpd->lock_ops = &genpd_spin_ops;
> +		if (genpd->flags & GENPD_FLAG_RT_SAFE) {
> +			raw_spin_lock_init(&genpd->rslock);
> +			genpd->lock_ops = &genpd_rawspin_ops;
> +		} else {
> +			spin_lock_init(&genpd->slock);
> +			genpd->lock_ops = &genpd_spin_ops;
> +		}
>  	} else {
>  		mutex_init(&genpd->mlock);
>  		genpd->lock_ops = &genpd_mtx_ops;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index c6cda024ff99..6b0f9de2be1c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -61,6 +61,14 @@
>   * GENPD_FLAG_MIN_RESIDENCY:	Enable the genpd governor to consider its
>   *				components' next wakeup when determining the
>   *				optimal idle state.
> + *
> + * GENPD_FLAG_RT_SAFE:		When used with GENPD_FLAG_IRQ_SAFE, this informs
> + *				genpd that its backend callbacks, ->power_on|off(),
> + *				do not use other spinlocks. They might use
> + *				raw_spinlocks or other pre-emption-disable
> + *				methods, all of which are PREEMPT_RT safe. Note
> + *				that, a genpd having this flag set, requires its
> + *				masterdomains to also have it set.

Now I see I might be missing such check in genpd_add_subdomain().

Best regards,
Krzysztof