[PATCH 0/2] Use raw spinlocks in the ls-extirq driver

[PATCH 0/2] Use raw spinlocks in the ls-extirq driver

[Vladimir Oltean]

The ls-extirq irqchip driver accesses regmap inside its implementation
of the struct irq_chip :: irq_set_type method, and currently regmap
only knows to lock using normal spinlocks. But the method above wants
raw spinlock context, so this isn't going to work and triggers a
"[ BUG: Invalid wait context ]" splat.

The best we can do given the arrangement of the code is to patch regmap
and the syscon driver: regmap to support raw spinlocks, and syscon to
request them on behalf of its ls-extirq consumer.

Link: https://lore.kernel.org/lkml/20210825135438.ubcuxm5vctt6ne2q@skbuf/T/#u

Vladimir Oltean (2):
  regmap: teach regmap to use raw spinlocks if requested in the config
  mfd: syscon: request a regmap with raw spinlocks for some devices

 drivers/base/regmap/internal.h |  4 ++++
 drivers/base/regmap/regmap.c   | 35 +++++++++++++++++++++++++++++-----
 drivers/mfd/syscon.c           | 16 ++++++++++++++++
 include/linux/regmap.h         |  2 ++
 4 files changed, 52 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Vladimir Oltean]

Some drivers might access regmap in a context where a raw spinlock is
held. An example is drivers/irqchip/irq-ls-extirq.c, which calls
regmap_update_bits() from struct irq_chip :: irq_set_type, which is a
method called by __irq_set_trigger() under the desc->lock raw spin lock.

Since desc->lock is a raw spin lock and the regmap internal lock for
mmio is a plain spinlock (which can become sleepable on RT), this is an
invalid locking scheme and we get a splat stating that this is a
"[ BUG: Invalid wait context ]".

It seems reasonable for regmap to have an option use a raw spinlock too,
so add that in the config such that drivers can request it.

Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/base/regmap/internal.h |  4 ++++
 drivers/base/regmap/regmap.c   | 35 +++++++++++++++++++++++++++++-----
 include/linux/regmap.h         |  2 ++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 0097696c31de..b1905916f7af 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -53,6 +53,10 @@ struct regmap {
 			spinlock_t spinlock;
 			unsigned long spinlock_flags;
 		};
+		struct {
+			raw_spinlock_t raw_spinlock;
+			unsigned long raw_spinlock_flags;
+		};
 	};
 	regmap_lock lock;
 	regmap_unlock unlock;
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index fe3e38dd5324..e7ae477d2fcf 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -533,6 +533,23 @@ __releases(&map->spinlock)
 	spin_unlock_irqrestore(&map->spinlock, map->spinlock_flags);
 }
 
