All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap
@ 2022-07-27 16:09 Vladimir Oltean
  2022-07-28  7:44 ` Marc Zyngier
  0 siblings, 1 reply; 3+ messages in thread
From: Vladimir Oltean @ 2022-07-27 16:09 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Thomas Gleixner, Marc Zyngier, Rasmus Villemoes,
	Arnd Bergmann, Hou Zhiqiang, Biwen Li, Sean Anderson

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 <vladimir.oltean@nxp.com>
---
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/20210825205041.927788-3-vladimir.oltean@nxp.com/
and v2 is at:
https://lore.kernel.org/lkml/20220722204019.969272-1-vladimir.oltean@nxp.com/

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 | 91 ++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 25 deletions(-)

diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
index 853b3972dbe7..cfbbe5959c8e 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,19 +15,40 @@
 #define LS1021A_SCFGREVCR 0x200
 
 struct ls_extirq_data {
-	struct regmap		*syscon;
-	u32			intpcr;
+	void __iomem		*intpcr;
+	u32			(*read)(void __iomem *addr);
+	void			(*write)(void __iomem *addr, u32 val);
 	bool			is_ls1021a_or_ls1043a;
 	u32			nirq;
 	struct irq_fwspec	map[MAXIRQ];
 };
 
+static u32 ls_extirq_read_be(void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static u32 ls_extirq_read(void __iomem *addr)
+{
+	return ioread32(addr);
+}
+
+static void ls_extirq_write_be(void __iomem *addr, u32 val)
+{
+	iowrite32be(val, addr);
+}
+
+static void ls_extirq_write(void __iomem *addr, u32 val)
+{
+	iowrite32(val, addr);
+}
+
 static int
 ls_extirq_set_type(struct irq_data *data, unsigned int type)
 {
 	struct ls_extirq_data *priv = data->chip_data;
 	irq_hw_number_t hwirq = data->hwirq;
-	u32 value, mask;
+	u32 intpcr, value, mask;
 
 	if (priv->is_ls1021a_or_ls1043a)
 		mask = 1U << (31 - hwirq);
@@ -51,7 +71,11 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
 	default:
 		return -EINVAL;
 	}
-	regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
+
+	intpcr = priv->read(priv->intpcr);
+	intpcr &= ~mask;
+	intpcr |= value;
+	priv->write(priv->intpcr, intpcr);
 
 	return irq_chip_set_type_parent(data, type);
 }
@@ -143,7 +167,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 +174,58 @@ 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;
+	}
+
+	if (of_device_is_big_endian(parent)) {
+		priv->read = ls_extirq_read_be;
+		priv->write = ls_extirq_write_be;
+	} else {
+		priv->read = ls_extirq_read;
+		priv->write = ls_extirq_write;
 	}
 
 	ret = ls_extirq_parse_map(priv, node);
 	if (ret)
-		goto out;
+		goto err_parse_map;
 
 	priv->is_ls1021a_or_ls1043a = of_device_is_compatible(node, "fsl,ls1021a-extirq") ||
 				      of_device_is_compatible(node, "fsl,ls1043a-extirq");
 
 	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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap
  2022-07-27 16:09 [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap Vladimir Oltean
@ 2022-07-28  7:44 ` Marc Zyngier
  2022-07-28 13:30   ` Vladimir Oltean
  0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2022-07-28  7:44 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: linux-kernel, Lee Jones, Thomas Gleixner, Rasmus Villemoes,
	Arnd Bergmann, Hou Zhiqiang, Biwen Li, Sean Anderson

On Wed, 27 Jul 2022 17:09:15 +0100,
Vladimir Oltean <vladimir.oltean@nxp.com> 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.

Interesting you say that...

> 
> 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 <vladimir.oltean@nxp.com>
> ---
> 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/20210825205041.927788-3-vladimir.oltean@nxp.com/
> and v2 is at:
> https://lore.kernel.org/lkml/20220722204019.969272-1-vladimir.oltean@nxp.com/
> 
> 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 | 91 ++++++++++++++++++++++++---------
>  1 file changed, 66 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
> index 853b3972dbe7..cfbbe5959c8e 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,19 +15,40 @@
>  #define LS1021A_SCFGREVCR 0x200
>  
>  struct ls_extirq_data {
> -	struct regmap		*syscon;
> -	u32			intpcr;
> +	void __iomem		*intpcr;
> +	u32			(*read)(void __iomem *addr);
> +	void			(*write)(void __iomem *addr, u32 val);
>  	bool			is_ls1021a_or_ls1043a;
>  	u32			nirq;
>  	struct irq_fwspec	map[MAXIRQ];
>  };
>  
> +static u32 ls_extirq_read_be(void __iomem *addr)
> +{
> +	return ioread32be(addr);
> +}
> +
> +static u32 ls_extirq_read(void __iomem *addr)
> +{
> +	return ioread32(addr);
> +}
> +
> +static void ls_extirq_write_be(void __iomem *addr, u32 val)
> +{
> +	iowrite32be(val, addr);
> +}
> +
> +static void ls_extirq_write(void __iomem *addr, u32 val)
> +{
> +	iowrite32(val, addr);
> +}
> +
>  static int
>  ls_extirq_set_type(struct irq_data *data, unsigned int type)
>  {
>  	struct ls_extirq_data *priv = data->chip_data;
>  	irq_hw_number_t hwirq = data->hwirq;
> -	u32 value, mask;
> +	u32 intpcr, value, mask;
>  
>  	if (priv->is_ls1021a_or_ls1043a)
>  		mask = 1U << (31 - hwirq);
> @@ -51,7 +71,11 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
>  	default:
>  		return -EINVAL;
>  	}
> -	regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
> +
> +	intpcr = priv->read(priv->intpcr);
> +	intpcr &= ~mask;
> +	intpcr |= value;
> +	priv->write(priv->intpcr, intpcr);

Which really begs a few questions:

- Where is the locking gone? Sure, the warning is gone. But the driver
  is now broken for *all* configurations, and not only RT. Result!

- Is it *really* worth it to have 4 dumb helpers that bring nothing in
  terms of abstraction, and two indirections for something that could
  equally be expressed with a conditional?

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap
  2022-07-28  7:44 ` Marc Zyngier
