| Message ID | 1458796269-6158-2-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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 ?
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.
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
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);
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(-)