diff mbox series

[v5,2/4] tools/lib/thermal: Make more generic the command encoding function

Message ID 20241014094309.1430126-3-daniel.lezcano@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series Add thermal user thresholds support | expand

Commit Message

Daniel Lezcano Oct. 14, 2024, 9:43 a.m. UTC 
The thermal netlink has been extended with more commands which require
an encoding with more information. The generic encoding function puts
the thermal zone id with the command name. It is the unique
parameters.

The next changes will provide more parameters to the command. Set the
scene for those new parameters by making the encoding function more
generic.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 tools/lib/thermal/commands.c | 41 ++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

Comments

Lukasz Luba Oct. 21, 2024, 7:49 p.m. UTC | #1 
On 10/14/24 10:43, Daniel Lezcano wrote:
> The thermal netlink has been extended with more commands which require
> an encoding with more information. The generic encoding function puts
> the thermal zone id with the command name. It is the unique
> parameters.
> 
> The next changes will provide more parameters to the command. Set the
> scene for those new parameters by making the encoding function more
> generic.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   tools/lib/thermal/commands.c | 41 ++++++++++++++++++++++++++++--------
>   1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
> index 73d4d4e8d6ec..a9223df91dcf 100644
> --- a/tools/lib/thermal/commands.c
> +++ b/tools/lib/thermal/commands.c
> @@ -261,8 +261,23 @@ static struct genl_ops thermal_cmd_ops = {
>   	.o_ncmds	= ARRAY_SIZE(thermal_cmds),
>   };
>   
> -static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int cmd,
> -					 int flags, void *arg)
> +struct cmd_param {
> +	int tz_id;
> +};
> +
> +typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
> +
> +static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
> +{
> +	if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th, cmd_cb_t cmd_cb,
> +					 struct cmd_param *param,
> +					 int cmd, int flags, void *arg)
>   {
>   	struct nl_msg *msg;
>   	void *hdr;
> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
>   	if (!hdr)
>   		return THERMAL_ERROR;
>   
> -	if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
> +	if (cmd_cb && cmd_cb(msg, param))
>   		return THERMAL_ERROR;

It's not in this code but also in older:
shouldn't we free the nlmsg_free(msg); before returns in this
function?


The rest design looks good
Daniel Lezcano Oct. 22, 2024, 7:12 a.m. UTC | #2 
On 21/10/2024 21:49, Lukasz Luba wrote:
> 
> 
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> The thermal netlink has been extended with more commands which require
>> an encoding with more information. The generic encoding function puts
>> the thermal zone id with the command name. It is the unique
>> parameters.
>>
>> The next changes will provide more parameters to the command. Set the
>> scene for those new parameters by making the encoding function more
>> generic.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---

[ ... ]

>> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th, 
>> cmd_cb_t cmd_cb,
>> +                     struct cmd_param *param,
>> +                     int cmd, int flags, void *arg)
>>   {
>>       struct nl_msg *msg;
>>       void *hdr;
>> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct 
>> thermal_handler *th, int id, int
>>       if (!hdr)
>>           return THERMAL_ERROR;
>> -    if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
>> +    if (cmd_cb && cmd_cb(msg, param))
>>           return THERMAL_ERROR;
> 
> It's not in this code but also in older:
> shouldn't we free the nlmsg_free(msg); before returns in this
> function?

Right, thanks for pointing this out

If it is ok, I will send a patch on top of this series to fix the entire 
function
Lukasz Luba Oct. 22, 2024, 9:43 a.m. UTC | #3 
On 10/22/24 08:12, Daniel Lezcano wrote:
> On 21/10/2024 21:49, Lukasz Luba wrote:
>>
>>
>> On 10/14/24 10:43, Daniel Lezcano wrote:
>>> The thermal netlink has been extended with more commands which require
>>> an encoding with more information. The generic encoding function puts
>>> the thermal zone id with the command name. It is the unique
>>> parameters.
>>>
>>> The next changes will provide more parameters to the command. Set the
>>> scene for those new parameters by making the encoding function more
>>> generic.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
> 
> [ ... ]
> 
>>> +static thermal_error_t thermal_genl_auto(struct thermal_handler *th, 
>>> cmd_cb_t cmd_cb,
>>> +                     struct cmd_param *param,
>>> +                     int cmd, int flags, void *arg)
>>>   {
>>>       struct nl_msg *msg;
>>>       void *hdr;
>>> @@ -276,7 +291,7 @@ static thermal_error_t thermal_genl_auto(struct 
>>> thermal_handler *th, int id, int
>>>       if (!hdr)
>>>           return THERMAL_ERROR;
>>> -    if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
>>> +    if (cmd_cb && cmd_cb(msg, param))
>>>           return THERMAL_ERROR;
>>
>> It's not in this code but also in older:
>> shouldn't we free the nlmsg_free(msg); before returns in this
>> function?
> 
> Right, thanks for pointing this out
> 
> If it is ok, I will send a patch on top of this series to fix the entire 
> function
> 
> 

That fine for me, unless Rafael wants a v6 version with those
new locking. I'm OK with both, I will review any version.

BTW I'm still struggling to cross-build or native build that
libthermal, so I cannot give you test results.
I need to sort our the headers and OS packages on dev board...
diff mbox series

Patch

diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
index 73d4d4e8d6ec..a9223df91dcf 100644
--- a/tools/lib/thermal/commands.c
+++ b/tools/lib/thermal/commands.c
@@ -261,8 +261,23 @@  static struct genl_ops thermal_cmd_ops = {
 	.o_ncmds	= ARRAY_SIZE(thermal_cmds),
 };
 
-static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int cmd,
-					 int flags, void *arg)
+struct cmd_param {
+	int tz_id;
+};
+
+typedef int (*cmd_cb_t)(struct nl_msg *, struct cmd_param *);
+
+static int thermal_genl_tz_id_encode(struct nl_msg *msg, struct cmd_param *p)
+{
+	if (p->tz_id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, p->tz_id))
+		return -1;
+
+	return 0;
+}
+
+static thermal_error_t thermal_genl_auto(struct thermal_handler *th, cmd_cb_t cmd_cb,
+					 struct cmd_param *param,
+					 int cmd, int flags, void *arg)
 {
 	struct nl_msg *msg;
 	void *hdr;
@@ -276,7 +291,7 @@  static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
 	if (!hdr)
 		return THERMAL_ERROR;
 
-	if (id >= 0 && nla_put_u32(msg, THERMAL_GENL_ATTR_TZ_ID, id))
+	if (cmd_cb && cmd_cb(msg, param))
 		return THERMAL_ERROR;
 
 	if (nl_send_msg(th->sk_cmd, th->cb_cmd, msg, genl_handle_msg, arg))
@@ -289,30 +304,38 @@  static thermal_error_t thermal_genl_auto(struct thermal_handler *th, int id, int
 
 thermal_error_t thermal_cmd_get_tz(struct thermal_handler *th, struct thermal_zone **tz)
 {
-	return thermal_genl_auto(th, -1, THERMAL_GENL_CMD_TZ_GET_ID,
+	return thermal_genl_auto(th, NULL, NULL, THERMAL_GENL_CMD_TZ_GET_ID,
 				 NLM_F_DUMP | NLM_F_ACK, tz);
 }
 
 thermal_error_t thermal_cmd_get_cdev(struct thermal_handler *th, struct thermal_cdev **tc)
 {
-	return thermal_genl_auto(th, -1, THERMAL_GENL_CMD_CDEV_GET,
+	return thermal_genl_auto(th, NULL, NULL, THERMAL_GENL_CMD_CDEV_GET,
 				 NLM_F_DUMP | NLM_F_ACK, tc);
 }
 
 thermal_error_t thermal_cmd_get_trip(struct thermal_handler *th, struct thermal_zone *tz)
 {
-	return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_TRIP,
-				 0, tz);
+	struct cmd_param p = { .tz_id = tz->id };
+
+	return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+				 THERMAL_GENL_CMD_TZ_GET_TRIP, 0, tz);
 }
 
 thermal_error_t thermal_cmd_get_governor(struct thermal_handler *th, struct thermal_zone *tz)
 {
-	return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_GOV, 0, tz);
+	struct cmd_param p = { .tz_id = tz->id };
+
+	return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+				 THERMAL_GENL_CMD_TZ_GET_GOV, 0, tz);
 }
 
 thermal_error_t thermal_cmd_get_temp(struct thermal_handler *th, struct thermal_zone *tz)
 {
-	return thermal_genl_auto(th, tz->id, THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
+	struct cmd_param p = { .tz_id = tz->id };
+
+	return thermal_genl_auto(th, thermal_genl_tz_id_encode, &p,
+				 THERMAL_GENL_CMD_TZ_GET_TEMP, 0, tz);
 }
 
 thermal_error_t thermal_cmd_exit(struct thermal_handler *th)