[PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Daniel Lezcano]

The cdev, tz and governor list, as well as their respective locks are
statically defined in the thermal_core.c file.

In order to give a sane access to these list, like browsing all the
thermal zones or all the cooling devices, let's define a set of
helpers where we pass a callback as a parameter to be called for each
thermal entity.

We keep the self-encapsulation and ensure the locks are correctly
taken when looking at the list.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 51 ++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_core.h |  9 ++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 2a3f83265d8b..e2f8d2550ecd 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -611,6 +611,57 @@ void thermal_zone_device_rebind_exception(struct thermal_zone_device *tz,
 	mutex_unlock(&thermal_list_lock);
 }
 
+int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
+			      void *data)
+{
+	struct thermal_governor *gov;
+	int ret = 0;
+
+	mutex_lock(&thermal_governor_lock);
+	list_for_each_entry(gov, &thermal_governor_list, governor_list) {
+		ret = cb(gov, data);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&thermal_governor_lock);
+
+	return ret;
+}
+
+int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
+					      void *), void *data)
+{
+	struct thermal_cooling_device *cdev;
+	int ret = 0;
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(cdev, &thermal_cdev_list, node) {
+		ret = cb(cdev, data);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&thermal_list_lock);
+
+	return ret;
+}
+
+int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
+			  void *data)
+{
+	struct thermal_zone_device *tz;
+	int ret = 0;
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(tz, &thermal_tz_list, node) {
+		ret = cb(tz, data);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&thermal_list_lock);
+
+	return ret;
+}
+
 void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
 					  const char *cdev_type, size_t size)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 4e271016b7a9..bb8f8aee79eb 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -41,6 +41,15 @@ extern struct thermal_governor *__governor_thermal_table_end[];
 	     __governor < __governor_thermal_table_end;	\
 	     __governor++)
 
+int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
+			  void *);
+
+int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
+					      void *), void *);
+
+int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
+			      void *thermal_governor);
+
 struct thermal_attr {
 	struct device_attribute attr;
 	char name[THERMAL_NAME_LENGTH];
-- 
2.17.1

[PATCH v2 2/5] thermal: core: Get thermal zone by id

[Daniel Lezcano]

The next patch will introduce the generic netlink protocol to handle
events, sampling and command from the thermal framework. In order to
deal with the thermal zone, it uses its unique identifier to
characterize it in the message. Passing an integer is more efficient
than passing an entire string.

This change provides a function returning back a thermal zone pointer
corresponding to the identifier passed as parameter.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 14 ++++++++++++++
 drivers/thermal/thermal_core.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index e2f8d2550ecd..58c95aeafb7f 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -662,6 +662,20 @@ int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
 	return ret;
 }
 
+struct thermal_zone_device *thermal_zone_get_by_id(int id)
+{
+	struct thermal_zone_device *tz = NULL;
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(tz, &thermal_tz_list, node) {
+		if (tz->id == id)
+			break;
+	}
+	mutex_unlock(&thermal_list_lock);
+
+	return tz;
+}
+
 void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
 					  const char *cdev_type, size_t size)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index bb8f8aee79eb..7e8f45db6bbf 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -50,6 +50,8 @@ int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
 int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
 			      void *thermal_governor);
 
+struct thermal_zone_device *thermal_zone_get_by_id(int id);
+
 struct thermal_attr {
 	struct device_attribute attr;
 	char name[THERMAL_NAME_LENGTH];
-- 
2.17.1

Re: [PATCH v2 2/5] thermal: core: Get thermal zone by id

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The next patch will introduce the generic netlink protocol to handle
> events, sampling and command from the thermal framework. In order to
> deal with the thermal zone, it uses its unique identifier to
> characterize it in the message. Passing an integer is more efficient
> than passing an entire string.
>
> This change provides a function returning back a thermal zone pointer
> corresponding to the identifier passed as parameter.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>

> ---
>  drivers/thermal/thermal_core.c | 14 ++++++++++++++
>  drivers/thermal/thermal_core.h |  2 ++
>  2 files changed, 16 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index e2f8d2550ecd..58c95aeafb7f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -662,6 +662,20 @@ int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
>         return ret;
>  }
>
> +struct thermal_zone_device *thermal_zone_get_by_id(int id)
> +{
> +       struct thermal_zone_device *tz = NULL;
> +
> +       mutex_lock(&thermal_list_lock);
> +       list_for_each_entry(tz, &thermal_tz_list, node) {
> +               if (tz->id == id)
> +                       break;
> +       }
> +       mutex_unlock(&thermal_list_lock);
> +
> +       return tz;
> +}
> +
>  void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
>                                           const char *cdev_type, size_t size)
>  {
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index bb8f8aee79eb..7e8f45db6bbf 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -50,6 +50,8 @@ int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
>  int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
>                               void *thermal_governor);
>
> +struct thermal_zone_device *thermal_zone_get_by_id(int id);
> +
>  struct thermal_attr {
>         struct device_attribute attr;
>         char name[THERMAL_NAME_LENGTH];
> --
> 2.17.1
>

Re: [PATCH v2 2/5] thermal: core: Get thermal zone by id

[Zhang Rui]

On Thu, 2020-06-25 at 16:45 +0200, Daniel Lezcano wrote:
> The next patch will introduce the generic netlink protocol to handle
> events, sampling and command from the thermal framework. In order to
> deal with the thermal zone, it uses its unique identifier to
> characterize it in the message. Passing an integer is more efficient
> than passing an entire string.
> 
> This change provides a function returning back a thermal zone pointer
> corresponding to the identifier passed as parameter.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 14 ++++++++++++++
>  drivers/thermal/thermal_core.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index e2f8d2550ecd..58c95aeafb7f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -662,6 +662,20 @@ int for_each_thermal_zone(int (*cb)(struct
> thermal_zone_device *, void *),
>  	return ret;
>  }
>  
> +struct thermal_zone_device *thermal_zone_get_by_id(int id)
> +{
> +	struct thermal_zone_device *tz = NULL;
> +
> +	mutex_lock(&thermal_list_lock);
> +	list_for_each_entry(tz, &thermal_tz_list, node) {
> +		if (tz->id == id)
> +			break;
> +	}
> +	mutex_unlock(&thermal_list_lock);
> +
> +	return tz;
> +}
> +

IMO, this one, as well as thermal_zone_get_zone_by_name(), should all
be in thermal_helper.c

thanks,
rui
>  void thermal_zone_device_unbind_exception(struct thermal_zone_device
> *tz,
>  					  const char *cdev_type, size_t
> size)
>  {
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index bb8f8aee79eb..7e8f45db6bbf 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -50,6 +50,8 @@ int for_each_thermal_cooling_device(int
> (*cb)(struct thermal_cooling_device *,
>  int for_each_thermal_governor(int (*cb)(struct thermal_governor *,
> void *),
>  			      void *thermal_governor);
>  
> +struct thermal_zone_device *thermal_zone_get_by_id(int id);
> +
>  struct thermal_attr {
>  	struct device_attribute attr;
>  	char name[THERMAL_NAME_LENGTH];

[PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Daniel Lezcano]

In order to set the scene for the new generic netlink thermal
management and notifications, let remove the old dead code remaining
in the uapi headers.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/thermal.h      |  5 -----
 include/uapi/linux/thermal.h | 12 +-----------
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index faf7ad031e42..fc93a6348082 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -302,11 +302,6 @@ struct thermal_zone_params {
 	int offset;
 };
 
-struct thermal_genl_event {
-	u32 orig;
-	enum events event;
-};
-
 /**
  * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
  *
diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
index 96218378dda8..22df67ed9e9c 100644
--- a/include/uapi/linux/thermal.h
+++ b/include/uapi/linux/thermal.h
@@ -6,21 +6,12 @@
 
 /* Adding event notification support elements */
 #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
-#define THERMAL_GENL_VERSION                    0x01
+#define THERMAL_GENL_VERSION                    0x02
 #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_grp"
 
-/* Events supported by Thermal Netlink */
-enum events {
-	THERMAL_AUX0,
-	THERMAL_AUX1,
-	THERMAL_CRITICAL,
-	THERMAL_DEV_FAULT,
-};
-
 /* attributes of thermal_genl_family */
 enum {
 	THERMAL_GENL_ATTR_UNSPEC,
-	THERMAL_GENL_ATTR_EVENT,
 	__THERMAL_GENL_ATTR_MAX,
 };
 #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
@@ -28,7 +19,6 @@ enum {
 /* commands supported by the thermal_genl_family */
 enum {
 	THERMAL_GENL_CMD_UNSPEC,
-	THERMAL_GENL_CMD_EVENT,
 	__THERMAL_GENL_CMD_MAX,
 };
 #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
-- 
2.17.1

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> In order to set the scene for the new generic netlink thermal
> management and notifications, let remove the old dead code remaining

s/management/management api/

s/let/let's/

> in the uapi headers.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  include/linux/thermal.h      |  5 -----
>  include/uapi/linux/thermal.h | 12 +-----------
>  2 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index faf7ad031e42..fc93a6348082 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -302,11 +302,6 @@ struct thermal_zone_params {
>         int offset;
>  };
>
> -struct thermal_genl_event {
> -       u32 orig;
> -       enum events event;
> -};
> -
>  /**
>   * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
>   *
> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> index 96218378dda8..22df67ed9e9c 100644
> --- a/include/uapi/linux/thermal.h
> +++ b/include/uapi/linux/thermal.h
> @@ -6,21 +6,12 @@
>
>  /* Adding event notification support elements */
>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> -#define THERMAL_GENL_VERSION                    0x01
> +#define THERMAL_GENL_VERSION                    0x02

This hunk should be removed since you set version back to 1 in the
next patch and we don't actually intend to bump the version yet.


>  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_grp"
>
> -/* Events supported by Thermal Netlink */
> -enum events {
> -       THERMAL_AUX0,
> -       THERMAL_AUX1,
> -       THERMAL_CRITICAL,
> -       THERMAL_DEV_FAULT,
> -};
> -
>  /* attributes of thermal_genl_family */
>  enum {
>         THERMAL_GENL_ATTR_UNSPEC,
> -       THERMAL_GENL_ATTR_EVENT,
>         __THERMAL_GENL_ATTR_MAX,
>  };
>  #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> @@ -28,7 +19,6 @@ enum {
>  /* commands supported by the thermal_genl_family */
>  enum {
>         THERMAL_GENL_CMD_UNSPEC,
> -       THERMAL_GENL_CMD_EVENT,
>         __THERMAL_GENL_CMD_MAX,
>  };
>  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> --
> 2.17.1
>

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Daniel Lezcano]

On 30/06/2020 13:47, Amit Kucheria wrote:
> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> In order to set the scene for the new generic netlink thermal
>> management and notifications, let remove the old dead code remaining
> 
> s/management/management api/
> 
> s/let/let's/
> 
>> in the uapi headers.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  include/linux/thermal.h      |  5 -----
>>  include/uapi/linux/thermal.h | 12 +-----------
>>  2 files changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index faf7ad031e42..fc93a6348082 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -302,11 +302,6 @@ struct thermal_zone_params {
>>         int offset;
>>  };
>>
>> -struct thermal_genl_event {
>> -       u32 orig;
>> -       enum events event;
>> -};
>> -
>>  /**
>>   * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
>>   *
>> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
>> index 96218378dda8..22df67ed9e9c 100644
>> --- a/include/uapi/linux/thermal.h
>> +++ b/include/uapi/linux/thermal.h
>> @@ -6,21 +6,12 @@
>>
>>  /* Adding event notification support elements */
>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>> -#define THERMAL_GENL_VERSION                    0x01
>> +#define THERMAL_GENL_VERSION                    0x02
> 
> This hunk should be removed since you set version back to 1 in the
> next patch and we don't actually intend to bump the version yet.

Well, I've been very strict here for git-bisecting.

I move to V2 because of the removal, but when adding the new genetlink
code, the family name changed, so we returned back to the V1 as it is a
new genetlink thermal brand.

The name is change because it is no longer event based but also sampling
and commands.

>>  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_grp"
>>
>> -/* Events supported by Thermal Netlink */
>> -enum events {
>> -       THERMAL_AUX0,
>> -       THERMAL_AUX1,
>> -       THERMAL_CRITICAL,
>> -       THERMAL_DEV_FAULT,
>> -};
>> -
>>  /* attributes of thermal_genl_family */
>>  enum {
>>         THERMAL_GENL_ATTR_UNSPEC,
>> -       THERMAL_GENL_ATTR_EVENT,
>>         __THERMAL_GENL_ATTR_MAX,
>>  };
>>  #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
>> @@ -28,7 +19,6 @@ enum {
>>  /* commands supported by the thermal_genl_family */
>>  enum {
>>         THERMAL_GENL_CMD_UNSPEC,
>> -       THERMAL_GENL_CMD_EVENT,
>>         __THERMAL_GENL_CMD_MAX,
>>  };
>>  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
>> --
>> 2.17.1
>>


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Amit Kucheria]

On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 30/06/2020 13:47, Amit Kucheria wrote:
> > On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> In order to set the scene for the new generic netlink thermal
> >> management and notifications, let remove the old dead code remaining
> >
> > s/management/management api/
> >
> > s/let/let's/
> >
> >> in the uapi headers.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>  include/linux/thermal.h      |  5 -----
> >>  include/uapi/linux/thermal.h | 12 +-----------
> >>  2 files changed, 1 insertion(+), 16 deletions(-)
> >>
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index faf7ad031e42..fc93a6348082 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -302,11 +302,6 @@ struct thermal_zone_params {
> >>         int offset;
> >>  };
> >>
> >> -struct thermal_genl_event {
> >> -       u32 orig;
> >> -       enum events event;
> >> -};
> >> -
> >>  /**
> >>   * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
> >>   *
> >> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> >> index 96218378dda8..22df67ed9e9c 100644
> >> --- a/include/uapi/linux/thermal.h
> >> +++ b/include/uapi/linux/thermal.h
> >> @@ -6,21 +6,12 @@
> >>
> >>  /* Adding event notification support elements */
> >>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> >> -#define THERMAL_GENL_VERSION                    0x01
> >> +#define THERMAL_GENL_VERSION                    0x02
> >
> > This hunk should be removed since you set version back to 1 in the
> > next patch and we don't actually intend to bump the version yet.
>
> Well, I've been very strict here for git-bisecting.
>
> I move to V2 because of the removal, but when adding the new genetlink
> code, the family name changed, so we returned back to the V1 as it is a
> new genetlink thermal brand.

I don't understand the move to v2 for an empty skeleton UAPI. For the
purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
bumping to v2) and then add a new UAPI in the next patch?

> The name is change because it is no longer event based but also sampling
> and commands.

In this case, just to avoid any confusion, the new UAPI could be v2
making the transition clear in case of bisection.

I'm afraid the v1->v2->v1 is a bit more confusing.

/Amit

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Daniel Lezcano]

On 01/07/2020 11:33, Amit Kucheria wrote:
> On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 30/06/2020 13:47, Amit Kucheria wrote:
>>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> In order to set the scene for the new generic netlink thermal
>>>> management and notifications, let remove the old dead code remaining
>>>
>>> s/management/management api/
>>>
>>> s/let/let's/
>>>
>>>> in the uapi headers.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>  include/linux/thermal.h      |  5 -----
>>>>  include/uapi/linux/thermal.h | 12 +-----------
>>>>  2 files changed, 1 insertion(+), 16 deletions(-)
>>>>
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index faf7ad031e42..fc93a6348082 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -302,11 +302,6 @@ struct thermal_zone_params {
>>>>         int offset;
>>>>  };
>>>>
>>>> -struct thermal_genl_event {
>>>> -       u32 orig;
>>>> -       enum events event;
>>>> -};
>>>> -
>>>>  /**
>>>>   * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
>>>>   *
>>>> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
>>>> index 96218378dda8..22df67ed9e9c 100644
>>>> --- a/include/uapi/linux/thermal.h
>>>> +++ b/include/uapi/linux/thermal.h
>>>> @@ -6,21 +6,12 @@
>>>>
>>>>  /* Adding event notification support elements */
>>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>>>> -#define THERMAL_GENL_VERSION                    0x01
>>>> +#define THERMAL_GENL_VERSION                    0x02
>>>
>>> This hunk should be removed since you set version back to 1 in the
>>> next patch and we don't actually intend to bump the version yet.
>>
>> Well, I've been very strict here for git-bisecting.
>>
>> I move to V2 because of the removal, but when adding the new genetlink
>> code, the family name changed, so we returned back to the V1 as it is a
>> new genetlink thermal brand.
> 
> I don't understand the move to v2 for an empty skeleton UAPI. For the
> purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
> bumping to v2) and then add a new UAPI in the next patch?
> 
>> The name is change because it is no longer event based but also sampling
>> and commands.
> 
> In this case, just to avoid any confusion, the new UAPI could be v2
> making the transition clear in case of bisection.
> 
> I'm afraid the v1->v2->v1 is a bit more confusing.

Let me elaborate a bit:

Why there is this patch ?
- By removing this code first, the next patch will just contain
additions, I thought it would be clearer

Why increase the version here ?
- Code must continue to compile and as the 'thermal_event' family is now
different from V1, the version is changed

Why the version goes to V1 in the next patch ?
- The family name is changed as it is not doing event only, so it is a
new netlink thermal protocol and we begin at V1

So the main reason of this patch is to be very strict in the iteration
changes. May be it is too much, in this case I can merge this patch with
4/5, the old netlink protocol removal will be lost in the addition of
the new protocol. I'm fine with that if you think it is simpler.




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Amit Kucheria]

On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 01/07/2020 11:33, Amit Kucheria wrote:
> > On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 30/06/2020 13:47, Amit Kucheria wrote:
> >>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>

> >>>>  /* Adding event notification support elements */
> >>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> >>>> -#define THERMAL_GENL_VERSION                    0x01
> >>>> +#define THERMAL_GENL_VERSION                    0x02
> >>>
> >>> This hunk should be removed since you set version back to 1 in the
> >>> next patch and we don't actually intend to bump the version yet.
> >>
> >> Well, I've been very strict here for git-bisecting.
> >>
> >> I move to V2 because of the removal, but when adding the new genetlink
> >> code, the family name changed, so we returned back to the V1 as it is a
> >> new genetlink thermal brand.
> >
> > I don't understand the move to v2 for an empty skeleton UAPI. For the
> > purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
> > bumping to v2) and then add a new UAPI in the next patch?
> >
> >> The name is change because it is no longer event based but also sampling
> >> and commands.
> >
> > In this case, just to avoid any confusion, the new UAPI could be v2
> > making the transition clear in case of bisection.
> >
> > I'm afraid the v1->v2->v1 is a bit more confusing.
>
> Let me elaborate a bit:
>
> Why there is this patch ?
> - By removing this code first, the next patch will just contain
> additions, I thought it would be clearer
>
> Why increase the version here ?
> - Code must continue to compile and as the 'thermal_event' family is now
> different from V1, the version is changed
>
> Why the version goes to V1 in the next patch ?
> - The family name is changed as it is not doing event only, so it is a
> new netlink thermal protocol and we begin at V1
>
> So the main reason of this patch is to be very strict in the iteration
> changes. May be it is too much, in this case I can merge this patch with
> 4/5, the old netlink protocol removal will be lost in the addition of
> the new protocol. I'm fine with that if you think it is simpler.

