MIME-Version: 1.0
References: <20230123065125.26350-1-marcan@marcan.st>
In-Reply-To: <20230123065125.26350-1-marcan@marcan.st>
From:   Eric Curtin <ecurtin@redhat.com>
Date:   Mon, 23 Jan 2023 11:22:53 +0000
Message-ID: <CAOgh=FwEpMGx-F6GZumOBkBYShtU_SVUWTzZ+WWY0Euc-75YGQ@mail.gmail.com>
Subject: Re: [PATCH] soc: apple: apple-pmgr-pwrstate: Switch to IRQ-safe mode
To:     Hector Martin <marcan@marcan.st>
Cc:     Sven Peter <sven@svenpeter.dev>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Janne Grunau <j@jannau.net>, asahi@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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