01.07.2020 16:56, Jon Hunter пишет:
>
> On 30/06/2020 19:54, Dmitry Osipenko wrote:
>> 30.06.2020 12:02, Jon Hunter пишет:
>>>
>>> On 29/06/2020 23:26, Dmitry Osipenko wrote:
>>>> The arm_cpuidle_simple_enter() returns the entered idle-index and not a
>>>> error code. It happened that TEGRA_C1=index=err=0, and hence this typo
>>>> was difficult to notice in the code since everything happened to work
>>>> properly. This patch fixes the minor typo, it doesn't fix any problem.
>>>
>>> I guess that is dependent on if CPUIDLE is enabled ...
>>>
>>> #ifdef CONFIG_CPU_IDLE
>>> extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>>> struct cpuidle_driver *drv, int index);
>>> #else
>>> static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>>> struct cpuidle_driver *drv, int index) { return -ENODEV; }
>>> #endif
>>>
>>> Looks like it could return an error.
>>
>> Hello Jon!
>>
>> The cpuidle's enter() callback returns an index of the entered state on
>> success, on negative value on failure.
>
> Yes, however, when I read the first sentence of the changelog it seemed
> to suggested it would never return and error code. Perhaps you meant in
> the context of the Tegra CPUIdle driver because CPU_IDLE is always enabled?
Yes, the commit message could be improved in regards to the error
condition clarification. I'll update it in v2, thank you for the suggestion!
>> The negative number *could be* a proper error code, but in the same time
>> it also doesn't matter what's the exact negative value is for the
>> cpuidle's core code. Please see more below!
>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>> ---
>>>> 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);
>>>>
>>>>
>>>
>>> However, I do think that there is something not right in the error handling
>>> here. Would also be nice to get rid of these -1.
>>
>> IIRC, the -1 was borrowed from some other cpuidle driver, for example
>> cpuidle-psci[1] and coupled.c[2] are returning -1 on a failure.
>
> Indeed. Maybe we just let sleeping dogs lie in this case.
+1
We could always return to this later on, once there will be a real need.