Considering that there are no users of v1 currently, it feels a bit
over engineered, IMHO.

Also, the new UAPI doesn't need to begin at v1. Just having it start
at v2 will avoid this confusion, no?

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Daniel Lezcano]

On 01/07/2020 14:10, Amit Kucheria wrote:
> On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 01/07/2020 11:33, Amit Kucheria wrote:
>>> On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 30/06/2020 13:47, Amit Kucheria wrote:
>>>>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
> 
>>>>>>  /* Adding event notification support elements */
>>>>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>>>>>> -#define THERMAL_GENL_VERSION                    0x01
>>>>>> +#define THERMAL_GENL_VERSION                    0x02
>>>>>
>>>>> This hunk should be removed since you set version back to 1 in the
>>>>> next patch and we don't actually intend to bump the version yet.
>>>>
>>>> Well, I've been very strict here for git-bisecting.
>>>>
>>>> I move to V2 because of the removal, but when adding the new genetlink
>>>> code, the family name changed, so we returned back to the V1 as it is a
>>>> new genetlink thermal brand.
>>>
>>> I don't understand the move to v2 for an empty skeleton UAPI. For the
>>> purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
>>> bumping to v2) and then add a new UAPI in the next patch?
>>>
>>>> The name is change because it is no longer event based but also sampling
>>>> and commands.
>>>
>>> In this case, just to avoid any confusion, the new UAPI could be v2
>>> making the transition clear in case of bisection.
>>>
>>> I'm afraid the v1->v2->v1 is a bit more confusing.
>>
>> Let me elaborate a bit:
>>
>> Why there is this patch ?
>> - By removing this code first, the next patch will just contain
>> additions, I thought it would be clearer
>>
>> Why increase the version here ?
>> - Code must continue to compile and as the 'thermal_event' family is now
>> different from V1, the version is changed
>>
>> Why the version goes to V1 in the next patch ?
>> - The family name is changed as it is not doing event only, so it is a
>> new netlink thermal protocol and we begin at V1
>>
>> So the main reason of this patch is to be very strict in the iteration
>> changes. May be it is too much, in this case I can merge this patch with
>> 4/5, the old netlink protocol removal will be lost in the addition of
>> the new protocol. I'm fine with that if you think it is simpler.
> 
> Considering that there are no users of v1 currently, it feels a bit
> over engineered, IMHO.
> 
> Also, the new UAPI doesn't need to begin at v1. Just having it start
> at v2 will avoid this confusion, no?

Ok, I will merge both patches but I will keep the V1 because the netlink
protocol is a new one.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Daniel Lezcano]

Initially the thermal framework had a very simple notification
mechanism to send generic netlink messages to the userspace.

The notification function was never called from anywhere and the
corresponding dead code was removed. It was probably a first attempt
to introduce the netlink notification.

At LPC2018, the presentation "Linux thermal: User kernel interface",
proposed to create the notifications to the userspace via a kfifo.

The advantage of the kfifo is the performance. It is usually used from
a 1:1 communication channel where a driver captures data and send them
as fast as possible to an userspace process.

The inconvenient is the process uses the notification channel
exclusively, thus no other process is allowed to use the channel to
get temperature or notifications.

The purpose of this series is to provide a fully netlink featured
thermal management.

This patch is defining a generic netlink API to discover the current
thermal setup, get events and temperature sampling. As any genetlink
protocol, it can evolve and the versionning allows to keep the backward
compatibility.

In order to not flood the user with a single channel data, there are
two multicast channels, one for the temperature sampling when the
thermal zone is updated and another one for the events, so the user
can get the events only without the thermal zone temperature
sampling. All these events are from the ones presented at the LPC2018.

Also, a list of commands to discover the thermal setup is given and
can be extended on purpose.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Makefile          |   2 +-
 drivers/thermal/thermal_core.h    |  19 +
 drivers/thermal/thermal_netlink.c | 645 ++++++++++++++++++++++++++++++
 include/linux/thermal.h           |  12 -
 include/uapi/linux/thermal.h      |  80 +++-
 5 files changed, 738 insertions(+), 20 deletions(-)
 create mode 100644 drivers/thermal/thermal_netlink.c

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 0c8b84a09b9a..1bbf0805fb04 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -5,7 +5,7 @@
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 thermal_sys-y			+= thermal_core.o thermal_sysfs.o \
-					thermal_helpers.o
+					thermal_helpers.o thermal_netlink.o
 
 # interface to/from other layers providing sensors
 thermal_sys-$(CONFIG_THERMAL_HWMON)		+= thermal_hwmon.o
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 7e8f45db6bbf..08eb03fe4f69 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -52,6 +52,25 @@ int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
 
 struct thermal_zone_device *thermal_zone_get_by_id(int id);
 
