cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter()

PATCH Superseded Dmitry Osipenko 2 participants 7 messages Jul 2, 2020 – Jul 9, 2020

Subsystems (2)

Tegra architecture support CPU idle time management framework

lore

Share & tools

lore

Dmitry Osipenko

author

2020-07-02 00:13 UTC #0 lore raw

PATCH v2 1/1 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 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 <[email protected]>

---

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

drivers/cpuidle/cpuidle-tegra.c +2 -2

index 150045849d78..9e9a9cccd755 100644
@@ -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);

Signature · 1 line

2.26.0

Jon Hunter

maintainer

2020-07-08 12:34 UTC #1 lore raw

Quoted 18 lines · 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.

Quoted 4 lines

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

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-07-08 14:32 UTC #2 lore raw

08.07.2020 15:34, Jon Hunter пишет:

Quoted 20 lines · Dmitry Osipenko

Quoted 18 lines
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.

Quoted 7 lines

Quoted 4 lines
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? :)

Jon Hunter

maintainer

2020-07-09 09:06 UTC #3 lore raw

Quoted 26 lines · Dmitry Osipenko

08.07.2020 15:34, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
Quoted 22 lines
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().

Quoted 7 lines

Quoted 2 lines
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

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-07-09 09:48 UTC #4 lore raw

09.07.2020 12:06, Jon Hunter пишет:

Quoted 37 lines · Dmitry Osipenko

Quoted 26 lines
08.07.2020 15:34, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
Quoted 22 lines
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().
Quoted 7 lines
Quoted 2 lines
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.

Quoted 5 lines

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

Jon Hunter

maintainer

2020-07-09 09:51 UTC #5 lore raw

Quoted 38 lines · Dmitry Osipenko

09.07.2020 12:06, Jon Hunter пишет:
Quoted 37 lines · Dmitry Osipenko
Quoted 33 lines
08.07.2020 15:34, Jon Hunter пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 18 lines
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.
Quoted 7 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 5 lines
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

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-07-09 10:21 UTC #6 lore raw

09.07.2020 12:51, Jon Hunter пишет:

Quoted 47 lines · Dmitry Osipenko

Quoted 44 lines
09.07.2020 12:06, Jon Hunter пишет:
Quoted 42 lines · Dmitry Osipenko
Quoted 26 lines
08.07.2020 15:34, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
Quoted 22 lines
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().
Quoted 7 lines
Quoted 2 lines
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?

Quoted 4 lines

Quoted 2 lines
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!