+static void regmap_lock_raw_spinlock(void *__map)
+__acquires(&map->raw_spinlock)
+{
+	struct regmap *map = __map;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&map->raw_spinlock, flags);
+	map->raw_spinlock_flags = flags;
+}
+
+static void regmap_unlock_raw_spinlock(void *__map)
+__releases(&map->raw_spinlock)
+{
+	struct regmap *map = __map;
+	raw_spin_unlock_irqrestore(&map->raw_spinlock, map->raw_spinlock_flags);
+}
+
 static void dev_get_regmap_release(struct device *dev, void *res)
 {
 	/*
@@ -770,11 +787,19 @@ struct regmap *__regmap_init(struct device *dev,
 	} else {
 		if ((bus && bus->fast_io) ||
 		    config->fast_io) {
-			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);
+			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;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f5f08dd0a116..7a3cda794501 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -344,6 +344,7 @@ typedef void (*regmap_unlock)(void *);
  * @ranges: Array of configuration entries for virtual address ranges.
  * @num_ranges: Number of range configuration entries.
  * @use_hwlock: Indicate if a hardware spinlock should be used.
+ * @use_raw_spinlock: Indicate if a raw spinlock should be used.
  * @hwlock_id: Specify the hardware spinlock id.
  * @hwlock_mode: The hardware spinlock mode, should be HWLOCK_IRQSTATE,
  *		 HWLOCK_IRQ or 0.
@@ -403,6 +404,7 @@ struct regmap_config {
 	unsigned int num_ranges;
 
 	bool use_hwlock;
+	bool use_raw_spinlock;
 	unsigned int hwlock_id;
 	unsigned int hwlock_mode;
 
-- 
2.25.1

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Thomas Gleixner]

On Wed, Aug 25 2021 at 23:50, Vladimir Oltean wrote:

> Some drivers might access regmap in a context where a raw spinlock is
> held. An example is drivers/irqchip/irq-ls-extirq.c, which calls
> regmap_update_bits() from struct irq_chip :: irq_set_type, which is a
> method called by __irq_set_trigger() under the desc->lock raw spin lock.
>
> Since desc->lock is a raw spin lock and the regmap internal lock for
> mmio is a plain spinlock (which can become sleepable on RT), this is an
> invalid locking scheme and we get a splat stating that this is a
> "[ BUG: Invalid wait context ]".
>
> It seems reasonable for regmap to have an option use a raw spinlock too,
> so add that in the config such that drivers can request it.

What's reasonable about that?

What exactly prevents the regmap locking to use a raw spinlock
unconditionally?

Even for the case where the regmap is not dealing with irq chips it does
not make any sense to protect low level operations on shared register
with a regular spinlock. I might be missing something though...

Thanks,

        tglx

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Vladimir Oltean]

On Fri, Aug 27, 2021 at 01:01:35AM +0200, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 23:50, Vladimir Oltean wrote:
> > It seems reasonable for regmap to have an option use a raw spinlock too,
> > so add that in the config such that drivers can request it.
>
> What's reasonable about that?
>
> What exactly prevents the regmap locking to use a raw spinlock
> unconditionally?
>
> Even for the case where the regmap is not dealing with irq chips it does
> not make any sense to protect low level operations on shared register
> with a regular spinlock. I might be missing something though...

Mark, any comments?

Generally it is said that misusing raw spinlocks has detrimential
performance upon the real-time aspects of the system, and I don't really
have a good feeling for what constitutes misuse vs what is truly justified
(in fact I did start the thread with "apologies for my novice level of
understanding").

On the other hand, while it does seem a bit too much overhead for
sequences of MMIO reads/writes to be able to be preempted, it doesn't
sound like it would break something either, so...

But I will say that I've tested that and it would solve both my problems
(the stack trace with ls-extirq and the fact that I would like to avoid
reworking the ls-extirq driver too much), as well as problems I never
knew I had: it turns out, armada_37xx_irq_set_type() uses regmap too
(and a sleepable spin lock too - irq_lock). The latter would still have
to be manually patched out.

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Thomas Gleixner]

On Fri, Aug 27 2021 at 16:12, Vladimir Oltean wrote:
> On Fri, Aug 27, 2021 at 01:01:35AM +0200, Thomas Gleixner wrote:
>> Even for the case where the regmap is not dealing with irq chips it does
>> not make any sense to protect low level operations on shared register
>> with a regular spinlock. I might be missing something though...
>
> Mark, any comments?
>
> Generally it is said that misusing raw spinlocks has detrimential
> performance upon the real-time aspects of the system, and I don't really
> have a good feeling for what constitutes misuse vs what is truly justified
> (in fact I did start the thread with "apologies for my novice level of
> understanding").
>
> On the other hand, while it does seem a bit too much overhead for
> sequences of MMIO reads/writes to be able to be preempted, it doesn't
> sound like it would break something either, so...

The question is how long those sequences are.

If it's just a pair or so then the raw spinlock protection has
definitely a smaller worst case than the sleeping spinlock in the
contended case.

OTOH, if regmap operations consist of several dozens of MMIO accesses,
then the preempt disabled region might be quite long.

I'm not familiar enough with regmaps to make a judgement here.

Thanks,

        tglx

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Vladimir Oltean]

On Fri, Aug 27, 2021 at 09:59:56PM +0200, Thomas Gleixner wrote:
> On Fri, Aug 27 2021 at 16:12, Vladimir Oltean wrote:
> > On Fri, Aug 27, 2021 at 01:01:35AM +0200, Thomas Gleixner wrote:
> >> Even for the case where the regmap is not dealing with irq chips it does
> >> not make any sense to protect low level operations on shared register
> >> with a regular spinlock. I might be missing something though...
> >
> > Mark, any comments?
> >
> > Generally it is said that misusing raw spinlocks has detrimential
> > performance upon the real-time aspects of the system, and I don't really
> > have a good feeling for what constitutes misuse vs what is truly justified
> > (in fact I did start the thread with "apologies for my novice level of
> > understanding").
> >
> > On the other hand, while it does seem a bit too much overhead for
> > sequences of MMIO reads/writes to be able to be preempted, it doesn't
> > sound like it would break something either, so...
>
> The question is how long those sequences are.
>
> If it's just a pair or so then the raw spinlock protection has
> definitely a smaller worst case than the sleeping spinlock in the
> contended case.
>
> OTOH, if regmap operations consist of several dozens of MMIO accesses,
> then the preempt disabled region might be quite long.
>
> I'm not familiar enough with regmaps to make a judgement here.

I think "how long are the read/write regmap sequences" is outside of
regmap's control, but rather a matter of usage. This would point towards
the current solution, where users select whether to use raw spinlocks or
not, being the preferable one.

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Rasmus Villemoes]

On 27/08/2021 01.01, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 23:50, Vladimir Oltean wrote:
> 
>> Some drivers might access regmap in a context where a raw spinlock is
>> held. An example is drivers/irqchip/irq-ls-extirq.c, which calls
>> regmap_update_bits() from struct irq_chip :: irq_set_type, which is a
>> method called by __irq_set_trigger() under the desc->lock raw spin lock.
>>
>> Since desc->lock is a raw spin lock and the regmap internal lock for
>> mmio is a plain spinlock (which can become sleepable on RT), this is an
>> invalid locking scheme and we get a splat stating that this is a
>> "[ BUG: Invalid wait context ]".
>>
>> It seems reasonable for regmap to have an option use a raw spinlock too,
>> so add that in the config such that drivers can request it.
> 
> What's reasonable about that?
> 
> What exactly prevents the regmap locking to use a raw spinlock
> unconditionally?

Perhaps this:

        /*
         * When we write in fast-paths with regmap_bulk_write() don't
allocate
         * scratch buffers with sleeping allocations.
         */
        if ((bus && bus->fast_io) || config->fast_io)
                map->alloc_flags = GFP_ATOMIC;
        else
                map->alloc_flags = GFP_KERNEL;

i.e. the regmap code can actually do allocations under whatever internal
lock it uses. So ISTM that any regmap that uses a raw_spinlock (whether
unconditionally or via Vladimir's opt-in) cannot be used with
regmap_bulk_write().

ISTM using regmap for mmio makes things more complicated than necessary.

Rasmus

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Mon, Aug 30, 2021 at 01:02:33PM +0200, Rasmus Villemoes wrote:

> i.e. the regmap code can actually do allocations under whatever internal
> lock it uses. So ISTM that any regmap that uses a raw_spinlock (whether
> unconditionally or via Vladimir's opt-in) cannot be used with
> regmap_bulk_write().

No, anything that's using a spinlock already needs to avoid any
allocations - ensuring that either there's no cache or that the
cache is fully initialized with defaults.  The only non-cache
allocations that might be done are only used by buses that sleep
anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Fri, Aug 27, 2021 at 01:01:35AM +0200, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 23:50, Vladimir Oltean wrote:

> > It seems reasonable for regmap to have an option use a raw spinlock too,
> > so add that in the config such that drivers can request it.

> What's reasonable about that?

> What exactly prevents the regmap locking to use a raw spinlock
> unconditionally?

We definitely can't use a raw spinlock unconditionally since we
support register maps on devices connected via buses which can't
be accessed atomically so we need the option of doing mutexes.

> Even for the case where the regmap is not dealing with irq chips it does
> not make any sense to protect low level operations on shared register
> with a regular spinlock. I might be missing something though...

That probably does make sense, I think we're just using regular
spinlocks for spinlocks mainly because they're the default rather
than because anyone put huge amounts of thought into it.  IIRC
the first users were using spinlocks for their locking when they
were converted.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Thomas Gleixner]

On Mon, Aug 30 2021 at 13:19, Mark Brown wrote:
> On Fri, Aug 27, 2021 at 01:01:35AM +0200, Thomas Gleixner wrote:
>> On Wed, Aug 25 2021 at 23:50, Vladimir Oltean wrote:
>
>> > It seems reasonable for regmap to have an option use a raw spinlock too,
>> > so add that in the config such that drivers can request it.
>
>> What's reasonable about that?
>
>> What exactly prevents the regmap locking to use a raw spinlock
>> unconditionally?
>
> We definitely can't use a raw spinlock unconditionally since we
> support register maps on devices connected via buses which can't
> be accessed atomically so we need the option of doing mutexes.

The mutex part is fine. For slow busses this is obviously required.

>> Even for the case where the regmap is not dealing with irq chips it does
>> not make any sense to protect low level operations on shared register
>> with a regular spinlock. I might be missing something though...
>
> That probably does make sense, I think we're just using regular
> spinlocks for spinlocks mainly because they're the default rather
> than because anyone put huge amounts of thought into it.  IIRC
> the first users were using spinlocks for their locking when they
> were converted.

So if the actual spinlock protected operations are not doing any other
business than accessing preallocated cache memory and a few MMIO
operations then converting them to raw spinlocks should have no real
impact on RT.

One way to do that is obviously starting with the patch from Vladimir
and then convert them one by one, so the assumption that they are not
doing anything nasty (in the RT sense) can be validated.

Thanks,

        tglx

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

On Mon, Aug 30, 2021 at 04:16:04PM +0200, Thomas Gleixner wrote:
> On Mon, Aug 30 2021 at 13:19, Mark Brown wrote:

> > That probably does make sense, I think we're just using regular
> > spinlocks for spinlocks mainly because they're the default rather
> > than because anyone put huge amounts of thought into it.  IIRC
> > the first users were using spinlocks for their locking when they
> > were converted.

> So if the actual spinlock protected operations are not doing any other
> business than accessing preallocated cache memory and a few MMIO
> operations then converting them to raw spinlocks should have no real
> impact on RT.

I think Vladimir's point that something might try to use one of the APIs
that can do multiple register writes atomically to generate a very long
register write sequence is valid here.  It's far from the common case
but it'd be hard to audit, it's going to be a lot easier to handle going
to raw spinlocks in the cases where it's specifically needed than to
keep on top of ensuring that none of the users are causing issues or
start causing issues in the future.  This does make me feel it's a bit
safer to leave the default the way it is since if you get it wrong then
lockdep will tend to notice very quickly while it's less likely that
we'd get tooling spotting issues the other way around.

> One way to do that is obviously starting with the patch from Vladimir
> and then convert them one by one, so the assumption that they are not
> doing anything nasty (in the RT sense) can be validated.

Vladimir's patch is in Linus' tree now so users that can safely do so
can start using raw spinlocks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regmap: teach regmap to use raw spinlocks if requested in the config

[Thomas Gleixner]

On Wed, Sep 01 2021 at 17:05, Mark Brown wrote:
> On Mon, Aug 30, 2021 at 04:16:04PM +0200, Thomas Gleixner wrote:
>> On Mon, Aug 30 2021 at 13:19, Mark Brown wrote:
>
>> > That probably does make sense, I think we're just using regular
>> > spinlocks for spinlocks mainly because they're the default rather
>> > than because anyone put huge amounts of thought into it.  IIRC
>> > the first users were using spinlocks for their locking when they
>> > were converted.
>
>> So if the actual spinlock protected operations are not doing any other
>> business than accessing preallocated cache memory and a few MMIO
>> operations then converting them to raw spinlocks should have no real
>> impact on RT.
>
> I think Vladimir's point that something might try to use one of the APIs
> that can do multiple register writes atomically to generate a very long
> register write sequence is valid here.  It's far from the common case
> but it'd be hard to audit, it's going to be a lot easier to handle going
> to raw spinlocks in the cases where it's specifically needed than to
> keep on top of ensuring that none of the users are causing issues or
> start causing issues in the future.  This does make me feel it's a bit
> safer to leave the default the way it is since if you get it wrong then
> lockdep will tend to notice very quickly while it's less likely that
> we'd get tooling spotting issues the other way around.

Fair enough.

>> One way to do that is obviously starting with the patch from Vladimir
>> and then convert them one by one, so the assumption that they are not
>> doing anything nasty (in the RT sense) can be validated.
>
> Vladimir's patch is in Linus' tree now so users that can safely do so
> can start using raw spinlocks.

ok

[PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Vladimir Oltean]

This patch solves a ls-extirq irqchip driver bug in a perhaps
non-intuitive (at least non-localized) way.

The issue is that ls-extirq uses regmap, and due to the fact that it is
being called by the IRQ core under raw spinlock context, it needs to use
raw spinlocks itself. So it needs to request raw spinlocks from the
regmap config.

All is fine so far, except the ls-extirq driver does not manage its own
regmap, instead it uses syscon_node_to_regmap() to get it from the
parent syscon (this driver).

Because the syscon regmap is initialized before any of the consumer
drivers (ls-extirq) probe, we need to know beforehand whether to request
raw spinlocks or not.

The solution seems to be to check some compatible string. The ls-extirq
driver probes on quite a few NXP Layerscape SoCs, all with different
compatible strings. This is potentially fragile and subject to bit rot
(since the fix is not localized to the ls-extirq driver, adding new
compatible strings there but not here seems plausible). Anyway, it is
probably the best we can do without major rework.

Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/mfd/syscon.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 765c0210cb52..70da4e87b072 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -83,6 +83,22 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	if (ret)
 		reg_io_width = 4;
 
+	/*
+	 * We might be providing a regmap to e.g. an irqchip driver, and in
+	 * that case, normal spinlocks won't do: the IRQ core holds raw
+	 * spinlocks, so it needs to be raw spinlocks all the way down.
+	 * Detect those drivers here (currently "ls-extirq") and request raw
+	 * spinlocks in the regmap config for them.
+	 */
+	if (of_device_is_compatible(np, "fsl,lx2160a-isc") ||
+	    of_device_is_compatible(np, "fsl,ls2080a-isc") ||
+	    of_device_is_compatible(np, "fsl,ls2080a-isc") ||
+	    of_device_is_compatible(np, "fsl,ls1088a-isc") ||
+	    of_device_is_compatible(np, "fsl,ls1043a-scfg") ||
+	    of_device_is_compatible(np, "fsl,ls1046a-scfg") ||
+	    of_device_is_compatible(np, "fsl,ls1021a-scfg"))
+		syscon_config.use_raw_spinlock = true;
+
 	ret = of_hwspin_lock_get_id(np, 0);
 	if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) {
 		syscon_config.use_hwlock = true;
-- 
2.25.1

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Arnd Bergmann]

On Wed, Aug 25, 2021 at 10:50 PM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> This patch solves a ls-extirq irqchip driver bug in a perhaps
> non-intuitive (at least non-localized) way.
>
> The issue is that ls-extirq uses regmap, and due to the fact that it is
> being called by the IRQ core under raw spinlock context, it needs to use
> raw spinlocks itself. So it needs to request raw spinlocks from the
> regmap config.
>
> All is fine so far, except the ls-extirq driver does not manage its own
> regmap, instead it uses syscon_node_to_regmap() to get it from the
> parent syscon (this driver).
>
> Because the syscon regmap is initialized before any of the consumer
> drivers (ls-extirq) probe, we need to know beforehand whether to request
> raw spinlocks or not.
>
> The solution seems to be to check some compatible string. The ls-extirq
> driver probes on quite a few NXP Layerscape SoCs, all with different
> compatible strings. This is potentially fragile and subject to bit rot
> (since the fix is not localized to the ls-extirq driver, adding new
> compatible strings there but not here seems plausible). Anyway, it is
> probably the best we can do without major rework.
>
> Suggested-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

This should work, but how hard would it be to change the ls-extirq
driver instead to not use the syscon driver at all but make the extirq
driver set up the regmap itself?

Are there any other users of the syscon?

         Arnd

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Vladimir Oltean]