+/* Netlink notification function */
+int thermal_notify_tz_create(int tz_id, const char *name);
+int thermal_notify_tz_delete(int tz_id);
+int thermal_notify_tz_enable(int tz_id);
+int thermal_notify_tz_disable(int tz_id);
+int thermal_notify_tz_trip_down(int tz_id, int id);
+int thermal_notify_tz_trip_up(int tz_id, int id);
+int thermal_notify_tz_trip_delete(int tz_id, int id);
+int thermal_notify_tz_trip_add(int tz_id, int id, int type,
+			       int temp, int hyst);
+int thermal_notify_tz_trip_change(int tz_id, int id, int type,
+				  int temp, int hyst);
+int thermal_notify_cdev_update(int cdev_id, int state);
+int thermal_notify_cdev_add(int cdev_id, const char *name,
+			    int min_state, int max_state);
+int thermal_notify_cdev_delete(int cdev_id);
+int thermal_notify_tz_gov_change(int tz_id, const char *name);
+int thermal_genl_sampling_temp(int id, int temp);
+
 struct thermal_attr {
 	struct device_attribute attr;
 	char name[THERMAL_NAME_LENGTH];
diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
new file mode 100644
index 000000000000..a8d6a815a21d
--- /dev/null
+++ b/drivers/thermal/thermal_netlink.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * Generic netlink for thermal management framework
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <net/genetlink.h>
+#include <uapi/linux/thermal.h>
+
+#include "thermal_core.h"
+
+static const struct genl_multicast_group thermal_genl_mcgrps[] = {
+	{ .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
+	{ .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
+};
+
+static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
+	/* Thermal zone */
+	[THERMAL_GENL_ATTR_TZ]			= { .type = NLA_NESTED },
+	[THERMAL_GENL_ATTR_TZ_ID]		= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_TEMP]		= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_TRIP]		= { .type = NLA_NESTED },
+	[THERMAL_GENL_ATTR_TZ_TRIP_ID]		= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_TRIP_TEMP]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_TRIP_TYPE]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_TRIP_HYST]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_MODE]		= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_TZ_NAME]		= { .type = NLA_STRING,
+						    .len = THERMAL_NAME_LENGTH },
+	/* Governor(s) */
+	[THERMAL_GENL_ATTR_TZ_GOV]		= { .type = NLA_NESTED },
+	[THERMAL_GENL_ATTR_TZ_GOV_NAME]		= { .type = NLA_STRING,
+						    .len = THERMAL_NAME_LENGTH },
+	/* Cooling devices */
+	[THERMAL_GENL_ATTR_CDEV]		= { .type = NLA_NESTED },
+	[THERMAL_GENL_ATTR_CDEV_ID]		= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_CDEV_CUR_STATE]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_CDEV_MAX_STATE]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_CDEV_MIN_STATE]	= { .type = NLA_U32 },
+	[THERMAL_GENL_ATTR_CDEV_NAME]		= { .type = NLA_STRING,
+						    .len = THERMAL_NAME_LENGTH },
+};
+
+struct param {
+	struct nlattr ** attrs;
+	struct sk_buff *msg;
+	const char *name;
+	int tz_id;
+	int cdev_id;
+	int id;
+	int temp;
+	int type;
+	int hyst;
+	int state;
+	int min_state;
+	int max_state;
+};
+
+typedef int (*cb_t)(struct param *);
+
+static struct genl_family thermal_gnl_family;
+
+/************************** Sampling encoding *******************************/
+
+int thermal_genl_sampling_temp(int id, int temp)
+{
+	struct sk_buff *skb;
+	void *hdr;
+
+	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = genlmsg_put(skb, 0, 0, &thermal_gnl_family, 0,
+			  THERMAL_GENL_SAMPLING_TEMP);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, THERMAL_GENL_ATTR_TZ_ID, id))
+		goto out_cancel;
+
+	if (nla_put_u32(skb, THERMAL_GENL_ATTR_TZ_TEMP, temp))
+		goto out_cancel;
+
+	genlmsg_end(skb, hdr);
+
+	genlmsg_multicast(&thermal_gnl_family, skb, 0, 0, GFP_KERNEL);
+
+	return 0;
+out_cancel:
+	genlmsg_cancel(skb, hdr);
+	nlmsg_free(skb);
+
+	return -EMSGSIZE;
+}
+
+/**************************** Event encoding *********************************/
+
+static int thermal_genl_event_tz_create(struct param *p)
+{
+	if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
+	    nla_put_string(p->msg, THERMAL_GENL_ATTR_TZ_NAME, p->name))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_tz(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_tz_trip_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_TRIP_ID, p->id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_tz_trip_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_TZ_TRIP_ID, p->id) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, p->type) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_TEMP, p->temp) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_HYST, p->hyst))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_tz_trip_delete(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_TRIP_ID, p->id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_cdev_add(struct param *p)
+{
+	if (nla_put_string(p->msg, THERMAL_GENL_ATTR_CDEV_NAME,
+			   p->name) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID,
+			p->cdev_id) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_MIN_STATE,
+			p->min_state) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_MAX_STATE,
+			p->max_state))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_cdev_delete(struct param *p)
+{
+	if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID, p->cdev_id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_cdev_update(struct param *p)
+{
+	if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID, p->cdev_id) ||
+	    nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_CUR_STATE, p->state))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_event_gov_change(struct param *p)
+{
+	if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
+	    nla_put_string(p->msg, THERMAL_GENL_ATTR_GOV_NAME, p->name))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+int thermal_genl_event_tz_delete(struct param *p)
+	__attribute__((alias("thermal_genl_event_tz")));
+
+int thermal_genl_event_tz_enable(struct param *p)
+	__attribute__((alias("thermal_genl_event_tz")));
+
+int thermal_genl_event_tz_disable(struct param *p)
+	__attribute__((alias("thermal_genl_event_tz")));
+
+int thermal_genl_event_tz_trip_down(struct param *p)
+	__attribute__((alias("thermal_genl_event_tz_trip_up")));
+
+int thermal_genl_event_tz_trip_change(struct param *)
+	__attribute__((alias("thermal_genl_event_tz_trip_add")));
+
+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,
+	[THERMAL_GENL_EVENT_TZ_ENABLE]		= thermal_genl_event_tz_enable,
+	[THERMAL_GENL_EVENT_TZ_DISABLE]		= thermal_genl_event_tz_disable,
+	[THERMAL_GENL_EVENT_TZ_TRIP_UP]		= thermal_genl_event_tz_trip_up,
+	[THERMAL_GENL_EVENT_TZ_TRIP_DOWN]	= thermal_genl_event_tz_trip_down,
+	[THERMAL_GENL_EVENT_TZ_TRIP_CHANGE]	= thermal_genl_event_tz_trip_change,
+	[THERMAL_GENL_EVENT_TZ_TRIP_ADD]	= thermal_genl_event_tz_trip_add,
+	[THERMAL_GENL_EVENT_TZ_TRIP_DELETE]	= thermal_genl_event_tz_trip_delete,
+	[THERMAL_GENL_EVENT_CDEV_ADD]		= thermal_genl_event_cdev_add,
+	[THERMAL_GENL_EVENT_CDEV_DELETE]	= thermal_genl_event_cdev_delete,
+	[THERMAL_GENL_EVENT_CDEV_UPDATE]	= thermal_genl_event_cdev_update,
+	[THERMAL_GENL_EVENT_TZ_GOV_CHANGE]	= thermal_genl_event_gov_change,
+};
+
+/*
+ * Generic netlink event encoding
+ */
+static int thermal_genl_send_event(enum thermal_genl_event event,
+				   struct param *p)
+{
+	struct sk_buff *msg;
+	int ret = -EMSGSIZE;
+	void *hdr;
+
+        msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	p->msg = msg;
+
+	hdr = genlmsg_put(msg, 0, 0, &thermal_gnl_family, 0, event);
+	if (!hdr)
+		goto out_free_msg;
+
+	ret = event_cb[event](p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+
+	genlmsg_multicast(&thermal_gnl_family, msg, 0, 1, GFP_KERNEL);
+
+	return 0;
+
+out_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+out_free_msg:
+	nlmsg_free(msg);
+
+	return ret;
+}
+
+int thermal_notify_tz_create(int tz_id, const char *name)
+{
+	struct param p = { .tz_id = tz_id, .name = name };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_CREATE, &p);
+}
+
+int thermal_notify_tz_delete(int tz_id)
+{
+	struct param p = { .tz_id = tz_id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DELETE, &p);
+}
+
+int thermal_notify_tz_enable(int tz_id)
+{
+	struct param p = { .tz_id = tz_id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_ENABLE, &p);
+}
+
+int thermal_notify_tz_disable(int tz_id)
+{
+	struct param p = { .tz_id = tz_id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DISABLE, &p);
+}
+
+int thermal_notify_tz_trip_down(int tz_id, int id)
+{
+	struct param p = { .tz_id = tz_id, .id = id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DOWN, &p);
+}
+
+int thermal_notify_tz_trip_up(int tz_id, int id)
+{
+	struct param p = { .tz_id = tz_id, .id = id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_UP, &p);
+}
+
+int thermal_notify_tz_trip_add(int tz_id, int id, int type, int temp, int hyst)
+{
+	struct param p = { .tz_id = tz_id, .id = id, .type = type,
+			   .temp = temp, .hyst = hyst };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_ADD, &p);
+}
+
+int thermal_notify_tz_trip_delete(int tz_id, int id)
+{
+	struct param p = { .tz_id = tz_id, .id = id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DELETE, &p);
+}
+
+int thermal_notify_tz_trip_change(int tz_id, int id, int type, int temp, int hyst)
+{
+	struct param p = { .tz_id = tz_id, .id = id, .type = type,
+			   .temp = temp, .hyst = hyst };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_CHANGE, &p);
+}
+
+int thermal_notify_cdev_update(int cdev_id, int state)
+{
+	struct param p = { .cdev_id = cdev_id, .state = state };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_UPDATE, &p);
+}
+
+int thermal_notify_cdev_add(int cdev_id, const char *name,
+			    int min_state, int max_state)
+{
+	struct param p = { .cdev_id = cdev_id, .name = name,
+			   .min_state = min_state, .max_state = max_state };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_ADD, &p);
+}
+
+int thermal_notify_cdev_delete(int cdev_id)
+{
+	struct param p = { .cdev_id = cdev_id };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_DELETE, &p);
+}
+
+int thermal_notify_tz_gov_change(int tz_id, const char *name)
+{
+	struct param p = { .tz_id = tz_id, .name = name };
+
+	return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_GOV_CHANGE, &p);
+}
+
+/*************************** Command encoding ********************************/
+
+static int __thermal_genl_cmd_tz_get(struct thermal_zone_device *tz, void *data)
+{
+	struct sk_buff *msg = data;
+
+	if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, tz->id) ||
+	    nla_put_string(msg, THERMAL_GENL_ATTR_TZ_NAME, tz->type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_cmd_tz_get(struct param *p)
+{
+	struct sk_buff *msg = p->msg;
+	struct nlattr *start_tz;
+	int ret;
+
+	start_tz = nla_nest_start(msg, THERMAL_GENL_ATTR_TZ);
+	if (!start_tz)
+		return -EMSGSIZE;
+
+	ret = for_each_thermal_zone(__thermal_genl_cmd_tz_get, msg);
+	if (ret)
+		goto out_cancel_nest;
+
+	nla_nest_end(msg, start_tz);
+
+	return 0;
+
+out_cancel_nest:
+	nla_nest_cancel(msg, start_tz);
+
+	return ret;
+}
+
+static int thermal_genl_cmd_tz_get_trip(struct param *p)
+{
+	struct sk_buff *msg = p->msg;
+	struct thermal_zone_device *tz;
+	struct nlattr *start_trip;
+	int i, id;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+		return -EINVAL;
+
+	id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_TZ_TRIP);
+	if (!start_trip)
+		return -EMSGSIZE;
+
+	mutex_lock(&tz->lock);
+
+	for (i = 0; i < tz->trips; i++) {
+
+		enum thermal_trip_type type;
+		int temp, hyst;
+
+		tz->ops->get_trip_type(tz, i, &type);
+		tz->ops->get_trip_temp(tz, i, &temp);
+		tz->ops->get_trip_hyst(tz, i, &hyst);
+
+		if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, i) ||
+		    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, type) ||
+		    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TEMP, temp) ||
+		    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_HYST, hyst))
+			goto out_cancel_nest;
+	}
+
+	mutex_unlock(&tz->lock);
+
+	nla_nest_end(msg, start_trip);
+
+	return 0;
+
+out_cancel_nest:
+	mutex_unlock(&tz->lock);
+
+	return -EMSGSIZE;
+}
+
+static int thermal_genl_cmd_tz_get_temp(struct param *p)
+{
+	struct sk_buff *msg = p->msg;
+	struct thermal_zone_device *tz;
+	int temp, ret, id;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+		return -EINVAL;
+
+	id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	ret = thermal_zone_get_temp(tz, &temp);
+	if (ret)
+		return ret;
+
+	if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id) ||
+	    nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TEMP, temp))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_cmd_tz_get_gov(struct param *p)
+{
+	struct sk_buff *msg = p->msg;
+	struct thermal_zone_device *tz;
+	int id, ret = 0;
+
+	if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
+		return -EINVAL;
+
+	id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
+
+	tz = thermal_zone_get_by_id(id);
+	if (!tz)
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+
+	if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id) ||
+	    nla_put_string(msg, THERMAL_GENL_ATTR_TZ_GOV_NAME,
+			   tz->governor->name))
+		ret = -EMSGSIZE;
+
+	mutex_unlock(&tz->lock);
+
+	return ret;
+}
+
+static int __thermal_genl_cmd_cdev_get(struct thermal_cooling_device *cdev,
+				       void *data)
+{
+	struct sk_buff *msg = data;
+
+	if (nla_put_u32(msg, THERMAL_GENL_ATTR_CDEV_ID, cdev->id))
+		return -EMSGSIZE;
+
+	if (nla_put_string(msg, THERMAL_GENL_ATTR_CDEV_NAME, cdev->type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int thermal_genl_cmd_cdev_get(struct param *p)
+{
+	struct sk_buff *msg = p->msg;
+	struct nlattr *start_cdev;
+	int ret;
+
+	start_cdev = nla_nest_start(msg, THERMAL_GENL_ATTR_CDEV);
+	if (!start_cdev)
+		return -EMSGSIZE;
+
+	ret = for_each_thermal_cooling_device(__thermal_genl_cmd_cdev_get, msg);
+	if (ret)
+		goto out_cancel_nest;
+
+	nla_nest_end(msg, start_cdev);
+
+	return 0;
+out_cancel_nest:
+	nla_nest_cancel(msg, start_cdev);
+
+	return ret;
+}
+
+static cb_t cmd_cb[] = {
+	[THERMAL_GENL_CMD_TZ_GET]	= thermal_genl_cmd_tz_get,
+	[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,
+};
+
+static int thermal_genl_cmd_dumpit(struct sk_buff *skb,
+				   struct netlink_callback *cb)
+{
+	struct param p = { .msg = skb };
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	int cmd = info->ops->cmd;
+	int ret = -EMSGSIZE;
+	void *hdr;
+
+	hdr = genlmsg_put(skb, 0, 0, &thermal_gnl_family, 0, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	ret = cmd_cb[cmd](&p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(skb, hdr);
+
+	return 0;
+
+out_cancel_msg:
+	genlmsg_cancel(skb, hdr);
+
+	return ret;
+}
+
+static int thermal_genl_cmd_doit(struct sk_buff *skb,
+				 struct genl_info *info)
+{
+	struct param p = { .attrs = info->attrs };
+	struct sk_buff *msg;
+	void *hdr;
+	int cmd = info->genlhdr->cmd;
+	int ret = -EMSGSIZE;
+
+        msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	p.msg = msg;
+
+        hdr = genlmsg_put_reply(msg, info, &thermal_gnl_family, 0, cmd);
+	if (!hdr)
+		goto out_free_msg;
+
+	ret = cmd_cb[cmd](&p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, info);
+
+out_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+out_free_msg:
+	nlmsg_free(msg);
+
+	return ret;
+}
+
+static const struct genl_ops thermal_genl_ops[] = {
+	{
+		.cmd = THERMAL_GENL_CMD_TZ_GET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.dumpit = thermal_genl_cmd_dumpit,
+	},
+	{
+		.cmd = THERMAL_GENL_CMD_TZ_GET_TRIP,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = thermal_genl_cmd_doit,
+	},
+	{
+		.cmd = THERMAL_GENL_CMD_TZ_GET_TEMP,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = thermal_genl_cmd_doit,
+	},
+	{
+		.cmd = THERMAL_GENL_CMD_TZ_GET_GOV,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = thermal_genl_cmd_doit,
+	},
+	{
+		.cmd = THERMAL_GENL_CMD_CDEV_GET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.dumpit = thermal_genl_cmd_dumpit,
+	},
+};
+
+static struct genl_family thermal_gnl_family __ro_after_init = {
+	.hdrsize	= 0,
+	.name		= THERMAL_GENL_FAMILY_NAME,
+	.version	= THERMAL_GENL_VERSION,
+	.maxattr	= THERMAL_GENL_ATTR_MAX,
+	.policy		= thermal_genl_policy,
+	.ops		= thermal_genl_ops,
+	.n_ops 		= ARRAY_SIZE(thermal_genl_ops),
+	.mcgrps		= thermal_genl_mcgrps,
+	.n_mcgrps	= ARRAY_SIZE(thermal_genl_mcgrps),
+};
+
+static int __init thermal_netlink_init(void)
+{
+	return genl_register_family(&thermal_gnl_family);
+}
+core_initcall(thermal_netlink_init);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index fc93a6348082..d92643e310e2 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -37,18 +37,6 @@ struct thermal_cooling_device;
 struct thermal_instance;
 struct thermal_attr;
 
-enum thermal_device_mode {
-	THERMAL_DEVICE_DISABLED = 0,
-	THERMAL_DEVICE_ENABLED,
-};
-
-enum thermal_trip_type {
-	THERMAL_TRIP_ACTIVE = 0,
-	THERMAL_TRIP_PASSIVE,
-	THERMAL_TRIP_HOT,
-	THERMAL_TRIP_CRITICAL,
-};
-
 enum thermal_trend {
 	THERMAL_TREND_STABLE, /* temperature is stable */
 	THERMAL_TREND_RAISING, /* temperature is raising */
diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
index 22df67ed9e9c..49289f9d993b 100644
--- a/include/uapi/linux/thermal.h
+++ b/include/uapi/linux/thermal.h
@@ -4,21 +4,87 @@
 
 #define THERMAL_NAME_LENGTH	20
 
+enum thermal_device_mode {
+	THERMAL_DEVICE_DISABLED = 0,
+	THERMAL_DEVICE_ENABLED,
+};
+
+enum thermal_trip_type {
+	THERMAL_TRIP_ACTIVE = 0,
+	THERMAL_TRIP_PASSIVE,
+	THERMAL_TRIP_HOT,
+	THERMAL_TRIP_CRITICAL,
+};
+
 /* Adding event notification support elements */
-#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
-#define THERMAL_GENL_VERSION                    0x02
-#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_grp"
+#define THERMAL_GENL_FAMILY_NAME		"thermal"
+#define THERMAL_GENL_VERSION			0x01
+#define THERMAL_GENL_SAMPLING_GROUP_NAME	"sampling"
+#define THERMAL_GENL_EVENT_GROUP_NAME		"event"
 
-/* attributes of thermal_genl_family */
-enum {
+/* Attributes of thermal_genl_family */
+enum thermal_genl_attr {
 	THERMAL_GENL_ATTR_UNSPEC,
+	THERMAL_GENL_ATTR_TZ,
+	THERMAL_GENL_ATTR_TZ_ID,
+	THERMAL_GENL_ATTR_TZ_TEMP,
+	THERMAL_GENL_ATTR_TZ_TRIP,
+	THERMAL_GENL_ATTR_TZ_TRIP_ID,
+	THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
+	THERMAL_GENL_ATTR_TZ_TRIP_TEMP,
+	THERMAL_GENL_ATTR_TZ_TRIP_HYST,
+	THERMAL_GENL_ATTR_TZ_MODE,
+	THERMAL_GENL_ATTR_TZ_NAME,
+	THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT,
+	THERMAL_GENL_ATTR_TZ_GOV,
+	THERMAL_GENL_ATTR_TZ_GOV_NAME,
+	THERMAL_GENL_ATTR_CDEV,
+	THERMAL_GENL_ATTR_CDEV_ID,
+	THERMAL_GENL_ATTR_CDEV_CUR_STATE,
+	THERMAL_GENL_ATTR_CDEV_MAX_STATE,
+	THERMAL_GENL_ATTR_CDEV_MIN_STATE,
+	THERMAL_GENL_ATTR_CDEV_NAME,
+	THERMAL_GENL_ATTR_GOV_NAME,
+
 	__THERMAL_GENL_ATTR_MAX,
 };
 #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
 
-/* commands supported by the thermal_genl_family */
-enum {
+enum thermal_genl_sampling {
+	THERMAL_GENL_SAMPLING_TEMP,
+	__THERMAL_GENL_SAMPLING_MAX,
+};
+#define THERMAL_GENL_SAMPLING_MAX (__THERMAL_GENL_SAMPLING_MAX - 1)
+
+/* Events of thermal_genl_family */
+enum thermal_genl_event {
+	THERMAL_GENL_EVENT_UNSPEC,
+	THERMAL_GENL_EVENT_TZ_CREATE,		/* Thermal zone creation */
+	THERMAL_GENL_EVENT_TZ_DELETE,		/* Thermal zone deletion */
+	THERMAL_GENL_EVENT_TZ_DISABLE,		/* Thermal zone disabed */
+	THERMAL_GENL_EVENT_TZ_ENABLE,		/* Thermal zone enabled */
+	THERMAL_GENL_EVENT_TZ_TRIP_UP,		/* Trip point crossed the way up */
+	THERMAL_GENL_EVENT_TZ_TRIP_DOWN,	/* Trip point crossed the way down */
+	THERMAL_GENL_EVENT_TZ_TRIP_CHANGE,	/* Trip point changed */
+	THERMAL_GENL_EVENT_TZ_TRIP_ADD,		/* Trip point added */
+	THERMAL_GENL_EVENT_TZ_TRIP_DELETE,	/* Trip point deleted */
+	THERMAL_GENL_EVENT_CDEV_ADD,		/* Cdev bound to the thermal zone */
+	THERMAL_GENL_EVENT_CDEV_DELETE,		/* Cdev unbound */
+	THERMAL_GENL_EVENT_CDEV_UPDATE,		/* Cdev state updated */
+	THERMAL_GENL_EVENT_TZ_GOV_CHANGE,	/* Governor policy changed  */
+	__THERMAL_GENL_EVENT_MAX,
+};
+#define THERMAL_GENL_EVENT_MAX (__THERMAL_GENL_EVENT_MAX - 1)
+
+/* Commands supported by the thermal_genl_family */
+enum thermal_genl_cmd {
 	THERMAL_GENL_CMD_UNSPEC,
+	THERMAL_GENL_CMD_TZ_GET,	/* List 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_MAX,
 };
 #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
-- 
2.17.1

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> Initially the thermal framework had a very simple notification
> mechanism to send generic netlink messages to the userspace.
>
> The notification function was never called from anywhere and the
> corresponding dead code was removed. It was probably a first attempt
> to introduce the netlink notification.
>
> At LPC2018, the presentation "Linux thermal: User kernel interface",
> proposed to create the notifications to the userspace via a kfifo.
>
> The advantage of the kfifo is the performance. It is usually used from
> a 1:1 communication channel where a driver captures data and send them

s/send them/sends it/

> as fast as possible to an userspace process.

s/an/a/

>
> The inconvenient is the process uses the notification channel
> exclusively, thus no other process is allowed to use the channel to

Suggest rephrasing to "The drawback is that only one process uses the
notification channel exclusively,"

> get temperature or notifications.
>
> The purpose of this series is to provide a fully netlink featured
> thermal management.

Suggest getting rid of this sentence, probably more appropriate for
the cover letter and instead

>
> This patch is defining a generic netlink API to discover the current

s/defining/defines/

s/generic/fully-featured generic/

> thermal setup, get events and temperature sampling. As any genetlink

s/setup, get events/setup, and adds event notifications/

> protocol, it can evolve and the versionning allows to keep the backward

Spell check: versioning

> compatibility.
>
> In order to not flood the user with a single channel data, there are

Consider rephrasing to "In order to prevent the user from getting
flooded with data on a single channel"

> two multicast channels, one for the temperature sampling when the
> thermal zone is updated and another one for the events, so the user
> can get the events only without the thermal zone temperature
> sampling. All these events are from the ones presented at the LPC2018.

We should remove this reference to LPC2018 here and just list the
events here, or remove this sentence and leave it for the code.

>
> Also, a list of commands to discover the thermal setup is given and

s/given/added/

> can be extended on purpose.

s/on purpose/when needed/

>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/Makefile          |   2 +-
>  drivers/thermal/thermal_core.h    |  19 +
>  drivers/thermal/thermal_netlink.c | 645 ++++++++++++++++++++++++++++++
>  include/linux/thermal.h           |  12 -
>  include/uapi/linux/thermal.h      |  80 +++-
>  5 files changed, 738 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/thermal/thermal_netlink.c
>
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 0c8b84a09b9a..1bbf0805fb04 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -5,7 +5,7 @@
>
>  obj-$(CONFIG_THERMAL)          += thermal_sys.o
>  thermal_sys-y                  += thermal_core.o thermal_sysfs.o \
> -                                       thermal_helpers.o
> +                                       thermal_helpers.o thermal_netlink.o
>
>  # interface to/from other layers providing sensors
>  thermal_sys-$(CONFIG_THERMAL_HWMON)            += thermal_hwmon.o
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 7e8f45db6bbf..08eb03fe4f69 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -52,6 +52,25 @@ int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
>
>  struct thermal_zone_device *thermal_zone_get_by_id(int id);
>
> +/* Netlink notification function */
> +int thermal_notify_tz_create(int tz_id, const char *name);
> +int thermal_notify_tz_delete(int tz_id);
> +int thermal_notify_tz_enable(int tz_id);
> +int thermal_notify_tz_disable(int tz_id);
> +int thermal_notify_tz_trip_down(int tz_id, int id);
> +int thermal_notify_tz_trip_up(int tz_id, int id);
> +int thermal_notify_tz_trip_delete(int tz_id, int id);
> +int thermal_notify_tz_trip_add(int tz_id, int id, int type,
> +                              int temp, int hyst);
> +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
> +                                 int temp, int hyst);
> +int thermal_notify_cdev_update(int cdev_id, int state);
> +int thermal_notify_cdev_add(int cdev_id, const char *name,
> +                           int min_state, int max_state);
> +int thermal_notify_cdev_delete(int cdev_id);
> +int thermal_notify_tz_gov_change(int tz_id, const char *name);
> +int thermal_genl_sampling_temp(int id, int temp);
> +
>  struct thermal_attr {
>         struct device_attribute attr;
>         char name[THERMAL_NAME_LENGTH];
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> new file mode 100644
> index 000000000000..a8d6a815a21d
> --- /dev/null
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * Generic netlink for thermal management framework
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <net/genetlink.h>
> +#include <uapi/linux/thermal.h>
> +
> +#include "thermal_core.h"
> +
> +static const struct genl_multicast_group thermal_genl_mcgrps[] = {
> +       { .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
> +       { .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
> +};
> +
> +static const struct nla_policy thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
> +       /* Thermal zone */
> +       [THERMAL_GENL_ATTR_TZ]                  = { .type = NLA_NESTED },
> +       [THERMAL_GENL_ATTR_TZ_ID]               = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_TEMP]             = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_TRIP]             = { .type = NLA_NESTED },
> +       [THERMAL_GENL_ATTR_TZ_TRIP_ID]          = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_TRIP_TEMP]        = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_TRIP_TYPE]        = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_TRIP_HYST]        = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_MODE]             = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT]      = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_TZ_NAME]             = { .type = NLA_STRING,
> +                                                   .len = THERMAL_NAME_LENGTH },
> +       /* Governor(s) */
> +       [THERMAL_GENL_ATTR_TZ_GOV]              = { .type = NLA_NESTED },
> +       [THERMAL_GENL_ATTR_TZ_GOV_NAME]         = { .type = NLA_STRING,
> +                                                   .len = THERMAL_NAME_LENGTH },
> +       /* Cooling devices */
> +       [THERMAL_GENL_ATTR_CDEV]                = { .type = NLA_NESTED },
> +       [THERMAL_GENL_ATTR_CDEV_ID]             = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_CDEV_CUR_STATE]      = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_CDEV_MAX_STATE]      = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_CDEV_MIN_STATE]      = { .type = NLA_U32 },
> +       [THERMAL_GENL_ATTR_CDEV_NAME]           = { .type = NLA_STRING,
> +                                                   .len = THERMAL_NAME_LENGTH },
> +};
> +
> +struct param {
> +       struct nlattr ** attrs;
> +       struct sk_buff *msg;
> +       const char *name;

You're reusing this variable to pass the name of the thermal zone,
cooling device and governor name. I guess its OK, but for the other
variables below, it makes sense to prefix with some string to make the
code easier to read.

> +       int tz_id;
> +       int cdev_id;
> +       int id;

Suggest renaming this to trip_id to make it easier to read in the code

> +       int temp;
> +       int type;
> +       int hyst;

Suggest prefixing these three with trip since these are properties of
trip points

> +       int state;
> +       int min_state;
> +       int max_state;

Suggest prefixing these three with cdev since these are properties of
cooling devices

> +};
> +
> +typedef int (*cb_t)(struct param *);
> +
> +static struct genl_family thermal_gnl_family;
> +
> +/************************** Sampling encoding *******************************/
> +
> +int thermal_genl_sampling_temp(int id, int temp)
> +{
> +       struct sk_buff *skb;
> +       void *hdr;
> +
> +       skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +       if (!skb)
> +               return -ENOMEM;
> +
> +       hdr = genlmsg_put(skb, 0, 0, &thermal_gnl_family, 0,
> +                         THERMAL_GENL_SAMPLING_TEMP);
> +       if (!hdr)
> +               return -EMSGSIZE;
> +
> +       if (nla_put_u32(skb, THERMAL_GENL_ATTR_TZ_ID, id))
> +               goto out_cancel;
> +
> +       if (nla_put_u32(skb, THERMAL_GENL_ATTR_TZ_TEMP, temp))
> +               goto out_cancel;
> +
> +       genlmsg_end(skb, hdr);
> +
> +       genlmsg_multicast(&thermal_gnl_family, skb, 0, 0, GFP_KERNEL);
> +
> +       return 0;
> +out_cancel:
> +       genlmsg_cancel(skb, hdr);
> +       nlmsg_free(skb);
> +
> +       return -EMSGSIZE;
> +}
> +
> +/**************************** Event encoding *********************************/
> +
> +static int thermal_genl_event_tz_create(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> +           nla_put_string(p->msg, THERMAL_GENL_ATTR_TZ_NAME, p->name))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_tz(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_tz_trip_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_TRIP_ID, p->id))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_tz_trip_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_TZ_TRIP_ID, p->id) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, p->type) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_TEMP, p->temp) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_TRIP_HYST, p->hyst))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_tz_trip_delete(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_TRIP_ID, p->id))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_cdev_add(struct param *p)
> +{
> +       if (nla_put_string(p->msg, THERMAL_GENL_ATTR_CDEV_NAME,
> +                          p->name) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID,
> +                       p->cdev_id) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_MIN_STATE,
> +                       p->min_state) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_MAX_STATE,
> +                       p->max_state))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_cdev_delete(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID, p->cdev_id))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_cdev_update(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_ID, p->cdev_id) ||
> +           nla_put_u32(p->msg, THERMAL_GENL_ATTR_CDEV_CUR_STATE, p->state))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_event_gov_change(struct param *p)
> +{
> +       if (nla_put_u32(p->msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id) ||
> +           nla_put_string(p->msg, THERMAL_GENL_ATTR_GOV_NAME, p->name))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +int thermal_genl_event_tz_delete(struct param *p)
> +       __attribute__((alias("thermal_genl_event_tz")));
> +
> +int thermal_genl_event_tz_enable(struct param *p)
> +       __attribute__((alias("thermal_genl_event_tz")));
> +
> +int thermal_genl_event_tz_disable(struct param *p)
> +       __attribute__((alias("thermal_genl_event_tz")));
> +
> +int thermal_genl_event_tz_trip_down(struct param *p)
> +       __attribute__((alias("thermal_genl_event_tz_trip_up")));
> +
> +int thermal_genl_event_tz_trip_change(struct param *)
> +       __attribute__((alias("thermal_genl_event_tz_trip_add")));
> +
> +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,
> +       [THERMAL_GENL_EVENT_TZ_ENABLE]          = thermal_genl_event_tz_enable,
> +       [THERMAL_GENL_EVENT_TZ_DISABLE]         = thermal_genl_event_tz_disable,
> +       [THERMAL_GENL_EVENT_TZ_TRIP_UP]         = thermal_genl_event_tz_trip_up,
> +       [THERMAL_GENL_EVENT_TZ_TRIP_DOWN]       = thermal_genl_event_tz_trip_down,
> +       [THERMAL_GENL_EVENT_TZ_TRIP_CHANGE]     = thermal_genl_event_tz_trip_change,
> +       [THERMAL_GENL_EVENT_TZ_TRIP_ADD]        = thermal_genl_event_tz_trip_add,
> +       [THERMAL_GENL_EVENT_TZ_TRIP_DELETE]     = thermal_genl_event_tz_trip_delete,
> +       [THERMAL_GENL_EVENT_CDEV_ADD]           = thermal_genl_event_cdev_add,
> +       [THERMAL_GENL_EVENT_CDEV_DELETE]        = thermal_genl_event_cdev_delete,
> +       [THERMAL_GENL_EVENT_CDEV_UPDATE]        = thermal_genl_event_cdev_update,
> +       [THERMAL_GENL_EVENT_TZ_GOV_CHANGE]      = thermal_genl_event_gov_change,
> +};
> +
> +/*
> + * Generic netlink event encoding
> + */
> +static int thermal_genl_send_event(enum thermal_genl_event event,
> +                                  struct param *p)
> +{
> +       struct sk_buff *msg;
> +       int ret = -EMSGSIZE;
> +       void *hdr;
> +
> +        msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);

