linux-kernel - Re: [PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init

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: <02a96f2c-dde1-73be-683d-b51d8c6b1b03@linaro.org>
Date:   Thu, 28 Feb 2019 15:42:53 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Sudeep Holla <sudeep.holla@....com>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
        Mark Rutland <mark.rutland@....com>
Cc:     Lina Iyer <ilina@...eaurora.org>, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] drivers: firmware: psci: Split
 psci_dt_cpu_init_idle()

On 28/02/2019 14:59, Ulf Hansson wrote:
> Let's split the psci_dt_cpu_init_idle() function into two functions. This
> makes the code clearer and provides better re-usability.
> 
> Cc: Lina Iyer <ilina@...eaurora.org>
> Co-developed-by: Lina Iyer <lina.iyer@...aro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@...aro.org>

but one question below.


> ---
>  drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index c80ec1d03274..9788bfc1cf8b 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
>  #ifdef CONFIG_CPU_IDLE
>  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
>  
> +static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> +{
> +	int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
> +
> +	if (err) {
> +		pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
> +		return err;
> +	}
> +
> +	if (!psci_power_state_is_valid(*state)) {
> +		pr_warn("Invalid PSCI power state %#x\n", *state);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  {
> -	int i, ret, count = 0;
> +	int i, ret = 0, count = 0;

Why do you need to intialize the ret variable? If the count is zero we
go directly to return 0, otherwise we enter in the loop and ret is
affected with the new function call.

>  	u32 *psci_states;
>  	struct device_node *state_node;
>  
> @@ -291,29 +308,16 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < count; i++) {
> -		u32 state;
> -
>  		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +		ret = psci_dt_parse_state_node(state_node, &psci_states[i]);
> +		of_node_put(state_node);
>  
> -		ret = of_property_read_u32(state_node,
> -					   "arm,psci-suspend-param",
> -					   &state);
> -		if (ret) {
> -			pr_warn(" * %pOF missing arm,psci-suspend-param property\n",
> -				state_node);
> -			of_node_put(state_node);
> +		if (ret)
>  			goto free_mem;
> -		}
>  
> -		of_node_put(state_node);
> -		pr_debug("psci-power-state %#x index %d\n", state, i);
> -		if (!psci_power_state_is_valid(state)) {
> -			pr_warn("Invalid PSCI power state %#x\n", state);
> -			ret = -EINVAL;
> -			goto free_mem;
> -		}
> -		psci_states[i] = state;
> +		pr_debug("psci-power-state %#x index %d\n", psci_states[i], i);
>  	}
> +
>  	/* Idle states parsed correctly, initialize per-cpu pointer */
>  	per_cpu(psci_power_state, cpu) = psci_states;
>  	return 0;
> 


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