Re: [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap

[Marc Zyngier <maz@kernel.org>]

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.