Leading Whitespace error

> +       if (!msg)
> +               return -ENOMEM;
> +       p->msg = msg;
> +
> +       hdr = genlmsg_put(msg, 0, 0, &thermal_gnl_family, 0, event);
> +       if (!hdr)
> +               goto out_free_msg;
> +
> +       ret = event_cb[event](p);
> +       if (ret)
> +               goto out_cancel_msg;
> +
> +       genlmsg_end(msg, hdr);
> +
> +       genlmsg_multicast(&thermal_gnl_family, msg, 0, 1, GFP_KERNEL);
> +
> +       return 0;
> +
> +out_cancel_msg:
> +       genlmsg_cancel(msg, hdr);
> +out_free_msg:
> +       nlmsg_free(msg);
> +
> +       return ret;
> +}
> +
> +int thermal_notify_tz_create(int tz_id, const char *name)
> +{
> +       struct param p = { .tz_id = tz_id, .name = name };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_CREATE, &p);
> +}
> +
> +int thermal_notify_tz_delete(int tz_id)
> +{
> +       struct param p = { .tz_id = tz_id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DELETE, &p);
> +}
> +
> +int thermal_notify_tz_enable(int tz_id)
> +{
> +       struct param p = { .tz_id = tz_id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_ENABLE, &p);
> +}
> +
> +int thermal_notify_tz_disable(int tz_id)
> +{
> +       struct param p = { .tz_id = tz_id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_DISABLE, &p);
> +}
> +
> +int thermal_notify_tz_trip_down(int tz_id, int id)
> +{
> +       struct param p = { .tz_id = tz_id, .id = id };
> +

it would be nice for id to be trip_id for readability here and further below.

> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DOWN, &p);
> +}
> +
> +int thermal_notify_tz_trip_up(int tz_id, int id)
> +{
> +       struct param p = { .tz_id = tz_id, .id = id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_UP, &p);
> +}
> +
> +int thermal_notify_tz_trip_add(int tz_id, int id, int type, int temp, int hyst)
> +{
> +       struct param p = { .tz_id = tz_id, .id = id, .type = type,
> +                          .temp = temp, .hyst = hyst };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_ADD, &p);
> +}
> +
> +int thermal_notify_tz_trip_delete(int tz_id, int id)
> +{
> +       struct param p = { .tz_id = tz_id, .id = id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_DELETE, &p);
> +}
> +
> +int thermal_notify_tz_trip_change(int tz_id, int id, int type, int temp, int hyst)
> +{
> +       struct param p = { .tz_id = tz_id, .id = id, .type = type,
> +                          .temp = temp, .hyst = hyst };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_TRIP_CHANGE, &p);
> +}
> +
> +int thermal_notify_cdev_update(int cdev_id, int state)
> +{
> +       struct param p = { .cdev_id = cdev_id, .state = state };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_UPDATE, &p);
> +}
> +
> +int thermal_notify_cdev_add(int cdev_id, const char *name,
> +                           int min_state, int max_state)
> +{
> +       struct param p = { .cdev_id = cdev_id, .name = name,
> +                          .min_state = min_state, .max_state = max_state };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_ADD, &p);
> +}
> +
> +int thermal_notify_cdev_delete(int cdev_id)
> +{
> +       struct param p = { .cdev_id = cdev_id };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_CDEV_DELETE, &p);
> +}
> +
> +int thermal_notify_tz_gov_change(int tz_id, const char *name)
> +{
> +       struct param p = { .tz_id = tz_id, .name = name };
> +
> +       return thermal_genl_send_event(THERMAL_GENL_EVENT_TZ_GOV_CHANGE, &p);
> +}
> +
> +/*************************** Command encoding ********************************/
> +
> +static int __thermal_genl_cmd_tz_get(struct thermal_zone_device *tz, void *data)
> +{
> +       struct sk_buff *msg = data;
> +
> +       if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, tz->id) ||
> +           nla_put_string(msg, THERMAL_GENL_ATTR_TZ_NAME, tz->type))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_cmd_tz_get(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct nlattr *start_tz;
> +       int ret;
> +
> +       start_tz = nla_nest_start(msg, THERMAL_GENL_ATTR_TZ);
> +       if (!start_tz)
> +               return -EMSGSIZE;
> +
> +       ret = for_each_thermal_zone(__thermal_genl_cmd_tz_get, msg);
> +       if (ret)
> +               goto out_cancel_nest;
> +
> +       nla_nest_end(msg, start_tz);
> +
> +       return 0;
> +
> +out_cancel_nest:
> +       nla_nest_cancel(msg, start_tz);
> +
> +       return ret;
> +}
> +
> +static int thermal_genl_cmd_tz_get_trip(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct thermal_zone_device *tz;
> +       struct nlattr *start_trip;
> +       int i, id;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> +       tz = thermal_zone_get_by_id(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       start_trip = nla_nest_start(msg, THERMAL_GENL_ATTR_TZ_TRIP);
> +       if (!start_trip)
> +               return -EMSGSIZE;
> +
> +       mutex_lock(&tz->lock);
> +
> +       for (i = 0; i < tz->trips; i++) {
> +
> +               enum thermal_trip_type type;
> +               int temp, hyst;
> +
> +               tz->ops->get_trip_type(tz, i, &type);
> +               tz->ops->get_trip_temp(tz, i, &temp);
> +               tz->ops->get_trip_hyst(tz, i, &hyst);
> +
> +               if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_ID, i) ||
> +                   nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TYPE, type) ||
> +                   nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_TEMP, temp) ||
> +                   nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TRIP_HYST, hyst))
> +                       goto out_cancel_nest;
> +       }
> +
> +       mutex_unlock(&tz->lock);
> +
> +       nla_nest_end(msg, start_trip);
> +
> +       return 0;
> +
> +out_cancel_nest:
> +       mutex_unlock(&tz->lock);
> +
> +       return -EMSGSIZE;
> +}
> +
> +static int thermal_genl_cmd_tz_get_temp(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct thermal_zone_device *tz;
> +       int temp, ret, id;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> +       tz = thermal_zone_get_by_id(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       ret = thermal_zone_get_temp(tz, &temp);
> +       if (ret)
> +               return ret;
> +
> +       if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id) ||
> +           nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_TEMP, temp))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_cmd_tz_get_gov(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct thermal_zone_device *tz;
> +       int id, ret = 0;
> +
> +       if (!p->attrs[THERMAL_GENL_ATTR_TZ_ID])
> +               return -EINVAL;
> +
> +       id = nla_get_u32(p->attrs[THERMAL_GENL_ATTR_TZ_ID]);
> +
> +       tz = thermal_zone_get_by_id(id);
> +       if (!tz)
> +               return -EINVAL;
> +
> +       mutex_lock(&tz->lock);
> +
> +       if (nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id) ||
> +           nla_put_string(msg, THERMAL_GENL_ATTR_TZ_GOV_NAME,
> +                          tz->governor->name))
> +               ret = -EMSGSIZE;
> +
> +       mutex_unlock(&tz->lock);
> +
> +       return ret;
> +}
> +
> +static int __thermal_genl_cmd_cdev_get(struct thermal_cooling_device *cdev,
> +                                      void *data)
> +{
> +       struct sk_buff *msg = data;
> +
> +       if (nla_put_u32(msg, THERMAL_GENL_ATTR_CDEV_ID, cdev->id))
> +               return -EMSGSIZE;
> +
> +       if (nla_put_string(msg, THERMAL_GENL_ATTR_CDEV_NAME, cdev->type))
> +               return -EMSGSIZE;
> +
> +       return 0;
> +}
> +
> +static int thermal_genl_cmd_cdev_get(struct param *p)
> +{
> +       struct sk_buff *msg = p->msg;
> +       struct nlattr *start_cdev;
> +       int ret;
> +
> +       start_cdev = nla_nest_start(msg, THERMAL_GENL_ATTR_CDEV);
> +       if (!start_cdev)
> +               return -EMSGSIZE;
> +
> +       ret = for_each_thermal_cooling_device(__thermal_genl_cmd_cdev_get, msg);
> +       if (ret)
> +               goto out_cancel_nest;
> +
> +       nla_nest_end(msg, start_cdev);
> +
> +       return 0;
> +out_cancel_nest:
> +       nla_nest_cancel(msg, start_cdev);
> +
> +       return ret;
> +}
> +
> +static cb_t cmd_cb[] = {
> +       [THERMAL_GENL_CMD_TZ_GET]       = thermal_genl_cmd_tz_get,
> +       [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,
> +};
> +
> +static int thermal_genl_cmd_dumpit(struct sk_buff *skb,
> +                                  struct netlink_callback *cb)
> +{
> +       struct param p = { .msg = skb };
> +       const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +       int cmd = info->ops->cmd;
> +       int ret = -EMSGSIZE;
> +       void *hdr;
> +
> +       hdr = genlmsg_put(skb, 0, 0, &thermal_gnl_family, 0, cmd);
> +       if (!hdr)
> +               return -EMSGSIZE;
> +
> +       ret = cmd_cb[cmd](&p);
> +       if (ret)
> +               goto out_cancel_msg;
> +
> +       genlmsg_end(skb, hdr);
> +
> +       return 0;
> +
> +out_cancel_msg:
> +       genlmsg_cancel(skb, hdr);
> +
> +       return ret;
> +}
> +
> +static int thermal_genl_cmd_doit(struct sk_buff *skb,
> +                                struct genl_info *info)
> +{
> +       struct param p = { .attrs = info->attrs };
> +       struct sk_buff *msg;
> +       void *hdr;
> +       int cmd = info->genlhdr->cmd;
> +       int ret = -EMSGSIZE;
> +
> +        msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);

whitespace error

> +       if (!msg)
> +               return -ENOMEM;
> +       p.msg = msg;
> +
> +        hdr = genlmsg_put_reply(msg, info, &thermal_gnl_family, 0, cmd);

whitespace error

> +       if (!hdr)
> +               goto out_free_msg;
> +
> +       ret = cmd_cb[cmd](&p);
> +       if (ret)
> +               goto out_cancel_msg;
> +
> +       genlmsg_end(msg, hdr);
> +
> +       return genlmsg_reply(msg, info);
> +
> +out_cancel_msg:
> +       genlmsg_cancel(msg, hdr);
> +out_free_msg:
> +       nlmsg_free(msg);
> +
> +       return ret;
> +}
> +
> +static const struct genl_ops thermal_genl_ops[] = {
> +       {
> +               .cmd = THERMAL_GENL_CMD_TZ_GET,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .dumpit = thermal_genl_cmd_dumpit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_TZ_GET_TRIP,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_TZ_GET_TEMP,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_TZ_GET_GOV,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = thermal_genl_cmd_doit,
> +       },
> +       {
> +               .cmd = THERMAL_GENL_CMD_CDEV_GET,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .dumpit = thermal_genl_cmd_dumpit,
> +       },
> +};
> +
> +static struct genl_family thermal_gnl_family __ro_after_init = {
> +       .hdrsize        = 0,
> +       .name           = THERMAL_GENL_FAMILY_NAME,
> +       .version        = THERMAL_GENL_VERSION,
> +       .maxattr        = THERMAL_GENL_ATTR_MAX,
> +       .policy         = thermal_genl_policy,
> +       .ops            = thermal_genl_ops,
> +       .n_ops          = ARRAY_SIZE(thermal_genl_ops),
> +       .mcgrps         = thermal_genl_mcgrps,
> +       .n_mcgrps       = ARRAY_SIZE(thermal_genl_mcgrps),
> +};
> +
> +static int __init thermal_netlink_init(void)
> +{
> +       return genl_register_family(&thermal_gnl_family);
> +}
> +core_initcall(thermal_netlink_init);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index fc93a6348082..d92643e310e2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -37,18 +37,6 @@ struct thermal_cooling_device;
>  struct thermal_instance;
>  struct thermal_attr;
>
> -enum thermal_device_mode {
> -       THERMAL_DEVICE_DISABLED = 0,
> -       THERMAL_DEVICE_ENABLED,
> -};
> -
> -enum thermal_trip_type {
> -       THERMAL_TRIP_ACTIVE = 0,
> -       THERMAL_TRIP_PASSIVE,
> -       THERMAL_TRIP_HOT,
> -       THERMAL_TRIP_CRITICAL,
> -};
> -

This move should happen in a separate patch, IMO.

