| Message ID | 20200702001354.27056-1-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter() | expand |
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
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? :)
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
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; }
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
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!
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);
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(-)