linux-kernel - Re: [PATCH v4 08/18] PM: EM: Add update_power() callback for runtime modifications

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hM3DvEZSyDubd29bkEPDv6JUm8FaKM8+suaWOpKESp+w@mail.gmail.com>
Date:   Tue, 26 Sep 2023 20:59:32 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        rafael@...nel.org, dietmar.eggemann@....com, rui.zhang@...el.com,
        amit.kucheria@...durent.com, amit.kachhap@...il.com,
        daniel.lezcano@...aro.org, viresh.kumar@...aro.org,
        len.brown@...el.com, pavel@....cz, mhiramat@...nel.org,
        qyousef@...alina.io, wvw@...gle.com
Subject: Re: [PATCH v4 08/18] PM: EM: Add update_power() callback for runtime modifications

On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@....com> wrote:
>
> The Energy Model (EM) is going to support runtime modifications. This
> new callback would be used in the upcoming EM changes. The drivers
> or frameworks which want to modify the EM have to implement the
> update_power() callback.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
> ---
>  include/linux/energy_model.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index d236e08e80dc..546dee90f716 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -168,6 +168,26 @@ struct em_data_callback {
>          */
>         int (*get_cost)(struct device *dev, unsigned long freq,
>                         unsigned long *cost);
> +
> +       /**
> +        * update_power() - Provide new power at the given performance state of
> +        *              a device

The meaning of the above is unclear to me.

> +        * @dev         : Device for which we do this operation (can be a CPU)

It is unclear what "we" means in this context.  Maybe say "Target
device (can be a CPU)"?

> +        * @freq        : Frequency at the performance state in kHz

What performance state does this refer to?  And the frequency of what?

> +        * @power       : New power value at the performance state
> +        *              (modified)

Similarly unclear as the above.

> +        * @priv        : Pointer to private data useful for tracking context
> +        *              during runtime modifications of EM.

Who's going to set this pointer and use this data?

> +        *
> +        * The update_power() is used by runtime modifiable EM. It aims to

I would drop "The" from the above.

> +        * provide updated power value for a given frequency, which is stored
> +        * in the performance state.

A given frequency of what and the performance state of what does this refer to?

> + The power value provided by this callback
> +        * should fit in the [0, EM_MAX_POWER] range.
> +        *
> +        * Return 0 on success, or appropriate error value in case of failure.
> +        */
> +       int (*update_power)(struct device *dev, unsigned long freq,
> +                           unsigned long *power, void *priv);
>  };
>  #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) ((em_cb).active_power = cb)
>  #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb)     \
> @@ -175,6 +195,7 @@ struct em_data_callback {
>           .get_cost = _cost_cb }
>  #define EM_DATA_CB(_active_power_cb)                   \
>                 EM_ADV_DATA_CB(_active_power_cb, NULL)
> +#define EM_UPDATE_CB(_update_power_cb) { .update_power = &_update_power_cb }
>
>  struct em_perf_domain *em_cpu_get(int cpu);
>  struct em_perf_domain *em_pd_get(struct device *dev);
> @@ -331,6 +352,7 @@ struct em_data_callback {};
>  #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb) { }
>  #define EM_DATA_CB(_active_power_cb) { }
>  #define EM_SET_ACTIVE_POWER_CB(em_cb, cb) do { } while (0)
> +#define EM_UPDATE_CB(_update_cb) { }
>
>  static inline
>  int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
> --