Re: [PATCH v2 1/5] PM: domains: Add GENPD_FLAG_RT_SAFE for PREEMPT_RT
From: Ulf Hansson
Date: Wed Jan 04 2023 - 10:47:19 EST
On Mon, 19 Dec 2022 at 16:15, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> Realtime kernels with PREEMPT_RT must use raw_spinlock_t for domains
> which are invoked from CPU idle (thus from atomic section). Example is
> cpuidle PSCI domain driver which itself is PREEMPT_RT safe, but is being
> called as part of cpuidle.
Just so I don't get this wrong, since the cpuidle-psci also calls
pm_runtime_* functions so it isn't PREEMPT_RT safe, at least not yet?
>
> 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@xxxxxxxxxx>
> Cc: Brian Masney <bmasney@xxxxxxxxxx>
> Cc: linux-rt-users@xxxxxxxxxxxxxxx
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
For genpd, overall, I think this looks like an okay approach to me.
Although, let me check the whole series (I need some more time to do
that) before I give this my blessing.
Kind regards
Uffe
>
> ---
>
> 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@xxxxxxxxxx/
> ---
> drivers/base/power/domain.c | 59 +++++++++++++++++++++++++++++++++++--
> include/linux/pm_domain.h | 13 ++++++++
> 2 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 967bcf9d415e..4dfce1d476f4 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)
> @@ -126,6 +168,8 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>
> #define genpd_status_on(genpd) (genpd->status == GENPD_STATE_ON)
> #define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE)
> +#define genpd_is_rt_safe(genpd) (genpd_is_irq_safe(genpd) && \
> + (genpd->flags & GENPD_FLAG_RT_SAFE))
> #define genpd_is_always_on(genpd) (genpd->flags & GENPD_FLAG_ALWAYS_ON)
> #define genpd_is_active_wakeup(genpd) (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
> #define genpd_is_cpu_domain(genpd) (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
> @@ -1838,6 +1882,12 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
> return -EINVAL;
> }
>
> + if (!genpd_is_rt_safe(genpd) && genpd_is_rt_safe(subdomain)) {
> + WARN(1, "Parent %s of subdomain %s must be RT safe\n",
> + genpd->name, subdomain->name);
> + return -EINVAL;
> + }
> +
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link)
> return -ENOMEM;
> @@ -2008,8 +2058,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 1cd41bdf73cf..0a1600244963 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.
> */
> #define GENPD_FLAG_PM_CLK (1U << 0)
> #define GENPD_FLAG_IRQ_SAFE (1U << 1)
> @@ -69,6 +77,7 @@
> #define GENPD_FLAG_CPU_DOMAIN (1U << 4)
> #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> +#define GENPD_FLAG_RT_SAFE (1U << 7)
>
> enum gpd_status {
> GENPD_STATE_ON = 0, /* PM domain is on */
> @@ -164,6 +173,10 @@ struct generic_pm_domain {
> spinlock_t slock;
> unsigned long lock_flags;
> };
> + struct {
> + raw_spinlock_t rslock;
> + unsigned long rlock_flags;
> + };
> };
>
> };
> --
> 2.34.1
>