Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Peter Geis]

On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
<sebastian.reichel@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
> Tested-by: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx> # On Rock 5B
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxx/

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