RE: [PATCH] mfd: altera-sysmgr: enable raw spinlock feature for preempt-rt kernel

[Li, Meng]

> -----Original Message-----
> From: Lee Jones <lee.jones@xxxxxxxxxx>
> Sent: Tuesday, November 16, 2021 11:39 PM
> To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>; thor.thayer@xxxxxxxxxxxxxxx; Linux
> Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] mfd: altera-sysmgr: enable raw spinlock feature for
> preempt-rt kernel
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, 16 Nov 2021, Li, Meng wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann <arnd@xxxxxxxx>
> > > Sent: Tuesday, November 16, 2021 8:02 PM
> > > To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>
> > > Cc: thor.thayer@xxxxxxxxxxxxxxx; Lee Jones <lee.jones@xxxxxxxxxx>;
> > > Arnd Bergmann <arnd@xxxxxxxx>; Linux Kernel Mailing List <linux-
> > > kernel@xxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH] mfd: altera-sysmgr: enable raw spinlock feature
> > > for preempt-rt kernel
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Tue, Nov 16, 2021 at 11:54 AM Meng Li <Meng.Li@xxxxxxxxxxxxx>
> wrote:
> > > > diff --git a/drivers/mfd/altera-sysmgr.c
> > > > b/drivers/mfd/altera-sysmgr.c index 5d3715a28b28..27271cec5d53
> > > > 100644
> > > > --- a/drivers/mfd/altera-sysmgr.c
> > > > +++ b/drivers/mfd/altera-sysmgr.c
> > > > @@ -83,6 +83,9 @@ static struct regmap_config
> > > > altr_sysmgr_regmap_cfg =
> > > {
> > > >         .fast_io = true,
> > > >         .use_single_read = true,
> > > >         .use_single_write = true,
> > > > +#ifdef CONFIG_PREEMPT_RT
> > > > +       .use_raw_spinlock = true,
> > > > +#endif
> > >
> > > I think you should remove the #ifdef here: if PREEMPT_RT is
> > > disabled, the flag has no effect because spinlock behaves the same
> > > way as raw_spinlock. If anything else starts requiring the use of
> > > raw spinlocks, then we probably want the flag to be set  here as well.
> > >
> >
> > Thanks for your suggestion, and I also agree with the spinlock action when
> PREEMPT_RT is disabled.
> > But please allow me to explain why I keep the "ifdef"
> > 1. although I send this patch to mainline upstream, I only want to fix this
> issue in RT kernel.
> >     Moreover, the commit 67021f25d952("regmap: teach regmap to use raw
> spinlocks if requested in the config ") is also for RT kernel even if it doesn't
> use "ifdef CONFIG_PREEMPT_RT"
> >     My ideal is that if this patch is merged into mainline, Linux-rt maintainer
> will not spend extra effort to focus on this patch. After all, this fixing is more
> related with driver.
> >     In addition, I found out there are other patches with "ifdef
> CONFIG_PREEMPT_RT" merged by mainline, so I also send this patch to
> mainline, not Linux-rt.
> >
> > 2. I check regmap.c code that is related with use_raw_spinlock. If
> PREEMPT_RT is disabled and use_raw_spinlock is set as true, the else case
> will not be entered any longer.
> >     In other words, in mainline standard kernel, if use_raw_spinlock is set as
> true, raw spinlock will be used forever, and the code in else case will become
> useless.
> >     I feel it is a little unreasonable. So, I keep the "ifdef"
> >       if ((bus && bus->fast_io) ||
> >                   config->fast_io) {
> >                       if (config->use_raw_spinlock) {
> >                               raw_spin_lock_init(&map->raw_spinlock);
> >                               map->lock = regmap_lock_raw_spinlock;
> >                               map->unlock = regmap_unlock_raw_spinlock;
> >                               lockdep_set_class_and_name(&map->raw_spinlock,
> >                                                          lock_key, lock_name);
> >                       } else {
> >                               spin_lock_init(&map->spinlock);
> >                               map->lock = regmap_lock_spinlock;
> >                               map->unlock = regmap_unlock_spinlock;
> >                               lockdep_set_class_and_name(&map->spinlock,
> >                                                          lock_key, lock_name);
> >                       }
> >               } else {
> >                       mutex_init(&map->mutex);
> >                       map->lock = regmap_lock_mutex;
> >                       map->unlock = regmap_unlock_mutex;
> >                       map->can_sleep = true;
> >                       lockdep_set_class_and_name(&map->mutex,
> >                                                  lock_key, lock_name);
> >               }
> >
> 
> I dislike #ifery as a general rule.  So with that in mind - if it's not required, I'd
> prefer that it's removed.
> 

Ok！
There is no real difference if remove the #ifery.
I will check the standard kernel and then sent v3 RR.

Thanks,
Limeng

> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services Linaro.org │ Open source
> software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog