diff mbox series

soc: apple: apple-pmgr-pwrstate: Switch to IRQ-safe mode

Message ID 20230123065125.26350-1-marcan@marcan.st
State New
Headers
Series soc: apple: apple-pmgr-pwrstate: Switch to IRQ-safe mode |

Commit Message

Hector Martin Jan. 23, 2023, 6:51 a.m. UTC 
  This requires changing the reset path locking primitives to the spinlock
path in genpd, instead of the mutex path.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/soc/apple/apple-pmgr-pwrstate.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Eric Curtin Jan. 23, 2023, 11:22 a.m. UTC | #1 
On Mon, 23 Jan 2023 at 07:01, Hector Martin <marcan@marcan.st> wrote:
>
> This requires changing the reset path locking primitives to the spinlock
> path in genpd, instead of the mutex path.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---

It seems we need this to avoid a race from reading #asahi-dev IRC,
commit message could be more detailed here.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Is mise le meas/Regards,

Eric Curtin

>  drivers/soc/apple/apple-pmgr-pwrstate.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
> index e1122288409a..a3e2bc1d2686 100644
> --- a/drivers/soc/apple/apple-pmgr-pwrstate.c
> +++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
> @@ -116,8 +116,9 @@ static int apple_pmgr_ps_power_off(struct generic_pm_domain *genpd)
>  static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
>  {
>         struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +       unsigned long flags;
>
> -       mutex_lock(&ps->genpd.mlock);
> +       spin_lock_irqsave(&ps->genpd.slock, flags);
>
>         if (ps->genpd.status == GENPD_STATE_OFF)
>                 dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
> @@ -129,7 +130,7 @@ static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned
>         regmap_update_bits(ps->regmap, ps->offset, APPLE_PMGR_FLAGS | APPLE_PMGR_RESET,
>                            APPLE_PMGR_RESET);
>
> -       mutex_unlock(&ps->genpd.mlock);
> +       spin_unlock_irqrestore(&ps->genpd.slock, flags);
>
>         return 0;
>  }
> @@ -137,8 +138,9 @@ static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned
>  static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>  {
>         struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +       unsigned long flags;
>
> -       mutex_lock(&ps->genpd.mlock);
> +       spin_lock_irqsave(&ps->genpd.slock, flags);
>
>         dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
>         regmap_update_bits(ps->regmap, ps->offset, APPLE_PMGR_FLAGS | APPLE_PMGR_RESET, 0);
> @@ -147,7 +149,7 @@ static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigne
>         if (ps->genpd.status == GENPD_STATE_OFF)
>                 dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
>
> -       mutex_unlock(&ps->genpd.mlock);
> +       spin_unlock_irqrestore(&ps->genpd.slock, flags);
>
>         return 0;
>  }
> @@ -222,6 +224,7 @@ static int apple_pmgr_ps_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       ps->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
>         ps->genpd.name = name;
>         ps->genpd.power_on = apple_pmgr_ps_power_on;
>         ps->genpd.power_off = apple_pmgr_ps_power_off;
> --
> 2.35.1
>
>
Hector Martin Jan. 23, 2023, 2:11 p.m. UTC | #2 
On 23/01/2023 20.22, Eric Curtin wrote:
> On Mon, 23 Jan 2023 at 07:01, Hector Martin <marcan@marcan.st> wrote:
>>
>> This requires changing the reset path locking primitives to the spinlock
>> path in genpd, instead of the mutex path.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
> 
> It seems we need this to avoid a race from reading #asahi-dev IRC,
> commit message could be more detailed here.
> 
> Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Not exactly, that was unrelated. We just need this to be able to switch
power states from IRQ/atomic context, which came up when adding runtime
PM to the DART driver (IIRC, this was a few weeks back and I threw
runtime PM into like 3 drivers that day so I might be misremembering,
but we definitely need it for one of them :-)).

The power state part of the driver itself is trivially IRQ-safe already,
so there is no reason not to do it. We just have to switch the locking
primitive the reset code uses (genpd already supports both), which is
only used to provide the reset functionality safely since it involves
the same register as the power state control.

- Hector
Sven Peter Jan. 28, 2023, 11:38 a.m. UTC | #3 
On Mon, Jan 23, 2023, at 07:51, Hector Martin wrote:
> This requires changing the reset path locking primitives to the spinlock
> path in genpd, instead of the mutex path.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---

Reviewed-by: Sven Peter <sven@svenpeter.dev>


Sven
Hector Martin Jan. 31, 2023, 11:41 a.m. UTC | #4 
On 23/01/2023 15.51, Hector Martin wrote:
> This requires changing the reset path locking primitives to the spinlock
> path in genpd, instead of the mutex path.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/soc/apple/apple-pmgr-pwrstate.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

Thanks for the reviews, applied to asahi-soc/soc!

- Hector
diff mbox series

Patch

diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
index e1122288409a..a3e2bc1d2686 100644
--- a/drivers/soc/apple/apple-pmgr-pwrstate.c
+++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
@@ -116,8 +116,9 @@  static int apple_pmgr_ps_power_off(struct generic_pm_domain *genpd)
 static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
 {
 	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
+	unsigned long flags;
 
-	mutex_lock(&ps->genpd.mlock);
+	spin_lock_irqsave(&ps->genpd.slock, flags);
 
 	if (ps->genpd.status == GENPD_STATE_OFF)
 		dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
@@ -129,7 +130,7 @@  static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned
 	regmap_update_bits(ps->regmap, ps->offset, APPLE_PMGR_FLAGS | APPLE_PMGR_RESET,
 			   APPLE_PMGR_RESET);
 
-	mutex_unlock(&ps->genpd.mlock);
+	spin_unlock_irqrestore(&ps->genpd.slock, flags);
 
 	return 0;
 }
@@ -137,8 +138,9 @@  static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned
 static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
 {
 	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
+	unsigned long flags;
 
-	mutex_lock(&ps->genpd.mlock);
+	spin_lock_irqsave(&ps->genpd.slock, flags);
 
 	dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
 	regmap_update_bits(ps->regmap, ps->offset, APPLE_PMGR_FLAGS | APPLE_PMGR_RESET, 0);
@@ -147,7 +149,7 @@  static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigne
 	if (ps->genpd.status == GENPD_STATE_OFF)
 		dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
 
-	mutex_unlock(&ps->genpd.mlock);
+	spin_unlock_irqrestore(&ps->genpd.slock, flags);
 
 	return 0;
 }
@@ -222,6 +224,7 @@  static int apple_pmgr_ps_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ps->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
 	ps->genpd.name = name;
 	ps->genpd.power_on = apple_pmgr_ps_power_on;
 	ps->genpd.power_off = apple_pmgr_ps_power_off;