On Wed, Aug 25, 2021 at 11:24:52PM +0200, Arnd Bergmann wrote:
> On Wed, Aug 25, 2021 at 10:50 PM Vladimir Oltean
> <vladimir.oltean@nxp.com> wrote:
> >
> > This patch solves a ls-extirq irqchip driver bug in a perhaps
> > non-intuitive (at least non-localized) way.
> >
> > The issue is that ls-extirq uses regmap, and due to the fact that it is
> > being called by the IRQ core under raw spinlock context, it needs to use
> > raw spinlocks itself. So it needs to request raw spinlocks from the
> > regmap config.
> >
> > All is fine so far, except the ls-extirq driver does not manage its own
> > regmap, instead it uses syscon_node_to_regmap() to get it from the
> > parent syscon (this driver).
> >
> > Because the syscon regmap is initialized before any of the consumer
> > drivers (ls-extirq) probe, we need to know beforehand whether to request
> > raw spinlocks or not.
> >
> > The solution seems to be to check some compatible string. The ls-extirq
> > driver probes on quite a few NXP Layerscape SoCs, all with different
> > compatible strings. This is potentially fragile and subject to bit rot
> > (since the fix is not localized to the ls-extirq driver, adding new
> > compatible strings there but not here seems plausible). Anyway, it is
> > probably the best we can do without major rework.
> >
> > Suggested-by: Mark Brown <broonie@kernel.org>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This should work, but how hard would it be to change the ls-extirq
> driver instead to not use the syscon driver at all but make the extirq
> driver set up the regmap itself?
>
> Are there any other users of the syscon?

