[RESEND,v4] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

The enter() callback of CPUIDLE drivers returns index of the entered idle
state on success or a negative value on failure. The negative value could
any negative value, i.e. it doesn't necessarily needs to be a error code.
That's because CPUIDLE core only cares about the fact of failure and not
about the reason of the enter() failure.

Like every other enter() callback, the arm_cpuidle_simple_enter() returns
the entered idle-index on success. Unlike some of other drivers, it never
fails. It happened that TEGRA_C1=index=err=0 in the code of cpuidle-tegra
driver, and thus, there is no problem for the cpuidle-tegra driver created
by the typo in the code which assumes that the arm_cpuidle_simple_enter()
returns a error code.

The arm_cpuidle_simple_enter() also may return a -ENODEV error if CPU_IDLE
is disabled in a kernel's config, but all CPUIDLE drivers are disabled if
CPU_IDLE is disabled, including the cpuidle-tegra driver. So we can't ever
see the error code from arm_cpuidle_simple_enter() today.

Of course the code may get some changes in the future and then the
typo may transform into a real bug, so let's correct the typo! The
tegra_cpuidle_state_enter() is now changed to make it return the entered
idle-index on success and negative error code on fail, which puts it on
par with the arm_cpuidle_simple_enter(), making code consistent in regards
to the error handling.

This patch fixes a minor typo in the code, it doesn't fix any bugs.

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

Changelog:

v4: No code changes. Added r-b from Jon Hunter and re-sending for 5.10.

v3: The tegra_cpuidle_state_enter() now returns entered idle-index on
    success instead of 0. Hence the error message will be shown by the
    tegra-cpuidle driver if arm_cpuidle_simple_enter() will ever fail.
    Again thanks to Jon Hunter!

v2: Improved commit message by clarifying what values are returned by
    arm_cpuidle_simple_enter() and when. Thanks to Jon Hunter for the
    suggestion!

 drivers/cpuidle/cpuidle-tegra.c | 34 +++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

31.08.2020 11:38, Dmitry Osipenko пишет:
> The enter() callback of CPUIDLE drivers returns index of the entered idle

> state on success or a negative value on failure. The negative value could

> any negative value, i.e. it doesn't necessarily needs to be a error code.

> That's because CPUIDLE core only cares about the fact of failure and not

> about the reason of the enter() failure.

> 

> Like every other enter() callback, the arm_cpuidle_simple_enter() returns

> the entered idle-index on success. Unlike some of other drivers, it never

> fails. It happened that TEGRA_C1=index=err=0 in the code of cpuidle-tegra

> driver, and thus, there is no problem for the cpuidle-tegra driver created

> by the typo in the code which assumes that the arm_cpuidle_simple_enter()

> returns a error code.

> 

> The arm_cpuidle_simple_enter() also may return a -ENODEV error if CPU_IDLE

> is disabled in a kernel's config, but all CPUIDLE drivers are disabled if

> CPU_IDLE is disabled, including the cpuidle-tegra driver. So we can't ever

> see the error code from arm_cpuidle_simple_enter() today.

> 

> Of course the code may get some changes in the future and then the

> typo may transform into a real bug, so let's correct the typo! The

> tegra_cpuidle_state_enter() is now changed to make it return the entered

> idle-index on success and negative error code on fail, which puts it on

> par with the arm_cpuidle_simple_enter(), making code consistent in regards

> to the error handling.

> 

> This patch fixes a minor typo in the code, it doesn't fix any bugs.

> 

> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

> ---


Hello, Daniel!

Pinging you just in a case if this patch slipped away from you :)

On Mon, Aug 31, 2020 at 10:39 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> The enter() callback of CPUIDLE drivers returns index of the entered idle
> state on success or a negative value on failure. The negative value could
> any negative value, i.e. it doesn't necessarily needs to be a error code.
> That's because CPUIDLE core only cares about the fact of failure and not
> about the reason of the enter() failure.
>
> Like every other enter() callback, the arm_cpuidle_simple_enter() returns
> the entered idle-index on success. Unlike some of other drivers, it never
> fails. It happened that TEGRA_C1=index=err=0 in the code of cpuidle-tegra
> driver, and thus, there is no problem for the cpuidle-tegra driver created
> by the typo in the code which assumes that the arm_cpuidle_simple_enter()
> returns a error code.
>
> The arm_cpuidle_simple_enter() also may return a -ENODEV error if CPU_IDLE
> is disabled in a kernel's config, but all CPUIDLE drivers are disabled if
> CPU_IDLE is disabled, including the cpuidle-tegra driver. So we can't ever
> see the error code from arm_cpuidle_simple_enter() today.
>
> Of course the code may get some changes in the future and then the
> typo may transform into a real bug, so let's correct the typo! The
> tegra_cpuidle_state_enter() is now changed to make it return the entered
> idle-index on success and negative error code on fail, which puts it on
> par with the arm_cpuidle_simple_enter(), making code consistent in regards
> to the error handling.
>
> This patch fixes a minor typo in the code, it doesn't fix any bugs.
>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Applied as 5.10 material, thanks!

