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

Lukasz Luba
2024-10-21T22:01:02Z

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.