Not that I can see, but that doesn't make the fact that it uses a syscon a bad decision.

For context, Layerscape devices have a "Misc" / "And Others" memory region
called "Supplemental Configuration Unit" (SCFG) which "provides the
chip-specific configuration and status registers for the device. It is the
chip-defined module for extending the device configuration unit (DCFG) module."
to quote the documentation.

The ls-extirq file is a driver around _a_single_register_ of SCFG. SCFG
provides an option of reversing the interrupt polarity of the external IRQ
pins: make them active-low instead of active-high, or rising instead of
falling.

The reason for the existence of the driver is that we got some pushback during
device tree submission: while we could describe in the device tree an interrupt
as "active-high" and going straight to the GIC, in reality that interrupt is
"active-low" but inverted by the SCFG (the inverted is enabled by default).
Additionally, the GIC cannot process active-low interrupts in the first place
AFAIR, which is why an inverter exists in front of it.

Some other SCFG registers are (at least on LS1021A):

Deep Sleep Control Register
eTSEC Clock Deep Sleep Control Register (eTSEC is our Ethernet controller)
Pixel Clock Control Register
PCIe PM Write Control Register
PCIe PM Read Control Register
USB3 parameter 1 control register
ETSEC MAC1 ICID
SATA ICID
QuadSPI configuration
Endianness Control Register
Snoop configuration
Interrupt Polarity <- this is the register controlled by ls-extirq
etc etc.

Also, even if you were to convince me that we shouldn't use a syscon, I feel
that the implication (change the device trees for 7 SoCs) just to solve a
kernel splat would be like hitting a nail with an atomic bomb. I'm not going to
do it.

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Arnd Bergmann]

On Thu, Aug 26, 2021 at 12:01 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Aug 25, 2021 at 11:24:52PM +0200, Arnd Bergmann wrote:
>
> > Are there any other users of the syscon?
>
> Not that I can see, but that doesn't make the fact that it uses a syscon a bad decision.
>
> For context, Layerscape devices have a "Misc" / "And Others" memory region
> called "Supplemental Configuration Unit" (SCFG) which "provides the
> chip-specific configuration and status registers for the device. It is the
> chip-defined module for extending the device configuration unit (DCFG) module."
> to quote the documentation.
>
> The ls-extirq file is a driver around _a_single_register_ of SCFG. SCFG
> provides an option of reversing the interrupt polarity of the external IRQ
> pins: make them active-low instead of active-high, or rising instead of
> falling.
>
> The reason for the existence of the driver is that we got some pushback during
> device tree submission: while we could describe in the device tree an interrupt
> as "active-high" and going straight to the GIC, in reality that interrupt is
> "active-low" but inverted by the SCFG (the inverted is enabled by default).
> Additionally, the GIC cannot process active-low interrupts in the first place
> AFAIR, which is why an inverter exists in front of it.
>
> Some other SCFG registers are (at least on LS1021A):
>
> Deep Sleep Control Register
> eTSEC Clock Deep Sleep Control Register (eTSEC is our Ethernet controller)
> Pixel Clock Control Register
> PCIe PM Write Control Register
> PCIe PM Read Control Register
> USB3 parameter 1 control register
> ETSEC MAC1 ICID
> SATA ICID
> QuadSPI configuration
> Endianness Control Register
> Snoop configuration
> Interrupt Polarity <- this is the register controlled by ls-extirq
> etc etc.
>
> Also, even if you were to convince me that we shouldn't use a syscon, I feel
> that the implication (change the device trees for 7 SoCs) just to solve a
> kernel splat would be like hitting a nail with an atomic bomb. I'm not going to
> do it.

