[PATCH 0/2] ARM: cpuidle: bug fix and a trivial improvement

[PATCH 0/2] ARM: cpuidle: bug fix and a trivial improvement

[Jisheng Zhang]

There's one corner case need to be fixed: !cpuidle_ops[cpu].init.
patch1 tries to address this corner case.

patch2 tries to improve arm_cpuidle_suspend() a bit by moving .suspend
check into arm_cpuidle_init().

Jisheng Zhang (2):
  ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init
  ARM: cpuidle: make arm_cpuidle_suspend() a bit more efficient

 arch/arm/kernel/cpuidle.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.8.0.rc3

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Jisheng Zhang]

Let's assume cpuidle_ops exists but it doesn't implement the according
init member, current arm_cpuidle_init() will return success to its
caller, but in fact it should return -EOPNOTSUPP.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/cpuidle.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index 703926e..f108d8f 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -143,8 +143,12 @@ int __init arm_cpuidle_init(int cpu)
 		return -ENODEV;
 
 	ret = arm_cpuidle_read_ops(cpu_node, cpu);
-	if (!ret && cpuidle_ops[cpu].init)
-		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+	if (!ret) {
+		if (cpuidle_ops[cpu].init)
+			ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+		else
+			ret = -EOPNOTSUPP;
+	}
 
 	of_node_put(cpu_node);
 
-- 
2.8.0.rc3

[PATCH 2/2] ARM: cpuidle: make arm_cpuidle_suspend() a bit more efficient

[Jisheng Zhang]

Currently, we check cpuidle_ops.suspend every time when entering a
low-power idle state. But this check could be avoided in this hot path
by moving it into arm_cpuidle_init() to reduce arm_cpuidle_suspend()
overhead a bit.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/cpuidle.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index f108d8f..bf68d49 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -52,13 +52,9 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
  */
 int arm_cpuidle_suspend(int index)
 {
-	int ret = -EOPNOTSUPP;
 	int cpu = smp_processor_id();
 
-	if (cpuidle_ops[cpu].suspend)
-		ret = cpuidle_ops[cpu].suspend(index);
-
-	return ret;
+	return cpuidle_ops[cpu].suspend(index);
 }
 
 /**
@@ -144,7 +140,7 @@ int __init arm_cpuidle_init(int cpu)
 
 	ret = arm_cpuidle_read_ops(cpu_node, cpu);
 	if (!ret) {
-		if (cpuidle_ops[cpu].init)
+		if (cpuidle_ops[cpu].init && cpuidle_ops[cpu].suspend)
 			ret = cpuidle_ops[cpu].init(cpu_node, cpu);
 		else
 			ret = -EOPNOTSUPP;
-- 
2.8.0.rc3

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Daniel Lezcano]

On 03/24/2016 06:11 AM, Jisheng Zhang wrote:
> Let's assume cpuidle_ops exists but it doesn't implement the according
> init member, current arm_cpuidle_init() will return success to its
> caller, but in fact it should return -EOPNOTSUPP.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   arch/arm/kernel/cpuidle.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 703926e..f108d8f 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -143,8 +143,12 @@ int __init arm_cpuidle_init(int cpu)
>   		return -ENODEV;
>
>   	ret = arm_cpuidle_read_ops(cpu_node, cpu);
> -	if (!ret && cpuidle_ops[cpu].init)
> -		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> +	if (!ret) {
> +		if (cpuidle_ops[cpu].init)
> +			ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> +		else
> +			ret = -EOPNOTSUPP;
> +	}

Hi Jisheng,

this should be handled in the arm_cpuidle_read_ops function.

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 2/2] ARM: cpuidle: make arm_cpuidle_suspend() a bit more efficient

[Daniel Lezcano]

On 03/24/2016 06:11 AM, Jisheng Zhang wrote:
> Currently, we check cpuidle_ops.suspend every time when entering a
> low-power idle state. But this check could be avoided in this hot path
> by moving it into arm_cpuidle_init() to reduce arm_cpuidle_suspend()
> overhead a bit.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   arch/arm/kernel/cpuidle.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index f108d8f..bf68d49 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -52,13 +52,9 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>    */
>   int arm_cpuidle_suspend(int index)
>   {
> -	int ret = -EOPNOTSUPP;
>   	int cpu = smp_processor_id();
>
> -	if (cpuidle_ops[cpu].suspend)
> -		ret = cpuidle_ops[cpu].suspend(index);
> -
> -	return ret;
> +	return cpuidle_ops[cpu].suspend(index);
>   }

