* Testing the thermal genetlink API
@ 2022-02-28 11:03 Nicolas Cavallari
2022-02-28 11:03 ` [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference Nicolas Cavallari
2022-02-28 12:21 ` Testing the thermal genetlink API Daniel Lezcano
0 siblings, 2 replies; 5+ messages in thread
From: Nicolas Cavallari @ 2022-02-28 11:03 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui
Cc: linux-pm, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference
2022-02-28 11:03 Testing the thermal genetlink API Nicolas Cavallari
@ 2022-02-28 11:03 ` Nicolas Cavallari
2022-03-01 15:14 ` Rafael J. Wysocki
2022-02-28 12:21 ` Testing the thermal genetlink API Daniel Lezcano
1 sibling, 1 reply; 5+ messages in thread
From: Nicolas Cavallari @ 2022-02-28 11:03 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui
Cc: linux-pm, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference
2022-02-28 11:03 ` [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference Nicolas Cavallari
@ 2022-03-01 15:14 ` Rafael J. Wysocki
2022-03-01 20:48 ` Daniel Lezcano
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2022-03-01 15:14 UTC (permalink / raw)
To: Nicolas Cavallari, Daniel Lezcano
Cc: Rafael J. Wysocki, Amit Kucheria, Zhang Rui, Linux PM,
Linux Kernel Mailing List
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference
2022-03-01 15:14 ` Rafael J. Wysocki
@ 2022-03-01 20:48 ` Daniel Lezcano
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Lezcano @ 2022-03-01 20:48 UTC (permalink / raw)
To: Rafael J. Wysocki, Nicolas Cavallari
Cc: Amit Kucheria, Zhang Rui, Linux PM, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Testing the thermal genetlink API
2022-02-28 11:03 Testing the thermal genetlink API Nicolas Cavallari
2022-02-28 11:03 ` [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference Nicolas Cavallari
@ 2022-02-28 12:21 ` Daniel Lezcano
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Lezcano @ 2022-02-28 12:21 UTC (permalink / raw)
To: Nicolas Cavallari, Rafael J. Wysocki, Amit Kucheria, Zhang Rui
Cc: linux-pm, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-01 20:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-28 11:03 Testing the thermal genetlink API Nicolas Cavallari
2022-02-28 11:03 ` [PATCH] thermal: genetlink: Fix TZ_GET_TRIP NULL pointer dereference Nicolas Cavallari
2022-03-01 15:14 ` Rafael J. Wysocki
2022-03-01 20:48 ` Daniel Lezcano
2022-02-28 12:21 ` Testing the thermal genetlink API Daniel Lezcano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.