Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1364122 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RESEND] mfd: arizona: Fix lockdep recursion warning on  set_irq_wake Lee Jones <lee.jones@linaro.org> - 2016-03-24 13:50 +0100
    Re: [PATCH RESEND] mfd: arizona: Fix lockdep recursion warning on  set_irq_wake Thomas Gleixner <tglx@linutronix.de> - 2016-03-24 14:00 +0100
      Re: [PATCH RESEND] mfd: arizona: Fix lockdep recursion warning on  set_irq_wake Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-03-24 16:40 +0100

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

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

[toc] | [next] | [standalone]

#1364125

On Thu, 24 Mar 2016, Lee Jones wrote:

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

irq_set_lockdep_class() exists for a reason. See kernel/irq/generic-chip.c or
drivers/gpio/gpiolib.c for examples.

[toc] | [prev] | [next] | [standalone]

#1364276

On Thu, Mar 24, 2016 at 01:56:52PM +0100, Thomas Gleixner wrote:
> On Thu, 24 Mar 2016, Lee Jones wrote:
> 
> > 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.
> 
> irq_set_lockdep_class() exists for a reason. See kernel/irq/generic-chip.c or
> drivers/gpio/gpiolib.c for examples.

Apologies for missing that. Thanks guys I will have a look and
respin the patch.

Thanks,
Charles

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web