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
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 <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
---
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
On Wed, Aug 25, 2021 at 10:50 PM Vladimir Oltean
<[email protected]> 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 <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
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
On Wed, Aug 25, 2021 at 11:24:52PM +0200, Arnd Bergmann wrote:
> On Wed, Aug 25, 2021 at 10:50 PM Vladimir Oltean
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Vladimir Oltean <[email protected]>
>
> 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.
On Thu, Aug 26, 2021 at 12:01 AM Vladimir Oltean <[email protected]> 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
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
On Thu, Aug 26, 2021 at 11:24:24AM +0200, Arnd Bergmann wrote:
> On Thu, Aug 26, 2021 at 12:01 AM Vladimir Oltean <[email protected]> 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?
On Thu, Aug 26, 2021 at 3:57 PM Vladimir Oltean <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> 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