The changelog actually sets what happens here, so why don't you put
that into the changelog too? Something like: "Split the allocation
and initialization of the EM table"
On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@arm.com> 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 | 55 ++++++++++++++++++++++---------------
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 3c8542443dd4..e7826403ae1d 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -142,18 +142,26 @@ 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,
> - int nr_states, struct em_data_callback *cb,
> + struct em_perf_state *table,
> + struct em_data_callback *cb,
> unsigned long flags)
> {
> unsigned long power, freq, prev_freq = 0;
> - struct em_perf_state *table;
> + int nr_states = pd->nr_perf_states;
> 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 +173,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 +183,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 +193,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 +202,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;
> -
> - 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 +235,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, cb, flags);
> + if (ret)
> + goto free_pd_table;
>
> if (_is_cpu_device(dev))
> for_each_cpu(cpu, cpus) {
> @@ -249,6 +254,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
> --
The code changes LGTM.