Testing the thermal genetlink API

Testing the thermal genetlink API

[Nicolas Cavallari]

I've played a bit with the thermal netlink interface and it wasn't pleasant:

1. The way attributes are used is painful.  Instead of using arrays of
   nested structs-like, it flattens them into a big nested attr where
   you have to guess when an entry starts and when it ends.
   libnl provides no helper for this case:

   [{nla_type=TZ|F_NESTED},
	[{nla_type=TZ_ID}, 1]
	[{nla_type=TZ_NAME}, "name1"]
	[{nla_type=TZ_ID}, 2]
	[{nla_type=TZ_NAME}, "name2"]
	[{nla_type=TZ_ID}, 3]
	[{nla_type=TZ_NAME}, "name3"]
	[{nla_type=TZ_ID}, 4]
	[{nla_type=TZ_NAME}, "name4"]
   ]

2. The genl_cmd types are not unique between multicast events and
   command replies.  If you send genl_cmd=3 (CMD_TZ_GET_TEMP) and you
   get a genl_cmd=3 reply, you cannot know if it is a CMD_TZ_GET_TEMP
   response or a EVENT_TZ_DISABLE because both have genl_cmd=3, but
   completely different semantics.

3. The API is heavy.  Getting the complete information about all thermal
   zones requires 1 + 6 * thermal_zones netlink requests, each of them
   only returning few information.  You need most of them to merely
   translate the event's TZ_ID/TZ_TRIP_ID/CDEV_ID to names.

4. THERMAL_GENL_CMD_TZ_GET_TRIP cause an oops if the thermal zone driver
   does not have a get_trip_hyst callback.
   This concerns all drivers, short of two.  A patch follows.

For the record, I couldn't find any open source program using this API.
It's also not enabled in all distributions.

[PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference

[Nicolas Cavallari]

Do not call get_trip_hyst() if the thermal zone does not define one.

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
 drivers/thermal/thermal_netlink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index a16dd4d5d710..73e68cce292e 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -419,11 +419,12 @@ static int thermal_genl_cmd_tz_get_trip(struct param *p)
 	for (i = 0; i < tz->trips; i++) {
 
 		enum thermal_trip_type type;
-		int temp, hyst;
+		int temp, hyst = 0;
 
 		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 (tz->ops->get_trip_hyst)
+			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) ||
-- 
2.35.1

Re: [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference

[Rafael J. Wysocki]

On Mon, Feb 28, 2022 at 12:04 PM Nicolas Cavallari
<nicolas.cavallari@green-communications.fr> wrote:
>
> Do not call get_trip_hyst() if the thermal zone does not define one.
>
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
>  drivers/thermal/thermal_netlink.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> index a16dd4d5d710..73e68cce292e 100644
> --- a/drivers/thermal/thermal_netlink.c
> +++ b/drivers/thermal/thermal_netlink.c
> @@ -419,11 +419,12 @@ static int thermal_genl_cmd_tz_get_trip(struct param *p)
>         for (i = 0; i < tz->trips; i++) {
>
>                 enum thermal_trip_type type;
> -               int temp, hyst;
> +               int temp, hyst = 0;
>
>                 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 (tz->ops->get_trip_hyst)
> +                       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) ||
> --

Applied, but I think this needs to go into 5.17-rc, doesn't it?

Daniel?

Re: [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference

[Daniel Lezcano]

On 01/03/2022 16:14, Rafael J. Wysocki wrote:
> On Mon, Feb 28, 2022 at 12:04 PM Nicolas Cavallari
> <nicolas.cavallari@green-communications.fr> wrote:
>>
>> Do not call get_trip_hyst() if the thermal zone does not define one.
>>
>> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
>> ---
>>   drivers/thermal/thermal_netlink.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
>> index a16dd4d5d710..73e68cce292e 100644
>> --- a/drivers/thermal/thermal_netlink.c
>> +++ b/drivers/thermal/thermal_netlink.c
>> @@ -419,11 +419,12 @@ static int thermal_genl_cmd_tz_get_trip(struct param *p)
>>          for (i = 0; i < tz->trips; i++) {
>>
>>                  enum thermal_trip_type type;
>> -               int temp, hyst;
>> +               int temp, hyst = 0;
>>
>>                  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 (tz->ops->get_trip_hyst)
>> +                       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) ||
>> --
> 
> Applied, but I think this needs to go into 5.17-rc, doesn't it?

Yes, correct


-- 
<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: Testing the thermal genetlink API

[Daniel Lezcano]

Hi Nicolas,

thanks for using the netlink and giving those feedbacks even you are 
unhappy with them.

On 28/02/2022 12:03, Nicolas Cavallari wrote:
> I've played a bit with the thermal netlink interface and it wasn't pleasant:
> 
> 1. The way attributes are used is painful.  Instead of using arrays of
>     nested structs-like, it flattens them into a big nested attr where
>     you have to guess when an entry starts and when it ends.
>     libnl provides no helper for this case:
> 
>     [{nla_type=TZ|F_NESTED},
> 	[{nla_type=TZ_ID}, 1]
> 	[{nla_type=TZ_NAME}, "name1"]
> 	[{nla_type=TZ_ID}, 2]
> 	[{nla_type=TZ_NAME}, "name2"]
> 	[{nla_type=TZ_ID}, 3]
> 	[{nla_type=TZ_NAME}, "name3"]
> 	[{nla_type=TZ_ID}, 4]
> 	[{nla_type=TZ_NAME}, "name4"]
>     ]
> 
> 2. The genl_cmd types are not unique between multicast events and
>     command replies.  If you send genl_cmd=3 (CMD_TZ_GET_TEMP) and you
>     get a genl_cmd=3 reply, you cannot know if it is a CMD_TZ_GET_TEMP
>     response or a EVENT_TZ_DISABLE because both have genl_cmd=3, but
>     completely different semantics.
> 3. The API is heavy.  Getting the complete information about all thermal
>     zones requires 1 + 6 * thermal_zones netlink requests, each of them
>     only returning few information.  You need most of them to merely
>     translate the event's TZ_ID/TZ_TRIP_ID/CDEV_ID to names.

That is part of the discovery and it should happen only once when you 
get the thermal information.

> 4. THERMAL_GENL_CMD_TZ_GET_TRIP cause an oops if the thermal zone driver
>     does not have a get_trip_hyst callback.
>     This concerns all drivers, short of two.  A patch follows.

Great, thanks for the fix.

> For the record, I couldn't find any open source program using this API.
> It's also not enabled in all distributions.

The netlink support is very recent. A library has been posted [1] and 
hopefully it can helps you to get rid of all the complexity.

Thanks for testing

   -- D.

[1] 
https://lore.kernel.org/all/20220218125334.995447-1-daniel.lezcano@linaro.org/


-- 
<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