From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Alexandre Bailon <abailon@baylibre.com>
Cc: "Zhang, Rui" <rui.zhang@intel.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Amit Kucheria <amitk@kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
ben.tseng@mediatek.com, Kevin Hilman <khilman@baylibre.com>,
Matthias Kaehlcke <mka@chromium.org>,
kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2 2/2] thermal: add a virtual sensor to aggregate temperatures
Date: Thu, 7 Oct 2021 18:38:22 +0200 [thread overview]
Message-ID: <CAJZ5v0hzS-bquhW_wbsd81EpJYx5RHxeXkamdZ+MBs4oczDw1A@mail.gmail.com> (raw)
In-Reply-To: <20210917072732.611140-3-abailon@baylibre.com>
On Fri, Sep 17, 2021 at 9:25 AM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This adds a virtual thermal sensor that reads temperature from
This should be "virtual sensor driver" I suppose.
> hardware sensor and return an aggregated temperature.
"returns"
Also wrapping it around a single hardware sensor makes a little sense
AFAICS, so I guess you mean "multiple hardware sensors" here.
> Currently, this supports three operations:
> the minimum, maximum and average temperature.
They are aggregation functions rather than "operations"
>
> Reported-by: kernel test robot <lkp@intel.com>
This is new code, so what does it fix?
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
> drivers/thermal/Kconfig | 8 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/virtual-thermal-sensor.h | 54 ++++
> drivers/thermal/virtual_thermal_sensor.c | 350 +++++++++++++++++++++++
> 4 files changed, 413 insertions(+)
> create mode 100644 drivers/thermal/virtual-thermal-sensor.h
> create mode 100644 drivers/thermal/virtual_thermal_sensor.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index d7f44deab5b1d..20bc93c48f5b1 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -228,6 +228,14 @@ config THERMAL_MMIO
> register or shared memory, is a potential candidate to work with this
> driver.
>
> +config VIRTUAL_THERMAL
> + tristate "Virtual thermal sensor driver"
This should be "DT-based virtual thermal sensor driver" I think.
> + depends on THERMAL_OF || COMPILE_TEST
> + help
> + This option enables the generic thermal sensor aggregator.
"generic DT-based"
> + This driver creates a thermal sensor that reads the hardware sensors
s/the/multiple/
> + and aggregate the temperature.
"aggregates their output"
> +
> config HISI_THERMAL
> tristate "Hisilicon thermal driver"
> depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 82fc3e616e54b..8bf55973059c5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o
> obj-$(CONFIG_AMLOGIC_THERMAL) += amlogic_thermal.o
> obj-$(CONFIG_SPRD_THERMAL) += sprd_thermal.o
> obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL) += khadas_mcu_fan.o
> +obj-$(CONFIG_VIRTUAL_THERMAL) += virtual_thermal_sensor.o
> diff --git a/drivers/thermal/virtual-thermal-sensor.h b/drivers/thermal/virtual-thermal-sensor.h
> new file mode 100644
> index 0000000000000..3bbf7c324dddc
> --- /dev/null
> +++ b/drivers/thermal/virtual-thermal-sensor.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 BayLibre
> + */
> +
> +#ifndef __THERMAL_VIRTUAL_SENSOR_H__
> +#define __THERMAL_VIRTUAL_SENSOR_H__
> +
> +#include <linux/device.h>
> +#include <linux/thermal.h>
> +
> +struct virtual_thermal_sensor;
> +struct thermal_sensor_data;
> +
> +#if IS_ENABLED(CONFIG_VIRTUAL_THERMAL)
> +struct thermal_sensor_data *
> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> + const struct thermal_zone_of_device_ops *ops);
> +void thermal_virtual_sensor_unregister(struct device *dev,
> + struct thermal_sensor_data *sensor_data);
> +struct thermal_sensor_data *
> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> + const struct thermal_zone_of_device_ops *ops);
> +
> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
> + struct virtual_thermal_sensor *sensor);
> +#else
> +static inline struct thermal_sensor_data *
> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> + const struct thermal_zone_of_device_ops *ops)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> +void thermal_virtual_sensor_unregister(struct device *dev,
> + struct thermal_sensor_data *sensor_data)
> +{
> +}
> +
> +static inline struct thermal_sensor_data *
> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> + const struct thermal_zone_of_device_ops *ops)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> +static inline
> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
> + struct virtual_thermal_sensor *sensor)
> +{
> +}
> +#endif
> +
> +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */
> diff --git a/drivers/thermal/virtual_thermal_sensor.c b/drivers/thermal/virtual_thermal_sensor.c
> new file mode 100644
> index 0000000000000..234563af6643e
> --- /dev/null
> +++ b/drivers/thermal/virtual_thermal_sensor.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 BayLibre
Please add information on what's in this file to the preamble.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +
> +#include <dt-bindings/thermal/virtual-sensor.h>
> +
> +#include "virtual-thermal-sensor.h"
> +
> +struct thermal_sensor_data {
> + struct list_head node;
> +
> + /* sensor interface */
> + int id;
> + void *sensor_data;
> + const struct thermal_zone_of_device_ops *ops;
> +};
> +
> +struct virtual_thermal_sensor {
> + int count;
> + struct thermal_sensor_data *sensors;
> + struct thermal_zone_device *tzd;
> + int (*aggr_temp)(int temp1, int temp2);
> +
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(thermal_sensors);
> +static LIST_HEAD(virtual_sensors);
These lists don't seem to be protected against concurrent access and
they should be.
> +
> +static int max_temp(int temp1, int temp2)
> +{
> + return max(temp1, temp2);
> +}
> +
> +static int min_temp(int temp1, int temp2)
> +{
> + return min(temp1, temp2);
> +}
> +
> +static int avg_temp(int temp1, int temp2)
> +{
> + return ((temp1) / 2) + ((temp2) / 2) + (((temp1) % 2 + (temp2) % 2) / 2);
There are a few redundant parens here.
And why not
return (temp1 + temp2) / 2;
> +}
> +
> +static int virtual_thermal_sensor_get_temp(void *data, int *temperature)
> +{
> + struct virtual_thermal_sensor *sensor = data;
> + int max_temp = INT_MIN;
> + int temp;
> + int i;
> +
Some synchronization (eg. locking) should be added to this. It is racy as is.
> + for (i = 0; i < sensor->count; i++) {
> + struct thermal_sensor_data *hw_sensor;
> +
> + hw_sensor = &sensor->sensors[i];
> + if (!hw_sensor->ops)
> + return -ENODEV;
> +
> + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
> + max_temp = sensor->aggr_temp(max_temp, temp);
> + }
> +
> + *temperature = max_temp;
> +
> + return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops virtual_thermal_sensor_ops = {
> + .get_temp = virtual_thermal_sensor_get_temp,
> +};
> +
> +static int virtual_sensor_add_sensor(struct virtual_thermal_sensor *sensor,
> + struct of_phandle_args args,
> + int index)
> +{
> + struct thermal_sensor_data *sensor_data;
> + int id;
> +
> + list_for_each_entry(sensor_data, &thermal_sensors, node) {
> + id = args.args_count ? args.args[0] : 0;
> + if (sensor_data->id == id) {
> + memcpy(&sensor->sensors[index], sensor_data,
> + sizeof(*sensor_data));
The data object being copied includes pointers, so I wouldn't copy
them like this. Especially the ops one annotated as const.
> + return 0;
> + }
> + }
> +
> + return -ENODEV;
> +}
> +
> +static int virtual_thermal_sensor_probe(struct platform_device *pdev)
> +{
> + struct virtual_thermal_sensor *sensor;
> + struct device *dev = &pdev->dev;
> + struct of_phandle_args args;
> + u32 type;
> + int ret;
> + int i;
> +
> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> + if (!sensor)
> + return -ENOMEM;
> +
> + sensor->count = of_count_phandle_with_args(dev->of_node,
> + "thermal-sensors",
> + "#thermal-sensor-cells");
> + if (sensor->count <= 0)
> + return -EINVAL;
> +
> + sensor->sensors = devm_kmalloc_array(dev, sensor->count,
> + sizeof(*sensor->sensors),
> + GFP_KERNEL);
> + if (!sensor->sensors)
> + return -ENOMEM;
> +
> + for (i = 0; i < sensor->count; i++) {
> + ret = of_parse_phandle_with_args(dev->of_node,
> + "thermal-sensors",
> + "#thermal-sensor-cells",
> + i,
> + &args);
> + if (ret)
> + return ret;
> +
> + ret = virtual_sensor_add_sensor(sensor, args, i);
> + if (ret)
> + return ret;
> + }
> +
> + ret = of_property_read_u32(dev->of_node, "aggregation-function", &type);
> + if (ret)
> + return ret;
> +
> + switch (type) {
> + case VIRTUAL_THERMAL_SENSOR_MAX:
> + sensor->aggr_temp = max_temp;
> + break;
> + case VIRTUAL_THERMAL_SENSOR_MIN:
> + sensor->aggr_temp = min_temp;
> + break;
> + case VIRTUAL_THERMAL_SENSOR_AVG:
> + sensor->aggr_temp = avg_temp;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor,
> + &virtual_thermal_sensor_ops);
> + if (IS_ERR(sensor->tzd))
> + return PTR_ERR(sensor->tzd);
> +
> + platform_set_drvdata(pdev, sensor);
> + list_add(&sensor->node, &virtual_sensors);
> +
> + return 0;
> +}
> +
> +static int virtual_thermal_sensor_remove(struct platform_device *pdev)
> +{
> + struct virtual_thermal_sensor *sensor;
> +
> + sensor = platform_get_drvdata(pdev);
> + list_del(&sensor->node);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id virtual_thermal_sensor_of_match[] = {
> + {
> + .compatible = "virtual,thermal-sensor",
> + },
> + {
> + },
> +};
> +MODULE_DEVICE_TABLE(of, virtual_thermal_sensor_of_match);
> +
> +static struct platform_driver virtual_thermal_sensor = {
> + .probe = virtual_thermal_sensor_probe,
> + .remove = virtual_thermal_sensor_remove,
> + .driver = {
> + .name = "virtual-thermal-sensor",
> + .of_match_table = virtual_thermal_sensor_of_match,
> + },
> +};
> +
> +/**
> + * thermal_virtual_sensor_register - registers a sensor that could be a virtual
> + * sensor
> + * @dev: a valid struct device pointer of a sensor device. Must contain
> + * a valid .of_node, for the sensor node.
> + * @sensor_id: a sensor identifier, in case the sensor IP has more
> + * than one sensor
> + * @data: a private pointer (owned by the caller) that will be passed
> + * back, when a temperature reading is needed.
> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
> + *
> + * This function will register a thermal sensor to make it available for later
> + * usage by a virtual sensor.
So who's expected to be calling it?
> + *
> + * The thermal zone temperature is provided by the @get_temp function
> + * pointer. When called, it will have the private pointer @data back.
It is unclear what you mean here. I think you are describing a return
mechanism, but as is it is hard to follow.
> + *
> + * Return: On success returns a valid struct thermal_zone_device,
That doesn't seem to be the case. It returns an address returned by
devm_kzalloc() and the memory at that address is expected to be a
thermal_sensor_data object.
> + * otherwise, it returns a corresponding ERR_PTR(). Caller must
> + * check the return value with help of IS_ERR() helper.
> + */
> +struct thermal_sensor_data *thermal_virtual_sensor_register(
> + struct device *dev, int sensor_id, void *data,
> + const struct thermal_zone_of_device_ops *ops)
> +{
> + struct thermal_sensor_data *sensor_data;
> +
> + sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL);
> + if (!sensor_data)
> + return ERR_PTR(-ENOMEM);
> +
> + sensor_data->id = sensor_id;
> + sensor_data->sensor_data = data;
> + sensor_data->ops = ops;
> +
> + list_add(&sensor_data->node, &thermal_sensors);
> +
> + return sensor_data;
Overall, this is non-modular code squeezed into a module.
If this module goes away and whoever called it wants to unregister the
sensor, they will crash, so to a minimum it should do a
try_module_get() on the module containing it.
> +}
> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register);
> +
> +/**
> + * thermal_virtual_sensor_unregister - unregisters a sensor
> + * @dev: a valid struct device pointer of a sensor device.
> + * @sensor_data: a pointer to struct thermal_sensor_data to unregister.
> + *
> + * This function removes the sensor from the list of available thermal sensors.
> + * If the sensor is in use, then the next call to .get_temp will return -ENODEV.
> + */
> +void thermal_virtual_sensor_unregister(struct device *dev,
> + struct thermal_sensor_data *sensor_data)
> +{
> + struct thermal_sensor_data *sd;
> + struct virtual_thermal_sensor *sensor;
> + int i;
> +
All of the code in this function is obviously racy.
> + list_del(&sensor_data->node);
> +
> + list_for_each_entry(sensor, &virtual_sensors, node) {
> + for (i = 0; i < sensor->count; i++) {
> + sd = &sensor->sensors[i];
> + if (sd->id == sensor_data->id &&
> + sd->sensor_data == sensor_data->sensor_data) {
> + sd->ops = NULL;
> + }
The inner braces are not necessary.
> + }
> + }
And again, this code requires the module to be protected from
unloading until the last instance of
thermal_virtual_sensor_unregister() has returned.
> +}
> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister);
> +
> +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res)
> +{
> + thermal_virtual_sensor_unregister(dev,
> + *(struct thermal_sensor_data **)res);
> +}
> +
> +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res,
> + void *data)
> +{
> + struct thermal_sensor_data **r = res;
> +
> + if (WARN_ON(!r || !*r))
> + return 0;
> +
> + return *r == data;
> +}
> +
> +/**
> + * devm_thermal_virtual_sensor_register - Resource managed version of
> + * thermal_virtual_sensor_register()
> + * @dev: a valid struct device pointer of a sensor device. Must contain
> + * a valid .of_node, for the sensor node.
> + * @sensor_id: a sensor identifier, in case the sensor IP has more
> + * than one sensor
> + * @data: a private pointer (owned by the caller) that will be passed
> + * back, when a temperature reading is needed.
> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
> + *
> + * Refer thermal_zone_of_sensor_register() for more details.
> + *
> + * Return: On success returns a valid struct virtual_sensor_data,
> + * otherwise, it returns a corresponding ERR_PTR(). Caller must
> + * check the return value with help of IS_ERR() helper.
> + * Registered virtual_sensor_data device will automatically be
> + * released when device is unbound.
> + */
> +struct thermal_sensor_data *devm_thermal_virtual_sensor_register(
> + struct device *dev, int sensor_id,
> + void *data, const struct thermal_zone_of_device_ops *ops)
> +{
> + struct thermal_sensor_data **ptr, *sensor_data;
> +
> + ptr = devres_alloc(devm_thermal_virtual_sensor_release, sizeof(*ptr),
> + GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + sensor_data = thermal_virtual_sensor_register(dev, sensor_id, data, ops);
> + if (IS_ERR(sensor_data)) {
> + devres_free(ptr);
> + return sensor_data;
> + }
> +
> + *ptr = sensor_data;
> + devres_add(dev, ptr);
> +
> + return sensor_data;
> +}
> +EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_register);
> +
> +/**
> + * devm_thermal_virtual_sensor_unregister - Resource managed version of
> + * thermal_virtual_sensor_unregister().
> + * @dev: Device for which resource was allocated.
> + * @sensor: a pointer to struct thermal_zone_device where the sensor is registered.
> + *
> + * This function removes the sensor from the list of sensors registered with
> + * devm_thermal_virtual_sensor_register() API.
> + * Normally this function will not need to be called and the resource
> + * management code will ensure that the resource is freed.
> + */
> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
> + struct virtual_thermal_sensor *sensor)
> +{
> + WARN_ON(devres_release(dev, devm_thermal_virtual_sensor_release,
> + devm_thermal_virtual_sensor_match, sensor));
> +}
> +EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_unregister);
> +
> +module_platform_driver(virtual_thermal_sensor);
> +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
> +MODULE_DESCRIPTION("Virtual thermal sensor");
> +MODULE_LICENSE("GPL v2");
> --
next prev parent reply other threads:[~2021-10-07 16:38 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-17 7:27 [PATCH v2 0/2] Add a generic virtual thermal sensor Alexandre Bailon
2021-09-17 7:27 ` [PATCH v2 1/2] dt-bindings: Add bindings for the " Alexandre Bailon
2021-09-17 7:27 ` [PATCH v2 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon
2021-10-07 16:38 ` Rafael J. Wysocki [this message]
2021-10-07 17:33 ` Rafael J. Wysocki
2021-10-09 14:46 ` Rafael J. Wysocki
2021-10-25 9:18 ` Alexandre Bailon
2021-09-17 12:41 ` [PATCH v2 0/2] Add a generic virtual thermal sensor Daniel Lezcano
2021-09-17 13:33 ` Alexandre Bailon
2021-09-17 14:03 ` Daniel Lezcano
2021-09-20 13:12 ` Alexandre Bailon
2021-09-22 8:10 ` Daniel Lezcano
2021-10-04 10:24 ` Alexandre Bailon
2021-10-04 13:42 ` Daniel Lezcano
2021-10-05 16:45 ` Rafael J. Wysocki
2021-10-06 16:06 ` Daniel Lezcano
2021-10-06 18:00 ` Rafael J. Wysocki
2021-10-06 19:51 ` Daniel Lezcano
2021-10-07 14:17 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJZ5v0hzS-bquhW_wbsd81EpJYx5RHxeXkamdZ+MBs4oczDw1A@mail.gmail.com \
--to=rafael@kernel.org \
--cc=abailon@baylibre.com \
--cc=amitk@kernel.org \
--cc=ben.tseng@mediatek.com \
--cc=daniel.lezcano@linaro.org \
--cc=khilman@baylibre.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lkp@intel.com \
--cc=mka@chromium.org \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.