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

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

[Meng Li]

When booting up preempt rt kernel on stratix10 platform, there is
below calltrace:
BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:969
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: swapper/0
Preemption disabled at:
[<ffff8000100b25b0>] __setup_irq+0xe0/0x730
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.78-rt54-yocto-preempt-rt #1
Hardware name: SoCFPGA Stratix 10 SoCDK (DT)
Call trace:
 ......
 ___might_sleep+0x144/0x180
 rt_spin_lock+0x34/0x9c
 regmap_lock_spinlock+0x24/0x40
 regmap_write+0x48/0x84
 a10_eccmgr_irq_unmask+0x34/0x40
 ......
 altr_edac_a10_probe+0x16c/0x2b0
 platform_drv_probe+0x60/0xb4
 ......
 ret_from_fork+0x10/0x38

Because regmap_write is invoked under preemption disabled status, and
might trigger sleep.
Recently, the commit 67021f25d952("regmap: teach regmap to use raw
spinlocks if requested in the config ") add an option for regmap to use
raw spinlock. So, enable raw spinlock in preempt-rt kernel to avoid the
might sleep issue.
Note: The commit 67021f25d952 is only in kernel v5.15, not included in
earlier versions. So, if intend to fix this issue in earlier preempt-rt
kernel, it is need to backport the 2 patches together, otherwise there
will be building issue.

Signed-off-by: Meng Li <Meng.Li@windriver.com>
---

v2:
 - improve the patch title.
 - improve the commit log. Clear which commit is depended by this patch.

---
 drivers/mfd/altera-sysmgr.c | 3 +++
 1 file changed, 3 insertions(+)

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
 };
 
 /**
-- 
2.17.1

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

[Arnd Bergmann]

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.

       Arnd

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

[Li, Meng]

> -----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);
		}

Thanks,
Limeng

>        Arnd

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

[Lee Jones]

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

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

[Li, Meng]

> -----Original Message-----
> From: Lee Jones <lee.jones@linaro.org>
> Sent: Tuesday, November 16, 2021 11:39 PM
> To: Li, Meng <Meng.Li@windriver.com>
> Cc: Arnd Bergmann <arnd@arndb.de>; 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
> 
> [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@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.
> 

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