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

From: Sean Anderson
Date: Thu Jul 28 2022 - 11:43:56 EST




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@xxxxxxxxxx/
>

On 7/24/22 6:31 AM, Marc Zyngier wrote:
> On Fri, 22 Jul 2022 21:40:19 +0100,
> Vladimir Oltean <vladimir.oltean@xxxxxxx> 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@xxxxxxx>
>> ---
>> 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@xxxxxxx/
>>
>> 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