>  enum thermal_trend {
>         THERMAL_TREND_STABLE, /* temperature is stable */
>         THERMAL_TREND_RAISING, /* temperature is raising */
> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
> index 22df67ed9e9c..49289f9d993b 100644
> --- a/include/uapi/linux/thermal.h
> +++ b/include/uapi/linux/thermal.h
> @@ -4,21 +4,87 @@
>
>  #define THERMAL_NAME_LENGTH    20
>
> +enum thermal_device_mode {
> +       THERMAL_DEVICE_DISABLED = 0,
> +       THERMAL_DEVICE_ENABLED,
> +};
> +
> +enum thermal_trip_type {
> +       THERMAL_TRIP_ACTIVE = 0,
> +       THERMAL_TRIP_PASSIVE,
> +       THERMAL_TRIP_HOT,
> +       THERMAL_TRIP_CRITICAL,
> +};
> +
>  /* Adding event notification support elements */
> -#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> -#define THERMAL_GENL_VERSION                    0x02
> -#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_grp"
> +#define THERMAL_GENL_FAMILY_NAME               "thermal"
> +#define THERMAL_GENL_VERSION                   0x01

You set it to 2 in the previous patch and now reset it to 1 here.

I think we are sticking to version 1, so the hunk can just be removed
from the previous patch.

> +#define THERMAL_GENL_SAMPLING_GROUP_NAME       "sampling"
> +#define THERMAL_GENL_EVENT_GROUP_NAME          "event"
>
> -/* attributes of thermal_genl_family */
> -enum {
> +/* Attributes of thermal_genl_family */
> +enum thermal_genl_attr {
>         THERMAL_GENL_ATTR_UNSPEC,
> +       THERMAL_GENL_ATTR_TZ,
> +       THERMAL_GENL_ATTR_TZ_ID,
> +       THERMAL_GENL_ATTR_TZ_TEMP,
> +       THERMAL_GENL_ATTR_TZ_TRIP,
> +       THERMAL_GENL_ATTR_TZ_TRIP_ID,
> +       THERMAL_GENL_ATTR_TZ_TRIP_TYPE,
> +       THERMAL_GENL_ATTR_TZ_TRIP_TEMP,
> +       THERMAL_GENL_ATTR_TZ_TRIP_HYST,
> +       THERMAL_GENL_ATTR_TZ_MODE,
> +       THERMAL_GENL_ATTR_TZ_NAME,
> +       THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT,
> +       THERMAL_GENL_ATTR_TZ_GOV,
> +       THERMAL_GENL_ATTR_TZ_GOV_NAME,
> +       THERMAL_GENL_ATTR_CDEV,
> +       THERMAL_GENL_ATTR_CDEV_ID,
> +       THERMAL_GENL_ATTR_CDEV_CUR_STATE,
> +       THERMAL_GENL_ATTR_CDEV_MAX_STATE,
> +       THERMAL_GENL_ATTR_CDEV_MIN_STATE,
> +       THERMAL_GENL_ATTR_CDEV_NAME,
> +       THERMAL_GENL_ATTR_GOV_NAME,
> +
>         __THERMAL_GENL_ATTR_MAX,
>  };
>  #define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
>
> -/* commands supported by the thermal_genl_family */
> -enum {
> +enum thermal_genl_sampling {
> +       THERMAL_GENL_SAMPLING_TEMP,
> +       __THERMAL_GENL_SAMPLING_MAX,
> +};
> +#define THERMAL_GENL_SAMPLING_MAX (__THERMAL_GENL_SAMPLING_MAX - 1)
> +
> +/* Events of thermal_genl_family */
> +enum thermal_genl_event {
> +       THERMAL_GENL_EVENT_UNSPEC,
> +       THERMAL_GENL_EVENT_TZ_CREATE,           /* Thermal zone creation */
> +       THERMAL_GENL_EVENT_TZ_DELETE,           /* Thermal zone deletion */
> +       THERMAL_GENL_EVENT_TZ_DISABLE,          /* Thermal zone disabed */
> +       THERMAL_GENL_EVENT_TZ_ENABLE,           /* Thermal zone enabled */
> +       THERMAL_GENL_EVENT_TZ_TRIP_UP,          /* Trip point crossed the way up */
> +       THERMAL_GENL_EVENT_TZ_TRIP_DOWN,        /* Trip point crossed the way down */
> +       THERMAL_GENL_EVENT_TZ_TRIP_CHANGE,      /* Trip point changed */
> +       THERMAL_GENL_EVENT_TZ_TRIP_ADD,         /* Trip point added */
> +       THERMAL_GENL_EVENT_TZ_TRIP_DELETE,      /* Trip point deleted */
> +       THERMAL_GENL_EVENT_CDEV_ADD,            /* Cdev bound to the thermal zone */
> +       THERMAL_GENL_EVENT_CDEV_DELETE,         /* Cdev unbound */
> +       THERMAL_GENL_EVENT_CDEV_UPDATE,         /* Cdev state updated */
> +       THERMAL_GENL_EVENT_TZ_GOV_CHANGE,       /* Governor policy changed  */
> +       __THERMAL_GENL_EVENT_MAX,
> +};
> +#define THERMAL_GENL_EVENT_MAX (__THERMAL_GENL_EVENT_MAX - 1)
> +
> +/* Commands supported by the thermal_genl_family */
> +enum thermal_genl_cmd {
>         THERMAL_GENL_CMD_UNSPEC,
> +       THERMAL_GENL_CMD_TZ_GET,        /* List thermal zones id */

Rename to THERMAL_GENL_CMD_TZ_GET_IDS to match intent ?

Also, s/List/List of/


> +       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_MAX,
>  };
>  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> --
> 2.17.1
>

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Zhang Rui]

On Thu, 2020-06-25 at 16:45 +0200, Daniel Lezcano wrote:
> Initially the thermal framework had a very simple notification
> mechanism to send generic netlink messages to the userspace.
> 
> The notification function was never called from anywhere and the
> corresponding dead code was removed. It was probably a first attempt
> to introduce the netlink notification.
> 
> At LPC2018, the presentation "Linux thermal: User kernel interface",
> proposed to create the notifications to the userspace via a kfifo.
> 
> The advantage of the kfifo is the performance. It is usually used
> from
> a 1:1 communication channel where a driver captures data and send
> them
> as fast as possible to an userspace process.
> 
> The inconvenient is the process uses the notification channel
> exclusively, thus no other process is allowed to use the channel to
> get temperature or notifications.
> 
> The purpose of this series is to provide a fully netlink featured
> thermal management.
> 
> This patch is defining a generic netlink API to discover the current
> thermal setup, get events and temperature sampling. As any genetlink
> protocol, it can evolve and the versionning allows to keep the
> backward
> compatibility.
> 
> In order to not flood the user with a single channel data, there are
> two multicast channels, one for the temperature sampling when the
> thermal zone is updated and another one for the events, so the user
> can get the events only without the thermal zone temperature
> sampling. All these events are from the ones presented at the
> LPC2018.
> 
> Also, a list of commands to discover the thermal setup is given and
> can be extended on purpose.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/Makefile          |   2 +-
>  drivers/thermal/thermal_core.h    |  19 +
>  drivers/thermal/thermal_netlink.c | 645
> ++++++++++++++++++++++++++++++
>  include/linux/thermal.h           |  12 -
>  include/uapi/linux/thermal.h      |  80 +++-
>  5 files changed, 738 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/thermal/thermal_netlink.c
> 
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 0c8b84a09b9a..1bbf0805fb04 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -5,7 +5,7 @@
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
>  thermal_sys-y			+= thermal_core.o
> thermal_sysfs.o \
> -					thermal_helpers.o
> +					thermal_helpers.o
> thermal_netlink.o
>  
>  # interface to/from other layers providing sensors
>  thermal_sys-$(CONFIG_THERMAL_HWMON)		+= thermal_hwmon.o
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 7e8f45db6bbf..08eb03fe4f69 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -52,6 +52,25 @@ int for_each_thermal_governor(int (*cb)(struct
> thermal_governor *, void *),
>  
>  struct thermal_zone_device *thermal_zone_get_by_id(int id);
>  
> +/* Netlink notification function */
> +int thermal_notify_tz_create(int tz_id, const char *name);
> +int thermal_notify_tz_delete(int tz_id);

> +int thermal_notify_tz_enable(int tz_id);
> +int thermal_notify_tz_disable(int tz_id);

these two will be used after merging the mode enhancement patches from
Andrzej Pietrasiewicz, right?


> +int thermal_notify_tz_trip_down(int tz_id, int id);
> +int thermal_notify_tz_trip_up(int tz_id, int id);

> +int thermal_notify_tz_trip_delete(int tz_id, int id);
> +int thermal_notify_tz_trip_add(int tz_id, int id, int type,
> +			       int temp, int hyst);

is there any case we need to use these two?

> +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
> +				  int temp, int hyst);
> +int thermal_notify_cdev_update(int cdev_id, int state);

This is used when the cdev cur_state is changed.
what about cases that cdev->max_state changes? I think we need an extra
event for it, right?
> 
> +int thermal_notify_cdev_add(int cdev_id, const char *name,
> +			    int min_state, int max_state);
> +int thermal_notify_cdev_delete(int cdev_id);

These two should be used in patch 5/5.

> +int thermal_notify_tz_gov_change(int tz_id, const char *name);
> +int thermal_genl_sampling_temp(int id, int temp);
> +

 struct thermal_attr {
>  	struct device_attribute attr;
>  	char name[THERMAL_NAME_LENGTH];
> diff --git a/drivers/thermal/thermal_netlink.c
> b/drivers/thermal/thermal_netlink.c
> new file mode 100644
> index 000000000000..a8d6a815a21d
> --- /dev/null
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * Generic netlink for thermal management framework
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <net/genetlink.h>
> +#include <uapi/linux/thermal.h>
> +
> +#include "thermal_core.h"
> +
> +static const struct genl_multicast_group thermal_genl_mcgrps[] = {
> +	{ .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
> +	{ .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
> +};
> +
> +static const struct nla_policy
> thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
> +	/* Thermal zone */
> +	[THERMAL_GENL_ATTR_TZ]			= { .type =
> NLA_NESTED },
> +	[THERMAL_GENL_ATTR_TZ_ID]		= { .type = NLA_U32 },
> +	[THERMAL_GENL_ATTR_TZ_TEMP]		= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_TZ_TRIP]		= { .type =
> NLA_NESTED },
> +	[THERMAL_GENL_ATTR_TZ_TRIP_ID]		= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_TZ_TRIP_TEMP]	= { .type = NLA_U32 },
> +	[THERMAL_GENL_ATTR_TZ_TRIP_TYPE]	= { .type = NLA_U32 },
> +	[THERMAL_GENL_ATTR_TZ_TRIP_HYST]	= { .type = NLA_U32 },
> +	[THERMAL_GENL_ATTR_TZ_MODE]		= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT]	= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_TZ_NAME]		= { .type =
> NLA_STRING,
> +						    .len =
> THERMAL_NAME_LENGTH },
> +	/* Governor(s) */
> +	[THERMAL_GENL_ATTR_TZ_GOV]		= { .type =
> NLA_NESTED },
> +	[THERMAL_GENL_ATTR_TZ_GOV_NAME]		= { .type =
> NLA_STRING,
> +						    .len =
> THERMAL_NAME_LENGTH },
> +	/* Cooling devices */
> +	[THERMAL_GENL_ATTR_CDEV]		= { .type = NLA_NESTED },
> +	[THERMAL_GENL_ATTR_CDEV_ID]		= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_CDEV_CUR_STATE]	= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_CDEV_MAX_STATE]	= { .type = NLA_U32
> },
> +	[THERMAL_GENL_ATTR_CDEV_MIN_STATE]	= { .type = NLA_U32
> },

is there any case that min_state does not equal 0?

> +	[THERMAL_GENL_ATTR_CDEV_NAME]		= { .type =
> NLA_STRING,
> +						    .len =
> THERMAL_NAME_LENGTH },
> +};
> +

[...]
> +
> +static cb_t cmd_cb[] = {
> +	[THERMAL_GENL_CMD_TZ_GET]	= thermal_genl_cmd_tz_get,

> +	[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,

A dumb question, can we share the same command for the above three?
Say,
THERMAL_GENL_CMD_GET_ALL_TZ or THERMAL_GENL_CMD_GET_TZS returns the id
&& name of all the thermal zones.
THERMAL_GENL_CMD_GET_TZ returns general information of a specified
thermal zone, include trip points, governor and temperature. My
understanding is that all these information will be queried once, in
the very beginning, and then userpsace relies on the netlink events to
give updated information, right?

This could simplify the kernel code and also userspace a bit.

thanks,
rui

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Daniel Lezcano]

Hi Zhang,

thanks for taking the time to review


On 30/06/2020 18:12, Zhang Rui wrote:

[ ... ]

>> +int thermal_notify_tz_enable(int tz_id);
>> +int thermal_notify_tz_disable(int tz_id);
> 
> these two will be used after merging the mode enhancement patches from
> Andrzej Pietrasiewicz, right?

Yes, that is correct.

>> +int thermal_notify_tz_trip_down(int tz_id, int id);
>> +int thermal_notify_tz_trip_up(int tz_id, int id);
> 
>> +int thermal_notify_tz_trip_delete(int tz_id, int id);
>> +int thermal_notify_tz_trip_add(int tz_id, int id, int type,
>> +			       int temp, int hyst);
> 
> is there any case we need to use these two?
There is the initial proposal to add/del trip points via sysfs which is
not merged because of no comments and the presentation from Srinivas
giving the 'add' and 'del' notification description, so I assumed the
feature would be added soon.

These functions are here ready to be connected to the core code. If
anyone is planning to implement add/del trip point, he won't have to
implement these two notifications as they will be ready to be called.

>> +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
>> +				  int temp, int hyst);
>> +int thermal_notify_cdev_update(int cdev_id, int state);
> 
> This is used when the cdev cur_state is changed.
> what about cases that cdev->max_state changes? I think we need an extra
> event for it, right?

Sure, I can add:

int thermal_notify_cdev_change(...)

>> +int thermal_notify_cdev_add(int cdev_id, const char *name,
>> +			    int min_state, int max_state);
>> +int thermal_notify_cdev_delete(int cdev_id);
> 
> These two should be used in patch 5/5.

Sure.

>> +int thermal_notify_tz_gov_change(int tz_id, const char *name);
>> +int thermal_genl_sampling_temp(int id, int temp);
>> +
> 
>  struct thermal_attr {
>>  	struct device_attribute attr;
>>  	char name[THERMAL_NAME_LENGTH];
>> diff --git a/drivers/thermal/thermal_netlink.c
>> b/drivers/thermal/thermal_netlink.c
>> new file mode 100644
>> index 000000000000..a8d6a815a21d
>> --- /dev/null
>> +++ b/drivers/thermal/thermal_netlink.c
>> @@ -0,0 +1,645 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2020 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * Generic netlink for thermal management framework
>> + */
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <net/genetlink.h>
>> +#include <uapi/linux/thermal.h>
>> +
>> +#include "thermal_core.h"
>> +
>> +static const struct genl_multicast_group thermal_genl_mcgrps[] = {
>> +	{ .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
>> +	{ .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
>> +};
>> +
>> +static const struct nla_policy
>> thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
>> +	/* Thermal zone */
>> +	[THERMAL_GENL_ATTR_TZ]			= { .type =
>> NLA_NESTED },
>> +	[THERMAL_GENL_ATTR_TZ_ID]		= { .type = NLA_U32 },
>> +	[THERMAL_GENL_ATTR_TZ_TEMP]		= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_TZ_TRIP]		= { .type =
>> NLA_NESTED },
>> +	[THERMAL_GENL_ATTR_TZ_TRIP_ID]		= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_TZ_TRIP_TEMP]	= { .type = NLA_U32 },
>> +	[THERMAL_GENL_ATTR_TZ_TRIP_TYPE]	= { .type = NLA_U32 },
>> +	[THERMAL_GENL_ATTR_TZ_TRIP_HYST]	= { .type = NLA_U32 },
>> +	[THERMAL_GENL_ATTR_TZ_MODE]		= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT]	= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_TZ_NAME]		= { .type =
>> NLA_STRING,
>> +						    .len =
>> THERMAL_NAME_LENGTH },
>> +	/* Governor(s) */
>> +	[THERMAL_GENL_ATTR_TZ_GOV]		= { .type =
>> NLA_NESTED },
>> +	[THERMAL_GENL_ATTR_TZ_GOV_NAME]		= { .type =
>> NLA_STRING,
>> +						    .len =
>> THERMAL_NAME_LENGTH },
>> +	/* Cooling devices */
>> +	[THERMAL_GENL_ATTR_CDEV]		= { .type = NLA_NESTED },
>> +	[THERMAL_GENL_ATTR_CDEV_ID]		= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_CDEV_CUR_STATE]	= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_CDEV_MAX_STATE]	= { .type = NLA_U32
>> },
>> +	[THERMAL_GENL_ATTR_CDEV_MIN_STATE]	= { .type = NLA_U32
>> },
> 
> is there any case that min_state does not equal 0?

You are right, there is no min state for a cooling device, only for the
instances in the thermal zones. I will fix that.

>> +	[THERMAL_GENL_ATTR_CDEV_NAME]		= { .type =
>> NLA_STRING,
>> +						    .len =
>> THERMAL_NAME_LENGTH },
>> +};
>> +
> 
> [...]
>> +
>> +static cb_t cmd_cb[] = {
>> +	[THERMAL_GENL_CMD_TZ_GET]	= thermal_genl_cmd_tz_get,
> 
>> +	[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,
> 
> A dumb question, can we share the same command for the above three?
> Say,
> THERMAL_GENL_CMD_GET_ALL_TZ or THERMAL_GENL_CMD_GET_TZS returns the id
> && name of all the thermal zones.
> THERMAL_GENL_CMD_GET_TZ returns general information of a specified
> thermal zone, include trip points, governor and temperature. My
> understanding is that all these information will be queried once, in
> the very beginning, and then userpsace relies on the netlink events to
> give updated information, right?
> 
> This could simplify the kernel code and also userspace a bit.

Actually the userspace still need a 'get temp' or 'get trip' commands.

get temp : Get the temperature at any time, like reading the sysfs
temperature

get trip : Get the trip point information when a trip event happens, no
need to get a full discovery of the thermal zones before.

If I send a bulk message containing all the thermal zones, their trips
point, the governors and the cooling devices, that will force the
userspace to deal with multiple level of nested arrays. With netlinks
that makes the code really more complicated and prone to errors.

With this approach, if the userspace is interested only on "gpu", it can
get the thermal zone id when getting all the thermal zones <id, name>
and then ask for the information it is interested in.

Well I thought that is be more flexible.





-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Zhang Rui]

Hi, Daniel,

On Tue, 2020-06-30 at 20:32 +0200, Daniel Lezcano wrote:
> Hi Zhang,
> 
> thanks for taking the time to review
> 
> 
> On 30/06/2020 18:12, Zhang Rui wrote:
> 
> [ ... ]
> 
> > > +int thermal_notify_tz_enable(int tz_id);
> > > +int thermal_notify_tz_disable(int tz_id);
> > 
> > these two will be used after merging the mode enhancement patches
> > from
> > Andrzej Pietrasiewicz, right?
> 
> Yes, that is correct.
> 
> > > +int thermal_notify_tz_trip_down(int tz_id, int id);
> > > +int thermal_notify_tz_trip_up(int tz_id, int id);
> > > +int thermal_notify_tz_trip_delete(int tz_id, int id);
> > > +int thermal_notify_tz_trip_add(int tz_id, int id, int type,
> > > +			       int temp, int hyst);
> > 
> > is there any case we need to use these two?
> 
> There is the initial proposal to add/del trip points via sysfs which
> is
> not merged because of no comments and the presentation from Srinivas
> giving the 'add' and 'del' notification description, so I assumed the
> feature would be added soon.
> 
> These functions are here ready to be connected to the core code. If
> anyone is planning to implement add/del trip point, he won't have to
> implement these two notifications as they will be ready to be called.
> 
Then I'd prefer we only introduce the events that are used or will be
used soon, like the tz disable/enable, to avoid some potential dead
code.
We can easily add more events when they are needed.

Srinivas, do you have plan to use the trip add/delete events?

> > > +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
> > > +				  int temp, int hyst);
> > > +int thermal_notify_cdev_update(int cdev_id, int state);
> > 
> > This is used when the cdev cur_state is changed.
> > what about cases that cdev->max_state changes? I think we need an
> > extra
> > event for it, right?
> 
> Sure, I can add:
> 
> int thermal_notify_cdev_change(...)

