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

[Viresh Kumar]

On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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