The irqchip->irq_set_type method is called by __irq_set_trigger() under
the desc->lock raw spinlock.
The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
regmap created by of_syscon_register(), which uses plain spinlocks
(the kind that are sleepable on RT).
Therefore, this is an invalid locking scheme for which we get a kernel
splat stating just that ("[ BUG: Invalid wait context ]"), because the
context in which the plain spinlock may sleep is atomic due to the raw
spinlock. We need to go raw spinlocks all the way.
Make this driver ioremap its INTPCR register on its own, and stop
relying on syscon to provide a regmap. Since the regmap we got from
syscon belonged to the parent and the newly ioremapped region belongs
just to us, the offset to the INTPCR register is now 0, because of the
address translation that takes place through the device tree.
One complication, due to the fact that this driver uses IRQCHIP_DECLARE
rather than traditional platform devices with probe and remove methods,
is that we cannot use devres, so we need to implement a full-blown
cleanup procedure on the error path.
Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
Signed-off-by: Vladimir Oltean <[email protected]>
---
v3->v4:
- fix the dumb decision of dropping spinlocks altogether in the extirq
driver, and add a comment as to why they're there
- drop priv->read() and priv->write() and open-code the read-modify-write
inside a new ls_extirq_intpcr_rmw() helper.
v2->v3:
- stop using regmap, do the rmw manually using function pointers for BE/LE
- adapt comment style to the subsystem
- use of_device_is_big_endian
- reword commit message
v1->v2:
- create a separate regmap for the ls-extirq driver rather than relying
on the one provided by syscon or modifying that.
For reference, v1 is at:
https://lore.kernel.org/lkml/[email protected]/
and v2 is at:
https://lore.kernel.org/lkml/[email protected]/
and v3 is at:
https://lore.kernel.org/lkml/[email protected]/
For extra reviewer convenience, the ls-extirq appears in the following
SoC device trees:
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi#L289
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi#L249
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi#L319
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi#L325
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi#L682
https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm/boot/dts/ls1021a.dtsi#L182
Patch tested on LX2160A and LS1021A.
drivers/irqchip/irq-ls-extirq.c | 87 ++++++++++++++++++++++++---------
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
index 853b3972dbe7..d8d48b1f7c29 100644
--- a/drivers/irqchip/irq-ls-extirq.c
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -6,8 +6,7 @@
#include <linux/irqchip.h>
#include <linux/irqdomain.h>
#include <linux/of.h>
-#include <linux/mfd/syscon.h>
-#include <linux/regmap.h>
+#include <linux/of_address.h>
#include <linux/slab.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -16,13 +15,41 @@
#define LS1021A_SCFGREVCR 0x200
struct ls_extirq_data {
- struct regmap *syscon;
- u32 intpcr;
+ void __iomem *intpcr;
+ raw_spinlock_t lock;
+ bool big_endian;
bool is_ls1021a_or_ls1043a;
u32 nirq;
struct irq_fwspec map[MAXIRQ];
};
+static void ls_extirq_intpcr_rmw(struct ls_extirq_data *priv, u32 mask,
+ u32 value)
+{
+ u32 intpcr;
+
+ /*
+ * Serialize concurrent calls to ls_extirq_set_type() from multiple
+ * IRQ descriptors, making sure the read-modify-write is atomic.
+ */
+ raw_spin_lock(&priv->lock);
+
+ if (priv->big_endian)
+ intpcr = ioread32be(priv->intpcr);
+ else
+ intpcr = ioread32(priv->intpcr);
+
+ intpcr &= ~mask;
+ intpcr |= value;
+
+ if (priv->big_endian)
+ iowrite32be(intpcr, priv->intpcr);
+ else
+ iowrite32(intpcr, priv->intpcr);
+
+ raw_spin_unlock(&priv->lock);
+}
+
static int
ls_extirq_set_type(struct irq_data *data, unsigned int type)
{
@@ -51,7 +78,8 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
default:
return -EINVAL;
}
- regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
+
+ ls_extirq_intpcr_rmw(priv, mask, value);
return irq_chip_set_type_parent(data, type);
}
@@ -143,7 +171,6 @@ ls_extirq_parse_map(struct ls_extirq_data *priv, struct device_node *node)
static int __init
ls_extirq_of_init(struct device_node *node, struct device_node *parent)
{
-
struct irq_domain *domain, *parent_domain;
struct ls_extirq_data *priv;
int ret;
@@ -151,40 +178,52 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent)
parent_domain = irq_find_host(parent);
if (!parent_domain) {
pr_err("Cannot find parent domain\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_irq_find_host;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- priv->syscon = syscon_node_to_regmap(node->parent);
- if (IS_ERR(priv->syscon)) {
- ret = PTR_ERR(priv->syscon);
- pr_err("Failed to lookup parent regmap\n");
- goto out;
+ if (!priv) {
+ ret = -ENOMEM;
+ goto err_alloc_priv;
}
- ret = of_property_read_u32(node, "reg", &priv->intpcr);
- if (ret) {
- pr_err("Missing INTPCR offset value\n");
- goto out;
+
+ /*
+ * All extirq OF nodes are under a scfg/syscon node with
+ * the 'ranges' property
+ */
+ priv->intpcr = of_iomap(node, 0);
+ if (!priv->intpcr) {
+ pr_err("Cannot ioremap OF node %pOF\n", node);
+ ret = -ENOMEM;
+ goto err_iomap;
}
ret = ls_extirq_parse_map(priv, node);
if (ret)
- goto out;
+ goto err_parse_map;
+ priv->big_endian = of_device_is_big_endian(parent);
priv->is_ls1021a_or_ls1043a = of_device_is_compatible(node, "fsl,ls1021a-extirq") ||
of_device_is_compatible(node, "fsl,ls1043a-extirq");
+ raw_spin_lock_init(&priv->lock);
domain = irq_domain_add_hierarchy(parent_domain, 0, priv->nirq, node,
&extirq_domain_ops, priv);
- if (!domain)
+ if (!domain) {
ret = -ENOMEM;
+ goto err_add_hierarchy;
+ }
-out:
- if (ret)
- kfree(priv);
+ return 0;
+
+err_add_hierarchy:
+err_parse_map:
+ iounmap(priv->intpcr);
+err_iomap:
+ kfree(priv);
+err_alloc_priv:
+err_irq_find_host:
return ret;
}
--
2.34.1
On 7/28/22 10:42 AM, Vladimir Oltean wrote:
> The irqchip->irq_set_type method is called by __irq_set_trigger() under
> the desc->lock raw spinlock.
>
> The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> regmap created by of_syscon_register(), which uses plain spinlocks
> (the kind that are sleepable on RT).
>
> Therefore, this is an invalid locking scheme for which we get a kernel
> splat stating just that ("[ BUG: Invalid wait context ]"), because the
> context in which the plain spinlock may sleep is atomic due to the raw
> spinlock. We need to go raw spinlocks all the way.
Could we just use use_raw_spinlock in the regmap config?
--Sean
> Make this driver ioremap its INTPCR register on its own, and stop
> relying on syscon to provide a regmap. Since the regmap we got from
> syscon belonged to the parent and the newly ioremapped region belongs
> just to us, the offset to the INTPCR register is now 0, because of the
> address translation that takes place through the device tree.
>
> One complication, due to the fact that this driver uses IRQCHIP_DECLARE
> rather than traditional platform devices with probe and remove methods,
> is that we cannot use devres, so we need to implement a full-blown
> cleanup procedure on the error path.
>
> Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
On 7/28/22 11:28 AM, Vladimir Oltean wrote:
> On Thu, Jul 28, 2022 at 11:25:23AM -0400, Sean Anderson wrote:
>> Could we just use use_raw_spinlock in the regmap config?
>
> That was v2, essentially:
> https://lore.kernel.org/lkml/[email protected]/
>
On 7/24/22 6:31 AM, Marc Zyngier wrote:
> On Fri, 22 Jul 2022 21:40:19 +0100,
> Vladimir Oltean <[email protected]> wrote:
>>
>> The irqchip->irq_set_type method is called by __irq_set_trigger() under
>> the desc->lock raw spinlock.
>>
>> The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
>> regmap created by of_syscon_register(), which uses plain spinlocks
>> (the kind that are sleepable on RT).
>>
>> Therefore, this is an invalid locking scheme for which we get a kernel
>> splat stating just that ("[ BUG: Invalid wait context ]"), because the
>> context in which the plain spinlock may sleep is atomic due to the raw
>> spinlock. We need to go raw spinlocks all the way.
>>
>> Make this driver create its own MMIO regmap, with use_raw_spinlock=true,
>> and stop relying on syscon to provide it. Since the regmap we got from
>> syscon belonged to the parent and this one belongs to us, the offset to
>> the INTPCR register is now 0, because of the address translation that
>> takes place through the device tree.
>>
>> Another complication that we need to deal with is the fact that we need
>> to parse the 'little-endian'/'big-endian' specifiers that exist in
>> device trees for the parent ourselves now, since we no longer involve
>> syscon.
>>
>> And yet one final complication, due to the fact that this driver uses
>> IRQCHIP_DECLARE rather than traditional platform devices with probe and
>> remove methods, is that we cannot use devres, so we need to implement a
>> full-blown cleanup procedure on the error path.
>>
>> This patch depends on commit 67021f25d952 ("regmap: teach regmap to use
>> raw spinlocks if requested in the config").
>
> This information doesn't belong to the commit message (and please read
> the documentation about the use of "This patch").
>
>>
>> Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
>> Signed-off-by: Vladimir Oltean <[email protected]>
>> ---
>> v1->v2: create a separate regmap for the ls-extirq driver rather than
>> relying on the one provided by syscon or modifying that.
>>
>> For reference, v1 is at:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> For extra reviewer convenience, the ls-extirq appears in the following
>> SoC device trees:
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi#L289
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi#L249
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi#L319
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi#L325
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi#L682
>> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm/boot/dts/ls1021a.dtsi#L182
>>
>> Patch tested on LX2160A and LS1021A.
>>
>> drivers/irqchip/irq-ls-extirq.c | 81 +++++++++++++++++++++++----------
>> 1 file changed, 58 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
>> index 853b3972dbe7..94a22642b3f2 100644
>> --- a/drivers/irqchip/irq-ls-extirq.c
>> +++ b/drivers/irqchip/irq-ls-extirq.c
>> @@ -6,7 +6,7 @@
>> #include <linux/irqchip.h>
>> #include <linux/irqdomain.h>
>> #include <linux/of.h>
>> -#include <linux/mfd/syscon.h>
>> +#include <linux/of_address.h>
>> #include <linux/regmap.h>
>> #include <linux/slab.h>
>>
>> @@ -16,8 +16,7 @@
>> #define LS1021A_SCFGREVCR 0x200
>>
>> struct ls_extirq_data {
>> - struct regmap *syscon;
>> - u32 intpcr;
>> + struct regmap *regmap;
>> bool is_ls1021a_or_ls1043a;
>> u32 nirq;
>> struct irq_fwspec map[MAXIRQ];
>> @@ -51,7 +50,10 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
>> default:
>> return -EINVAL;
>> }
>> - regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
>> + /* INTPCR is the only register of our regmap,
>> + * therefore its offset is 0
>> + */
>
> For multi-line comments, please use the normal kernel comment style,
> not the one that is mandated for net.
>
>> + regmap_update_bits(priv->regmap, 0, mask, value);
>>
>> return irq_chip_set_type_parent(data, type);
>> }
>> @@ -143,48 +145,81 @@ ls_extirq_parse_map(struct ls_extirq_data *priv, struct device_node *node)
>> static int __init
>> ls_extirq_of_init(struct device_node *node, struct device_node *parent)
>> {
>> -
>> + struct regmap_config extirq_regmap_config = {
>> + .name = "intpcr",
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .use_raw_spinlock = true,
>> + };
>> struct irq_domain *domain, *parent_domain;
>> struct ls_extirq_data *priv;
>> + void __iomem *base;
>> int ret;
>>
>> parent_domain = irq_find_host(parent);
>> if (!parent_domain) {
>> pr_err("Cannot find parent domain\n");
>> - return -ENODEV;
>> + ret = -ENODEV;
>> + goto err_irq_find_host;
>> }
>>
>> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> - if (!priv)
>> - return -ENOMEM;
>> -
>> - priv->syscon = syscon_node_to_regmap(node->parent);
>> - if (IS_ERR(priv->syscon)) {
>> - ret = PTR_ERR(priv->syscon);
>> - pr_err("Failed to lookup parent regmap\n");
>> - goto out;
>> + if (!priv) {
>> + ret = -ENOMEM;
>> + goto err_alloc_priv;
>> + }
>> +
>> + /* All extirq OF nodes are under a scfg/syscon node with
>> + * the 'ranges' property
>> + */
>> + base = of_iomap(node, 0);
>> + if (!base) {
>> + pr_err("Cannot ioremap OF node %pOF\n", node);
>> + ret = -ENOMEM;
>> + goto err_iomap;
>> }
>> - ret = of_property_read_u32(node, "reg", &priv->intpcr);
>> - if (ret) {
>> - pr_err("Missing INTPCR offset value\n");
>> - goto out;
>> +
>> + /* Parse the parent device's DT node for an endianness specification */
>> + if (of_property_read_bool(parent, "big-endian"))
>> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
>> + else if (of_property_read_bool(parent, "little-endian"))
>> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>> + else if (of_property_read_bool(parent, "native-endian"))
>> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_NATIVE;
>
> All of this should be rewritten to use of_device_is_big_endian(), and
> reduce the whole thing to two cases (I don't think native endian makes
> much sense anyway). I also wonder what the result is if none of these
> properties is present...
I think regmap_get_val_endian would be better here.
>> +
>> + priv->regmap = regmap_init_mmio(NULL, base, &extirq_regmap_config);
It could also be done automatically if we pass the syscon dev instead of
NULL. The only downside is that some regmap error messages will use the
syscon device
>> + if (IS_ERR(priv->regmap)) {
>> + pr_err("Cannot create MMIO regmap: %pe\n", priv->regmap);
>> + ret = PTR_ERR(priv->regmap);
>> + goto err_regmap_init;
>
> Finally, what is the actual benefit of using a regmap here? It seems
> like a very roundabout way of performing a RMW on a register whilst
> holding a lock... Passing NULL for a device to regmap_init_mmio() also
> seems to be an extremely rare idiom (only 5 cases in the tree), and
> this doesn't seem completely right to me.
The benefit is that you don't have to write (yet another) set of
endian-converting read/write functions. The above (non-NULL) usage of
regmap_init would also address your criticism here.
--Sean
On Thu, Jul 28, 2022 at 11:25:23AM -0400, Sean Anderson wrote:
> Could we just use use_raw_spinlock in the regmap config?
That was v2, essentially:
https://lore.kernel.org/lkml/[email protected]/
On Thu, Jul 28, 2022 at 11:43:40AM -0400, Sean Anderson wrote:
> > All of this should be rewritten to use of_device_is_big_endian(), and
> > reduce the whole thing to two cases (I don't think native endian makes
> > much sense anyway). I also wonder what the result is if none of these
> > properties is present...
>
> I think regmap_get_val_endian would be better here.
It needs a struct device.
> >> +
> >> + priv->regmap = regmap_init_mmio(NULL, base, &extirq_regmap_config);
>
> It could also be done automatically if we pass the syscon dev instead of
> NULL. The only downside is that some regmap error messages will use the
> syscon device
How do you get the struct device of the syscon?
> > Finally, what is the actual benefit of using a regmap here? It seems
> > like a very roundabout way of performing a RMW on a register whilst
> > holding a lock... Passing NULL for a device to regmap_init_mmio() also
> > seems to be an extremely rare idiom (only 5 cases in the tree), and
> > this doesn't seem completely right to me.
>
> The benefit is that you don't have to write (yet another) set of
> endian-converting read/write functions. The above (non-NULL) usage of
> regmap_init would also address your criticism here.
I don't have a particular attraction towards using regmap for a single
register either, to be honest.
On 7/28/22 12:19 PM, Vladimir Oltean wrote:
> On Thu, Jul 28, 2022 at 11:43:40AM -0400, Sean Anderson wrote:
>> > All of this should be rewritten to use of_device_is_big_endian(), and
>> > reduce the whole thing to two cases (I don't think native endian makes
>> > much sense anyway). I also wonder what the result is if none of these
>> > properties is present...
>>
>> I think regmap_get_val_endian would be better here.
>
> It needs a struct device.
>
>> >> +
>> >> + priv->regmap = regmap_init_mmio(NULL, base, &extirq_regmap_config);
>>
>> It could also be done automatically if we pass the syscon dev instead of
>> NULL. The only downside is that some regmap error messages will use the
>> syscon device
>
> How do you get the struct device of the syscon?
Oh, interesting, we don't have a device in this driver.
>> > Finally, what is the actual benefit of using a regmap here? It seems
>> > like a very roundabout way of performing a RMW on a register whilst
>> > holding a lock... Passing NULL for a device to regmap_init_mmio() also
>> > seems to be an extremely rare idiom (only 5 cases in the tree), and
>> > this doesn't seem completely right to me.
>>
>> The benefit is that you don't have to write (yet another) set of
>> endian-converting read/write functions. The above (non-NULL) usage of
>> regmap_init would also address your criticism here.
>
> I don't have a particular attraction towards using regmap for a single
> register either, to be honest.
>
Yeah I suppose it's not a terrible burden here.
--Sean
Sean
Please don't mix the threads. This makes it impossible to follow.
On Thu, 28 Jul 2022 16:43:40 +0100,
Sean Anderson <[email protected]> wrote:
>
>
>
> On 7/28/22 11:28 AM, Vladimir Oltean wrote:
> > On Thu, Jul 28, 2022 at 11:25:23AM -0400, Sean Anderson wrote:
> >> Could we just use use_raw_spinlock in the regmap config?
> >
> > That was v2, essentially:
> > https://lore.kernel.org/lkml/[email protected]/
> >
>
> On 7/24/22 6:31 AM, Marc Zyngier wrote:
> > On Fri, 22 Jul 2022 21:40:19 +0100,
> > Vladimir Oltean <[email protected]> wrote:
> >>
> >> The irqchip->irq_set_type method is called by __irq_set_trigger() under
> >> the desc->lock raw spinlock.
> >>
> >> The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> >> regmap created by of_syscon_register(), which uses plain spinlocks
> >> (the kind that are sleepable on RT).
> >>
> >> Therefore, this is an invalid locking scheme for which we get a kernel
> >> splat stating just that ("[ BUG: Invalid wait context ]"), because the
> >> context in which the plain spinlock may sleep is atomic due to the raw
> >> spinlock. We need to go raw spinlocks all the way.
> >>
> >> Make this driver create its own MMIO regmap, with use_raw_spinlock=true,
> >> and stop relying on syscon to provide it. Since the regmap we got from
> >> syscon belonged to the parent and this one belongs to us, the offset to
> >> the INTPCR register is now 0, because of the address translation that
> >> takes place through the device tree.
> >>
> >> Another complication that we need to deal with is the fact that we need
> >> to parse the 'little-endian'/'big-endian' specifiers that exist in
> >> device trees for the parent ourselves now, since we no longer involve
> >> syscon.
> >>
> >> And yet one final complication, due to the fact that this driver uses
> >> IRQCHIP_DECLARE rather than traditional platform devices with probe and
> >> remove methods, is that we cannot use devres, so we need to implement a
> >> full-blown cleanup procedure on the error path.
> >>
> >> This patch depends on commit 67021f25d952 ("regmap: teach regmap to use
> >> raw spinlocks if requested in the config").
> >
> > This information doesn't belong to the commit message (and please read
> > the documentation about the use of "This patch").
> >
> >>
> >> Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> >> Signed-off-by: Vladimir Oltean <[email protected]>
> >> ---
> >> v1->v2: create a separate regmap for the ls-extirq driver rather than
> >> relying on the one provided by syscon or modifying that.
> >>
> >> For reference, v1 is at:
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >> For extra reviewer convenience, the ls-extirq appears in the following
> >> SoC device trees:
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi#L289
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi#L249
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi#L319
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi#L325
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi#L682
> >> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm/boot/dts/ls1021a.dtsi#L182
> >>
> >> Patch tested on LX2160A and LS1021A.
> >>
> >> drivers/irqchip/irq-ls-extirq.c | 81 +++++++++++++++++++++++----------
> >> 1 file changed, 58 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
> >> index 853b3972dbe7..94a22642b3f2 100644
> >> --- a/drivers/irqchip/irq-ls-extirq.c
> >> +++ b/drivers/irqchip/irq-ls-extirq.c
> >> @@ -6,7 +6,7 @@
> >> #include <linux/irqchip.h>
> >> #include <linux/irqdomain.h>
> >> #include <linux/of.h>
> >> -#include <linux/mfd/syscon.h>
> >> +#include <linux/of_address.h>
> >> #include <linux/regmap.h>
> >> #include <linux/slab.h>
> >>
> >> @@ -16,8 +16,7 @@
> >> #define LS1021A_SCFGREVCR 0x200
> >>
> >> struct ls_extirq_data {
> >> - struct regmap *syscon;
> >> - u32 intpcr;
> >> + struct regmap *regmap;
> >> bool is_ls1021a_or_ls1043a;
> >> u32 nirq;
> >> struct irq_fwspec map[MAXIRQ];
> >> @@ -51,7 +50,10 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
> >> default:
> >> return -EINVAL;
> >> }
> >> - regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
> >> + /* INTPCR is the only register of our regmap,
> >> + * therefore its offset is 0
> >> + */
> >
> > For multi-line comments, please use the normal kernel comment style,
> > not the one that is mandated for net.
> >
> >> + regmap_update_bits(priv->regmap, 0, mask, value);
> >>
> >> return irq_chip_set_type_parent(data, type);
> >> }
> >> @@ -143,48 +145,81 @@ ls_extirq_parse_map(struct ls_extirq_data *priv, struct device_node *node)
> >> static int __init
> >> ls_extirq_of_init(struct device_node *node, struct device_node *parent)
> >> {
> >> -
> >> + struct regmap_config extirq_regmap_config = {
> >> + .name = "intpcr",
> >> + .reg_bits = 32,
> >> + .val_bits = 32,
> >> + .reg_stride = 4,
> >> + .use_raw_spinlock = true,
> >> + };
> >> struct irq_domain *domain, *parent_domain;
> >> struct ls_extirq_data *priv;
> >> + void __iomem *base;
> >> int ret;
> >>
> >> parent_domain = irq_find_host(parent);
> >> if (!parent_domain) {
> >> pr_err("Cannot find parent domain\n");
> >> - return -ENODEV;
> >> + ret = -ENODEV;
> >> + goto err_irq_find_host;
> >> }
> >>
> >> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >> - if (!priv)
> >> - return -ENOMEM;
> >> -
> >> - priv->syscon = syscon_node_to_regmap(node->parent);
> >> - if (IS_ERR(priv->syscon)) {
> >> - ret = PTR_ERR(priv->syscon);
> >> - pr_err("Failed to lookup parent regmap\n");
> >> - goto out;
> >> + if (!priv) {
> >> + ret = -ENOMEM;
> >> + goto err_alloc_priv;
> >> + }
> >> +
> >> + /* All extirq OF nodes are under a scfg/syscon node with
> >> + * the 'ranges' property
> >> + */
> >> + base = of_iomap(node, 0);
> >> + if (!base) {
> >> + pr_err("Cannot ioremap OF node %pOF\n", node);
> >> + ret = -ENOMEM;
> >> + goto err_iomap;
> >> }
> >> - ret = of_property_read_u32(node, "reg", &priv->intpcr);
> >> - if (ret) {
> >> - pr_err("Missing INTPCR offset value\n");
> >> - goto out;
> >> +
> >> + /* Parse the parent device's DT node for an endianness specification */
> >> + if (of_property_read_bool(parent, "big-endian"))
> >> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
> >> + else if (of_property_read_bool(parent, "little-endian"))
> >> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >> + else if (of_property_read_bool(parent, "native-endian"))
> >> + extirq_regmap_config.val_format_endian = REGMAP_ENDIAN_NATIVE;
> >
> > All of this should be rewritten to use of_device_is_big_endian(), and
> > reduce the whole thing to two cases (I don't think native endian makes
> > much sense anyway). I also wonder what the result is if none of these
> > properties is present...
>
> I think regmap_get_val_endian would be better here.
Why?
>
> >> +
> >> + priv->regmap = regmap_init_mmio(NULL, base, &extirq_regmap_config);
>
> It could also be done automatically if we pass the syscon dev instead of
> NULL. The only downside is that some regmap error messages will use the
> syscon device
>
> >> + if (IS_ERR(priv->regmap)) {
> >> + pr_err("Cannot create MMIO regmap: %pe\n", priv->regmap);
> >> + ret = PTR_ERR(priv->regmap);
> >> + goto err_regmap_init;
> >
> > Finally, what is the actual benefit of using a regmap here? It seems
> > like a very roundabout way of performing a RMW on a register whilst
> > holding a lock... Passing NULL for a device to regmap_init_mmio() also
> > seems to be an extremely rare idiom (only 5 cases in the tree), and
> > this doesn't seem completely right to me.
>
> The benefit is that you don't have to write (yet another) set of
> endian-converting read/write functions. The above (non-NULL) usage of
> regmap_init would also address your criticism here.
I really don't see the point of adding yet another abstraction layer
for something can be efficiently be managed locally.
regmap is good at managing things that are repeatedly updated, and
keep the most up to date version. This isn't the case here. This
register is read/written *once* per interrupt being configured. There
is no performance requirement.
So no, I don't think we need this at all.
M.
--
Without deviation from the norm, progress is not possible.
Hi,
On Thu, Jul 28, 2022 at 05:42:54PM +0300, Vladimir Oltean wrote:
> The irqchip->irq_set_type method is called by __irq_set_trigger() under
> the desc->lock raw spinlock.
>
> The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> regmap created by of_syscon_register(), which uses plain spinlocks
> (the kind that are sleepable on RT).
>
> Therefore, this is an invalid locking scheme for which we get a kernel
> splat stating just that ("[ BUG: Invalid wait context ]"), because the
> context in which the plain spinlock may sleep is atomic due to the raw
> spinlock. We need to go raw spinlocks all the way.
>
> Make this driver ioremap its INTPCR register on its own, and stop
> relying on syscon to provide a regmap. Since the regmap we got from
> syscon belonged to the parent and the newly ioremapped region belongs
> just to us, the offset to the INTPCR register is now 0, because of the
> address translation that takes place through the device tree.
>
> One complication, due to the fact that this driver uses IRQCHIP_DECLARE
> rather than traditional platform devices with probe and remove methods,
> is that we cannot use devres, so we need to implement a full-blown
> cleanup procedure on the error path.
>
> Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
Just checking in on this patch to make sure it hasn't been forgotten.
Hi Marc, Arnd,
On Thu, Aug 18, 2022 at 05:13:09PM +0300, Vladimir Oltean wrote:
> Hi,
>
> On Thu, Jul 28, 2022 at 05:42:54PM +0300, Vladimir Oltean wrote:
> > The irqchip->irq_set_type method is called by __irq_set_trigger() under
> > the desc->lock raw spinlock.
> >
> > The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> > regmap created by of_syscon_register(), which uses plain spinlocks
> > (the kind that are sleepable on RT).
> >
> > Therefore, this is an invalid locking scheme for which we get a kernel
> > splat stating just that ("[ BUG: Invalid wait context ]"), because the
> > context in which the plain spinlock may sleep is atomic due to the raw
> > spinlock. We need to go raw spinlocks all the way.
> >
> > Make this driver ioremap its INTPCR register on its own, and stop
> > relying on syscon to provide a regmap. Since the regmap we got from
> > syscon belonged to the parent and the newly ioremapped region belongs
> > just to us, the offset to the INTPCR register is now 0, because of the
> > address translation that takes place through the device tree.
> >
> > One complication, due to the fact that this driver uses IRQCHIP_DECLARE
> > rather than traditional platform devices with probe and remove methods,
> > is that we cannot use devres, so we need to implement a full-blown
> > cleanup procedure on the error path.
> >
> > Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> > Signed-off-by: Vladimir Oltean <[email protected]>
> > ---
>
> Just checking in on this patch to make sure it hasn't been forgotten.
Is there something else I need to do such that this patch gets accepted?
Thanks,
Vladimir
On Mon, 03 Oct 2022 10:45:43 +0100,
Vladimir Oltean <[email protected]> wrote:
>
> Hi Marc, Arnd,
>
> On Thu, Aug 18, 2022 at 05:13:09PM +0300, Vladimir Oltean wrote:
> > Hi,
> >
> > On Thu, Jul 28, 2022 at 05:42:54PM +0300, Vladimir Oltean wrote:
> > > The irqchip->irq_set_type method is called by __irq_set_trigger() under
> > > the desc->lock raw spinlock.
> > >
> > > The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> > > regmap created by of_syscon_register(), which uses plain spinlocks
> > > (the kind that are sleepable on RT).
> > >
> > > Therefore, this is an invalid locking scheme for which we get a kernel
> > > splat stating just that ("[ BUG: Invalid wait context ]"), because the
> > > context in which the plain spinlock may sleep is atomic due to the raw
> > > spinlock. We need to go raw spinlocks all the way.
> > >
> > > Make this driver ioremap its INTPCR register on its own, and stop
> > > relying on syscon to provide a regmap. Since the regmap we got from
> > > syscon belonged to the parent and the newly ioremapped region belongs
> > > just to us, the offset to the INTPCR register is now 0, because of the
> > > address translation that takes place through the device tree.
> > >
> > > One complication, due to the fact that this driver uses IRQCHIP_DECLARE
> > > rather than traditional platform devices with probe and remove methods,
> > > is that we cannot use devres, so we need to implement a full-blown
> > > cleanup procedure on the error path.
> > >
> > > Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> > > Signed-off-by: Vladimir Oltean <[email protected]>
> > > ---
> >
> > Just checking in on this patch to make sure it hasn't been forgotten.
>
> Is there something else I need to do such that this patch gets accepted?
No, it just went under the radar. I would have liked an ack from
Rasmus, but this has been there for long enough.
I'll queue this as a fix for 6.1.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 1b00adce8afdb842615a5bf3774510f14a9b769a
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/1b00adce8afdb842615a5bf3774510f14a9b769a
Author: Vladimir Oltean <[email protected]>
AuthorDate: Thu, 28 Jul 2022 17:42:54 +03:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Mon, 03 Oct 2022 16:29:17 +01:00
irqchip/ls-extirq: Fix invalid wait context by avoiding to use regmap
The irqchip->irq_set_type method is called by __irq_set_trigger() under
the desc->lock raw spinlock.
The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
regmap created by of_syscon_register(), which uses plain spinlocks
(the kind that are sleepable on RT).
Therefore, this is an invalid locking scheme for which we get a kernel
splat stating just that ("[ BUG: Invalid wait context ]"), because the
context in which the plain spinlock may sleep is atomic due to the raw
spinlock. We need to go raw spinlocks all the way.
Make this driver ioremap its INTPCR register on its own, and stop
relying on syscon to provide a regmap.
Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
Signed-off-by: Vladimir Oltean <[email protected]>
[maz: trimmed down commit log]
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-ls-extirq.c | 87 +++++++++++++++++++++++---------
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
index 853b397..d8d48b1 100644
--- a/drivers/irqchip/irq-ls-extirq.c
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -6,8 +6,7 @@
#include <linux/irqchip.h>
#include <linux/irqdomain.h>
#include <linux/of.h>
-#include <linux/mfd/syscon.h>
-#include <linux/regmap.h>
+#include <linux/of_address.h>
#include <linux/slab.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -16,13 +15,41 @@
#define LS1021A_SCFGREVCR 0x200
struct ls_extirq_data {
- struct regmap *syscon;
- u32 intpcr;
+ void __iomem *intpcr;
+ raw_spinlock_t lock;
+ bool big_endian;
bool is_ls1021a_or_ls1043a;
u32 nirq;
struct irq_fwspec map[MAXIRQ];
};
+static void ls_extirq_intpcr_rmw(struct ls_extirq_data *priv, u32 mask,
+ u32 value)
+{
+ u32 intpcr;
+
+ /*
+ * Serialize concurrent calls to ls_extirq_set_type() from multiple
+ * IRQ descriptors, making sure the read-modify-write is atomic.
+ */
+ raw_spin_lock(&priv->lock);
+
+ if (priv->big_endian)
+ intpcr = ioread32be(priv->intpcr);
+ else
+ intpcr = ioread32(priv->intpcr);
+
+ intpcr &= ~mask;
+ intpcr |= value;
+
+ if (priv->big_endian)
+ iowrite32be(intpcr, priv->intpcr);
+ else
+ iowrite32(intpcr, priv->intpcr);
+
+ raw_spin_unlock(&priv->lock);
+}
+
static int
ls_extirq_set_type(struct irq_data *data, unsigned int type)
{
@@ -51,7 +78,8 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
default:
return -EINVAL;
}
- regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
+
+ ls_extirq_intpcr_rmw(priv, mask, value);
return irq_chip_set_type_parent(data, type);
}
@@ -143,7 +171,6 @@ ls_extirq_parse_map(struct ls_extirq_data *priv, struct device_node *node)
static int __init
ls_extirq_of_init(struct device_node *node, struct device_node *parent)
{
-
struct irq_domain *domain, *parent_domain;
struct ls_extirq_data *priv;
int ret;
@@ -151,40 +178,52 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent)
parent_domain = irq_find_host(parent);
if (!parent_domain) {
pr_err("Cannot find parent domain\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_irq_find_host;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- priv->syscon = syscon_node_to_regmap(node->parent);
- if (IS_ERR(priv->syscon)) {
- ret = PTR_ERR(priv->syscon);
- pr_err("Failed to lookup parent regmap\n");
- goto out;
+ if (!priv) {
+ ret = -ENOMEM;
+ goto err_alloc_priv;
}
- ret = of_property_read_u32(node, "reg", &priv->intpcr);
- if (ret) {
- pr_err("Missing INTPCR offset value\n");
- goto out;
+
+ /*
+ * All extirq OF nodes are under a scfg/syscon node with
+ * the 'ranges' property
+ */
+ priv->intpcr = of_iomap(node, 0);
+ if (!priv->intpcr) {
+ pr_err("Cannot ioremap OF node %pOF\n", node);
+ ret = -ENOMEM;
+ goto err_iomap;
}
ret = ls_extirq_parse_map(priv, node);
if (ret)
- goto out;
+ goto err_parse_map;
+ priv->big_endian = of_device_is_big_endian(parent);
priv->is_ls1021a_or_ls1043a = of_device_is_compatible(node, "fsl,ls1021a-extirq") ||
of_device_is_compatible(node, "fsl,ls1043a-extirq");
+ raw_spin_lock_init(&priv->lock);
domain = irq_domain_add_hierarchy(parent_domain, 0, priv->nirq, node,
&extirq_domain_ops, priv);
- if (!domain)
+ if (!domain) {
ret = -ENOMEM;
+ goto err_add_hierarchy;
+ }
-out:
- if (ret)
- kfree(priv);
+ return 0;
+
+err_add_hierarchy:
+err_parse_map:
+ iounmap(priv->intpcr);
+err_iomap:
+ kfree(priv);
+err_alloc_priv:
+err_irq_find_host:
return ret;
}
On 03/10/2022 17.28, Marc Zyngier wrote:
> On Mon, 03 Oct 2022 10:45:43 +0100,
> Vladimir Oltean <[email protected]> wrote:
>>
>>> Just checking in on this patch to make sure it hasn't been forgotten.
>>
>> Is there something else I need to do such that this patch gets accepted?
>
> No, it just went under the radar. I would have liked an ack from
> Rasmus, but this has been there for long enough.
>
> I'll queue this as a fix for 6.1.
Sorry, I've not been working on that ls1021a board for a long time, so
while this was on my radar, I haven't really looked at it until just
now. I wonder why we haven't seen that splat; we do use the -rt patches.
Anyway, it looks quite sane. FWIW and if you haven't queued it up already
Acked-by: Rasmus Villemoes <[email protected]>
Rasmus