thermal_notify_cdev_change() and thermal_notify_cdev_update() looks
confusing to me.
Can we use thermal_notify_cdev_update_cur() and
thermal_notify_cdev_update_max()?
Or can we use one event, with updated cur_state and max_state?

> > > +int thermal_notify_cdev_add(int cdev_id, const char *name,
> > > +			    int min_state, int max_state);
> > > +int thermal_notify_cdev_delete(int cdev_id);
> > 
> > These two should be used in patch 5/5.
> 
> Sure.
> 
> > > +int thermal_notify_tz_gov_change(int tz_id, const char *name);
> > > +int thermal_genl_sampling_temp(int id, int temp);
> > > +
> > 
> >  struct thermal_attr {
> > >  	struct device_attribute attr;
> > >  	char name[THERMAL_NAME_LENGTH];
> > > diff --git a/drivers/thermal/thermal_netlink.c
> > > b/drivers/thermal/thermal_netlink.c
> > > new file mode 100644
> > > index 000000000000..a8d6a815a21d
> > > --- /dev/null
> > > +++ b/drivers/thermal/thermal_netlink.c
> > > @@ -0,0 +1,645 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2020 Linaro Limited
> > > + *
> > > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > + *
> > > + * Generic netlink for thermal management framework
> > > + */
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <net/genetlink.h>
> > > +#include <uapi/linux/thermal.h>
> > > +
> > > +#include "thermal_core.h"
> > > +
> > > +static const struct genl_multicast_group thermal_genl_mcgrps[] =
> > > {
> > > +	{ .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
> > > +	{ .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
> > > +};
> > > +
> > > +static const struct nla_policy
> > > thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
> > > +	/* Thermal zone */
> > > +	[THERMAL_GENL_ATTR_TZ]			= { .type =
> > > NLA_NESTED },
> > > +	[THERMAL_GENL_ATTR_TZ_ID]		= { .type = NLA_U32 },
> > > +	[THERMAL_GENL_ATTR_TZ_TEMP]		= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_TZ_TRIP]		= { .type =
> > > NLA_NESTED },
> > > +	[THERMAL_GENL_ATTR_TZ_TRIP_ID]		= { .type =
> > > NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_TZ_TRIP_TEMP]	= { .type = NLA_U32 },
> > > +	[THERMAL_GENL_ATTR_TZ_TRIP_TYPE]	= { .type = NLA_U32 },
> > > +	[THERMAL_GENL_ATTR_TZ_TRIP_HYST]	= { .type = NLA_U32 },
> > > +	[THERMAL_GENL_ATTR_TZ_MODE]		= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_TZ_CDEV_WEIGHT]	= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_TZ_NAME]		= { .type =
> > > NLA_STRING,
> > > +						    .len =
> > > THERMAL_NAME_LENGTH },
> > > +	/* Governor(s) */
> > > +	[THERMAL_GENL_ATTR_TZ_GOV]		= { .type =
> > > NLA_NESTED },
> > > +	[THERMAL_GENL_ATTR_TZ_GOV_NAME]		= { .type =
> > > NLA_STRING,
> > > +						    .len =
> > > THERMAL_NAME_LENGTH },
> > > +	/* Cooling devices */
> > > +	[THERMAL_GENL_ATTR_CDEV]		= { .type = NLA_NESTED },
> > > +	[THERMAL_GENL_ATTR_CDEV_ID]		= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_CDEV_CUR_STATE]	= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_CDEV_MAX_STATE]	= { .type = NLA_U32
> > > },
> > > +	[THERMAL_GENL_ATTR_CDEV_MIN_STATE]	= { .type = NLA_U32
> > > },
> > 
> > is there any case that min_state does not equal 0?
> 
> You are right, there is no min state for a cooling device, only for
> the
> instances in the thermal zones. I will fix that.
> 
> > > +	[THERMAL_GENL_ATTR_CDEV_NAME]		= { .type =
> > > NLA_STRING,
> > > +						    .len =
> > > THERMAL_NAME_LENGTH },
> > > +};
> > > +
> > 
> > [...]
> > > +
> > > +static cb_t cmd_cb[] = {
> > > +	[THERMAL_GENL_CMD_TZ_GET]	= thermal_genl_cmd_tz_get,
> > > +	[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,
> > 
> > A dumb question, can we share the same command for the above three?
> > Say,
> > THERMAL_GENL_CMD_GET_ALL_TZ or THERMAL_GENL_CMD_GET_TZS returns the
> > id
> > && name of all the thermal zones.
> > THERMAL_GENL_CMD_GET_TZ returns general information of a specified
> > thermal zone, include trip points, governor and temperature. My
> > understanding is that all these information will be queried once,
> > in
> > the very beginning, and then userpsace relies on the netlink events
> > to
> > give updated information, right?
> > 
> > This could simplify the kernel code and also userspace a bit.
> 
> Actually the userspace still need a 'get temp' or 'get trip'
> commands.
> 
> get temp : Get the temperature at any time, like reading the sysfs
> temperature
> 
> get trip : Get the trip point information when a trip event happens,
> no
> need to get a full discovery of the thermal zones before.
> 
> If I send a bulk message containing all the thermal zones, their
> trips
> point, the governors and the cooling devices, that will force the
> userspace to deal with multiple level of nested arrays. With netlinks
> that makes the code really more complicated and prone to errors.
> 
> With this approach, if the userspace is interested only on "gpu", it
> can
> get the thermal zone id when getting all the thermal zones <id, name>
> and then ask for the information it is interested in.
> 
> Well I thought that is be more flexible.

Then I'm totally okay with this implementation. Thanks for the
explanation.

thanks,
rui
> 
> 
> 
> 
> 

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Daniel Lezcano]

On 01/07/2020 09:41, Zhang Rui wrote:
> Hi, Daniel,
> 
> On Tue, 2020-06-30 at 20:32 +0200, Daniel Lezcano wrote:
>> Hi Zhang,
>>
>> thanks for taking the time to review
>>
>>
>> On 30/06/2020 18:12, Zhang Rui wrote:
>>
>> [ ... ]
>>
>>>> +int thermal_notify_tz_enable(int tz_id);
>>>> +int thermal_notify_tz_disable(int tz_id);
>>>
>>> these two will be used after merging the mode enhancement patches
>>> from
>>> Andrzej Pietrasiewicz, right?
>>
>> Yes, that is correct.
>>
>>>> +int thermal_notify_tz_trip_down(int tz_id, int id);
>>>> +int thermal_notify_tz_trip_up(int tz_id, int id);
>>>> +int thermal_notify_tz_trip_delete(int tz_id, int id);
>>>> +int thermal_notify_tz_trip_add(int tz_id, int id, int type,
>>>> +			       int temp, int hyst);
>>>
>>> is there any case we need to use these two?
>>
>> There is the initial proposal to add/del trip points via sysfs which
>> is
>> not merged because of no comments and the presentation from Srinivas
>> giving the 'add' and 'del' notification description, so I assumed the
>> feature would be added soon.
>>
>> These functions are here ready to be connected to the core code. If
>> anyone is planning to implement add/del trip point, he won't have to
>> implement these two notifications as they will be ready to be called.
>>
> Then I'd prefer we only introduce the events that are used or will be
> used soon, like the tz disable/enable, to avoid some potential dead
> code.
> We can easily add more events when they are needed.

Sure, if Srinivas does not need them, I can drop these notifications.

However, I would suggest to keep at the uapi header file with the
definition of the events and the attributes in order to reduce the
impact of the userspace change when adding these two notifications in
the future.

> Srinivas, do you have plan to use the trip add/delete events?
> 
>>>> +int thermal_notify_tz_trip_change(int tz_id, int id, int type,
>>>> +				  int temp, int hyst);
>>>> +int thermal_notify_cdev_update(int cdev_id, int state);
>>>
>>> This is used when the cdev cur_state is changed.
>>> what about cases that cdev->max_state changes? I think we need an
>>> extra
>>> event for it, right?
>>
>> Sure, I can add:
>>
>> int thermal_notify_cdev_change(...)
> 
> thermal_notify_cdev_change() and thermal_notify_cdev_update() looks
> confusing to me.
> Can we use thermal_notify_cdev_update_cur() and
> thermal_notify_cdev_update_max()?
> Or can we use one event, with updated cur_state and max_state?

I think it is a good idea to use the same event and depending on the
change we can add the cur_state or the max_state attribute. Up to the
userspace to figure out which one is present.

[ ... ]

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Srinivas Pandruvada]

[...]

> Then I'd prefer we only introduce the events that are used or will be
> used soon, like the tz disable/enable, to avoid some potential dead
> code.
> We can easily add more events when they are needed.
> 
> Srinivas, do you have plan to use the trip add/delete events?
Yes and also trip modify.
Also I need to have one more event for heartbeat like event which needs
confirmation from user space to hardware the user process controlling
thermal is active not dead. So whenever hardware wants to check health 
it will send an event, which user space should acknowledge

Thanks,
Srinivas
> > 
> > 
> > 

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Daniel Lezcano]

On 01/07/2020 17:49, Srinivas Pandruvada wrote:
> 
> [...]
> 
>> Then I'd prefer we only introduce the events that are used or will be
>> used soon, like the tz disable/enable, to avoid some potential dead
>> code.
>> We can easily add more events when they are needed.
>>
>> Srinivas, do you have plan to use the trip add/delete events?
> Yes and also trip modify.

Ok I will keep those then.

> Also I need to have one more event for heartbeat like event which needs
> confirmation from user space to hardware the user process controlling
> thermal is active not dead. So whenever hardware wants to check health 
> it will send an event, which user space should acknowledge

Could it be the opposite? The userspace sends periodically a message to
tell it is alive instead of having the kernel asking the userspace?

Is it ok if we add this in a separate series ?



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Srinivas Pandruvada]

On Wed, 2020-07-01 at 18:31 +0200, Daniel Lezcano wrote:
> On 01/07/2020 17:49, Srinivas Pandruvada wrote:
> > [...]
> > 
> > > Then I'd prefer we only introduce the events that are used or
> > > will be
> > > used soon, like the tz disable/enable, to avoid some potential
> > > dead
> > > code.
> > > We can easily add more events when they are needed.
> > > 
> > > Srinivas, do you have plan to use the trip add/delete events?
> > Yes and also trip modify.
> 
> Ok I will keep those then.
> 
> > Also I need to have one more event for heartbeat like event which
> > needs
> > confirmation from user space to hardware the user process
> > controlling
> > thermal is active not dead. So whenever hardware wants to check
> > health 
> > it will send an event, which user space should acknowledge
> 
> Could it be the opposite? The userspace sends periodically a message
> to
> tell it is alive instead of having the kernel asking the userspace?
> 
Kernel doesn't ask (there is no timer in the kernel). The HW/FW send
special event for confirmation and expects reply (This is upto the
vendors). So user space can't send randomly.

> Is it ok if we add this in a separate series ?
Yes.

Thanks,
Srinivas

> 
> 
> 

[PATCH v2 5/5] thermal: core: Add notifications call in the framework

[Daniel Lezcano]

The generic netlink protocol is implemented but the different
notification functions are not yet connected to the core code.

These changes add the notification calls in the different
corresponding places.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c    | 21 +++++++++++++++++++++
 drivers/thermal/thermal_helpers.c | 11 +++++++++--
 drivers/thermal/thermal_sysfs.c   | 15 ++++++++++++++-
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 58c95aeafb7f..1388c03d1190 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -215,6 +215,8 @@ int thermal_zone_device_set_policy(struct thermal_zone_device *tz,
 	mutex_unlock(&tz->lock);
 	mutex_unlock(&thermal_governor_lock);
 
+	thermal_notify_tz_gov_change(tz->id, policy);
+
 	return ret;
 }
 
@@ -406,12 +408,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 {
 	enum thermal_trip_type type;
+	int trip_temp, hyst = 0;
 
 	/* Ignore disabled trip points */
 	if (test_bit(trip, &tz->trips_disabled))
 		return;
 
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
 	tz->ops->get_trip_type(tz, trip, &type);
+	if (tz->ops->get_trip_hyst)
+		tz->ops->get_trip_hyst(tz, trip, &hyst);
+
+	if (tz->last_temperature != THERMAL_TEMP_INVALID) {
+		if (tz->last_temperature < trip_temp &&
+		    tz->temperature >= trip_temp)
+			thermal_notify_tz_trip_up(tz->id, trip);
+		if (tz->last_temperature >= trip_temp &&
+		    tz->temperature < (trip_temp - hyst))
+			thermal_notify_tz_trip_down(tz->id, trip);
+	}
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip, type);
@@ -443,6 +458,8 @@ static void update_temperature(struct thermal_zone_device *tz)
 	mutex_unlock(&tz->lock);
 
 	trace_thermal_temperature(tz);
+
+	thermal_genl_sampling_temp(tz->id, temp);
 }
 
 static void thermal_zone_device_init(struct thermal_zone_device *tz)
@@ -1398,6 +1415,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	if (atomic_cmpxchg(&tz->need_update, 1, 0))
 		thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
 
+	thermal_notify_tz_create(tz->id, tz->type);
+
 	return tz;
 
 unregister:
@@ -1469,6 +1488,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	ida_destroy(&tz->ida);
 	mutex_destroy(&tz->lock);
 	device_unregister(&tz->device);
+
+	thermal_notify_tz_delete(tz->id);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
 
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 6ed24b4e23d3..7893ace1d90f 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -175,6 +175,14 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
 	mutex_unlock(&tz->lock);
 }
 
+void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev, int target)
+{
+	if (cdev->ops->set_cur_state(cdev, target))
+		return;
+	thermal_notify_cdev_update(cdev->id, target);
+	thermal_cooling_device_stats_update(cdev, target);
+}
+
 void thermal_cdev_update(struct thermal_cooling_device *cdev)
 {
 	struct thermal_instance *instance;
@@ -192,8 +200,7 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
 			target = instance->target;
 	}
 
-	if (!cdev->ops->set_cur_state(cdev, target))
-		thermal_cooling_device_stats_update(cdev, target);
+	thermal_cdev_set_cur_state(cdev, target);
 
 	mutex_unlock(&cdev->lock);
 	trace_cdev_update(cdev, target);
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index aa99edb4dff7..ff449943f757 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -124,7 +124,8 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
 	int trip, ret;
-	int temperature;
+	int temperature, hyst = 0;
+	enum thermal_trip_type type;
 
 	if (!tz->ops->set_trip_temp)
 		return -EPERM;
@@ -139,6 +140,18 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
+	if (tz->ops->get_trip_hyst) {
+		ret = tz->ops->get_trip_hyst(tz, trip, &hyst);
+		if (ret)
+			return ret;
+	}
+
+	ret = tz->ops->get_trip_type(tz, trip, &type);
+	if (ret)
+		return ret;
+
+	thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst);
+
 	thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
 
 	return count;
-- 
2.17.1

