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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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