On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >
> > 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.
[...]
Kind regards
Uffe
