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

[Lukasz Luba <lukasz.luba@arm.com>]

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.