@ 2022-07-28 13:30   ` Vladimir Oltean
  0 siblings, 0 replies; 3+ messages in thread
From: Vladimir Oltean @ 2022-07-28 13:30 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-kernel, Lee Jones, Thomas Gleixner, Rasmus Villemoes,
	Arnd Bergmann, Z.Q. Hou, Biwen Li, Sean Anderson

Hello Marc,

On Thu, Jul 28, 2022 at 08:44:58AM +0100, Marc Zyngier wrote:
> On Wed, 27 Jul 2022 17:09:15 +0100,
> Vladimir Oltean <vladimir.oltean@nxp.com> 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.
> 
> Interesting you say that...
> > -	regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
> > +
> > +	intpcr = priv->read(priv->intpcr);
> > +	intpcr &= ~mask;
> > +	intpcr |= value;
> > +	priv->write(priv->intpcr, intpcr);
> 
> Which really begs a few questions:
> 
> - Where is the locking gone? Sure, the warning is gone. But the driver
>   is now broken for *all* configurations, and not only RT. Result!

Correct, I had assumed that calls to irq_chip :: irq_set_type() are
implicitly serialized with respect to each other by means of the irq
descriptor's desc->lock, but clearly that is only true for a single
interrupt line. So I'll add back a lock that keeps the rmw atomic.

> - Is it *really* worth it to have 4 dumb helpers that bring nothing in
>   terms of abstraction, and two indirections for something that could
>   equally be expressed with a conditional?

Probably not, but it was a choice no worse than going through regmap's
own indirection. I'll try to come up with something that avoids function
pointers.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-07-28 13:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 16:09 [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap Vladimir Oltean
2022-07-28  7:44 ` Marc Zyngier
2022-07-28 13:30   ` Vladimir Oltean

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.