[v5,1/4] thermal/netlink: Add the commands and the events for the thresholds

The thresholds exist but there is no notification neither action code
related to them yet.

These changes implement the netlink for the notifications when the
thresholds are crossed, added, deleted or flushed as well as the
commands which allows to get the list of the thresholds, flush them,
add and delete.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
 drivers/thermal/thermal_netlink.h    |  34 ++++
 drivers/thermal/thermal_thresholds.c |  36 ++--
 drivers/thermal/thermal_thresholds.h |   2 +-
 include/uapi/linux/thermal.h         |  27 ++-
 5 files changed, 307 insertions(+), 28 deletions(-)

On Mon, Oct 14, 2024 at 11:43 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The thresholds exist but there is no notification neither action code
> related to them yet.
>
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
>  drivers/thermal/thermal_netlink.h    |  34 ++++
>  drivers/thermal/thermal_thresholds.c |  36 ++--
>  drivers/thermal/thermal_thresholds.h |   2 +-
>  include/uapi/linux/thermal.h         |  27 ++-
>  5 files changed, 307 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> index f3c58c708969..978cd46c15e8 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/notifier.h>
>  #include <linux/kernel.h>
> +#include <net/sock.h>
>  #include <net/genetlink.h>
>  #include <uapi/linux/thermal.h>
>
> @@ -49,6 +50,11 @@ static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] =
>         [THERMAL_GENL_ATTR_CPU_CAPABILITY_ID]           = { .type = NLA_U32 },
>         [THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE]  = { .type = NLA_U32 },
>         [THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY]   = { .type = NLA_U32 },
> +
> +       /* Thresholds */
> +       [THERMAL_GENL_ATTR_THRESHOLD]           = { .type = NLA_NESTED },
> +       [THERMAL_GENL_ATTR_THRESHOLD_TEMP]      = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
>  };
>
>  struct param {
> @@ -62,6 +68,8 @@ struct param {
>         int trip_type;
>         int trip_hyst;
>         int temp;
> +       int prev_temp;
> +       int direction;
>         int cdev_state;
>         int cdev_max_state;
>         struct thermal_genl_cpu_caps *cpu_capabilities;
> @@ -234,6 +242,34 @@ static int thermal_genl_event_cpu_capability_change(struct param *p)
>         return -EMSGSIZE;
>  }
>
> +static int thermal_genl_event_threshold_add(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, p->temp) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, p->direction))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_threshold_flush(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_threshold_up(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_PREV_TEMP, p->prev_temp) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TEMP, p->temp))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
>  int thermal_genl_event_tz_delete(struct param *p)
>         __attribute__((alias("thermal_genl_event_tz")));
>
> @@ -246,6 +282,12 @@ int thermal_genl_event_tz_disable(struct param *p)
>  int thermal_genl_event_tz_trip_down(struct param *p)
>         __attribute__((alias("thermal_genl_event_tz_trip_up")));
>
> +int thermal_genl_event_threshold_delete(struct param *p)
> +       __attribute__((alias("thermal_genl_event_threshold_add")));
> +
> +int thermal_genl_event_threshold_down(struct param *p)
> +       __attribute__((alias("thermal_genl_event_threshold_up")));
> +
>  static cb_t event_cb[] = {
>         [THERMAL_GENL_EVENT_TZ_CREATE]          = thermal_genl_event_tz_create,
>         [THERMAL_GENL_EVENT_TZ_DELETE]          = thermal_genl_event_tz_delete,
> @@ -259,6 +301,11 @@ static cb_t event_cb[] = {
>         [THERMAL_GENL_EVENT_CDEV_STATE_UPDATE]  = thermal_genl_event_cdev_state_update,
>         [THERMAL_GENL_EVENT_TZ_GOV_CHANGE]      = thermal_genl_event_gov_change,
>         [THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE] = thermal_genl_event_cpu_capability_change,
> +       [THERMAL_GENL_EVENT_THRESHOLD_ADD]      = thermal_genl_event_threshold_add,
> +       [THERMAL_GENL_EVENT_THRESHOLD_DELETE]   = thermal_genl_event_threshold_delete,
> +       [THERMAL_GENL_EVENT_THRESHOLD_FLUSH]    = thermal_genl_event_threshold_flush,
> +       [THERMAL_GENL_EVENT_THRESHOLD_DOWN]     = thermal_genl_event_threshold_down,
> +       [THERMAL_GENL_EVENT_THRESHOLD_UP]       = thermal_genl_event_threshold_up,
>  };
>
>  /*
> @@ -401,6 +448,43 @@ int thermal_genl_cpu_capability_event(int count,
>  }
>  EXPORT_SYMBOL_GPL(thermal_genl_cpu_capability_event);
>
> +int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> +                                int temperature, int direction)
> +{
> +       struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_ADD, &p);
> +}
> +
> +int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> +                                   int temperature, int direction)
> +{
> +       struct param p = { .tz_id = tz->id, .temp = temperature, .direction = direction };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DELETE, &p);
> +}
> +
> +int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> +       struct param p = { .tz_id = tz->id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_FLUSH, &p);
> +}
> +
> +int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> +       struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_DOWN, &p);
> +}
> +
> +int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> +       struct param p = { .tz_id = tz->id, .temp = tz->temperature, .prev_temp = tz->last_temperature };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_THRESHOLD_UP, &p);
> +}
> +
>  /*************************** Command encoding ********************************/
>
>  static int __thermal_genl_cmd_tz_get_id(struct thermal_zone_device *tz,
> @@ -572,12 +656,132 @@ static int thermal_genl_cmd_cdev_get(struct param *p)
>         return ret;
>  }
>
> +static int __thermal_genl_cmd_threshold_get(struct user_threshold *threshold, void *arg)
> +{
> +       struct sk_buff *msg = arg;
> +
> +       if (nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_TEMP, threshold->temperature) ||
> +           nla_put_u32(msg, THERMAL_GENL_ATTR_THRESHOLD_DIRECTION, threshold->direction))
> +               return -1;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_cmd_threshold_get(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct nlattr *start_trip;
> +       int id, ret;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> +       CLASS(thermal_zone_get_by_id, tz)(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_THRESHOLD);
> +       if (!start_trip)
> +               return -EMSGSIZE;
> +
> +       ret = thermal_thresholds_for_each(tz, __thermal_genl_cmd_threshold_get, msg);
> +       if (ret)
> +               return -EMSGSIZE;
> +
> +       nla_nest_end(msg, start_trip);
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_cmd_threshold_add(struct param *p)
> +{
> +       int id, temp, direction, ret = 0;
> +
> +       if (!capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
> +           !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
> +           !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +       temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
> +       direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
> +
> +       CLASS(thermal_zone_get_by_id, tz)(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       mutex_lock(&tz->lock);
> +       ret = thermal_thresholds_add(tz, temp, direction);
> +       mutex_unlock(&tz->lock);
> +
> +       return ret;
> +}
> +
> +static int thermal_genl_cmd_threshold_delete(struct param *p)
> +{
> +       int id, temp, direction, ret = 0;
> +
> +       if (!capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID] ||
> +           !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP] ||
> +           !p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +       temp = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_TEMP]);
> +       direction = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_THRESHOLD_DIRECTION]);
> +
> +       CLASS(thermal_zone_get_by_id, tz)(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       mutex_lock(&tz->lock);
> +       ret = thermal_thresholds_delete(tz, temp, direction);
> +       mutex_unlock(&tz->lock);
> +
> +       return ret;
> +}
> +
> +static int thermal_genl_cmd_threshold_flush(struct param *p)
> +{
> +       int id;
> +
> +       if (!capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> +       CLASS(thermal_zone_get_by_id, tz)(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       mutex_lock(&tz->lock);
> +       thermal_thresholds_flush(tz);
> +       mutex_unlock(&tz->lock);
> +
> +       return 0;
> +}
> +
>  static cb_t cmd_cb[] = {
> -       [THERMAL_GENL_CMD_TZ_GET_ID]    = thermal_genl_cmd_tz_get_id,
> -       [THERMAL_GENL_CMD_TZ_GET_TRIP]  = thermal_genl_cmd_tz_get_trip,
> -       [THERMAL_GENL_CMD_TZ_GET_TEMP]  = thermal_genl_cmd_tz_get_temp,
> -       [THERMAL_GENL_CMD_TZ_GET_GOV]   = thermal_genl_cmd_tz_get_gov,
> -       [THERMAL_GENL_CMD_CDEV_GET]     = thermal_genl_cmd_cdev_get,
> +       [THERMAL_GENL_CMD_TZ_GET_ID]            = thermal_genl_cmd_tz_get_id,
> +       [THERMAL_GENL_CMD_TZ_GET_TRIP]          = thermal_genl_cmd_tz_get_trip,
> +       [THERMAL_GENL_CMD_TZ_GET_TEMP]          = thermal_genl_cmd_tz_get_temp,
> +       [THERMAL_GENL_CMD_TZ_GET_GOV]           = thermal_genl_cmd_tz_get_gov,
> +       [THERMAL_GENL_CMD_CDEV_GET]             = thermal_genl_cmd_cdev_get,
> +       [THERMAL_GENL_CMD_THRESHOLD_GET]        = thermal_genl_cmd_threshold_get,
> +       [THERMAL_GENL_CMD_THRESHOLD_ADD]        = thermal_genl_cmd_threshold_add,
> +       [THERMAL_GENL_CMD_THRESHOLD_DELETE]     = thermal_genl_cmd_threshold_delete,
> +       [THERMAL_GENL_CMD_THRESHOLD_FLUSH]      = thermal_genl_cmd_threshold_flush,
>  };
>
>  static int thermal_genl_cmd_dumpit(struct sk_buff *skb,
> @@ -688,6 +892,26 @@ static const struct genl_small_ops thermal_genl_ops[] = {
>                 .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>                 .dumpit = thermal_genl_cmd_dumpit,
>         },
> +       {
> +               .cmd = THERMAL_GENL_CMD_THRESHOLD_GET,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_THRESHOLD_ADD,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_THRESHOLD_DELETE,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_THRESHOLD_FLUSH,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
>  };
>
>  static struct genl_family thermal_genl_family __ro_after_init = {
> @@ -700,7 +924,7 @@ static struct genl_family thermal_genl_family __ro_after_init = {
>         .unbind         = thermal_genl_unbind,
>         .small_ops      = thermal_genl_ops,
>         .n_small_ops    = ARRAY_SIZE(thermal_genl_ops),
> -       .resv_start_op  = THERMAL_GENL_CMD_CDEV_GET + 1,
> +       .resv_start_op  = __THERMAL_GENL_CMD_MAX,
>         .mcgrps         = thermal_genl_mcgrps,
>         .n_mcgrps       = ARRAY_SIZE(thermal_genl_mcgrps),
>  };
> diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
> index e01221e8816b..075e9ae85f3d 100644
> --- a/drivers/thermal/thermal_netlink.h
> +++ b/drivers/thermal/thermal_netlink.h
> @@ -53,6 +53,13 @@ int thermal_notify_tz_gov_change(const struct thermal_zone_device *tz,
>  int thermal_genl_sampling_temp(int id, int temp);
>  int thermal_genl_cpu_capability_event(int count,
>                                       struct thermal_genl_cpu_caps *caps);
> +int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> +                                int temperature, int direction);
> +int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> +                                   int temperature, int direction);
> +int thermal_notify_threshold_flush(const struct thermal_zone_device *tz);
> +int thermal_notify_threshold_down(const struct thermal_zone_device *tz);
> +int thermal_notify_threshold_up(const struct thermal_zone_device *tz);
>  #else
>  static inline int thermal_netlink_init(void)
>  {
> @@ -139,6 +146,33 @@ static inline int thermal_genl_cpu_capability_event(int count, struct thermal_ge
>         return 0;
>  }
>
> +static inline int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> +                                              int temperature, int direction)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> +                                                 int temperature, int direction)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> +       return 0;
> +}
> +
> +static inline int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> +       return 0;
> +}
> +
>  static inline void __init thermal_netlink_exit(void) {}
>
>  #endif /* CONFIG_THERMAL_NETLINK */
> diff --git a/drivers/thermal/thermal_thresholds.c b/drivers/thermal/thermal_thresholds.c
> index f33b6d5474d8..ea4aa5a2e86c 100644
> --- a/drivers/thermal/thermal_thresholds.c
> +++ b/drivers/thermal/thermal_thresholds.c
> @@ -32,6 +32,8 @@ void thermal_thresholds_flush(struct thermal_zone_device *tz)
>                 kfree(entry);
>         }
>
> +       thermal_notify_threshold_flush(tz);
> +
>         __thermal_zone_device_update(tz, THERMAL_TZ_FLUSH_THRESHOLDS);
>  }
>
> @@ -122,7 +124,6 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
>
>         int temperature = tz->temperature;
>         int last_temperature = tz->last_temperature;
> -       bool notify;
>
>         lockdep_assert_held(&tz->lock);
>
> @@ -144,19 +145,19 @@ void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *hi
>          * - increased : thresholds are crossed the way up
>          * - decreased : thresholds are crossed the way down
>          */
> -       if (temperature > last_temperature)
> -               notify = thermal_thresholds_handle_raising(thresholds, temperature,
> -                                                          last_temperature, low, high);
> -       else
> -               notify = thermal_thresholds_handle_dropping(thresholds, temperature,
> -                                                           last_temperature, low, high);
> -
> -       if (notify)
> -               pr_debug("A threshold has been crossed the way %s, with a temperature=%d, last_temperature=%d\n",
> -                        temperature > last_temperature ? "up" : "down", temperature, last_temperature);
> +       if (temperature > last_temperature) {
> +               if (thermal_thresholds_handle_raising(thresholds, temperature,
> +                                                     last_temperature, low, high))
> +                       thermal_notify_threshold_up(tz);
> +       } else {
> +               if (thermal_thresholds_handle_dropping(thresholds, temperature,
> +                                                      last_temperature, low, high))
> +                       thermal_notify_threshold_down(tz);
> +       }
>  }
>
> -int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_add(struct thermal_zone_device *tz,
> +                          int temperature, int direction)
>  {
>         struct list_head *thresholds = &tz->user_thresholds;
>         struct user_threshold *t;
> @@ -182,12 +183,15 @@ int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int
>                 list_sort(NULL, thresholds, __thermal_thresholds_cmp);
>         }
>
> +       thermal_notify_threshold_add(tz, temperature, direction);
> +
>         __thermal_zone_device_update(tz, THERMAL_TZ_ADD_THRESHOLD);
>
>         return 0;
>  }
>
> -int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_delete(struct thermal_zone_device *tz,
> +                             int temperature, int direction)
>  {
>         struct list_head *thresholds = &tz->user_thresholds;
>         struct user_threshold *t;
> @@ -205,6 +209,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
>                 t->direction &= ~direction;
>         }
>
> +       thermal_notify_threshold_delete(tz, temperature, direction);
> +
>         __thermal_zone_device_update(tz, THERMAL_TZ_DEL_THRESHOLD);
>
>         return 0;
> @@ -217,7 +223,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
>         struct user_threshold *entry;
>         int ret;
>
> -       lockdep_assert_held(&tz->lock);
> +       mutex_lock(&tz->lock);
>
>         list_for_each_entry(entry, thresholds, list_node) {
>                 ret = cb(entry, arg);
> @@ -225,5 +231,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
>                         return ret;

The lock needs to be released before returning.

However, I would generally prefer this to go in after

https://lore.kernel.org/linux-pm/4985597.31r3eYUQgx@rjwysocki.net/

so I'll convert it to using guards before applying if you don't mind
and this issue will go away then.

Otherwise the patch looks good to me.

>         }
>
> +       mutex_unlock(&tz->lock);
> +
>         return 0;
>  }
> diff --git a/drivers/thermal/thermal_thresholds.h b/drivers/thermal/thermal_thresholds.h
> index 232f4e8089af..cb372659a20d 100644
> --- a/drivers/thermal/thermal_thresholds.h
> +++ b/drivers/thermal/thermal_thresholds.h
> @@ -10,8 +10,8 @@ struct user_threshold {
>
>  int thermal_thresholds_init(struct thermal_zone_device *tz);
>  void thermal_thresholds_exit(struct thermal_zone_device *tz);
> -void thermal_thresholds_flush(struct thermal_zone_device *tz);
>  void thermal_thresholds_handle(struct thermal_zone_device *tz, int *low, int *high);
> +void thermal_thresholds_flush(struct thermal_zone_device *tz);
>  int thermal_thresholds_add(struct thermal_zone_device *tz, int temperature, int direction);
>  int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction);
>  int thermal_thresholds_for_each(struct thermal_zone_device *tz,
> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> index 2e6f60a36173..ba8604bdf206 100644
> --- a/include/uapi/linux/thermal.h
> +++ b/include/uapi/linux/thermal.h
> @@ -20,7 +20,7 @@ enum thermal_trip_type {
>
>  /* Adding event notification support elements */
>  #define THERMAL_GENL_FAMILY_NAME               "thermal"
> -#define THERMAL_GENL_VERSION                   0x01
> +#define THERMAL_GENL_VERSION                   0x02
>  #define THERMAL_GENL_SAMPLING_GROUP_NAME       "sampling"
>  #define THERMAL_GENL_EVENT_GROUP_NAME          "event"
>
> @@ -30,6 +30,7 @@ enum thermal_genl_attr {
>         THERMAL_GENL_ATTR_TZ,
>         THERMAL_GENL_ATTR_TZ_ID,
>         THERMAL_GENL_ATTR_TZ_TEMP,
> +       THERMAL_GENL_ATTR_TZ_PREV_TEMP,
>         THERMAL_GENL_ATTR_TZ_TRIP,
>         THERMAL_GENL_ATTR_TZ_TRIP_ID,
>         THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
> @@ -50,6 +51,9 @@ enum thermal_genl_attr {
>         THERMAL_GENL_ATTR_CPU_CAPABILITY_ID,
>         THERMAL_GENL_ATTR_CPU_CAPABILITY_PERFORMANCE,
>         THERMAL_GENL_ATTR_CPU_CAPABILITY_EFFICIENCY,
> +       THERMAL_GENL_ATTR_THRESHOLD,
> +       THERMAL_GENL_ATTR_THRESHOLD_TEMP,
> +       THERMAL_GENL_ATTR_THRESHOLD_DIRECTION,
>         __THERMAL_GENL_ATTR_MAX,
>  };
>  #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> @@ -77,6 +81,11 @@ enum thermal_genl_event {
>         THERMAL_GENL_EVENT_CDEV_STATE_UPDATE,   /* Cdev state updated */
>         THERMAL_GENL_EVENT_TZ_GOV_CHANGE,       /* Governor policy changed  */
>         THERMAL_GENL_EVENT_CPU_CAPABILITY_CHANGE,       /* CPU capability changed */
> +       THERMAL_GENL_EVENT_THRESHOLD_ADD,       /* A thresold has been added */
> +       THERMAL_GENL_EVENT_THRESHOLD_DELETE,    /* A thresold has been deleted */
> +       THERMAL_GENL_EVENT_THRESHOLD_FLUSH,     /* All thresolds have been deleted */
> +       THERMAL_GENL_EVENT_THRESHOLD_UP,        /* A thresold has been crossed the way up */
> +       THERMAL_GENL_EVENT_THRESHOLD_DOWN,      /* A thresold has been crossed the way down */
>         __THERMAL_GENL_EVENT_MAX,
>  };
>  #define THERMAL_GENL_EVENT_MAX (__THERMAL_GENL_EVENT_MAX - 1)
> @@ -84,12 +93,16 @@ enum thermal_genl_event {
>  /* Commands supported by the thermal_genl_family */
>  enum thermal_genl_cmd {
>         THERMAL_GENL_CMD_UNSPEC,
> -       THERMAL_GENL_CMD_TZ_GET_ID,     /* List of thermal zones id */
> -       THERMAL_GENL_CMD_TZ_GET_TRIP,   /* List of thermal trips */
> -       THERMAL_GENL_CMD_TZ_GET_TEMP,   /* Get the thermal zone temperature */
> -       THERMAL_GENL_CMD_TZ_GET_GOV,    /* Get the thermal zone governor */
> -       THERMAL_GENL_CMD_TZ_GET_MODE,   /* Get the thermal zone mode */
> -       THERMAL_GENL_CMD_CDEV_GET,      /* List of cdev id */
> +       THERMAL_GENL_CMD_TZ_GET_ID,             /* List of thermal zones id */
> +       THERMAL_GENL_CMD_TZ_GET_TRIP,           /* List of thermal trips */
> +       THERMAL_GENL_CMD_TZ_GET_TEMP,           /* Get the thermal zone temperature */
> +       THERMAL_GENL_CMD_TZ_GET_GOV,            /* Get the thermal zone governor */
> +       THERMAL_GENL_CMD_TZ_GET_MODE,           /* Get the thermal zone mode */
> +       THERMAL_GENL_CMD_CDEV_GET,              /* List of cdev id */
> +       THERMAL_GENL_CMD_THRESHOLD_GET,         /* List of thresholds */
> +       THERMAL_GENL_CMD_THRESHOLD_ADD,         /* Add a threshold */
> +       THERMAL_GENL_CMD_THRESHOLD_DELETE,      /* Delete a threshold */
> +       THERMAL_GENL_CMD_THRESHOLD_FLUSH,       /* Flush all the thresholds */
>         __THERMAL_GENL_CMD_MAX,
>  };
>  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> --
> 2.43.0
>

Hi Daniel,

On 10/14/24 10:43, Daniel Lezcano wrote:
> The thresholds exist but there is no notification neither action code
> related to them yet.
> 
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
>   drivers/thermal/thermal_netlink.h    |  34 ++++
>   drivers/thermal/thermal_thresholds.c |  36 ++--
>   drivers/thermal/thermal_thresholds.h |   2 +-
>   include/uapi/linux/thermal.h         |  27 ++-
>   5 files changed, 307 insertions(+), 28 deletions(-)
> 

Since Rafael asked to wait a bit for the other
changes to go first, I will skip the detailed review
of that patch. I will do that after you add that
new locking scheme.

In general the code looks good.

Regards,
Lukasz

On Mon, Oct 21, 2024 at 9:41 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel,
>
> On 10/14/24 10:43, Daniel Lezcano wrote:
> > The thresholds exist but there is no notification neither action code
> > related to them yet.
> >
> > These changes implement the netlink for the notifications when the
> > thresholds are crossed, added, deleted or flushed as well as the
> > commands which allows to get the list of the thresholds, flush them,
> > add and delete.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >   drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
> >   drivers/thermal/thermal_netlink.h    |  34 ++++
> >   drivers/thermal/thermal_thresholds.c |  36 ++--
> >   drivers/thermal/thermal_thresholds.h |   2 +-
> >   include/uapi/linux/thermal.h         |  27 ++-
> >   5 files changed, 307 insertions(+), 28 deletions(-)
> >
>
> Since Rafael asked to wait a bit for the other
> changes to go first, I will skip the detailed review
> of that patch. I will do that after you add that
> new locking scheme.

Well, this is almost orthogonal to my locking changes and can be
easily rebased on top of them, so I don't see a need to respin it.

Please review.

> In general the code looks good.

OK

On 10/21/24 20:47, Rafael J. Wysocki wrote:
> On Mon, Oct 21, 2024 at 9:41 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Daniel,
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thresholds exist but there is no notification neither action code
>>> related to them yet.
>>>
>>> These changes implement the netlink for the notifications when the
>>> thresholds are crossed, added, deleted or flushed as well as the
>>> commands which allows to get the list of the thresholds, flush them,
>>> add and delete.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>    drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
>>>    drivers/thermal/thermal_netlink.h    |  34 ++++
>>>    drivers/thermal/thermal_thresholds.c |  36 ++--
>>>    drivers/thermal/thermal_thresholds.h |   2 +-
>>>    include/uapi/linux/thermal.h         |  27 ++-
>>>    5 files changed, 307 insertions(+), 28 deletions(-)
>>>
>>
>> Since Rafael asked to wait a bit for the other
>> changes to go first, I will skip the detailed review
>> of that patch. I will do that after you add that
>> new locking scheme.
> 
> Well, this is almost orthogonal to my locking changes and can be
> easily rebased on top of them, so I don't see a need to respin it.
> 
> Please review.

OK, I will then.
I have an issue with the build process (cross compiling the libthermal)
but I will manage that differently for testing. I suspect some memory
leak in patch 2/4, so testing would help me...

> 
>> In general the code looks good.
> 
> OK

On 10/14/24 10:43, Daniel Lezcano wrote:
> The thresholds exist but there is no notification neither action code
> related to them yet.
> 
> These changes implement the netlink for the notifications when the
> thresholds are crossed, added, deleted or flushed as well as the
> commands which allows to get the list of the thresholds, flush them,
> add and delete.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/thermal/thermal_netlink.c    | 236 ++++++++++++++++++++++++++-
>   drivers/thermal/thermal_netlink.h    |  34 ++++
>   drivers/thermal/thermal_thresholds.c |  36 ++--
>   drivers/thermal/thermal_thresholds.h |   2 +-
>   include/uapi/linux/thermal.h         |  27 ++-
>   5 files changed, 307 insertions(+), 28 deletions(-)
> 

[snip]

>   
> +static inline int thermal_notify_threshold_add(const struct thermal_zone_device *tz,
> +					       int temperature, int direction)
> +{
> +	return 0;
> +}
> +
> +static inline int thermal_notify_threshold_delete(const struct thermal_zone_device *tz,
> +						  int temperature, int direction)
> +{
> +	return 0;
> +}
> +
> +static inline int thermal_notify_threshold_flush(const struct thermal_zone_device *tz)
> +{
> +	return 0;
> +}
> +
> +static inline int thermal_notify_threshold_down(const struct thermal_zone_device *tz)
> +{
> +	return 0;
> +}
> +
> +static inline int thermal_notify_threshold_up(const struct thermal_zone_device *tz)
> +{
> +	return 0;
> +}

These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
not defined. Although, since we don't check the output of those
functions  - we are OK. We just have to remember about these zeros,
one day when we would like to add the check of the return.

> +
>   static inline void __init thermal_netlink_exit(void) {}
>   
>   #endif /* CONFIG_THERMAL_NETLINK */

>   
> -int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, int direction)
> +int thermal_thresholds_delete(struct thermal_zone_device *tz,
> +			      int temperature, int direction)
>   {
>   	struct list_head *thresholds = &tz->user_thresholds;
>   	struct user_threshold *t;
> @@ -205,6 +209,8 @@ int thermal_thresholds_delete(struct thermal_zone_device *tz, int temperature, i
>   		t->direction &= ~direction;
>   	}
>   
> +	thermal_notify_threshold_delete(tz, temperature, direction);
> +
>   	__thermal_zone_device_update(tz, THERMAL_TZ_DEL_THRESHOLD);
>   
>   	return 0;
> @@ -217,7 +223,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
>   	struct user_threshold *entry;
>   	int ret;
>   
> -	lockdep_assert_held(&tz->lock);
> +	mutex_lock(&tz->lock);
>   
>   	list_for_each_entry(entry, thresholds, list_node) {
>   		ret = cb(entry, arg);
> @@ -225,5 +231,7 @@ int thermal_thresholds_for_each(struct thermal_zone_device *tz,
>   			return ret;

I agree with Rafael here. The lock should be released before return.

The rest looks good.

Hi Lukasz,

On Tue, Oct 22, 2024 at 11:39 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel,
>
> On 10/22/24 08:09, Daniel Lezcano wrote:
> > On 22/10/2024 00:02, Lukasz Luba wrote:
> >>
> >>
> >> On 10/14/24 10:43, Daniel Lezcano wrote:
> >>> The thresholds exist but there is no notification neither action code
> >>> related to them yet.
> >>>
> >>> These changes implement the netlink for the notifications when the
> >>> thresholds are crossed, added, deleted or flushed as well as the
> >>> commands which allows to get the list of the thresholds, flush them,
> >>> add and delete.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> ---
> >
> > [ ... ]
> >
> >>> +static inline int thermal_notify_threshold_up(const struct
> >>> thermal_zone_device *tz)
> >>> +{
> >>> +    return 0;
> >>> +}
> >>
> >> These 'return 0' look a bit odd. We usually use 'return -EINVAL' in
> >> not defined. Although, since we don't check the output of those
> >> functions  - we are OK. We just have to remember about these zeros,
> >> one day when we would like to add the check of the return.
> >
> > The return error really depends on the context of the call site. There
> > are other subsystems returning 0 when the service is not enabled (eg.
> > cpufreq.h, devfreq.h, device_cgroup.h, etc ...)
> >
> >
>
> Fair enough. As I said, we would just keep them in mind if we one
> day decide to add the checks of the returns.
>
> I'm waiting for your next version with the new locking scheme that
> Rafael asked and I will add my review tags.

My plan was to take this patch and replace the open-coded locking with
guards when applying it (that would be a mechanical change AFAICS).

I guess it's OK to add a R-by from you in that case?