Received-SPF: pass (google.com: domain of linux-kernel+bounces-17145-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240104171553.2080674-1-lukasz.luba@arm.com> <20240104171553.2080674-8-lukasz.luba@arm.com>
In-Reply-To: <20240104171553.2080674-8-lukasz.luba@arm.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 4 Jan 2024 20:18:15 +0100
Message-ID: <CAJZ5v0jETcdATvjgULoGmr_g+cGA3=XWyCTjO1vkBZURCsGuLw@mail.gmail.com>
Subject: Re: [PATCH v6 07/23] PM: EM: Refactor how the EM table is allocated
 and populated
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, qyousef@layalina.io, 
	wvw@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.com> wr=
ote:
>
> 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, str=
uct em_perf_state *table,
>         return 0;
>  }
>
> +static int em_allocate_perf_table(struct em_perf_domain *pd,
> +                                 int nr_states)
> +{
> +       pd->table =3D 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_domai=
n *pd,
> -                               int nr_states, struct em_data_callback *c=
b,
> +                               struct em_perf_state *table,
> +                               struct em_data_callback *cb,
>                                 unsigned long flags)
>  {
>         unsigned long power, freq, prev_freq =3D 0;
> -       struct em_perf_state *table;
> +       int nr_states =3D pd->nr_perf_states;
>         int i, ret;
>
> -       table =3D kcalloc(nr_states, sizeof(*table), GFP_KERNEL);
> -       if (!table)
> -               return -ENOMEM;
> -
>         /* Build the list of performance states for this performance doma=
in */
>         for (i =3D 0, freq =3D 0; i < nr_states; i++, freq++) {
>                 /*
> @@ -165,7 +173,7 @@ static int em_create_perf_table(struct device *dev, s=
truct 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, s=
truct em_perf_domain *pd,
>                 if (freq <=3D 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, s=
truct 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 =3D power;
> @@ -194,16 +202,9 @@ static int em_create_perf_table(struct device *dev, =
struct em_perf_domain *pd,
>
>         ret =3D em_compute_costs(dev, table, cb, nr_states, flags);
>         if (ret)
> -               goto free_ps_table;
> -
> -       pd->table =3D table;
> -       pd->nr_perf_states =3D 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 =3D em_create_perf_table(dev, pd, nr_states, cb, flags);
> -       if (ret) {
> -               kfree(pd);
> -               return ret;
> -       }
> +       pd->nr_perf_states =3D nr_states;
> +
> +       ret =3D em_allocate_perf_table(pd, nr_states);
> +       if (ret)
> +               goto free_pd;
> +
> +       ret =3D 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_s=
tates,
>         dev->em_pd =3D pd;
>
>         return 0;
> +
> +free_pd_table:
> +       kfree(pd->table);
> +free_pd:
> +       kfree(pd);
> +       return -EINVAL;
>  }
>
>  static void
> --

The code changes LGTM.