I agree with the optimization but, same comment than the previous patch, 
it should be handled in arm_cpuidle_read_ops.

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Jisheng Zhang]

Hi Daniel,

On Fri, 25 Mar 2016 12:46:10 +0100 Daniel Lezcano wrote:

> On 03/24/2016 06:11 AM, Jisheng Zhang wrote:
> > Let's assume cpuidle_ops exists but it doesn't implement the according
> > init member, current arm_cpuidle_init() will return success to its
> > caller, but in fact it should return -EOPNOTSUPP.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   arch/arm/kernel/cpuidle.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> > index 703926e..f108d8f 100644
> > --- a/arch/arm/kernel/cpuidle.c
> > +++ b/arch/arm/kernel/cpuidle.c
> > @@ -143,8 +143,12 @@ int __init arm_cpuidle_init(int cpu)
> >   		return -ENODEV;
> >
> >   	ret = arm_cpuidle_read_ops(cpu_node, cpu);
> > -	if (!ret && cpuidle_ops[cpu].init)
> > -		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> > +	if (!ret) {
> > +		if (cpuidle_ops[cpu].init)
> > +			ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> > +		else
> > +			ret = -EOPNOTSUPP;
> > +	}  
> 
> Hi Jisheng,
> 
> this should be handled in the arm_cpuidle_read_ops function.
> 

Thanks for reviewing. After some consideration, I think this patch isn't correct
There may be platforms which doesn't need the init member at all, although
currently I don't see such platforms in mainline, So I'll drop this patch
and send out one v2 only does the optimization.

Thanks a lot,
Jisheng

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Daniel Lezcano]

On 03/30/2016 09:16 AM, Jisheng Zhang wrote:
> Hi Daniel,

[ ... ]

Added Lorenzo and Catalin.

>> Hi Jisheng,
>>
>> this should be handled in the arm_cpuidle_read_ops function.
>>
>
> Thanks for reviewing. After some consideration, I think this patch isn't correct
> There may be platforms which doesn't need the init member at all, although
> currently I don't see such platforms in mainline, So I'll drop this patch
> and send out one v2 only does the optimization.

There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the 
arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the 
init function is not there for cpuidle.

I don't think it is a problem, but as ARM/ARM64 are sharing the same 
cpuidle-arm.c driver it would make sense to unify the behavior between 
both archs.


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Jisheng Zhang]

On Wed, 30 Mar 2016 10:09:12 +0200 Daniel Lezcano wrote:

> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:
> > Hi Daniel,  
> 
> [ ... ]
> 
> Added Lorenzo and Catalin.
> 
> >> Hi Jisheng,
> >>
> >> this should be handled in the arm_cpuidle_read_ops function.
> >>  
> >
> > Thanks for reviewing. After some consideration, I think this patch isn't correct
> > There may be platforms which doesn't need the init member at all, although
> > currently I don't see such platforms in mainline, So I'll drop this patch
> > and send out one v2 only does the optimization.  
> 
> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the 
> arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the 
> init function is not there for cpuidle.

yes.
arm64's arm_cpuidle_init() returns -EOPNOTSUPP if init callback isn't defined

> 
> I don't think it is a problem, but as ARM/ARM64 are sharing the same 
> cpuidle-arm.c driver it would make sense to unify the behavior between 
> both archs.

yes, agree with you. From "unify" point of view, could I move back the suspend
callback check and init callback check into arm_cpuidle_init() for arm as V1 does?

Thanks for reviewing,
Jisheng

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Daniel Lezcano]

