Messages in this thread | | | Subject | Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling | From | Zhang Rui <> | Date | Wed, 01 Jul 2020 15:41:38 +0800 |
| |
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 > > > > >
| |