Re: [PATCH RESEND] mfd: arizona: Fix lockdep recursion warning on set_irq_wake

[Lee Jones <lee.jones@linaro.org>]

FAO Thomas

> Lockdep explicitly sets all the irq_desc locks as a single lock-class,
> which causes a "possible recursive locking detected" warning when we
> attempt to propagate the IRQ wake to our parent IRQ in
> arizona_irq_set_wake. Although this appears to be a false positive
> because an IRQ is unlikely to be its own parent, this was clearly
> intentionally prohibited.
> 
> To avoid this lockdep warning, take a cue from the regmap-irq system,
> and add bus lock callbacks on the IRQ chip and propagate the wake in
> the bus unlock which will happen after the desc lock has been released
> and thus avoid the issue.

This looks like a hack to me.  I'd like Thomas (Cc'ed) to look it over.

> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-irq.c        | 35 ++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/arizona/core.h |  3 +++
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index 5fef014..6497a65 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -154,17 +154,48 @@ static void arizona_irq_disable(struct irq_data *data)
>  {
>  }
>  
> +static void arizona_irq_lock(struct irq_data *data)
> +{
> +	struct arizona *arizona = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&arizona->irq_lock);
> +}
> +
> +static void arizona_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct arizona *arizona = irq_data_get_irq_chip_data(data);
> +	int i;
> +
> +	if (arizona->pending_wake_count < 0)
> +		for (i = arizona->pending_wake_count; i < 0; i++)
> +			irq_set_irq_wake(arizona->irq, 0);
> +	else if (arizona->pending_wake_count > 0)
> +		for (i = 0; i < arizona->pending_wake_count; i++)
> +			irq_set_irq_wake(arizona->irq, 1);
> +
> +	arizona->pending_wake_count = 0;
> +
> +	mutex_unlock(&arizona->irq_lock);
> +}
> +
>  static int arizona_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>  	struct arizona *arizona = irq_data_get_irq_chip_data(data);
>  
> -	return irq_set_irq_wake(arizona->irq, on);
> +	if (on)
> +		arizona->pending_wake_count++;
> +	else
> +		arizona->pending_wake_count--;
> +
> +	return 0;
>  }
>  
>  static struct irq_chip arizona_irq_chip = {
>  	.name			= "arizona",
>  	.irq_disable		= arizona_irq_disable,
>  	.irq_enable		= arizona_irq_enable,
> +	.irq_bus_lock		= arizona_irq_lock,
> +	.irq_bus_sync_unlock	= arizona_irq_sync_unlock,
>  	.irq_set_wake		= arizona_irq_set_wake,
>  };
>  
> @@ -193,6 +224,8 @@ int arizona_irq_init(struct arizona *arizona)
>  	const struct regmap_irq_chip *aod, *irq;
>  	struct irq_data *irq_data;
>  
> +	mutex_init(&arizona->irq_lock);
> +
>  	arizona->ctrlif_error = true;
>  
>  	switch (arizona->type) {
> diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
> index d55a422..a0374ea 100644
> --- a/include/linux/mfd/arizona/core.h
> +++ b/include/linux/mfd/arizona/core.h
> @@ -148,6 +148,9 @@ struct arizona {
>  	uint16_t dac_comp_coeff;
>  	uint8_t dac_comp_enabled;
>  	struct mutex dac_comp_lock;
> +
> +	int pending_wake_count;
> +	struct mutex irq_lock;
>  };
>  
>  int arizona_clk32k_enable(struct arizona *arizona);


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog