[PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[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 in the code by
making tegra_cpuidle_enter() to directly return the index returned by the
arm_cpuidle_simple_enter().

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

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

Changelog:

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
index 150045849d78..9e9a9cccd755 100644
--- a/drivers/cpuidle/cpuidle-tegra.c
+++ b/drivers/cpuidle/cpuidle-tegra.c
@@ -236,14 +236,14 @@ static int tegra_cpuidle_enter(struct cpuidle_device *dev,
 			       int index)
 {
 	unsigned int cpu = cpu_logical_map(dev->cpu);
-	int err;
+	int err = 0;
 
 	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);
+		index = arm_cpuidle_simple_enter(dev, drv, index);
 	else
 		err = tegra_cpuidle_state_enter(dev, index, cpu);
 
-- 
2.26.0

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Jon Hunter]

On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
> making tegra_cpuidle_enter() to directly return the index returned by the
> arm_cpuidle_simple_enter().

Are you suggesting that arm_cpuidle_simple_enter() could be updated to
actually return an error? Sorry it is not clear to me what you are implying.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Dmitry Osipenko]

08.07.2020 15:34, Jon Hunter пишет:
> 
> On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
>> making tegra_cpuidle_enter() to directly return the index returned by the
>> arm_cpuidle_simple_enter().
> 
> Are you suggesting that arm_cpuidle_simple_enter() could be updated to
> actually return an error? Sorry it is not clear to me what you are implying.

Hello, Jon!

Yes, I'm saying that *potentially* arm_cpuidle_simple_enter() could be
updated to actually return error.

The implication is that the current code is wrong and it may strike back
in the future if we won't fix it. Is this more clear? :)

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Jon Hunter]

On 08/07/2020 15:32, Dmitry Osipenko wrote:
> 08.07.2020 15:34, Jon Hunter пишет:
>>
>> On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
>>> making tegra_cpuidle_enter() to directly return the index returned by the
>>> arm_cpuidle_simple_enter().
>>
>> Are you suggesting that arm_cpuidle_simple_enter() could be updated to
>> actually return an error? Sorry it is not clear to me what you are implying.
> 
> Hello, Jon!
> 
> Yes, I'm saying that *potentially* arm_cpuidle_simple_enter() could be
> updated to actually return error.


OK, then I am confused, because after your change, we would now ignore
any error that could be returned in the future. Yes the current code
does not set the variable 'index' correctly, but before we set the value
of 'index' shouldn't we check that the value being returned is not a
negative error code first?

Jon

-- 
nvpublic

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Dmitry Osipenko]

09.07.2020 12:06, Jon Hunter пишет:
> 
> On 08/07/2020 15:32, Dmitry Osipenko wrote:
>> 08.07.2020 15:34, Jon Hunter пишет:
>>>
>>> On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
>>>> making tegra_cpuidle_enter() to directly return the index returned by the
>>>> arm_cpuidle_simple_enter().
>>>
>>> Are you suggesting that arm_cpuidle_simple_enter() could be updated to
>>> actually return an error? Sorry it is not clear to me what you are implying.
>>
>> Hello, Jon!
>>
>> Yes, I'm saying that *potentially* arm_cpuidle_simple_enter() could be
>> updated to actually return error.
> 
> 
> OK, then I am confused, because after your change, we would now ignore
> any error that could be returned in the future. Yes the current code
> does not set the variable 'index' correctly, but before we set the value
> of 'index' shouldn't we check that the value being returned is not a
> negative error code first?

Could you please clarify what do you mean by "ignore any error"? Do you
mean the error message?

After this patch, the tegra_cpuidle_enter() will directly return the
index returned by the arm_cpuidle_simple_enter(). I guess this may be
unclear if you're only looking at the patch and not at the whole code.

Please see how tegra_cpuidle_enter() looks after applying this patch:

static int tegra_cpuidle_enter(struct cpuidle_device *dev,
			       struct cpuidle_driver *drv,
			       int index)
{
	unsigned int cpu = cpu_logical_map(dev->cpu);
	int err = 0;

	index = tegra_cpuidle_adjust_state_index(index, cpu);
	if (dev->states_usage[index].disable)
		return -1;

	if (index == TEGRA_C1)
		index = arm_cpuidle_simple_enter(dev, drv, index);
	else
		err = 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);

	return err ? -1 : index;
}

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Jon Hunter]

On 09/07/2020 10:48, Dmitry Osipenko wrote:
> 09.07.2020 12:06, Jon Hunter пишет:
>>
>> On 08/07/2020 15:32, Dmitry Osipenko wrote:
>>> 08.07.2020 15:34, Jon Hunter пишет:
>>>>
>>>> On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
>>>>> making tegra_cpuidle_enter() to directly return the index returned by the
>>>>> arm_cpuidle_simple_enter().
>>>>
>>>> Are you suggesting that arm_cpuidle_simple_enter() could be updated to
>>>> actually return an error? Sorry it is not clear to me what you are implying.
>>>
>>> Hello, Jon!
>>>
>>> Yes, I'm saying that *potentially* arm_cpuidle_simple_enter() could be
>>> updated to actually return error.
>>
>>
>> OK, then I am confused, because after your change, we would now ignore
>> any error that could be returned in the future. Yes the current code
>> does not set the variable 'index' correctly, but before we set the value
>> of 'index' shouldn't we check that the value being returned is not a
>> negative error code first?
> 
> Could you please clarify what do you mean by "ignore any error"? Do you
> mean the error message?

Yes exactly. We would skip that, which seems a bit odd.

Jon

-- 
nvpublic

Re: [PATCH v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

[Dmitry Osipenko]

09.07.2020 12:51, Jon Hunter пишет:
> 
> 
> On 09/07/2020 10:48, Dmitry Osipenko wrote:
>> 09.07.2020 12:06, Jon Hunter пишет:
>>>
>>> On 08/07/2020 15:32, Dmitry Osipenko wrote:
>>>> 08.07.2020 15:34, Jon Hunter пишет:
>>>>>
>>>>> On 02/07/2020 01:13, Dmitry Osipenko 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 in the code by
>>>>>> making tegra_cpuidle_enter() to directly return the index returned by the
>>>>>> arm_cpuidle_simple_enter().
>>>>>
>>>>> Are you suggesting that arm_cpuidle_simple_enter() could be updated to
>>>>> actually return an error? Sorry it is not clear to me what you are implying.
>>>>
>>>> Hello, Jon!
>>>>
>>>> Yes, I'm saying that *potentially* arm_cpuidle_simple_enter() could be
>>>> updated to actually return error.
>>>
>>>
>>> OK, then I am confused, because after your change, we would now ignore
>>> any error that could be returned in the future. Yes the current code
>>> does not set the variable 'index' correctly, but before we set the value
>>> of 'index' shouldn't we check that the value being returned is not a
>>> negative error code first?
>>
>> Could you please clarify what do you mean by "ignore any error"? Do you
>> mean the error message?
> 
> Yes exactly. We would skip that, which seems a bit odd.

Yeah.. well, I was a bit doubtful about the error message, maybe
arm_cpuidle_simple_enter() could have its own message? :)

But since you're asking about it now, then perhaps it should be better
and more clear to everyone not to skip the error message.

Alright, I'll prepare the v3.

Thank you for the review!