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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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
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
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
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.
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
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
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