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 <[email protected]>
> > > + *
> > > + * 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
>
>
>
>
>