I was not suggesting changing the DT files. The way we describe syscon
devices is generally meant to allow replacing them with a custom driver
as an implementation detail of the OS, you just have a driver that binds
against the more specific compatible string as opposed to the generic
compatible="syscon" check, and you replace all
syscon_regmap_lookup_by_phandle() calls with direct function calls
into exported symbols from that driver that perform high-level functions.

In this particular case, I think a high-level interface from a drviers/soc/
driver works just as well as the syscon method if there was raw_spinlock
requirement, but with the irqchip driver needing the regmap, the custom
driver would a better interface.

        Arnd

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Vladimir Oltean]

On Thu, Aug 26, 2021 at 11:24:24AM +0200, Arnd Bergmann wrote:
> On Thu, Aug 26, 2021 at 12:01 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Wed, Aug 25, 2021 at 11:24:52PM +0200, Arnd Bergmann wrote:
> >
> > > Are there any other users of the syscon?
> >
> > Not that I can see, but that doesn't make the fact that it uses a syscon a bad decision.
> >
> > For context, Layerscape devices have a "Misc" / "And Others" memory region
> > called "Supplemental Configuration Unit" (SCFG) which "provides the
> > chip-specific configuration and status registers for the device. It is the
> > chip-defined module for extending the device configuration unit (DCFG) module."
> > to quote the documentation.
> >
> > The ls-extirq file is a driver around _a_single_register_ of SCFG. SCFG
> > provides an option of reversing the interrupt polarity of the external IRQ
> > pins: make them active-low instead of active-high, or rising instead of
> > falling.
> >
> > The reason for the existence of the driver is that we got some pushback during
> > device tree submission: while we could describe in the device tree an interrupt
> > as "active-high" and going straight to the GIC, in reality that interrupt is
> > "active-low" but inverted by the SCFG (the inverted is enabled by default).
> > Additionally, the GIC cannot process active-low interrupts in the first place
> > AFAIR, which is why an inverter exists in front of it.
> >
> > Some other SCFG registers are (at least on LS1021A):
> >
> > Deep Sleep Control Register
> > eTSEC Clock Deep Sleep Control Register (eTSEC is our Ethernet controller)
> > Pixel Clock Control Register
> > PCIe PM Write Control Register
> > PCIe PM Read Control Register
> > USB3 parameter 1 control register
> > ETSEC MAC1 ICID
> > SATA ICID
> > QuadSPI configuration
> > Endianness Control Register
> > Snoop configuration
> > Interrupt Polarity <- this is the register controlled by ls-extirq
> > etc etc.
> >
> > Also, even if you were to convince me that we shouldn't use a syscon, I feel
> > that the implication (change the device trees for 7 SoCs) just to solve a
> > kernel splat would be like hitting a nail with an atomic bomb. I'm not going to
> > do it.
> 
> I was not suggesting changing the DT files. The way we describe syscon
> devices is generally meant to allow replacing them with a custom driver
> as an implementation detail of the OS, you just have a driver that binds
> against the more specific compatible string as opposed to the generic
> compatible="syscon" check, and you replace all
> syscon_regmap_lookup_by_phandle() calls with direct function calls
> into exported symbols from that driver that perform high-level functions.
> 
> In this particular case, I think a high-level interface from a drviers/soc/
> driver works just as well as the syscon method if there was raw_spinlock
> requirement, but with the irqchip driver needing the regmap, the custom
> driver would a better interface.