On 03/30/2016 10:17 AM, Jisheng Zhang wrote:
> On Wed, 30 Mar 2016 10:09:12 +0200 Daniel Lezcano wrote:
>
>> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:
>>> Hi Daniel,
>>
>> [ ... ]
>>
>> Added Lorenzo and Catalin.
>>
>>>> Hi Jisheng,
>>>>
>>>> this should be handled in the arm_cpuidle_read_ops function.
>>>>
>>>
>>> Thanks for reviewing. After some consideration, I think this patch isn't correct
>>> There may be platforms which doesn't need the init member at all, although
>>> currently I don't see such platforms in mainline, So I'll drop this patch
>>> and send out one v2 only does the optimization.
>>
>> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the
>> arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the
>> init function is not there for cpuidle.
>
> yes.
> arm64's arm_cpuidle_init() returns -EOPNOTSUPP if init callback isn't defined
>
>>
>> I don't think it is a problem, but as ARM/ARM64 are sharing the same
>> cpuidle-arm.c driver it would make sense to unify the behavior between
>> both archs.
>
> yes, agree with you. From "unify" point of view, could I move back the suspend
> callback check and init callback check into arm_cpuidle_init() for arm as V1 does?

Why ? To be consistent with ARM64 ?


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Jisheng Zhang]

On Wed, 30 Mar 2016 10:41:09 +0200 Daniel Lezcano wrote:

> On 03/30/2016 10:17 AM, Jisheng Zhang wrote:
> > On Wed, 30 Mar 2016 10:09:12 +0200 Daniel Lezcano wrote:
> >  
> >> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:  
> >>> Hi Daniel,  
> >>
> >> [ ... ]
> >>
> >> Added Lorenzo and Catalin.
> >>  
> >>>> Hi Jisheng,
> >>>>
> >>>> this should be handled in the arm_cpuidle_read_ops function.
> >>>>  
> >>>
> >>> Thanks for reviewing. After some consideration, I think this patch isn't correct
> >>> There may be platforms which doesn't need the init member at all, although
> >>> currently I don't see such platforms in mainline, So I'll drop this patch
> >>> and send out one v2 only does the optimization.  
> >>
> >> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the
> >> arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the
> >> init function is not there for cpuidle.  
> >
> > yes.
> > arm64's arm_cpuidle_init() returns -EOPNOTSUPP if init callback isn't defined
> >  
> >>
> >> I don't think it is a problem, but as ARM/ARM64 are sharing the same
> >> cpuidle-arm.c driver it would make sense to unify the behavior between
> >> both archs.  
> >
> > yes, agree with you. From "unify" point of view, could I move back the suspend
> > callback check and init callback check into arm_cpuidle_init() for arm as V1 does?  
> 
> Why ? To be consistent with ARM64 ?

Yes, that's my intention.

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Daniel Lezcano]

On 03/30/2016 10:43 AM, Jisheng Zhang wrote:
> On Wed, 30 Mar 2016 10:41:09 +0200 Daniel Lezcano wrote:
>
>> On 03/30/2016 10:17 AM, Jisheng Zhang wrote:
>>> On Wed, 30 Mar 2016 10:09:12 +0200 Daniel Lezcano wrote:
>>>
>>>> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:
>>>>> Hi Daniel,
>>>>
>>>> [ ... ]
>>>>
>>>> Added Lorenzo and Catalin.
>>>>
>>>>>> Hi Jisheng,
>>>>>>
>>>>>> this should be handled in the arm_cpuidle_read_ops function.
>>>>>>
>>>>>
>>>>> Thanks for reviewing. After some consideration, I think this patch isn't correct
>>>>> There may be platforms which doesn't need the init member at all, although
>>>>> currently I don't see such platforms in mainline, So I'll drop this patch
>>>>> and send out one v2 only does the optimization.
>>>>
>>>> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the
>>>> arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the
>>>> init function is not there for cpuidle.
>>>
>>> yes.
>>> arm64's arm_cpuidle_init() returns -EOPNOTSUPP if init callback isn't defined
>>>
>>>>
>>>> I don't think it is a problem, but as ARM/ARM64 are sharing the same
>>>> cpuidle-arm.c driver it would make sense to unify the behavior between
>>>> both archs.
>>>
>>> yes, agree with you. From "unify" point of view, could I move back the suspend
>>> callback check and init callback check into arm_cpuidle_init() for arm as V1 does?
>>
>> Why ? To be consistent with ARM64 ?
>
> Yes, that's my intention.

Well, I don't have a strong opinion on that. ARM64 cpu_ops is slightly 
different from cpuidle_ops as the cpu boot / hotplug operations are 
placed in a different place and that explains why on ARM64 we can have 
an successful 'get_ops' because we use the partially filled structure. 
On ARM, it is cpuidle_ops only, so we can gracefully fail if the ops are 
not defined.

