Re: [PATCH v2 4/5] thermal: core: genetlink support for events/cmd/sampling

[Zhang Rui]

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@xxxxxxxxxx>
> > > + *
> > > + * 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
> 
> 
> 
> 
>