So basically you want me to create a platform driver under drivers/soc
which probes on:
	"fsl,lx2160a-isc"
	"fsl,ls2080a-isc"
	"fsl,ls2080a-isc"
	"fsl,ls1088a-isc"
	"fsl,ls1043a-scfg"
	"fsl,ls1046a-scfg"
	"fsl,ls1021a-scfg"
and does the same thing as syscon, but sets "syscon_config.use_raw_spinlock = true;"
and that is the only difference?

By the way, how does syscon probe its children exactly? And how would
this driver probe its children?

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Arnd Bergmann]

On Thu, Aug 26, 2021 at 3:57 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Thu, Aug 26, 2021 at 11:24:24AM +0200, Arnd Bergmann wrote:
>
> So basically you want me to create a platform driver under drivers/soc
> which probes on:
>         "fsl,lx2160a-isc"
>         "fsl,ls2080a-isc"
>         "fsl,ls2080a-isc"
>         "fsl,ls1088a-isc"
>         "fsl,ls1043a-scfg"
>         "fsl,ls1046a-scfg"
>         "fsl,ls1021a-scfg"
> and does the same thing as syscon, but sets "syscon_config.use_raw_spinlock = true;"
> and that is the only difference?

That would work, but it's not what I was suggesting.

> By the way, how does syscon probe its children exactly? And how would
> this driver probe its children?

