| Message ID | 1540830201-2947-3-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] base/drivers/arch_topology: Remove useless check | expand |
On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > When the function topology_parse_cpu_capacity() fails, we set the boolean > cap_parsing_failed to true and we free the raw_capacity. This is correct as > the function begins with a check against cap_parsing_failed thus protecting > the function to be re-entered. > > However, even it is impossible that can happen with the current code, let's Why impossible ? > move in the instructions: > > - cap_parsing_failed = true; > - free_raw_capacity(); > > ... in the 'else' block when the error is detected, that is more semantically > correct. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/base/arch_topology.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index b19d6d4..7311641 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) > pr_err("cpu_capacity: missing %pOF raw capacity\n", > cpu_node); > pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n"); > + cap_parsing_failed = true; > + free_raw_capacity(); > } > - cap_parsing_failed = true; > - free_raw_capacity(); While it is fine to move free_raw_capacity(), it is not to move the other line. With your patch what will happen if the first CPU in DT doesn't have the "capacity-dmips-mhz" property set ? We will never set cap_parsing_failed and keep on re-entering this routine which wasn't required. Note that the current implementation isn't written to always print an error where this property is only partially filled and the same wouldn't happen with your patch as well. -- viresh
On 30/10/2018 07:12, Viresh Kumar wrote: > On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> When the function topology_parse_cpu_capacity() fails, we set the boolean >> cap_parsing_failed to true and we free the raw_capacity. This is correct as >> the function begins with a check against cap_parsing_failed thus protecting >> the function to be re-entered. >> >> However, even it is impossible that can happen with the current code, let's > > Why impossible ? >> move in the instructions: >> >> - cap_parsing_failed = true; >> - free_raw_capacity(); >> >> ... in the 'else' block when the error is detected, that is more semantically >> correct. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/base/arch_topology.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c >> index b19d6d4..7311641 100644 >> --- a/drivers/base/arch_topology.c >> +++ b/drivers/base/arch_topology.c >> @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) >> pr_err("cpu_capacity: missing %pOF raw capacity\n", >> cpu_node); >> pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n"); >> + cap_parsing_failed = true; >> + free_raw_capacity(); >> } >> - cap_parsing_failed = true; >> - free_raw_capacity(); > > While it is fine to move free_raw_capacity(), it is not to move the > other line. With your > patch what will happen if the first CPU in DT doesn't have the > "capacity-dmips-mhz" > property set ? We will never set cap_parsing_failed and keep on > re-entering this routine > which wasn't required. Ok. -- <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
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index b19d6d4..7311641 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) pr_err("cpu_capacity: missing %pOF raw capacity\n", cpu_node); pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n"); + cap_parsing_failed = true; + free_raw_capacity(); } - cap_parsing_failed = true; - free_raw_capacity(); } return !ret;
When the function topology_parse_cpu_capacity() fails, we set the boolean cap_parsing_failed to true and we free the raw_capacity. This is correct as the function begins with a check against cap_parsing_failed thus protecting the function to be re-entered. However, even it is impossible that can happen with the current code, let's move in the instructions: - cap_parsing_failed = true; - free_raw_capacity(); ... in the 'else' block when the error is detected, that is more semantically correct. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/base/arch_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.7.4