IMO, it still make sense to keep the checks in arm_cpuidle_read_ops for ARM.


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Jisheng Zhang]

Hi Daniel,

On Wed, 30 Mar 2016 11:31:39 +0200 Daniel Lezcano wrote:

> On 03/30/2016 10:43 AM, Jisheng Zhang wrote:
> > On Wed, 30 Mar 2016 10:41:09 +0200 Daniel Lezcano wrote:
> >  
> >> On 03/30/2016 10:17 AM, Jisheng Zhang wrote:  
> >>> On Wed, 30 Mar 2016 10:09:12 +0200 Daniel Lezcano wrote:
> >>>  
> >>>> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:  
> >>>>> Hi Daniel,  
> >>>>
> >>>> [ ... ]
> >>>>
> >>>> Added Lorenzo and Catalin.
> >>>>  
> >>>>>> Hi Jisheng,
> >>>>>>
> >>>>>> this should be handled in the arm_cpuidle_read_ops function.
> >>>>>>  
> >>>>>
> >>>>> Thanks for reviewing. After some consideration, I think this patch isn't correct
> >>>>> There may be platforms which doesn't need the init member at all, although
> >>>>> currently I don't see such platforms in mainline, So I'll drop this patch
> >>>>> and send out one v2 only does the optimization.  
> >>>>
> >>>> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops', the
> >>>> arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP when the
> >>>> init function is not there for cpuidle.  
> >>>
> >>> yes.
> >>> arm64's arm_cpuidle_init() returns -EOPNOTSUPP if init callback isn't defined
> >>>  
> >>>>
> >>>> I don't think it is a problem, but as ARM/ARM64 are sharing the same
> >>>> cpuidle-arm.c driver it would make sense to unify the behavior between
> >>>> both archs.  
> >>>
> >>> yes, agree with you. From "unify" point of view, could I move back the suspend
> >>> callback check and init callback check into arm_cpuidle_init() for arm as V1 does?  
> >>
> >> Why ? To be consistent with ARM64 ?  
> >
> > Yes, that's my intention.  
> 
> Well, I don't have a strong opinion on that. ARM64 cpu_ops is slightly 
> different from cpuidle_ops as the cpu boot / hotplug operations are 
> placed in a different place and that explains why on ARM64 we can have 
> an successful 'get_ops' because we use the partially filled structure. 
> On ARM, it is cpuidle_ops only, so we can gracefully fail if the ops are 
> not defined.
> 
> IMO, it still make sense to keep the checks in arm_cpuidle_read_ops for ARM.
> 

Got your points. I'll send a v3 to add init check. These checks will be
in arm_cpuidle_read_ops.

Thanks,
Jisheng

[PATCH 1/2] ARM: cpuidle: fix !cpuidle_ops[cpu].init case during init

[Lorenzo Pieralisi]

On Wed, Mar 30, 2016 at 10:09:12AM +0200, Daniel Lezcano wrote:
> On 03/30/2016 09:16 AM, Jisheng Zhang wrote:
> >Hi Daniel,
> 
> [ ... ]
> 
> Added Lorenzo and Catalin.
> 
> >>Hi Jisheng,
> >>
> >>this should be handled in the arm_cpuidle_read_ops function.
> >>
> >
> >Thanks for reviewing. After some consideration, I think this patch isn't correct
> >There may be platforms which doesn't need the init member at all, although
> >currently I don't see such platforms in mainline, So I'll drop this patch
> >and send out one v2 only does the optimization.
> 
> There is an inconsistency between ARM and ARM64. The 'cpu_get_ops',
> the arm_cpuidle_read_ops from the ARM64 side, returns -EOPNOTSUPP
> when the init function is not there for cpuidle.
> 
> I don't think it is a problem, but as ARM/ARM64 are sharing the same
> cpuidle-arm.c driver it would make sense to unify the behavior
> between both archs.

I agree and I think it makes sense to have an arm back-end that fails
if there is no cpuidle_ops.init function registered, I doubt any
usage of the cpuidle_ops.suspend is reasonable if it was not
initialized by a corresponding cpuidle_ops.init at boot, at least
that's how I see it working, I am open to other point of views.

Thanks,
Lorenzo