Received-SPF: pass (google.com: domain of linux-kernel+bounces-2763-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 17 Dec 2023 17:59:04 +0000
From: Qais Yousef <qyousef@layalina.io>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	rafael@kernel.org, dietmar.eggemann@arm.com, rui.zhang@intel.com,
	amit.kucheria@verdurent.com, amit.kachhap@gmail.com,
	daniel.lezcano@linaro.org, viresh.kumar@linaro.org,
	len.brown@intel.com, pavel@ucw.cz, mhiramat@kernel.org,
	wvw@google.com
Subject: Re: [PATCH v5 07/23] PM: EM: Refactor how the EM table is allocated
 and populated
Message-ID: <20231217175904.v3aaoqyp3n7pqyph@airbuntu>
References: <20231129110853.94344-1-lukasz.luba@arm.com>
 <20231129110853.94344-8-lukasz.luba@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231129110853.94344-8-lukasz.luba@arm.com>

On 11/29/23 11:08, Lukasz Luba wrote:
> Split the process of allocation and data initialization for the EM table.
> The upcoming changes for modifiable EM will use it.
> 
> This change is not expected to alter the general functionality.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  kernel/power/energy_model.c | 52 ++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 3c8542443dd4..99426b5eedb6 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -142,18 +142,25 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>  	return 0;
>  }
>  
> +static int em_allocate_perf_table(struct em_perf_domain *pd,
> +				  int nr_states)
> +{
> +	pd->table = kcalloc(nr_states, sizeof(struct em_perf_state),
> +			    GFP_KERNEL);
> +	if (!pd->table)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
> +				struct em_perf_state *table,
>  				int nr_states, struct em_data_callback *cb,
>  				unsigned long flags)
>  {
>  	unsigned long power, freq, prev_freq = 0;
> -	struct em_perf_state *table;
>  	int i, ret;
>  
> -	table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL);
> -	if (!table)
> -		return -ENOMEM;
> -
>  	/* Build the list of performance states for this performance domain */
>  	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
>  		/*
> @@ -165,7 +172,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  		if (ret) {
>  			dev_err(dev, "EM: invalid perf. state: %d\n",
>  				ret);
> -			goto free_ps_table;
> +			return -EINVAL;
>  		}
>  
>  		/*
> @@ -175,7 +182,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  		if (freq <= prev_freq) {
>  			dev_err(dev, "EM: non-increasing freq: %lu\n",
>  				freq);
> -			goto free_ps_table;
> +			return -EINVAL;
>  		}
>  
>  		/*
> @@ -185,7 +192,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  		if (!power || power > EM_MAX_POWER) {
>  			dev_err(dev, "EM: invalid power: %lu\n",
>  				power);
> -			goto free_ps_table;
> +			return -EINVAL;
>  		}
>  
>  		table[i].power = power;
> @@ -194,16 +201,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>  
>  	ret = em_compute_costs(dev, table, cb, nr_states, flags);
>  	if (ret)
> -		goto free_ps_table;

We don't care about propagating the error number here stored in ret?

> -
> -	pd->table = table;
> -	pd->nr_perf_states = nr_states;
> +		return -EINVAL;
>  
>  	return 0;
> -
> -free_ps_table:
> -	kfree(table);
> -	return -EINVAL;
>  }
>  
>  static int em_create_pd(struct device *dev, int nr_states,
> @@ -234,11 +234,15 @@ static int em_create_pd(struct device *dev, int nr_states,
>  			return -ENOMEM;
>  	}
>  
> -	ret = em_create_perf_table(dev, pd, nr_states, cb, flags);
> -	if (ret) {
> -		kfree(pd);
> -		return ret;
> -	}
> +	pd->nr_perf_states = nr_states;
> +
> +	ret = em_allocate_perf_table(pd, nr_states);
> +	if (ret)
> +		goto free_pd;
> +
> +	ret = em_create_perf_table(dev, pd, pd->table, nr_states, cb, flags);
> +	if (ret)
> +		goto free_pd_table;

Ditto for all the above


Cheers

--
Qais Yousef

>  
>  	if (_is_cpu_device(dev))
>  		for_each_cpu(cpu, cpus) {
> @@ -249,6 +253,12 @@ static int em_create_pd(struct device *dev, int nr_states,
>  	dev->em_pd = pd;
>  
>  	return 0;
> +
> +free_pd_table:
> +	kfree(pd->table);
> +free_pd:
> +	kfree(pd);
> +	return -EINVAL;
>  }
>  
>  static void
> -- 
> 2.25.1
>