Messages in this thread | | | Subject | Re: [PATCH v4] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap | From | Sean Anderson <> | Date | Thu, 28 Jul 2022 11:43:40 -0400 |
| |
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/874jz6dcp6.wl-maz@kernel.org/ >
On 7/24/22 6:31 AM, Marc Zyngier wrote: > On Fri, 22 Jul 2022 21:40:19 +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. >> >> 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 <vladimir.oltean@nxp.com> >> --- >> 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/ >> >> 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
| |