Re: [PATCH v2 5/5] thermal: core: Add notifications call in the framework

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The generic netlink protocol is implemented but the different
> notification functions are not yet connected to the core code.
>
> These changes add the notification calls in the different
> corresponding places.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c    | 21 +++++++++++++++++++++
>  drivers/thermal/thermal_helpers.c | 11 +++++++++--
>  drivers/thermal/thermal_sysfs.c   | 15 ++++++++++++++-
>  3 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 58c95aeafb7f..1388c03d1190 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -215,6 +215,8 @@ int thermal_zone_device_set_policy(struct thermal_zone_device *tz,
>         mutex_unlock(&tz->lock);
>         mutex_unlock(&thermal_governor_lock);
>
> +       thermal_notify_tz_gov_change(tz->id, policy);
> +
>         return ret;
>  }
>
> @@ -406,12 +408,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>  static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>  {
>         enum thermal_trip_type type;
> +       int trip_temp, hyst = 0;
>
>         /* Ignore disabled trip points */
>         if (test_bit(trip, &tz->trips_disabled))
>                 return;
>
> +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
>         tz->ops->get_trip_type(tz, trip, &type);
> +       if (tz->ops->get_trip_hyst)
> +               tz->ops->get_trip_hyst(tz, trip, &hyst);
> +
> +       if (tz->last_temperature != THERMAL_TEMP_INVALID) {
> +               if (tz->last_temperature < trip_temp &&
> +                   tz->temperature >= trip_temp)
> +                       thermal_notify_tz_trip_up(tz->id, trip);
> +               if (tz->last_temperature >= trip_temp &&
> +                   tz->temperature < (trip_temp - hyst))
> +                       thermal_notify_tz_trip_down(tz->id, trip);
> +       }
>
>         if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
>                 handle_critical_trips(tz, trip, type);
> @@ -443,6 +458,8 @@ static void update_temperature(struct thermal_zone_device *tz)
>         mutex_unlock(&tz->lock);
>
>         trace_thermal_temperature(tz);
> +
> +       thermal_genl_sampling_temp(tz->id, temp);

Does this need any rate limiting? How many times is update_temperature
called on a platform with a dozen sensors?

>  }
>
>  static void thermal_zone_device_init(struct thermal_zone_device *tz)
> @@ -1398,6 +1415,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
>         if (atomic_cmpxchg(&tz->need_update, 1, 0))
>                 thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
>
> +       thermal_notify_tz_create(tz->id, tz->type);
> +
>         return tz;
>
>  unregister:
> @@ -1469,6 +1488,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>         ida_destroy(&tz->ida);
>         mutex_destroy(&tz->lock);
>         device_unregister(&tz->device);
> +
> +       thermal_notify_tz_delete(tz->id);
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index 6ed24b4e23d3..7893ace1d90f 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -175,6 +175,14 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
>         mutex_unlock(&tz->lock);
>  }
>
> +void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev, int target)
> +{
> +       if (cdev->ops->set_cur_state(cdev, target))
> +               return;
> +       thermal_notify_cdev_update(cdev->id, target);
> +       thermal_cooling_device_stats_update(cdev, target);
> +}
> +
>  void thermal_cdev_update(struct thermal_cooling_device *cdev)
>  {
>         struct thermal_instance *instance;
> @@ -192,8 +200,7 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
>                         target = instance->target;
>         }
>
> -       if (!cdev->ops->set_cur_state(cdev, target))
> -               thermal_cooling_device_stats_update(cdev, target);
> +       thermal_cdev_set_cur_state(cdev, target);
>
>         mutex_unlock(&cdev->lock);
>         trace_cdev_update(cdev, target);
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb4dff7..ff449943f757 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -124,7 +124,8 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
>  {
>         struct thermal_zone_device *tz = to_thermal_zone(dev);
>         int trip, ret;
> -       int temperature;
> +       int temperature, hyst = 0;
> +       enum thermal_trip_type type;
>
>         if (!tz->ops->set_trip_temp)
>                 return -EPERM;
> @@ -139,6 +140,18 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
>         if (ret)
>                 return ret;
>
> +       if (tz->ops->get_trip_hyst) {
> +               ret = tz->ops->get_trip_hyst(tz, trip, &hyst);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       ret = tz->ops->get_trip_type(tz, trip, &type);
> +       if (ret)
> +               return ret;
> +
> +       thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst);
> +
>         thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
>
>         return count;
> --
> 2.17.1
>

Re: [PATCH v2 5/5] thermal: core: Add notifications call in the framework

[Daniel Lezcano]

On 30/06/2020 13:49, Amit Kucheria wrote:
> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> The generic netlink protocol is implemented but the different
>> notification functions are not yet connected to the core code.
>>
>> These changes add the notification calls in the different
>> corresponding places.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/thermal/thermal_core.c    | 21 +++++++++++++++++++++
>>  drivers/thermal/thermal_helpers.c | 11 +++++++++--
>>  drivers/thermal/thermal_sysfs.c   | 15 ++++++++++++++-
>>  3 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 58c95aeafb7f..1388c03d1190 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -215,6 +215,8 @@ int thermal_zone_device_set_policy(struct thermal_zone_device *tz,
>>         mutex_unlock(&tz->lock);
>>         mutex_unlock(&thermal_governor_lock);
>>
>> +       thermal_notify_tz_gov_change(tz->id, policy);
>> +
>>         return ret;
>>  }
>>
>> @@ -406,12 +408,25 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>>  static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>>  {
>>         enum thermal_trip_type type;
>> +       int trip_temp, hyst = 0;
>>
>>         /* Ignore disabled trip points */
>>         if (test_bit(trip, &tz->trips_disabled))
>>                 return;
>>
>> +       tz->ops->get_trip_temp(tz, trip, &trip_temp);
>>         tz->ops->get_trip_type(tz, trip, &type);
>> +       if (tz->ops->get_trip_hyst)
>> +               tz->ops->get_trip_hyst(tz, trip, &hyst);
>> +
>> +       if (tz->last_temperature != THERMAL_TEMP_INVALID) {
>> +               if (tz->last_temperature < trip_temp &&
>> +                   tz->temperature >= trip_temp)
>> +                       thermal_notify_tz_trip_up(tz->id, trip);
>> +               if (tz->last_temperature >= trip_temp &&
>> +                   tz->temperature < (trip_temp - hyst))
>> +                       thermal_notify_tz_trip_down(tz->id, trip);
>> +       }
>>
>>         if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
>>                 handle_critical_trips(tz, trip, type);
>> @@ -443,6 +458,8 @@ static void update_temperature(struct thermal_zone_device *tz)
>>         mutex_unlock(&tz->lock);
>>
>>         trace_thermal_temperature(tz);
>> +
>> +       thermal_genl_sampling_temp(tz->id, temp);
> 
> Does this need any rate limiting? How many times is update_temperature
> called on a platform with a dozen sensors?

Assuming they are all in polling mode, it is dozen messages average per
second.

If *all* sensors are doing passive cooling with 100ms, then it is around
120 messages per second in the very worst case (fast polling, all above
the trip point).

We do not need any rate limiting ATM. If we reach this limit in the
future we can improve the notification.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The cdev, tz and governor list, as well as their respective locks are
> statically defined in the thermal_core.c file.
>
> In order to give a sane access to these list, like browsing all the
> thermal zones or all the cooling devices, let's define a set of
> helpers where we pass a callback as a parameter to be called for each
> thermal entity.
>
> We keep the self-encapsulation and ensure the locks are correctly
> taken when looking at the list.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 51 ++++++++++++++++++++++++++++++++++

Is the idea to not use thermal_helpers.c from now on? It fits
perfectly with a patch I have to merge all its contents to
thermal_core.c :-)


>  drivers/thermal/thermal_core.h |  9 ++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 2a3f83265d8b..e2f8d2550ecd 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -611,6 +611,57 @@ void thermal_zone_device_rebind_exception(struct thermal_zone_device *tz,
>         mutex_unlock(&thermal_list_lock);
>  }
>
> +int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> +                             void *data)


> +{
> +       struct thermal_governor *gov;
> +       int ret = 0;
> +
> +       mutex_lock(&thermal_governor_lock);
> +       list_for_each_entry(gov, &thermal_governor_list, governor_list) {
> +               ret = cb(gov, data);
> +               if (ret)
> +                       break;
> +       }
> +       mutex_unlock(&thermal_governor_lock);
> +
> +       return ret;
> +}
> +
> +int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
> +                                             void *), void *data)
> +{
> +       struct thermal_cooling_device *cdev;
> +       int ret = 0;
> +
> +       mutex_lock(&thermal_list_lock);
> +       list_for_each_entry(cdev, &thermal_cdev_list, node) {
> +               ret = cb(cdev, data);
> +               if (ret)
> +                       break;
> +       }
> +       mutex_unlock(&thermal_list_lock);
> +
> +       return ret;
> +}
> +
> +int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
> +                         void *data)
> +{
> +       struct thermal_zone_device *tz;
> +       int ret = 0;
> +
> +       mutex_lock(&thermal_list_lock);
> +       list_for_each_entry(tz, &thermal_tz_list, node) {
> +               ret = cb(tz, data);
> +               if (ret)
> +                       break;
> +       }
> +       mutex_unlock(&thermal_list_lock);
> +
> +       return ret;
> +}
> +
>  void thermal_zone_device_unbind_exception(struct thermal_zone_device *tz,
>                                           const char *cdev_type, size_t size)
>  {
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 4e271016b7a9..bb8f8aee79eb 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -41,6 +41,15 @@ extern struct thermal_governor *__governor_thermal_table_end[];
>              __governor < __governor_thermal_table_end; \
>              __governor++)
>
> +int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
> +                         void *);
> +
> +int for_each_thermal_cooling_device(int (*cb)(struct thermal_cooling_device *,
> +                                             void *), void *);
> +
> +int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *),
> +                             void *thermal_governor);
> +
>  struct thermal_attr {
>         struct device_attribute attr;
>         char name[THERMAL_NAME_LENGTH];
> --
> 2.17.1
>

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Zhang Rui]

Hi, Daniel,

seems that you forgot to cc linux-pm mailing list.

On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > 
> > The cdev, tz and governor list, as well as their respective locks
> > are
> > statically defined in the thermal_core.c file.
> > 
> > In order to give a sane access to these list, like browsing all the
> > thermal zones or all the cooling devices, let's define a set of
> > helpers where we pass a callback as a parameter to be called for
> > each
> > thermal entity.
> > 
> > We keep the self-encapsulation and ensure the locks are correctly
> > taken when looking at the list.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  drivers/thermal/thermal_core.c | 51
> > ++++++++++++++++++++++++++++++++++
> 
> Is the idea to not use thermal_helpers.c from now on? It fits
> perfectly with a patch I have to merge all its contents to
> thermal_core.c :-)

I agree these changes should be in thermal_helper.c

thanks,
rui
> 
> 
> >  drivers/thermal/thermal_core.h |  9 ++++++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index 2a3f83265d8b..e2f8d2550ecd 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -611,6 +611,57 @@ void
> > thermal_zone_device_rebind_exception(struct thermal_zone_device
> > *tz,
> >         mutex_unlock(&thermal_list_lock);
> >  }
> > 
> > +int for_each_thermal_governor(int (*cb)(struct thermal_governor *,
> > void *),
> > +                             void *data)
> 
> 
> > +{
> > +       struct thermal_governor *gov;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&thermal_governor_lock);
> > +       list_for_each_entry(gov, &thermal_governor_list,
> > governor_list) {
> > +               ret = cb(gov, data);
> > +               if (ret)
> > +                       break;
> > +       }
> > +       mutex_unlock(&thermal_governor_lock);
> > +
> > +       return ret;
> > +}
> > +
> > +int for_each_thermal_cooling_device(int (*cb)(struct
> > thermal_cooling_device *,
> > +                                             void *), void *data)
> > +{
> > +       struct thermal_cooling_device *cdev;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&thermal_list_lock);
> > +       list_for_each_entry(cdev, &thermal_cdev_list, node) {
> > +               ret = cb(cdev, data);
> > +               if (ret)
> > +                       break;
> > +       }
> > +       mutex_unlock(&thermal_list_lock);
> > +
> > +       return ret;
> > +}
> > +
> > +int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *,
> > void *),
> > +                         void *data)
> > +{
> > +       struct thermal_zone_device *tz;
> > +       int ret = 0;
> > +
> > +       mutex_lock(&thermal_list_lock);
> > +       list_for_each_entry(tz, &thermal_tz_list, node) {
> > +               ret = cb(tz, data);
> > +               if (ret)
> > +                       break;
> > +       }
> > +       mutex_unlock(&thermal_list_lock);
> > +
> > +       return ret;
> > +}
> > +
> >  void thermal_zone_device_unbind_exception(struct
> > thermal_zone_device *tz,
> >                                           const char *cdev_type,
> > size_t size)
> >  {
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h
> > index 4e271016b7a9..bb8f8aee79eb 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -41,6 +41,15 @@ extern struct thermal_governor
> > *__governor_thermal_table_end[];
> >              __governor < __governor_thermal_table_end; \
> >              __governor++)
> > 
> > +int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *,
> > void *),
> > +                         void *);
> > +
> > +int for_each_thermal_cooling_device(int (*cb)(struct
> > thermal_cooling_device *,
> > +                                             void *), void *);
> > +
> > +int for_each_thermal_governor(int (*cb)(struct thermal_governor *,
> > void *),
> > +                             void *thermal_governor);
> > +
> >  struct thermal_attr {
> >         struct device_attribute attr;
> >         char name[THERMAL_NAME_LENGTH];
> > --
> > 2.17.1
> > 

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Amit Kucheria]

On Tue, Jun 30, 2020 at 8:40 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Hi, Daniel,
>
> seems that you forgot to cc linux-pm mailing list.
>
> On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
> > On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> > >
> > > The cdev, tz and governor list, as well as their respective locks
> > > are
> > > statically defined in the thermal_core.c file.
> > >
> > > In order to give a sane access to these list, like browsing all the
> > > thermal zones or all the cooling devices, let's define a set of
> > > helpers where we pass a callback as a parameter to be called for
> > > each
> > > thermal entity.
> > >
> > > We keep the self-encapsulation and ensure the locks are correctly
> > > taken when looking at the list.
> > >
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > ---
> > >  drivers/thermal/thermal_core.c | 51
> > > ++++++++++++++++++++++++++++++++++
> >
> > Is the idea to not use thermal_helpers.c from now on? It fits
> > perfectly with a patch I have to merge all its contents to
> > thermal_core.c :-)
>
> I agree these changes should be in thermal_helper.c

I was actually serious about killing thermal_helper.c :-)

What is the reason for those 5-6 functions to live outside
thermal_core.c? Functions in thermal_helper.c are called by governors
and drivers, just like the functions in thermal_core.c. I couldn't
find a pattern.

Regards,
Amit

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Daniel Lezcano]

On 30/06/2020 20:46, Amit Kucheria wrote:
> On Tue, Jun 30, 2020 at 8:40 PM Zhang Rui <rui.zhang@intel.com> wrote:
>>
>> Hi, Daniel,
>>
>> seems that you forgot to cc linux-pm mailing list.
>>
>> On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
>>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> The cdev, tz and governor list, as well as their respective locks
>>>> are
>>>> statically defined in the thermal_core.c file.
>>>>
>>>> In order to give a sane access to these list, like browsing all the
>>>> thermal zones or all the cooling devices, let's define a set of
>>>> helpers where we pass a callback as a parameter to be called for
>>>> each
>>>> thermal entity.
>>>>
>>>> We keep the self-encapsulation and ensure the locks are correctly
>>>> taken when looking at the list.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>  drivers/thermal/thermal_core.c | 51
>>>> ++++++++++++++++++++++++++++++++++
>>>
>>> Is the idea to not use thermal_helpers.c from now on? It fits
>>> perfectly with a patch I have to merge all its contents to
>>> thermal_core.c :-)
>>
>> I agree these changes should be in thermal_helper.c
> 
> I was actually serious about killing thermal_helper.c :-)
> 
> What is the reason for those 5-6 functions to live outside
> thermal_core.c? Functions in thermal_helper.c are called by governors
> and drivers, just like the functions in thermal_core.c. I couldn't
> find a pattern.

I propose to move these functions in the thermal_helper.c for now as
requested by Rui. Then you can send a patch to merge thermal_helper.c to
thermal_core.c after and we can comment the move.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Daniel Lezcano]

On 30/06/2020 17:09, Zhang Rui wrote:
> Hi, Daniel,
> 
> seems that you forgot to cc linux-pm mailing list.
> 
> On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>>
>>> The cdev, tz and governor list, as well as their respective locks
>>> are
>>> statically defined in the thermal_core.c file.
>>>
>>> In order to give a sane access to these list, like browsing all the
>>> thermal zones or all the cooling devices, let's define a set of
>>> helpers where we pass a callback as a parameter to be called for
>>> each
>>> thermal entity.
>>>
>>> We keep the self-encapsulation and ensure the locks are correctly
>>> taken when looking at the list.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>  drivers/thermal/thermal_core.c | 51
>>> ++++++++++++++++++++++++++++++++++
>>
>> Is the idea to not use thermal_helpers.c from now on? It fits
>> perfectly with a patch I have to merge all its contents to
>> thermal_core.c :-)
> 
> I agree these changes should be in thermal_helper.c

Oh, actually I remind put those functions in the thermal_core.c file
because they need the locks which are statically defined in there.

If the functions are moved to thermal_helper.c that will imply to export
the locks outside of the file, thus breaking the self-encapsulation.

Do you want to move them out?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Zhang Rui]

On Wed, 2020-07-01 at 09:35 +0200, Daniel Lezcano wrote:
> On 30/06/2020 17:09, Zhang Rui wrote:
> > Hi, Daniel,
> > 
> > seems that you forgot to cc linux-pm mailing list.
> > 
> > On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
> > > On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > > > 
> > > > The cdev, tz and governor list, as well as their respective
> > > > locks
> > > > are
> > > > statically defined in the thermal_core.c file.
> > > > 
> > > > In order to give a sane access to these list, like browsing all
> > > > the
> > > > thermal zones or all the cooling devices, let's define a set of
> > > > helpers where we pass a callback as a parameter to be called
> > > > for
> > > > each
> > > > thermal entity.
> > > > 
> > > > We keep the self-encapsulation and ensure the locks are
> > > > correctly
> > > > taken when looking at the list.
> > > > 
> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > > ---
> > > >  drivers/thermal/thermal_core.c | 51
> > > > ++++++++++++++++++++++++++++++++++
> > > 
> > > Is the idea to not use thermal_helpers.c from now on? It fits
> > > perfectly with a patch I have to merge all its contents to
> > > thermal_core.c :-)
> > 
> > I agree these changes should be in thermal_helper.c
> 
> Oh, actually I remind put those functions in the thermal_core.c file
> because they need the locks which are statically defined in there.
> 
> If the functions are moved to thermal_helper.c that will imply to
> export
> the locks outside of the file, thus breaking the self-encapsulation.
> 
> Do you want to move them out?

Then no. I don't have any objection of removing thermal_helper.c, so
you can just leave these functions in thermal_core.c

