[3/4] base/drivers/topology: Move instructions in the error path

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
Related show

Commit Message

Daniel Lezcano Oct. 29, 2018, 4:23 p.m.
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

Comments

Viresh Kumar Oct. 30, 2018, 6:12 a.m. | #1
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
Daniel Lezcano Oct. 30, 2018, 8:32 a.m. | #2
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

Patch

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;