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

[Dmitry Osipenko <digetx@gmail.com>]

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@gmail.com>
>>>> ---
>>>>  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.