thanks,
rui
> 
> 

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Amit Kucheria]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Wed, Jul 1, 2020 at 1:27 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Wed, 2020-07-01 at 09:35 +0200, Daniel Lezcano wrote:
> > On 30/06/2020 17:09, Zhang Rui wrote:
> > > Hi, Daniel,
> > >
> > > seems that you forgot to cc linux-pm mailing list.
> > >
> > > On Tue, 2020-06-30 at 17:16 +0530, Amit Kucheria wrote:
> > > > On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> > > > <daniel.lezcano@linaro.org> wrote:
> > > > >
> > > > > The cdev, tz and governor list, as well as their respective
> > > > > locks
> > > > > are
> > > > > statically defined in the thermal_core.c file.
> > > > >
> > > > > In order to give a sane access to these list, like browsing all
> > > > > the
> > > > > thermal zones or all the cooling devices, let's define a set of
> > > > > helpers where we pass a callback as a parameter to be called
> > > > > for
> > > > > each
> > > > > thermal entity.
> > > > >
> > > > > We keep the self-encapsulation and ensure the locks are
> > > > > correctly
> > > > > taken when looking at the list.
> > > > >
> > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > > > ---
> > > > >  drivers/thermal/thermal_core.c | 51
> > > > > ++++++++++++++++++++++++++++++++++
> > > >
> > > > Is the idea to not use thermal_helpers.c from now on? It fits
> > > > perfectly with a patch I have to merge all its contents to
> > > > thermal_core.c :-)
> > >
> > > I agree these changes should be in thermal_helper.c
> >
> > Oh, actually I remind put those functions in the thermal_core.c file
> > because they need the locks which are statically defined in there.
> >
> > If the functions are moved to thermal_helper.c that will imply to
> > export
> > the locks outside of the file, thus breaking the self-encapsulation.
> >
> > Do you want to move them out?
>
> Then no. I don't have any objection of removing thermal_helper.c, so
> you can just leave these functions in thermal_core.c

In that case, Daniel, please find attached a patch to move the
contents of thermal_helpers into thermal_core.c

Feel free to include it at the top of this series and modify as you see fit.

Regards,
Amit

[-- Attachment #2: 0001-thermal-core-Merge-thermal_helpers.c-into-thermal_co.patch --]
[-- Type: text/x-patch, Size: 14115 bytes --]

From e09befc6c6717c3e4554b0688c7d83b0696d2554 Mon Sep 17 00:00:00 2001
Message-Id: <e09befc6c6717c3e4554b0688c7d83b0696d2554.1593595413.git.amit.kucheria@linaro.org>
From: Amit Kucheria <amit.kucheria@linaro.org>
Date: Wed, 1 Jul 2020 14:48:47 +0530
Subject: [PATCH] thermal/core: Merge thermal_helpers.c into thermal_core.c

Moving several helper functions to this file require unnecessarily
exposing locks outside thermal_core.c.

It is better to simply move the few functions in there directly into
thermal_core.c.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/Makefile          |   3 +-
 drivers/thermal/thermal_core.c    | 216 +++++++++++++++++++++++++++
 drivers/thermal/thermal_helpers.c | 238 ------------------------------
 3 files changed, 217 insertions(+), 240 deletions(-)
 delete mode 100644 drivers/thermal/thermal_helpers.c

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 0c8b84a09b9aa..ed2fc83fe2128 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -4,8 +4,7 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
-thermal_sys-y			+= thermal_core.o thermal_sysfs.o \
-					thermal_helpers.o
+thermal_sys-y			+= thermal_core.o thermal_sysfs.o
 
 # interface to/from other layers providing sensors
 thermal_sys-$(CONFIG_THERMAL_HWMON)		+= thermal_hwmon.o
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index b71196eaf90e8..a9e927f7224e9 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -43,6 +43,222 @@ static bool power_off_triggered;
 
 static struct thermal_governor *def_governor;
 
+/*  Various helper functions */
+
+int get_tz_trend(struct thermal_zone_device *tz, int trip)
+{
+	enum thermal_trend trend;
+
+	if (tz->emul_temperature || !tz->ops->get_trend ||
+	    tz->ops->get_trend(tz, trip, &trend)) {
+		if (tz->temperature > tz->last_temperature)
+			trend = THERMAL_TREND_RAISING;
+		else if (tz->temperature < tz->last_temperature)
+			trend = THERMAL_TREND_DROPPING;
+		else
+			trend = THERMAL_TREND_STABLE;
+	}
+
+	return trend;
+}
+EXPORT_SYMBOL(get_tz_trend);
+
+struct thermal_instance *
+get_thermal_instance(struct thermal_zone_device *tz,
+		     struct thermal_cooling_device *cdev, int trip)
+{
+	struct thermal_instance *pos = NULL;
+	struct thermal_instance *target_instance = NULL;
+
+	mutex_lock(&tz->lock);
+	mutex_lock(&cdev->lock);
+
+	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
+		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
+			target_instance = pos;
+			break;
+		}
+	}
+
+	mutex_unlock(&cdev->lock);
+	mutex_unlock(&tz->lock);
+
+	return target_instance;
+}
+EXPORT_SYMBOL(get_thermal_instance);
+
+/**
+ * thermal_zone_get_temp() - returns the temperature of a thermal zone
+ * @tz: a valid pointer to a struct thermal_zone_device
+ * @temp: a valid pointer to where to store the resulting temperature.
+ *
+ * When a valid thermal zone reference is passed, it will fetch its
+ * temperature and fill @temp.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	int ret = -EINVAL;
+	int count;
+	int crit_temp = INT_MAX;
+	enum thermal_trip_type type;
+
+	if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
+		goto exit;
+
+	mutex_lock(&tz->lock);
+
+	ret = tz->ops->get_temp(tz, temp);
+
+	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
+		for (count = 0; count < tz->trips; count++) {
+			ret = tz->ops->get_trip_type(tz, count, &type);
+			if (!ret && type == THERMAL_TRIP_CRITICAL) {
+				ret = tz->ops->get_trip_temp(tz, count,
+						&crit_temp);
+				break;
+			}
+		}
+
+		/*
+		 * Only allow emulating a temperature when the real temperature
+		 * is below the critical temperature so that the emulation code
+		 * cannot hide critical conditions.
+		 */
+		if (!ret && *temp < crit_temp)
+			*temp = tz->emul_temperature;
+	}
+
+	mutex_unlock(&tz->lock);
+exit:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
+
+/**
+ * thermal_zone_set_trips - Computes the next trip points for the driver
+ * @tz: a pointer to a thermal zone device structure
+ *
+ * The function computes the next temperature boundaries by browsing
+ * the trip points. The result is the closer low and high trip points
+ * to the current temperature. These values are passed to the backend
+ * driver to let it set its own notification mechanism (usually an
+ * interrupt).
+ *
+ * It does not return a value
+ */
+void thermal_zone_set_trips(struct thermal_zone_device *tz)
+{
+	int low = -INT_MAX;
+	int high = INT_MAX;
+	int trip_temp, hysteresis;
+	int i, ret;
+
+	mutex_lock(&tz->lock);
+
+	if (!tz->ops->set_trips || !tz->ops->get_trip_hyst)
+		goto exit;
+
+	for (i = 0; i < tz->trips; i++) {
+		int trip_low;
+
+		tz->ops->get_trip_temp(tz, i, &trip_temp);
+		tz->ops->get_trip_hyst(tz, i, &hysteresis);
+
+		trip_low = trip_temp - hysteresis;
+
+		if (trip_low < tz->temperature && trip_low > low)
+			low = trip_low;
+
+		if (trip_temp > tz->temperature && trip_temp < high)
+			high = trip_temp;
+	}
+
+	/* No need to change trip points */
+	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
+		goto exit;
+
+	tz->prev_low_trip = low;
+	tz->prev_high_trip = high;
+
+	dev_dbg(&tz->device,
+		"new temperature boundaries: %d < x < %d\n", low, high);
+
+	/*
+	 * Set a temperature window. When this window is left the driver
+	 * must inform the thermal core via thermal_zone_device_update.
+	 */
+	ret = tz->ops->set_trips(tz, low, high);
+	if (ret)
+		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
+
+exit:
+	mutex_unlock(&tz->lock);
+}
+
+void thermal_cdev_update(struct thermal_cooling_device *cdev)
+{
+	struct thermal_instance *instance;
+	unsigned long target = 0;
+
+	mutex_lock(&cdev->lock);
+	/* cooling device is updated*/
+	if (cdev->updated) {
+		mutex_unlock(&cdev->lock);
+		return;
+	}
+
+	/* Make sure cdev enters the deepest cooling state */
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
+			instance->tz->id, instance->target);
+		if (instance->target == THERMAL_NO_TARGET)
+			continue;
+		if (instance->target > target)
+			target = instance->target;
+	}
+
+	if (!cdev->ops->set_cur_state(cdev, target))
+		thermal_cooling_device_stats_update(cdev, target);
+
+	cdev->updated = true;
+	mutex_unlock(&cdev->lock);
+	trace_cdev_update(cdev, target);
+	dev_dbg(&cdev->device, "set to state %lu\n", target);
+}
+EXPORT_SYMBOL(thermal_cdev_update);
+
+/**
+ * thermal_zone_get_slope - return the slope attribute of the thermal zone
+ * @tz: thermal zone device with the slope attribute
+ *
+ * Return: If the thermal zone device has a slope attribute, return it, else
+ * return 1.
+ */
+int thermal_zone_get_slope(struct thermal_zone_device *tz)
+{
+	if (tz && tz->tzp)
+		return tz->tzp->slope;
+	return 1;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_slope);
+
+/**
+ * thermal_zone_get_offset - return the offset attribute of the thermal zone
+ * @tz: thermal zone device with the offset attribute
+ *
+ * Return: If the thermal zone device has a offset attribute, return it, else
+ * return 0.
+ */
+int thermal_zone_get_offset(struct thermal_zone_device *tz)
+{
+	if (tz && tz->tzp)
+		return tz->tzp->offset;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
+
 /*
  * Governor section: set of functions to handle thermal governors
  *
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
deleted file mode 100644
index 87b1256fa2f20..0000000000000
--- a/drivers/thermal/thermal_helpers.c
+++ /dev/null
@@ -1,238 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- *  thermal_helpers.c - helper functions to handle thermal devices
- *
- *  Copyright (C) 2016 Eduardo Valentin <edubezval@gmail.com>
- *
- *  Highly based on original thermal_core.c
- *  Copyright (C) 2008 Intel Corp
- *  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
- *  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/export.h>
-#include <linux/slab.h>
-#include <linux/string.h>
-#include <linux/sysfs.h>
-
-#include <trace/events/thermal.h>
-
-#include "thermal_core.h"
-
-int get_tz_trend(struct thermal_zone_device *tz, int trip)
-{
-	enum thermal_trend trend;
-
-	if (tz->emul_temperature || !tz->ops->get_trend ||
-	    tz->ops->get_trend(tz, trip, &trend)) {
-		if (tz->temperature > tz->last_temperature)
-			trend = THERMAL_TREND_RAISING;
-		else if (tz->temperature < tz->last_temperature)
-			trend = THERMAL_TREND_DROPPING;
-		else
-			trend = THERMAL_TREND_STABLE;
-	}
-
-	return trend;
-}
-EXPORT_SYMBOL(get_tz_trend);
-
-struct thermal_instance *
-get_thermal_instance(struct thermal_zone_device *tz,
-		     struct thermal_cooling_device *cdev, int trip)
-{
-	struct thermal_instance *pos = NULL;
-	struct thermal_instance *target_instance = NULL;
-
-	mutex_lock(&tz->lock);
-	mutex_lock(&cdev->lock);
-
-	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
-		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
-			target_instance = pos;
-			break;
-		}
-	}
-
-	mutex_unlock(&cdev->lock);
-	mutex_unlock(&tz->lock);
-
-	return target_instance;
-}
-EXPORT_SYMBOL(get_thermal_instance);
-
-/**
- * thermal_zone_get_temp() - returns the temperature of a thermal zone
- * @tz: a valid pointer to a struct thermal_zone_device
- * @temp: a valid pointer to where to store the resulting temperature.
- *
- * When a valid thermal zone reference is passed, it will fetch its
- * temperature and fill @temp.
- *
- * Return: On success returns 0, an error code otherwise
- */
-int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
-{
-	int ret = -EINVAL;
-	int count;
-	int crit_temp = INT_MAX;
-	enum thermal_trip_type type;
-
-	if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
-		goto exit;
-
-	mutex_lock(&tz->lock);
-
-	ret = tz->ops->get_temp(tz, temp);
-
-	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
-		for (count = 0; count < tz->trips; count++) {
-			ret = tz->ops->get_trip_type(tz, count, &type);
-			if (!ret && type == THERMAL_TRIP_CRITICAL) {
-				ret = tz->ops->get_trip_temp(tz, count,
-						&crit_temp);
-				break;
-			}
-		}
-
-		/*
-		 * Only allow emulating a temperature when the real temperature
-		 * is below the critical temperature so that the emulation code
-		 * cannot hide critical conditions.
-		 */
-		if (!ret && *temp < crit_temp)
-			*temp = tz->emul_temperature;
-	}
-
-	mutex_unlock(&tz->lock);
-exit:
-	return ret;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
-
-/**
- * thermal_zone_set_trips - Computes the next trip points for the driver
- * @tz: a pointer to a thermal zone device structure
- *
- * The function computes the next temperature boundaries by browsing
- * the trip points. The result is the closer low and high trip points
- * to the current temperature. These values are passed to the backend
- * driver to let it set its own notification mechanism (usually an
- * interrupt).
- *
- * It does not return a value
- */
-void thermal_zone_set_trips(struct thermal_zone_device *tz)
-{
-	int low = -INT_MAX;
-	int high = INT_MAX;
-	int trip_temp, hysteresis;
-	int i, ret;
-
-	mutex_lock(&tz->lock);
-
-	if (!tz->ops->set_trips || !tz->ops->get_trip_hyst)
-		goto exit;
-
-	for (i = 0; i < tz->trips; i++) {
-		int trip_low;
-
-		tz->ops->get_trip_temp(tz, i, &trip_temp);
-		tz->ops->get_trip_hyst(tz, i, &hysteresis);
-
-		trip_low = trip_temp - hysteresis;
-
-		if (trip_low < tz->temperature && trip_low > low)
-			low = trip_low;
-
-		if (trip_temp > tz->temperature && trip_temp < high)
-			high = trip_temp;
-	}
-
-	/* No need to change trip points */
-	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
-		goto exit;
-
-	tz->prev_low_trip = low;
-	tz->prev_high_trip = high;
-
-	dev_dbg(&tz->device,
-		"new temperature boundaries: %d < x < %d\n", low, high);
-
-	/*
-	 * Set a temperature window. When this window is left the driver
-	 * must inform the thermal core via thermal_zone_device_update.
-	 */
-	ret = tz->ops->set_trips(tz, low, high);
-	if (ret)
-		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
-
-exit:
-	mutex_unlock(&tz->lock);
-}
-
-void thermal_cdev_update(struct thermal_cooling_device *cdev)
-{
-	struct thermal_instance *instance;
-	unsigned long target = 0;
-
-	mutex_lock(&cdev->lock);
-	/* cooling device is updated*/
-	if (cdev->updated) {
-		mutex_unlock(&cdev->lock);
-		return;
-	}
-
-	/* Make sure cdev enters the deepest cooling state */
-	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
-		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
-			instance->tz->id, instance->target);
-		if (instance->target == THERMAL_NO_TARGET)
-			continue;
-		if (instance->target > target)
-			target = instance->target;
-	}
-
-	if (!cdev->ops->set_cur_state(cdev, target))
-		thermal_cooling_device_stats_update(cdev, target);
-
-	cdev->updated = true;
-	mutex_unlock(&cdev->lock);
-	trace_cdev_update(cdev, target);
-	dev_dbg(&cdev->device, "set to state %lu\n", target);
-}
-EXPORT_SYMBOL(thermal_cdev_update);
-
-/**
- * thermal_zone_get_slope - return the slope attribute of the thermal zone
- * @tz: thermal zone device with the slope attribute
- *
- * Return: If the thermal zone device has a slope attribute, return it, else
- * return 1.
- */
-int thermal_zone_get_slope(struct thermal_zone_device *tz)
-{
-	if (tz && tz->tzp)
-		return tz->tzp->slope;
-	return 1;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_slope);
-
-/**
- * thermal_zone_get_offset - return the offset attribute of the thermal zone
- * @tz: thermal zone device with the offset attribute
- *
- * Return: If the thermal zone device has a offset attribute, return it, else
- * return 0.
- */
-int thermal_zone_get_offset(struct thermal_zone_device *tz)
-{
-	if (tz && tz->tzp)
-		return tz->tzp->offset;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
-- 
2.25.1

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Daniel Lezcano]

On 01/07/2020 11:26, Amit Kucheria wrote:
> On Wed, Jul 1, 2020 at 1:27 PM Zhang Rui <rui.zhang@intel.com> wrote:

[ ... ]

>> Then no. I don't have any objection of removing thermal_helper.c, so
>> you can just leave these functions in thermal_core.c
> 
> In that case, Daniel, please find attached a patch to move the
> contents of thermal_helpers into thermal_core.c
> 
> Feel free to include it at the top of this series and modify as you see fit.

I will keep it separate for the moment until this series is accepted and
then send a respin of this patch on top of the merged series.

Thanks

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Daniel Lezcano]

On 01/07/2020 09:57, Zhang Rui wrote:

[ ... ]

>> Do you want to move them out?
> 
> Then no. I don't have any objection of removing thermal_helper.c, so
> you can just leave these functions in thermal_core.c

Shall I consider that as an ack for this patch ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/5] thermal: core: Add helpers to browse the cdev, tz and governor list

[Zhang Rui]

On Wed, 2020-07-01 at 11:50 +0200, Daniel Lezcano wrote:
> On 01/07/2020 09:57, Zhang Rui wrote:
> 
> [ ... ]
> 
> > > Do you want to move them out?
> > 
> > Then no. I don't have any objection of removing thermal_helper.c,
> > so
> > you can just leave these functions in thermal_core.c
> 
> Shall I consider that as an ack for this patch ?
> 
sure.

Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
>