> ---
>
> Changelog:
>
> v4: No code changes. Added r-b from Jon Hunter and re-sending for 5.10.
>
> v3: The tegra_cpuidle_state_enter() now returns entered idle-index on
>     success instead of 0. Hence the error message will be shown by the
>     tegra-cpuidle driver if arm_cpuidle_simple_enter() will ever fail.
>     Again thanks to Jon Hunter!
>
> v2: Improved commit message by clarifying what values are returned by
>     arm_cpuidle_simple_enter() and when. Thanks to Jon Hunter for the
>     suggestion!
>
>  drivers/cpuidle/cpuidle-tegra.c | 34 +++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
> index a12fb141875a..e8956706a291 100644
> --- a/drivers/cpuidle/cpuidle-tegra.c
> +++ b/drivers/cpuidle/cpuidle-tegra.c
> @@ -172,7 +172,7 @@ static int tegra_cpuidle_coupled_barrier(struct cpuidle_device *dev)
>  static int tegra_cpuidle_state_enter(struct cpuidle_device *dev,
>                                      int index, unsigned int cpu)
>  {
> -       int ret;
> +       int err;
>
>         /*
>          * CC6 state is the "CPU cluster power-off" state.  In order to
> @@ -183,9 +183,9 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev,
>          * CPU cores, GIC and L2 cache).
>          */
>         if (index == TEGRA_CC6) {
> -               ret = tegra_cpuidle_coupled_barrier(dev);
> -               if (ret)
> -                       return ret;
> +               err = tegra_cpuidle_coupled_barrier(dev);
> +               if (err)
> +                       return err;
>         }
>
>         local_fiq_disable();
> @@ -194,15 +194,15 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev,
>
>         switch (index) {
>         case TEGRA_C7:
> -               ret = tegra_cpuidle_c7_enter();
> +               err = tegra_cpuidle_c7_enter();
>                 break;
>
>         case TEGRA_CC6:
> -               ret = tegra_cpuidle_cc6_enter(cpu);
> +               err = tegra_cpuidle_cc6_enter(cpu);
>                 break;
>
>         default:
> -               ret = -EINVAL;
> +               err = -EINVAL;
>                 break;
>         }
>
> @@ -210,7 +210,7 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev,
>         tegra_pm_clear_cpu_in_lp2();
>         local_fiq_enable();
>
> -       return ret;
> +       return err ?: index;
>  }
>
>  static int tegra_cpuidle_adjust_state_index(int index, unsigned int cpu)
> @@ -236,21 +236,27 @@ static int tegra_cpuidle_enter(struct cpuidle_device *dev,
>                                int index)
>  {
>         unsigned int cpu = cpu_logical_map(dev->cpu);
> -       int err;
> +       int ret;
>
>         index = tegra_cpuidle_adjust_state_index(index, cpu);
>         if (dev->states_usage[index].disable)
>                 return -1;
>
>         if (index == TEGRA_C1)
> -               err = arm_cpuidle_simple_enter(dev, drv, index);
> +               ret = arm_cpuidle_simple_enter(dev, drv, index);
>         else
> -               err = tegra_cpuidle_state_enter(dev, index, cpu);
> +               ret = tegra_cpuidle_state_enter(dev, index, cpu);
>
> -       if (err && (err != -EINTR || index != TEGRA_CC6))
> -               pr_err_once("failed to enter state %d err: %d\n", index, err);
> +       if (ret < 0) {
> +               if (ret != -EINTR || index != TEGRA_CC6)
> +                       pr_err_once("failed to enter state %d err: %d\n",
> +                                   index, ret);
> +               index = -1;
> +       } else {
> +               index = ret;
> +       }
>
> -       return err ? -1 : index;
> +       return index;
>  }
>
>  static int tegra114_enter_s2idle(struct cpuidle_device *dev,
> --
> 2.27.0
>