Date:   Tue, 16 Nov 2021 15:39:04 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     "Li, Meng" <Meng.Li@windriver.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        "thor.thayer@linux.intel.com" <thor.thayer@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mfd: altera-sysmgr: enable raw spinlock feature for
 preempt-rt kernel
Message-ID: <YZPQmDJ1PI+TlG7C@google.com>
References: <20211116105424.12685-1-Meng.Li@windriver.com>
 <CAK8P3a241g2vQrfiEf_YcQ0=t5fhN0R0CrzQE5DK7XQgAGxKYg@mail.gmail.com>
 <PH0PR11MB5191FEE6D75B558D629D517AF1999@PH0PR11MB5191.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <PH0PR11MB5191FEE6D75B558D629D517AF1999@PH0PR11MB5191.namprd11.prod.outlook.com>
Precedence: bulk

On Tue, 16 Nov 2021, Li, Meng wrote:

> 
> 
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@arndb.de>
> > Sent: Tuesday, November 16, 2021 8:02 PM
> > To: Li, Meng <Meng.Li@windriver.com>
> > Cc: thor.thayer@linux.intel.com; Lee Jones <lee.jones@linaro.org>; Arnd
> > Bergmann <arnd@arndb.de>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>
> > 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@windriver.com> 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.

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