[PATCH 1/2] mfd: arizona: Fix race between runtime suspend and IRQs

[PATCH 1/2] mfd: arizona: Fix race between runtime suspend and IRQs

[Charles Keepax]

The function arizona_irq_thread (the threaded handler for the arizona
IRQs) calls pm_runtime_get_sync at the start to ensure that the chip is
active as we handle the IRQ. If the chip is part way through a runtime
suspend when an IRQ arrives the PM core will wait for the suspend to
complete, before resuming. However, since commit 4f0216409f7c
("mfd: arizona: Add better support for system suspend") the runtime
suspend function may call disable_irq, if the chip is going to fully
power off, which will try to wait for any outstanding IRQs to complete.
This results in deadlock as the IRQ thread is waiting for the PM
operation to complete and the PM thread is waiting for the IRQ to
complete.

To avoid this situation we use disable_irq_nosync, which allows the
suspending thread to finish the suspend without waiting for the IRQ to
complete. This is safe because if an IRQ is being processed it can only
be blocked at the pm_runtime_get_sync at the start of the handler
otherwise it wouldn't be possible to suspend.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index bebf58a..e60bcd9 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -651,7 +651,7 @@ static int arizona_runtime_suspend(struct device *dev)
 
 		arizona->has_fully_powered_off = true;
 
-		disable_irq(arizona->irq);
+		disable_irq_nosync(arizona->irq);
 		arizona_enable_reset(arizona);
 		regulator_bulk_disable(arizona->num_core_supplies,
 				       arizona->core_supplies);
-- 
1.7.2.5

[PATCH 2/2] mfd: arizona: Fix initialisation of the PM runtime

[Charles Keepax]

The PM runtime core by default assumes a chip is suspended when runtime
PM is enabled. Currently the arizona driver enables runtime PM when the
chip is fully active and then disables the DCVDD regulator at the end of
arizona_dev_init. This however has several problems, firstly the if we
reach the end of arizona_dev_init, we did not properly follow all the
proceedures for shutting down the chip, and most notably we never marked
the chip as cache only so any writes occurring between then and the next
PM runtime resume will be lost. Secondly, if we are already resumed when
we reach the end of dev_init, then at best we get unbalanced regulator
enable/disables at work we lose DCVDD whilst we need it.

Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better
support for system suspend"), the PM runtime operations may
disable/enable the IRQ, so the IRQs must now be enabled before we call
any PM operations.

This patch adds a call to pm_runtime_set_active to inform the PM core
that the device is starting up active and moves the PM enabling to
around the IRQ initialisation to avoid any PM callbacks happening until
the IRQs are initialised.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index e60bcd9..a72ddb2 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -1141,10 +1141,6 @@ int arizona_dev_init(struct arizona *arizona)
 			     arizona->pdata.gpio_defaults[i]);
 	}
 
-	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
-	pm_runtime_use_autosuspend(arizona->dev);
-	pm_runtime_enable(arizona->dev);
-
 	/* Chip default */
 	if (!arizona->pdata.clk32k_src)
 		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
@@ -1245,11 +1241,17 @@ int arizona_dev_init(struct arizona *arizona)
 					   arizona->pdata.spk_fmt[i]);
 	}
 
+	pm_runtime_set_active(arizona->dev);
+	pm_runtime_enable(arizona->dev);
+
 	/* Set up for interrupts */
 	ret = arizona_irq_init(arizona);
 	if (ret != 0)
 		goto err_reset;
 
+	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
+	pm_runtime_use_autosuspend(arizona->dev);
+
 	arizona_request_irq(arizona, ARIZONA_IRQ_CLKGEN_ERR, "CLKGEN error",
 			    arizona_clkgen_err, arizona);
 	arizona_request_irq(arizona, ARIZONA_IRQ_OVERCLOCKED, "Overclocked",
@@ -1278,10 +1280,6 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_irq;
 	}
 
-#ifdef CONFIG_PM
-	regulator_disable(arizona->dcvdd);
-#endif
-
 	return 0;
 
 err_irq:
-- 
1.7.2.5

Re: [PATCH 2/2] mfd: arizona: Fix initialisation of the PM runtime

[Lee Jones]

On Sun, 14 Jun 2015, Charles Keepax wrote:

> The PM runtime core by default assumes a chip is suspended when runtime
> PM is enabled. Currently the arizona driver enables runtime PM when the
> chip is fully active and then disables the DCVDD regulator at the end of
> arizona_dev_init. This however has several problems, firstly the if we
> reach the end of arizona_dev_init, we did not properly follow all the
> proceedures for shutting down the chip, and most notably we never marked
> the chip as cache only so any writes occurring between then and the next
> PM runtime resume will be lost. Secondly, if we are already resumed when
> we reach the end of dev_init, then at best we get unbalanced regulator
> enable/disables at work we lose DCVDD whilst we need it.
> 
> Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better
> support for system suspend"), the PM runtime operations may
> disable/enable the IRQ, so the IRQs must now be enabled before we call
> any PM operations.
> 
> This patch adds a call to pm_runtime_set_active to inform the PM core
> that the device is starting up active and moves the PM enabling to
> around the IRQ initialisation to avoid any PM callbacks happening until
> the IRQs are initialised.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index e60bcd9..a72ddb2 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -1141,10 +1141,6 @@ int arizona_dev_init(struct arizona *arizona)
>  			     arizona->pdata.gpio_defaults[i]);
>  	}
>  
> -	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
> -	pm_runtime_use_autosuspend(arizona->dev);
> -	pm_runtime_enable(arizona->dev);
> -
>  	/* Chip default */
>  	if (!arizona->pdata.clk32k_src)
>  		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
> @@ -1245,11 +1241,17 @@ int arizona_dev_init(struct arizona *arizona)
>  					   arizona->pdata.spk_fmt[i]);
>  	}
>  
> +	pm_runtime_set_active(arizona->dev);
> +	pm_runtime_enable(arizona->dev);
> +
>  	/* Set up for interrupts */
>  	ret = arizona_irq_init(arizona);
>  	if (ret != 0)
>  		goto err_reset;
>  
> +	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
> +	pm_runtime_use_autosuspend(arizona->dev);
> +
>  	arizona_request_irq(arizona, ARIZONA_IRQ_CLKGEN_ERR, "CLKGEN error",
>  			    arizona_clkgen_err, arizona);
>  	arizona_request_irq(arizona, ARIZONA_IRQ_OVERCLOCKED, "Overclocked",
> @@ -1278,10 +1280,6 @@ int arizona_dev_init(struct arizona *arizona)
>  		goto err_irq;
>  	}
>  
> -#ifdef CONFIG_PM
> -	regulator_disable(arizona->dcvdd);
> -#endif
> -
>  	return 0;
>  
>  err_irq:

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

Re: [PATCH 1/2] mfd: arizona: Fix race between runtime suspend and IRQs

[Lee Jones]

On Sun, 14 Jun 2015, Charles Keepax wrote:

> The function arizona_irq_thread (the threaded handler for the arizona
> IRQs) calls pm_runtime_get_sync at the start to ensure that the chip is
> active as we handle the IRQ. If the chip is part way through a runtime
> suspend when an IRQ arrives the PM core will wait for the suspend to
> complete, before resuming. However, since commit 4f0216409f7c
> ("mfd: arizona: Add better support for system suspend") the runtime
> suspend function may call disable_irq, if the chip is going to fully
> power off, which will try to wait for any outstanding IRQs to complete.
> This results in deadlock as the IRQ thread is waiting for the PM
> operation to complete and the PM thread is waiting for the IRQ to
> complete.
> 
> To avoid this situation we use disable_irq_nosync, which allows the
> suspending thread to finish the suspend without waiting for the IRQ to
> complete. This is safe because if an IRQ is being processed it can only
> be blocked at the pm_runtime_get_sync at the start of the handler
> otherwise it wouldn't be possible to suspend.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index bebf58a..e60bcd9 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -651,7 +651,7 @@ static int arizona_runtime_suspend(struct device *dev)
>  
>  		arizona->has_fully_powered_off = true;
>  
> -		disable_irq(arizona->irq);
> +		disable_irq_nosync(arizona->irq);
>  		arizona_enable_reset(arizona);
>  		regulator_bulk_disable(arizona->num_core_supplies,
>  				       arizona->core_supplies);

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