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

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

[Charles Keepax]

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.

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);
-- 
2.1.4

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

[Lee Jones]

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

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

[Thomas Gleixner]

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.

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

[Charles Keepax]

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