syscon does not probe its children. As far as I can tell, your irqchip driver
works by accident because it uses IRQCHIP_DECLARE() rather than
builtin_platform_driver(), so it works even when no platform_device gets
created, as IRQCHIP_DECLARE() just looks for the compatible
string on its own.

My suggestion was to have the driver that binds to the isc node export a
high-level interface such as fsl_isc_set_extirq_polarity(). If the extirq
irqchip driver can work as a platform_driver(), then the new isc driver
can also be responsible for probing its children using the mfd
infrastructure, which solves the probe order problem, and lets you
have child devices that are not irqchip or clock controller.

       Arnd

Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

[Lee Jones]

On Wed, 25 Aug 2021, Vladimir Oltean wrote:

> This patch solves a ls-extirq irqchip driver bug in a perhaps
> non-intuitive (at least non-localized) way.
> 
> The issue is that ls-extirq uses regmap, and due to the fact that it is
> being called by the IRQ core under raw spinlock context, it needs to use
> raw spinlocks itself. So it needs to request raw spinlocks from the
> regmap config.
> 
> All is fine so far, except the ls-extirq driver does not manage its own
> regmap, instead it uses syscon_node_to_regmap() to get it from the
> parent syscon (this driver).
> 
> Because the syscon regmap is initialized before any of the consumer
> drivers (ls-extirq) probe, we need to know beforehand whether to request
> raw spinlocks or not.
> 
> The solution seems to be to check some compatible string. The ls-extirq
> driver probes on quite a few NXP Layerscape SoCs, all with different
> compatible strings. This is potentially fragile and subject to bit rot
> (since the fix is not localized to the ls-extirq driver, adding new
> compatible strings there but not here seems plausible). Anyway, it is
> probably the best we can do without major rework.
> 
> Suggested-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/mfd/syscon.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 765c0210cb52..70da4e87b072 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -83,6 +83,22 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  	if (ret)
>  		reg_io_width = 4;
>  
> +	/*
> +	 * We might be providing a regmap to e.g. an irqchip driver, and in
> +	 * that case, normal spinlocks won't do: the IRQ core holds raw
> +	 * spinlocks, so it needs to be raw spinlocks all the way down.
> +	 * Detect those drivers here (currently "ls-extirq") and request raw
> +	 * spinlocks in the regmap config for them.
> +	 */
> +	if (of_device_is_compatible(np, "fsl,lx2160a-isc") ||
> +	    of_device_is_compatible(np, "fsl,ls2080a-isc") ||
> +	    of_device_is_compatible(np, "fsl,ls2080a-isc") ||
> +	    of_device_is_compatible(np, "fsl,ls1088a-isc") ||
> +	    of_device_is_compatible(np, "fsl,ls1043a-scfg") ||
> +	    of_device_is_compatible(np, "fsl,ls1046a-scfg") ||
> +	    of_device_is_compatible(np, "fsl,ls1021a-scfg"))
> +		syscon_config.use_raw_spinlock = true;
> +

Since syscon is meant to be a generic solution, I'd like to avoid
spraying platform specific hacks throughout.  So, *IF* this is the
chosen solution, I'd prefer to solve this with a generic DT property,
rather than matching on a bunch of compatible strings.

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

Re: (subset) [PATCH 0/2] Use raw spinlocks in the ls-extirq driver

[Mark Brown]

On Wed, 25 Aug 2021 23:50:39 +0300, Vladimir Oltean wrote:
> The ls-extirq irqchip driver accesses regmap inside its implementation
> of the struct irq_chip :: irq_set_type method, and currently regmap
> only knows to lock using normal spinlocks. But the method above wants
> raw spinlock context, so this isn't going to work and triggers a
> "[ BUG: Invalid wait context ]" splat.
> 
> The best we can do given the arrangement of the code is to patch regmap
> and the syscon driver: regmap to support raw spinlocks, and syscon to
> request them on behalf of its ls-extirq consumer.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/2] regmap: teach regmap to use raw spinlocks if requested in the config
      commit: 67021f25d95292d285dd213c58401642b98eaf24

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark