Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 4/5] thermal: core: genetlink support for
 events/cmd/sampling
To:     Zhang Rui <rui.zhang@intel.com>
Cc:     srinivas.pandruvada@linux.intel.com, rkumbako@codeaurora.org,
        amit.kucheria@linaro.org, linux-kernel@vger.kernel.org
References: <20200625144509.17918-1-daniel.lezcano@linaro.org>
 <20200625144509.17918-4-daniel.lezcano@linaro.org>
 <73150ef4ca536368f087672b917dd9773417020e.camel@intel.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <23458be6-1e5f-7397-4935-c5268b9f52a6@linaro.org>
Date:   Tue, 30 Jun 2020 20:32:21 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <73150ef4ca536368f087672b917dd9773417020e.camel@intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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