Date:   Sun, 24 Jul 2022 11:31:17 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     linux-kernel@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>,
        Hou Zhiqiang <Zhiqiang.Hou@....com>,
        Biwen Li <biwen.li@....com>,
        Sean Anderson <sean.anderson@...o.com>
Subject: Re: [PATCH v2] irqchip/ls-extirq: use raw spinlocks for regmap to avoid invalid wait context

On Fri, 22 Jul 2022 21:40:19 +0100,
Vladimir Oltean <vladimir.oltean@....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@....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...

> +
> +	priv->regmap = regmap_init_mmio(NULL, base, &extirq_regmap_config);
> +	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.

	M.

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

Powered by blists - more mailing lists