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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <340a6999-2281-2eaf-5a83-b1a0898a2ab5@linaro.org>
Date:   Tue, 30 Oct 2018 09:32:44 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 3/4] base/drivers/topology: Move instructions in the error
 path

On 30/10/2018 07:12, Viresh Kumar wrote:
> On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
> <daniel.lezcano@...aro.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@...aro.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