Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 28 Jul 2022 08:44:58 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap |
| |
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.
| ![\](/images/icornerr.gif) |