Message-ID: <CAMdYzYqLq=kSC8fiBapRS_8w0s8PaL9Yd46VgM56YbTEmUG1xA@mail.gmail.com>
Date: Wed, 11 Dec 2024 14:53:34 -0500
From: Peter Geis <pgwipeout@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Mark Brown <broonie@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Elaine Zhang <zhangqing@...k-chips.com>, 
	Adrián Martínez Larumbe <adrian.larumbe@...labora.com>, 
	Boris Brezillon <boris.brezillon@...labora.com>, Chen-Yu Tsai <wens@...e.org>, 
	devicetree@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, kernel@...labora.com, 
	Dragan Simic <dsimic@...jaro.org>
Subject: Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain
 errors

On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
<sebastian.reichel@...labora.com> wrote:
>
> Currently rockchip_do_pmu_set_power_domain prints a warning if there
> have been errors turning on the power domain, but it does not return
> any errors and rockchip_pd_power() tries to continue setting up the
> QOS registers. This usually results in accessing unpowered registers,
> which triggers an SError and a full system hang.
>
> This improves the error handling by forwarding the error to avoid
> kernel panics.

Good Afternoon,

I think we should merge your patch here with my patch for returning
errors from rockchip_pmu_set_idle_request [1].

>
> Reviewed-by: Heiko Stuebner <heiko@...ech.de>
> Tested-by: Heiko Stuebner <heiko@...ech.de>
> Tested-by: Adrian Larumbe <adrian.larumbe@...labora.com> # On Rock 5B
> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> ---
>  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index a161ee13c633..8f440f2883db 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
>         return ret;
>  }
>
> -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> -                                            bool on)
> +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> +                                           bool on)
>  {
>         struct rockchip_pmu *pmu = pd->pmu;
>         struct generic_pm_domain *genpd = &pd->genpd;
>         u32 pd_pwr_offset = pd->info->pwr_offset;
>         bool is_on, is_mem_on = false;
> +       int ret;
>
>         if (pd->info->pwr_mask == 0)
> -               return;
> +               return 0;
>
>         if (on && pd->info->mem_status_mask)
>                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>
>         wmb();
>
> -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> -               return;
> +       if (is_mem_on) {
> +               ret = rockchip_pmu_domain_mem_reset(pd);
> +               if (ret)
> +                       return ret;
> +       }
>
> -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> -                                     is_on == on, 0, 10000)) {
> -               dev_err(pmu->dev,
> -                       "failed to set domain '%s', val=%d\n",
> -                       genpd->name, is_on);
> -               return;
> +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> +                                       is_on == on, 0, 10000);
> +       if (ret) {
> +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> +                       genpd->name, on ? "on" : "off", is_on);
> +               return ret;
>         }
> +
> +       return 0;
>  }
>
>  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>                         rockchip_pmu_set_idle_request(pd, true);
>                 }
>
> -               rockchip_do_pmu_set_power_domain(pd, power_on);
> +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> +               if (ret < 0) {
> +                       clk_bulk_disable(pd->num_clks, pd->clks);
> +                       return ret;

Looking at it, we shouldn't return directly from here because the
mutex never gets unlocked.
Instead of repeating clk_bulk_disable and return ret for each failure,
we can initialize ret = 0, have a goto: out pointing to
clk_bulk_disable, and change return 0 to return ret at the end.

What do you think?

Very Respectfully,
Peter Geis

[1] https://lore.kernel.org/linux-rockchip/20241210013010.81257-2-pgwipeout@gmail.com/

> +               }
>
>                 if (power_on) {
>                         /* if powering up, leave idle mode */
> --
> 2.45.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists