Message-ID: <532c8141-2a8b-6842-c9a2-cc4d17afd73d@seco.com>
Date:   Thu, 28 Jul 2022 11:43:40 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>,
        "Z.Q. Hou" <zhiqiang.hou@....com>, Biwen Li <biwen.li@....com>
Subject: Re: [PATCH v4] irqchip/ls-extirq: fix invalid wait context by
 avoiding to use regmap



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@....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...

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

Powered by blists - more mailing lists