* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-05-26 16:29 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-05-26 16:29 UTC (permalink / raw)
The NVMe controller reports up to nine temperature values in the SMART /
Health log page (the composite temperature and temperature sensor 1 through
temperature sensor 8).
This provides these temperatures via thermal zone devices.
Once the controller is identified, the thermal zone devices are created for
all implemented temperature sensors including the composite temperature.
/sys/class/thermal/thermal_zone[0-*]:
|---type: 'nvme<instance>-temp<sensor>'
|---temp: Temperature
|---trip_point_0_temp: Over temperature threshold
The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
device.
On the other hand, the following symlinks to the thermal zone devices are
created in the nvme device sysfs directory.
- temp0: Composite temperature
- temp1: Temperature sensor 1
...
- temp8: Temperature sensor 8
Cc: Zhang Rui <rui.zhang at intel.com>
Cc: Eduardo Valentin <edubezval at gmail.com>
Cc: Daniel Lezcano <daniel.lezcano at linaro.org>
Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Minwoo Im <minwoo.im.dev at gmail.com>
Cc: Kenneth Heitke <kenneth.heitke at intel.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
* v3
- Change the type name of thermal zone devices from 'nvme_temp<sensor>' to
'nvme<instance>_temp<sensor>'
- Pass a NULL to the status argument of nvme_set_feature()
- Change the name of symbolic link from 'nvme_temp<sensor>' to 'temp<sensor>'
- Don't make it fatal error if the device provides a response
- Don't register thermal zone for composite temperature if smart log reports
zero value
- Move the thermal zones registration and unregistration into the core module.
drivers/nvme/host/core.c | 288 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 9 ++
include/linux/nvme.h | 5 +
3 files changed, 302 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c950916..4c8271a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2200,6 +2200,289 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
}
}
+#ifdef CONFIG_THERMAL
+
+static int nvme_get_temp(struct nvme_ctrl *ctrl, unsigned int sensor, int *temp)
+{
+ struct nvme_smart_log *log;
+ int ret;
+
+ BUILD_BUG_ON(ARRAY_SIZE(log->temp_sensor) + 1 !=
+ ARRAY_SIZE(ctrl->tzdev));
+
+ if (WARN_ON_ONCE(sensor > ARRAY_SIZE(log->temp_sensor)))
+ return -EINVAL;
+
+ log = kzalloc(sizeof(*log), GFP_KERNEL);
+ if (!log)
+ return -ENOMEM;
+
+ ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
+ log, sizeof(*log), 0);
+ if (ret) {
+ ret = ret > 0 ? -EINVAL : ret;
+ goto free_log;
+ }
+
+ if (sensor)
+ *temp = le16_to_cpu(log->temp_sensor[sensor - 1]);
+ else
+ *temp = get_unaligned_le16(log->temperature);
+
+free_log:
+ kfree(log);
+
+ return ret;
+}
+
+static unsigned int nvme_tz_type_to_sensor(const char *type)
+{
+ int instance;
+ unsigned int sensor;
+
+ if (sscanf(type, "nvme%d_temp%u", &instance, &sensor) != 2)
+ return UINT_MAX;
+
+ return sensor;
+}
+
+#define KELVIN_TO_MILLICELSIUS(t) DECI_KELVIN_TO_MILLICELSIUS((t) * 10)
+#define MILLICELSIUS_TO_KELVIN(t) ((MILLICELSIUS_TO_DECI_KELVIN(t) + 5) / 10)
+
+static int nvme_tz_get_temp(struct thermal_zone_device *tzdev, int *temp)
+{
+ unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
+ struct nvme_ctrl *ctrl = tzdev->devdata;
+ int ret;
+
+ ret = nvme_get_temp(ctrl, sensor, temp);
+ if (!ret)
+ *temp = KELVIN_TO_MILLICELSIUS(*temp);
+
+ return ret;
+}
+
+static int nvme_tz_get_trip_type(struct thermal_zone_device *tzdev,
+ int trip, enum thermal_trip_type *type)
+{
+ *type = THERMAL_TRIP_ACTIVE;
+
+ return 0;
+}
+
+static int nvme_get_over_temp_thresh(struct nvme_ctrl *ctrl,
+ unsigned int sensor, int *temp)
+{
+ unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
+ int status;
+ int ret;
+
+ if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
+ return -EINVAL;
+
+ ret = nvme_get_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
+ &status);
+ if (!ret)
+ *temp = status & NVME_TEMP_THRESH_MASK;
+
+ return ret > 0 ? -EINVAL : ret;
+}
+
+static int nvme_set_over_temp_thresh(struct nvme_ctrl *ctrl,
+ unsigned int sensor, int temp)
+{
+ unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
+ int ret;
+
+ if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
+ return -EINVAL;
+
+ if (temp > NVME_TEMP_THRESH_MASK)
+ return -EINVAL;
+
+ threshold |= temp & NVME_TEMP_THRESH_MASK;
+
+ ret = nvme_set_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
+ NULL);
+
+ return ret > 0 ? -EINVAL : ret;
+}
+
+static int nvme_tz_get_trip_temp(struct thermal_zone_device *tzdev,
+ int trip, int *temp)
+{
+ unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
+ struct nvme_ctrl *ctrl = tzdev->devdata;
+ int ret;
+
+ ret = nvme_get_over_temp_thresh(ctrl, sensor, temp);
+ if (!ret)
+ *temp = KELVIN_TO_MILLICELSIUS(*temp);
+
+ return ret;
+}
+
+static int nvme_tz_set_trip_temp(struct thermal_zone_device *tzdev,
+ int trip, int temp)
+{
+ unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
+ struct nvme_ctrl *ctrl = tzdev->devdata;
+
+ temp = MILLICELSIUS_TO_KELVIN(temp);
+
+ return nvme_set_over_temp_thresh(ctrl, sensor, temp);
+}
+
+static struct thermal_zone_device_ops nvme_tz_ops = {
+ .get_temp = nvme_tz_get_temp,
+ .get_trip_type = nvme_tz_get_trip_type,
+ .get_trip_temp = nvme_tz_get_trip_temp,
+ .set_trip_temp = nvme_tz_set_trip_temp,
+};
+
+static struct thermal_zone_params nvme_tz_params = {
+ .governor_name = "user_space",
+ .no_hwmon = true,
+};
+
+static struct thermal_zone_device *
+nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
+{
+ struct thermal_zone_device *tzdev;
+ char name[THERMAL_NAME_LENGTH];
+ int ret;
+
+ snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
+
+ tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
+ &nvme_tz_params, 0, 0);
+ if (IS_ERR(tzdev)) {
+ dev_err(ctrl->device,
+ "Failed to register thermal zone device: %ld\n",
+ PTR_ERR(tzdev));
+ return tzdev;
+ }
+
+ snprintf(name, sizeof(name), "temp%d", sensor);
+ ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
+ name);
+ if (ret)
+ goto device_unregister;
+
+ ret = sysfs_create_link(&tzdev->device.kobj,
+ &ctrl->ctrl_device.kobj, "device");
+ if (ret)
+ goto remove_link;
+
+ return tzdev;
+
+remove_link:
+ sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
+device_unregister:
+ thermal_zone_device_unregister(tzdev);
+
+ return ERR_PTR(ret);
+}
+
+/**
+ * nvme_thermal_zones_register() - register nvme thermal zone devices
+ * @ctrl: controller instance
+ *
+ * This function creates up to nine thermal zone devices for all implemented
+ * temperature sensors including the composite temperature.
+ * Each thermal zone device provides a single trip point temperature that is
+ * associated with an over temperature threshold.
+ */
+static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
+{
+ struct nvme_smart_log *log;
+ int ret;
+ int i;
+
+ log = kzalloc(sizeof(*log), GFP_KERNEL);
+ if (!log)
+ return 0; /* non-fatal error */
+
+ ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
+ log, sizeof(*log), 0);
+ if (ret) {
+ dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
+ /* If the device provided a response, then it's non-fatal */
+ if (ret > 0)
+ ret = 0;
+ goto free_log;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
+ struct thermal_zone_device *tzdev;
+ int temp;
+
+ if (i)
+ temp = le16_to_cpu(log->temp_sensor[i - 1]);
+ else
+ temp = get_unaligned_le16(log->temperature);
+
+ /*
+ * All implemented temperature sensors report a non-zero value
+ * in temperature sensor fields in the smart log page.
+ */
+ if (!temp)
+ continue;
+ if (ctrl->tzdev[i])
+ continue;
+
+ tzdev = nvme_thermal_zone_register(ctrl, i);
+ if (!IS_ERR(tzdev))
+ ctrl->tzdev[i] = tzdev;
+ }
+
+free_log:
+ kfree(log);
+
+ return ret;
+}
+
+/**
+ * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
+ * @ctrl: controller instance
+ *
+ * This function removes the registered thermal zone devices and symlinks.
+ */
+static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
+ struct thermal_zone_device *tzdev = ctrl->tzdev[i];
+ char name[20];
+
+ if (!tzdev)
+ continue;
+
+ sysfs_remove_link(&tzdev->device.kobj, "device");
+
+ snprintf(name, sizeof(name), "temp%d", i);
+ sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
+
+ thermal_zone_device_unregister(tzdev);
+
+ ctrl->tzdev[i] = NULL;
+ }
+}
+
+#else
+
+static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
+{
+ return 0;
+}
+
+static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
+{
+}
+
+#endif /* CONFIG_THERMAL */
+
struct nvme_core_quirk_entry {
/*
* NVMe model and firmware strings are padded with spaces. For
@@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
if (ret < 0)
return ret;
+ ret = nvme_thermal_zones_register(ctrl);
+ if (ret < 0)
+ return ret;
+
ctrl->identified = true;
return 0;
@@ -3756,6 +4043,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
{
nvme_mpath_stop(ctrl);
nvme_stop_keep_alive(ctrl);
+ nvme_thermal_zones_unregister(ctrl);
flush_work(&ctrl->async_event_work);
cancel_work_sync(&ctrl->fw_act_work);
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 802aa19..53f0b24 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -15,6 +15,7 @@
#include <linux/sed-opal.h>
#include <linux/fault-inject.h>
#include <linux/rcupdate.h>
+#include <linux/thermal.h>
extern unsigned int nvme_io_timeout;
#define NVME_IO_TIMEOUT (nvme_io_timeout * HZ)
@@ -248,6 +249,14 @@ struct nvme_ctrl {
struct page *discard_page;
unsigned long discard_page_busy;
+
+#ifdef CONFIG_THERMAL
+ /*
+ * tzdev[0]: composite temperature
+ * tzdev[1-8]: temperature sensor 1 through 8
+ */
+ struct thermal_zone_device *tzdev[9];
+#endif
};
enum nvme_iopolicy {
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 658ac75..54f0a13 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -780,6 +780,11 @@ struct nvme_write_zeroes_cmd {
/* Features */
+enum {
+ NVME_TEMP_THRESH_MASK = 0xffff,
+ NVME_TEMP_THRESH_SELECT_SHIFT = 16,
+};
+
struct nvme_feat_auto_pst {
__le64 entries[32];
};
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-05-26 16:29 ` Akinobu Mita
@ 2019-05-29 15:15 ` Minwoo Im
-1 siblings, 0 replies; 40+ messages in thread
From: Minwoo Im @ 2019-05-29 15:15 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Kenneth Heitke, Chaitanya Kulkarni
> +/**
> + * nvme_thermal_zones_register() - register nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function creates up to nine thermal zone devices for all implemented
> + * temperature sensors including the composite temperature.
> + * Each thermal zone device provides a single trip point temperature that is
> + * associated with an over temperature threshold.
> + */
> +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> + int i;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return 0; /* non-fatal error */
Do we need to print something like warning here? If kzalloc() fails, it
would be good to be distinguished between the nvme failure and internal
failure like this.
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> + /* If the device provided a response, then it's non-fatal */
> + if (ret > 0)
> + ret = 0;
It seems like that nvme_init_identify() is just check the internal error
which is in negative value now as you have posted. Why don't we just
return the value of "ret" itself without updating it to 0 ?
> + goto free_log;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev;
> + int temp;
> +
> + if (i)
> + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> + else
> + temp = get_unaligned_le16(log->temperature);
> +
> + /*
> + * All implemented temperature sensors report a non-zero value
> + * in temperature sensor fields in the smart log page.
> + */
> + if (!temp)
> + continue;
> + if (ctrl->tzdev[i])
> + continue;
> +
> + tzdev = nvme_thermal_zone_register(ctrl, i);
> + if (!IS_ERR(tzdev))
> + ctrl->tzdev[i] = tzdev;
> + }
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +/**
> + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function removes the registered thermal zone devices and symlinks.
> + */
> +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> + char name[20];
Simple query here :)
If we are not going to allow the name of link exceed the length of its
own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
here? If the name of link is not exactly about the device name itself,
then it's fine. What do you think about it ?
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-05-29 15:15 ` Minwoo Im
0 siblings, 0 replies; 40+ messages in thread
From: Minwoo Im @ 2019-05-29 15:15 UTC (permalink / raw)
> +/**
> + * nvme_thermal_zones_register() - register nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function creates up to nine thermal zone devices for all implemented
> + * temperature sensors including the composite temperature.
> + * Each thermal zone device provides a single trip point temperature that is
> + * associated with an over temperature threshold.
> + */
> +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> + int i;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return 0; /* non-fatal error */
Do we need to print something like warning here? If kzalloc() fails, it
would be good to be distinguished between the nvme failure and internal
failure like this.
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> + /* If the device provided a response, then it's non-fatal */
> + if (ret > 0)
> + ret = 0;
It seems like that nvme_init_identify() is just check the internal error
which is in negative value now as you have posted. Why don't we just
return the value of "ret" itself without updating it to 0 ?
> + goto free_log;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev;
> + int temp;
> +
> + if (i)
> + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> + else
> + temp = get_unaligned_le16(log->temperature);
> +
> + /*
> + * All implemented temperature sensors report a non-zero value
> + * in temperature sensor fields in the smart log page.
> + */
> + if (!temp)
> + continue;
> + if (ctrl->tzdev[i])
> + continue;
> +
> + tzdev = nvme_thermal_zone_register(ctrl, i);
> + if (!IS_ERR(tzdev))
> + ctrl->tzdev[i] = tzdev;
> + }
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +/**
> + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function removes the registered thermal zone devices and symlinks.
> + */
> +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> + char name[20];
Simple query here :)
If we are not going to allow the name of link exceed the length of its
own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
here? If the name of link is not exactly about the device name itself,
then it's fine. What do you think about it ?
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-05-29 15:15 ` Minwoo Im
@ 2019-05-29 16:47 ` Akinobu Mita
-1 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-05-29 16:47 UTC (permalink / raw)
To: Minwoo Im
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Kenneth Heitke, Chaitanya Kulkarni
2019年5月30日(木) 0:15 Minwoo Im <minwoo.im.dev@gmail.com>:
>
> > +/**
> > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function creates up to nine thermal zone devices for all implemented
> > + * temperature sensors including the composite temperature.
> > + * Each thermal zone device provides a single trip point temperature that is
> > + * associated with an over temperature threshold.
> > + */
> > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + struct nvme_smart_log *log;
> > + int ret;
> > + int i;
> > +
> > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > + if (!log)
> > + return 0; /* non-fatal error */
>
> Do we need to print something like warning here? If kzalloc() fails, it
> would be good to be distinguished between the nvme failure and internal
> failure like this.
We usually remove the error message on kmalloc failure because it's
redundant as long as we don't set __GFP_NOWARN.
> > +
> > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > + log, sizeof(*log), 0);
> > + if (ret) {
> > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > + /* If the device provided a response, then it's non-fatal */
> > + if (ret > 0)
> > + ret = 0;
>
> It seems like that nvme_init_identify() is just check the internal error
> which is in negative value now as you have posted. Why don't we just
> return the value of "ret" itself without updating it to 0 ?
Both ways work for me.
I personally prefer not to return (leak) the nvme status code from
foo_register() function.
> > + goto free_log;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev;
> > + int temp;
> > +
> > + if (i)
> > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > + else
> > + temp = get_unaligned_le16(log->temperature);
> > +
> > + /*
> > + * All implemented temperature sensors report a non-zero value
> > + * in temperature sensor fields in the smart log page.
> > + */
> > + if (!temp)
> > + continue;
> > + if (ctrl->tzdev[i])
> > + continue;
> > +
> > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > + if (!IS_ERR(tzdev))
> > + ctrl->tzdev[i] = tzdev;
> > + }
> > +
> > +free_log:
> > + kfree(log);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function removes the registered thermal zone devices and symlinks.
> > + */
> > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > + char name[20];
>
> Simple query here :)
>
> If we are not going to allow the name of link exceed the length of its
> own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
> here? If the name of link is not exactly about the device name itself,
> then it's fine. What do you think about it ?
Of course we can use THERMAL_NAME_LENGTH here. But this char array is
used only for the symbolic link name.
It's not used for thermal cooling device type, thermal zone device type,
or thermal governor name. So I just used a random constant to avoid
confusion.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-05-29 16:47 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-05-29 16:47 UTC (permalink / raw)
2019?5?30?(?) 0:15 Minwoo Im <minwoo.im.dev at gmail.com>:
>
> > +/**
> > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function creates up to nine thermal zone devices for all implemented
> > + * temperature sensors including the composite temperature.
> > + * Each thermal zone device provides a single trip point temperature that is
> > + * associated with an over temperature threshold.
> > + */
> > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + struct nvme_smart_log *log;
> > + int ret;
> > + int i;
> > +
> > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > + if (!log)
> > + return 0; /* non-fatal error */
>
> Do we need to print something like warning here? If kzalloc() fails, it
> would be good to be distinguished between the nvme failure and internal
> failure like this.
We usually remove the error message on kmalloc failure because it's
redundant as long as we don't set __GFP_NOWARN.
> > +
> > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > + log, sizeof(*log), 0);
> > + if (ret) {
> > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > + /* If the device provided a response, then it's non-fatal */
> > + if (ret > 0)
> > + ret = 0;
>
> It seems like that nvme_init_identify() is just check the internal error
> which is in negative value now as you have posted. Why don't we just
> return the value of "ret" itself without updating it to 0 ?
Both ways work for me.
I personally prefer not to return (leak) the nvme status code from
foo_register() function.
> > + goto free_log;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev;
> > + int temp;
> > +
> > + if (i)
> > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > + else
> > + temp = get_unaligned_le16(log->temperature);
> > +
> > + /*
> > + * All implemented temperature sensors report a non-zero value
> > + * in temperature sensor fields in the smart log page.
> > + */
> > + if (!temp)
> > + continue;
> > + if (ctrl->tzdev[i])
> > + continue;
> > +
> > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > + if (!IS_ERR(tzdev))
> > + ctrl->tzdev[i] = tzdev;
> > + }
> > +
> > +free_log:
> > + kfree(log);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function removes the registered thermal zone devices and symlinks.
> > + */
> > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > + char name[20];
>
> Simple query here :)
>
> If we are not going to allow the name of link exceed the length of its
> own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
> here? If the name of link is not exactly about the device name itself,
> then it's fine. What do you think about it ?
Of course we can use THERMAL_NAME_LENGTH here. But this char array is
used only for the symbolic link name.
It's not used for thermal cooling device type, thermal zone device type,
or thermal governor name. So I just used a random constant to avoid
confusion.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-05-29 16:47 ` Akinobu Mita
@ 2019-05-30 10:18 ` Minwoo Im
-1 siblings, 0 replies; 40+ messages in thread
From: Minwoo Im @ 2019-05-30 10:18 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Kenneth Heitke, Chaitanya Kulkarni
On 19-05-30 01:47:08, Akinobu Mita wrote:
> 2019年5月30日(木) 0:15 Minwoo Im <minwoo.im.dev@gmail.com>:
> >
> > > +/**
> > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function creates up to nine thermal zone devices for all implemented
> > > + * temperature sensors including the composite temperature.
> > > + * Each thermal zone device provides a single trip point temperature that is
> > > + * associated with an over temperature threshold.
> > > + */
> > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + struct nvme_smart_log *log;
> > > + int ret;
> > > + int i;
> > > +
> > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > + if (!log)
> > > + return 0; /* non-fatal error */
> >
> > Do we need to print something like warning here? If kzalloc() fails, it
> > would be good to be distinguished between the nvme failure and internal
> > failure like this.
>
> We usually remove the error message on kmalloc failure because it's
> redundant as long as we don't set __GFP_NOWARN.
I see what you point.
>
> > > +
> > > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > > + log, sizeof(*log), 0);
> > > + if (ret) {
> > > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > > + /* If the device provided a response, then it's non-fatal */
> > > + if (ret > 0)
> > > + ret = 0;
> >
> > It seems like that nvme_init_identify() is just check the internal error
> > which is in negative value now as you have posted. Why don't we just
> > return the value of "ret" itself without updating it to 0 ?
>
> Both ways work for me.
>
> I personally prefer not to return (leak) the nvme status code from
> foo_register() function.
Okay. In the perspective of registration, the nvme status might not be
needed to be returned. But I think if we return the nvme status here,
it would be great for the later time when we need to figure out if the nvme
command has failed or not.
But, amyway, I'm fine with this :)
Thanks for your comment on this trivial query.
>
> > > + goto free_log;
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev;
> > > + int temp;
> > > +
> > > + if (i)
> > > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > > + else
> > > + temp = get_unaligned_le16(log->temperature);
> > > +
> > > + /*
> > > + * All implemented temperature sensors report a non-zero value
> > > + * in temperature sensor fields in the smart log page.
> > > + */
> > > + if (!temp)
> > > + continue;
> > > + if (ctrl->tzdev[i])
> > > + continue;
> > > +
> > > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > > + if (!IS_ERR(tzdev))
> > > + ctrl->tzdev[i] = tzdev;
> > > + }
> > > +
> > > +free_log:
> > > + kfree(log);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function removes the registered thermal zone devices and symlinks.
> > > + */
> > > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > > + char name[20];
> >
> > Simple query here :)
> >
> > If we are not going to allow the name of link exceed the length of its
> > own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
> > here? If the name of link is not exactly about the device name itself,
> > then it's fine. What do you think about it ?
>
> Of course we can use THERMAL_NAME_LENGTH here. But this char array is
> used only for the symbolic link name.
> It's not used for thermal cooling device type, thermal zone device type,
> or thermal governor name. So I just used a random constant to avoid
> confusion.
Agreed.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-05-30 10:18 ` Minwoo Im
0 siblings, 0 replies; 40+ messages in thread
From: Minwoo Im @ 2019-05-30 10:18 UTC (permalink / raw)
On 19-05-30 01:47:08, Akinobu Mita wrote:
> 2019?5?30?(?) 0:15 Minwoo Im <minwoo.im.dev at gmail.com>:
> >
> > > +/**
> > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function creates up to nine thermal zone devices for all implemented
> > > + * temperature sensors including the composite temperature.
> > > + * Each thermal zone device provides a single trip point temperature that is
> > > + * associated with an over temperature threshold.
> > > + */
> > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + struct nvme_smart_log *log;
> > > + int ret;
> > > + int i;
> > > +
> > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > + if (!log)
> > > + return 0; /* non-fatal error */
> >
> > Do we need to print something like warning here? If kzalloc() fails, it
> > would be good to be distinguished between the nvme failure and internal
> > failure like this.
>
> We usually remove the error message on kmalloc failure because it's
> redundant as long as we don't set __GFP_NOWARN.
I see what you point.
>
> > > +
> > > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > > + log, sizeof(*log), 0);
> > > + if (ret) {
> > > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > > + /* If the device provided a response, then it's non-fatal */
> > > + if (ret > 0)
> > > + ret = 0;
> >
> > It seems like that nvme_init_identify() is just check the internal error
> > which is in negative value now as you have posted. Why don't we just
> > return the value of "ret" itself without updating it to 0 ?
>
> Both ways work for me.
>
> I personally prefer not to return (leak) the nvme status code from
> foo_register() function.
Okay. In the perspective of registration, the nvme status might not be
needed to be returned. But I think if we return the nvme status here,
it would be great for the later time when we need to figure out if the nvme
command has failed or not.
But, amyway, I'm fine with this :)
Thanks for your comment on this trivial query.
>
> > > + goto free_log;
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev;
> > > + int temp;
> > > +
> > > + if (i)
> > > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > > + else
> > > + temp = get_unaligned_le16(log->temperature);
> > > +
> > > + /*
> > > + * All implemented temperature sensors report a non-zero value
> > > + * in temperature sensor fields in the smart log page.
> > > + */
> > > + if (!temp)
> > > + continue;
> > > + if (ctrl->tzdev[i])
> > > + continue;
> > > +
> > > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > > + if (!IS_ERR(tzdev))
> > > + ctrl->tzdev[i] = tzdev;
> > > + }
> > > +
> > > +free_log:
> > > + kfree(log);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function removes the registered thermal zone devices and symlinks.
> > > + */
> > > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > > + char name[20];
> >
> > Simple query here :)
> >
> > If we are not going to allow the name of link exceed the length of its
> > own device name like nvmeX_tempY, then can we THERMAL_NAME_LENGTH macro
> > here? If the name of link is not exactly about the device name itself,
> > then it's fine. What do you think about it ?
>
> Of course we can use THERMAL_NAME_LENGTH here. But this char array is
> used only for the symbolic link name.
> It's not used for thermal cooling device type, thermal zone device type,
> or thermal governor name. So I just used a random constant to avoid
> confusion.
Agreed.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-05-26 16:29 ` Akinobu Mita
@ 2019-06-01 9:02 ` Christoph Hellwig
-1 siblings, 0 replies; 40+ messages in thread
From: Christoph Hellwig @ 2019-06-01 9:02 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Minwoo Im, Kenneth Heitke, Chaitanya Kulkarni
On Mon, May 27, 2019 at 01:29:02AM +0900, Akinobu Mita wrote:
> The NVMe controller reports up to nine temperature values in the SMART /
> Health log page (the composite temperature and temperature sensor 1 through
> temperature sensor 8).
>
> This provides these temperatures via thermal zone devices.
Can you explain a bit more why we'd do this? I shows up some sysfs
files, but we could easily do that with nvme-cli, too. Is there some
greater benefit of this integration?
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-01 9:02 ` Christoph Hellwig
0 siblings, 0 replies; 40+ messages in thread
From: Christoph Hellwig @ 2019-06-01 9:02 UTC (permalink / raw)
On Mon, May 27, 2019@01:29:02AM +0900, Akinobu Mita wrote:
> The NVMe controller reports up to nine temperature values in the SMART /
> Health log page (the composite temperature and temperature sensor 1 through
> temperature sensor 8).
>
> This provides these temperatures via thermal zone devices.
Can you explain a bit more why we'd do this? I shows up some sysfs
files, but we could easily do that with nvme-cli, too. Is there some
greater benefit of this integration?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-01 9:02 ` Christoph Hellwig
@ 2019-06-02 13:19 ` Akinobu Mita
-1 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-02 13:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Sagi Grimberg,
Minwoo Im, Kenneth Heitke, Chaitanya Kulkarni
2019年6月1日(土) 18:03 Christoph Hellwig <hch@lst.de>:
>
> On Mon, May 27, 2019 at 01:29:02AM +0900, Akinobu Mita wrote:
> > The NVMe controller reports up to nine temperature values in the SMART /
> > Health log page (the composite temperature and temperature sensor 1 through
> > temperature sensor 8).
> >
> > This provides these temperatures via thermal zone devices.
>
> Can you explain a bit more why we'd do this? I shows up some sysfs
> files, but we could easily do that with nvme-cli, too. Is there some
> greater benefit of this integration?
As long as the user space thermal governor is used, there is nothing more
than that from a functional perspective. And I suppose that this is used
with user_space governor (i.e. there is still some work to be able to bind
actual thermal cooling device).
The main purpose of this is to turn on a fan when overheated without
polling the device that could prevent the lower power state transitions.
But as you noted, we could do that with the existing AEN notifications
through uevent.
So frankly speaking, the benefit of this is providing generic thermal sysfs
interface and the tools like tmon (linux/tools/thermal/tmon) can show the
nvme temperatures.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-02 13:19 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-02 13:19 UTC (permalink / raw)
2019?6?1?(?) 18:03 Christoph Hellwig <hch at lst.de>:
>
> On Mon, May 27, 2019@01:29:02AM +0900, Akinobu Mita wrote:
> > The NVMe controller reports up to nine temperature values in the SMART /
> > Health log page (the composite temperature and temperature sensor 1 through
> > temperature sensor 8).
> >
> > This provides these temperatures via thermal zone devices.
>
> Can you explain a bit more why we'd do this? I shows up some sysfs
> files, but we could easily do that with nvme-cli, too. Is there some
> greater benefit of this integration?
As long as the user space thermal governor is used, there is nothing more
than that from a functional perspective. And I suppose that this is used
with user_space governor (i.e. there is still some work to be able to bind
actual thermal cooling device).
The main purpose of this is to turn on a fan when overheated without
polling the device that could prevent the lower power state transitions.
But as you noted, we could do that with the existing AEN notifications
through uevent.
So frankly speaking, the benefit of this is providing generic thermal sysfs
interface and the tools like tmon (linux/tools/thermal/tmon) can show the
nvme temperatures.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-02 13:19 ` Akinobu Mita
@ 2019-06-04 7:31 ` Christoph Hellwig
-1 siblings, 0 replies; 40+ messages in thread
From: Christoph Hellwig @ 2019-06-04 7:31 UTC (permalink / raw)
To: Akinobu Mita
Cc: Christoph Hellwig, linux-nvme, linux-pm, Zhang Rui,
Eduardo Valentin, Daniel Lezcano, Keith Busch, Jens Axboe,
Sagi Grimberg, Minwoo Im, Kenneth Heitke, Chaitanya Kulkarni
On Sun, Jun 02, 2019 at 10:19:08PM +0900, Akinobu Mita wrote:
> As long as the user space thermal governor is used, there is nothing more
> than that from a functional perspective. And I suppose that this is used
> with user_space governor (i.e. there is still some work to be able to bind
> actual thermal cooling device).
>
> The main purpose of this is to turn on a fan when overheated without
> polling the device that could prevent the lower power state transitions.
> But as you noted, we could do that with the existing AEN notifications
> through uevent.
>
> So frankly speaking, the benefit of this is providing generic thermal sysfs
> interface and the tools like tmon (linux/tools/thermal/tmon) can show the
> nvme temperatures.
I'm just a little worried about bloating the nvme driver with features
that look kinda nifty but don't buy us much. I'd rather keep at least
the core and PCIe drivers as minimal. Now the thermal device support
is pretty small and except for the smart uevents I can't find anything
actually bad, but I'm not exactly excited either.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-04 7:31 ` Christoph Hellwig
0 siblings, 0 replies; 40+ messages in thread
From: Christoph Hellwig @ 2019-06-04 7:31 UTC (permalink / raw)
On Sun, Jun 02, 2019@10:19:08PM +0900, Akinobu Mita wrote:
> As long as the user space thermal governor is used, there is nothing more
> than that from a functional perspective. And I suppose that this is used
> with user_space governor (i.e. there is still some work to be able to bind
> actual thermal cooling device).
>
> The main purpose of this is to turn on a fan when overheated without
> polling the device that could prevent the lower power state transitions.
> But as you noted, we could do that with the existing AEN notifications
> through uevent.
>
> So frankly speaking, the benefit of this is providing generic thermal sysfs
> interface and the tools like tmon (linux/tools/thermal/tmon) can show the
> nvme temperatures.
I'm just a little worried about bloating the nvme driver with features
that look kinda nifty but don't buy us much. I'd rather keep at least
the core and PCIe drivers as minimal. Now the thermal device support
is pretty small and except for the smart uevents I can't find anything
actually bad, but I'm not exactly excited either.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-04 7:31 ` Christoph Hellwig
@ 2019-06-05 15:42 ` Akinobu Mita
-1 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-05 15:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin,
Daniel Lezcano, Keith Busch, Jens Axboe, Sagi Grimberg,
Minwoo Im, Kenneth Heitke, Chaitanya Kulkarni
2019年6月4日(火) 16:32 Christoph Hellwig <hch@lst.de>:
>
> On Sun, Jun 02, 2019 at 10:19:08PM +0900, Akinobu Mita wrote:
> > As long as the user space thermal governor is used, there is nothing more
> > than that from a functional perspective. And I suppose that this is used
> > with user_space governor (i.e. there is still some work to be able to bind
> > actual thermal cooling device).
> >
> > The main purpose of this is to turn on a fan when overheated without
> > polling the device that could prevent the lower power state transitions.
> > But as you noted, we could do that with the existing AEN notifications
> > through uevent.
> >
> > So frankly speaking, the benefit of this is providing generic thermal sysfs
> > interface and the tools like tmon (linux/tools/thermal/tmon) can show the
> > nvme temperatures.
>
> I'm just a little worried about bloating the nvme driver with features
> that look kinda nifty but don't buy us much. I'd rather keep at least
> the core and PCIe drivers as minimal. Now the thermal device support
> is pretty small and except for the smart uevents I can't find anything
> actually bad, but I'm not exactly excited either.
I'll add thermal.c file in order to minimize the core driver changes and a
module parameter to disable the thermal zone support.
Device tree thermal zone will also be supported. It enables to use thermal
governor other than user_space governor.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-05 15:42 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-05 15:42 UTC (permalink / raw)
2019?6?4?(?) 16:32 Christoph Hellwig <hch at lst.de>:
>
> On Sun, Jun 02, 2019@10:19:08PM +0900, Akinobu Mita wrote:
> > As long as the user space thermal governor is used, there is nothing more
> > than that from a functional perspective. And I suppose that this is used
> > with user_space governor (i.e. there is still some work to be able to bind
> > actual thermal cooling device).
> >
> > The main purpose of this is to turn on a fan when overheated without
> > polling the device that could prevent the lower power state transitions.
> > But as you noted, we could do that with the existing AEN notifications
> > through uevent.
> >
> > So frankly speaking, the benefit of this is providing generic thermal sysfs
> > interface and the tools like tmon (linux/tools/thermal/tmon) can show the
> > nvme temperatures.
>
> I'm just a little worried about bloating the nvme driver with features
> that look kinda nifty but don't buy us much. I'd rather keep at least
> the core and PCIe drivers as minimal. Now the thermal device support
> is pretty small and except for the smart uevents I can't find anything
> actually bad, but I'm not exactly excited either.
I'll add thermal.c file in order to minimize the core driver changes and a
module parameter to disable the thermal zone support.
Device tree thermal zone will also be supported. It enables to use thermal
governor other than user_space governor.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-05-26 16:29 ` Akinobu Mita
@ 2019-06-03 2:36 ` Eduardo Valentin
-1 siblings, 0 replies; 40+ messages in thread
From: Eduardo Valentin @ 2019-06-03 2:36 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Daniel Lezcano, Keith Busch,
Jens Axboe, Christoph Hellwig, Sagi Grimberg, Minwoo Im,
Kenneth Heitke, Chaitanya Kulkarni
Hey Mita,
On Mon, May 27, 2019 at 01:29:02AM +0900, Akinobu Mita wrote:
> The NVMe controller reports up to nine temperature values in the SMART /
> Health log page (the composite temperature and temperature sensor 1 through
> temperature sensor 8).
>
> This provides these temperatures via thermal zone devices.
>
> Once the controller is identified, the thermal zone devices are created for
> all implemented temperature sensors including the composite temperature.
>
> /sys/class/thermal/thermal_zone[0-*]:
> |---type: 'nvme<instance>-temp<sensor>'
> |---temp: Temperature
> |---trip_point_0_temp: Over temperature threshold
>
> The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> device.
>
> On the other hand, the following symlinks to the thermal zone devices are
> created in the nvme device sysfs directory.
>
> - temp0: Composite temperature
> - temp1: Temperature sensor 1
> ...
> - temp8: Temperature sensor 8
>
These questions on V2 are still unanswered:
a. Can we get a more descriptive string into tz->type?
b. Can these APIs support DT probing too?
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Minwoo Im <minwoo.im.dev@gmail.com>
> Cc: Kenneth Heitke <kenneth.heitke@intel.com>
> Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v3
> - Change the type name of thermal zone devices from 'nvme_temp<sensor>' to
> 'nvme<instance>_temp<sensor>'
> - Pass a NULL to the status argument of nvme_set_feature()
> - Change the name of symbolic link from 'nvme_temp<sensor>' to 'temp<sensor>'
> - Don't make it fatal error if the device provides a response
> - Don't register thermal zone for composite temperature if smart log reports
> zero value
> - Move the thermal zones registration and unregistration into the core module.
>
> drivers/nvme/host/core.c | 288 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h | 9 ++
> include/linux/nvme.h | 5 +
> 3 files changed, 302 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c950916..4c8271a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2200,6 +2200,289 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
> }
> }
>
> +#ifdef CONFIG_THERMAL
> +
> +static int nvme_get_temp(struct nvme_ctrl *ctrl, unsigned int sensor, int *temp)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(log->temp_sensor) + 1 !=
> + ARRAY_SIZE(ctrl->tzdev));
> +
> + if (WARN_ON_ONCE(sensor > ARRAY_SIZE(log->temp_sensor)))
> + return -EINVAL;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return -ENOMEM;
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + ret = ret > 0 ? -EINVAL : ret;
> + goto free_log;
> + }
> +
> + if (sensor)
> + *temp = le16_to_cpu(log->temp_sensor[sensor - 1]);
> + else
> + *temp = get_unaligned_le16(log->temperature);
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +static unsigned int nvme_tz_type_to_sensor(const char *type)
> +{
> + int instance;
> + unsigned int sensor;
> +
> + if (sscanf(type, "nvme%d_temp%u", &instance, &sensor) != 2)
> + return UINT_MAX;
> +
> + return sensor;
> +}
> +
> +#define KELVIN_TO_MILLICELSIUS(t) DECI_KELVIN_TO_MILLICELSIUS((t) * 10)
> +#define MILLICELSIUS_TO_KELVIN(t) ((MILLICELSIUS_TO_DECI_KELVIN(t) + 5) / 10)
> +
> +static int nvme_tz_get_temp(struct thermal_zone_device *tzdev, int *temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> + int ret;
> +
> + ret = nvme_get_temp(ctrl, sensor, temp);
> + if (!ret)
> + *temp = KELVIN_TO_MILLICELSIUS(*temp);
> +
> + return ret;
> +}
> +
> +static int nvme_tz_get_trip_type(struct thermal_zone_device *tzdev,
> + int trip, enum thermal_trip_type *type)
> +{
> + *type = THERMAL_TRIP_ACTIVE;
> +
> + return 0;
> +}
> +
> +static int nvme_get_over_temp_thresh(struct nvme_ctrl *ctrl,
> + unsigned int sensor, int *temp)
> +{
> + unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
> + int status;
> + int ret;
> +
> + if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
> + return -EINVAL;
> +
> + ret = nvme_get_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
> + &status);
> + if (!ret)
> + *temp = status & NVME_TEMP_THRESH_MASK;
> +
> + return ret > 0 ? -EINVAL : ret;
> +}
> +
> +static int nvme_set_over_temp_thresh(struct nvme_ctrl *ctrl,
> + unsigned int sensor, int temp)
> +{
> + unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
> + int ret;
> +
> + if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
> + return -EINVAL;
> +
> + if (temp > NVME_TEMP_THRESH_MASK)
> + return -EINVAL;
> +
> + threshold |= temp & NVME_TEMP_THRESH_MASK;
> +
> + ret = nvme_set_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
> + NULL);
> +
> + return ret > 0 ? -EINVAL : ret;
> +}
> +
> +static int nvme_tz_get_trip_temp(struct thermal_zone_device *tzdev,
> + int trip, int *temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> + int ret;
> +
> + ret = nvme_get_over_temp_thresh(ctrl, sensor, temp);
> + if (!ret)
> + *temp = KELVIN_TO_MILLICELSIUS(*temp);
> +
> + return ret;
> +}
> +
> +static int nvme_tz_set_trip_temp(struct thermal_zone_device *tzdev,
> + int trip, int temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> +
> + temp = MILLICELSIUS_TO_KELVIN(temp);
> +
> + return nvme_set_over_temp_thresh(ctrl, sensor, temp);
> +}
> +
> +static struct thermal_zone_device_ops nvme_tz_ops = {
> + .get_temp = nvme_tz_get_temp,
> + .get_trip_type = nvme_tz_get_trip_type,
> + .get_trip_temp = nvme_tz_get_trip_temp,
> + .set_trip_temp = nvme_tz_set_trip_temp,
> +};
> +
> +static struct thermal_zone_params nvme_tz_params = {
> + .governor_name = "user_space",
Also,
Was there any particular reason why defaulting to user_space here?
> + .no_hwmon = true,
> +};
> +
> +static struct thermal_zone_device *
> +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> +{
> + struct thermal_zone_device *tzdev;
> + char name[THERMAL_NAME_LENGTH];
> + int ret;
> +
> + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> +
> + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> + &nvme_tz_params, 0, 0);
> + if (IS_ERR(tzdev)) {
> + dev_err(ctrl->device,
> + "Failed to register thermal zone device: %ld\n",
> + PTR_ERR(tzdev));
> + return tzdev;
> + }
> +
> + snprintf(name, sizeof(name), "temp%d", sensor);
> + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> + name);
> + if (ret)
> + goto device_unregister;
> +
> + ret = sysfs_create_link(&tzdev->device.kobj,
> + &ctrl->ctrl_device.kobj, "device");
> + if (ret)
> + goto remove_link;
> +
> + return tzdev;
> +
> +remove_link:
> + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> +device_unregister:
> + thermal_zone_device_unregister(tzdev);
> +
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * nvme_thermal_zones_register() - register nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function creates up to nine thermal zone devices for all implemented
> + * temperature sensors including the composite temperature.
> + * Each thermal zone device provides a single trip point temperature that is
> + * associated with an over temperature threshold.
> + */
> +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> + int i;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return 0; /* non-fatal error */
I am not sure about this API design here. I would leave the error
handling and judging if this is fatal or not to the caller.
I mean, if I ask to register a nvme thermal zone and I get
a 0 as response, I would assume the thermal zone exists from
now on, right?
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> + /* If the device provided a response, then it's non-fatal */
> + if (ret > 0)
> + ret = 0;
> + goto free_log;
Once again, hiding errors is never a strategy that scales..
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev;
> + int temp;
> +
> + if (i)
> + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> + else
> + temp = get_unaligned_le16(log->temperature);
> +
> + /*
> + * All implemented temperature sensors report a non-zero value
> + * in temperature sensor fields in the smart log page.
> + */
> + if (!temp)
> + continue;
> + if (ctrl->tzdev[i])
> + continue;
> +
> + tzdev = nvme_thermal_zone_register(ctrl, i);
> + if (!IS_ERR(tzdev))
> + ctrl->tzdev[i] = tzdev;
Here again, I would not hide errors, the API should propagate errors
if something goes wrong.
> + }
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +/**
> + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function removes the registered thermal zone devices and symlinks.
> + */
> +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> + char name[20];
> +
> + if (!tzdev)
> + continue;
> +
> + sysfs_remove_link(&tzdev->device.kobj, "device");
> +
> + snprintf(name, sizeof(name), "temp%d", i);
> + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> +
> + thermal_zone_device_unregister(tzdev);
> +
> + ctrl->tzdev[i] = NULL;
> + }
> +}
> +
> +#else
> +
> +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + return 0;
> +}
> +
> +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> +}
> +
> +#endif /* CONFIG_THERMAL */
> +
> struct nvme_core_quirk_entry {
> /*
> * NVMe model and firmware strings are padded with spaces. For
> @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> if (ret < 0)
> return ret;
>
> + ret = nvme_thermal_zones_register(ctrl);
> + if (ret < 0)
> + return ret;
I would definitely keep this code the way it is here in
nvme_init_identify(), but if I read this right, your
nvme_thermal_zones_register() will never return < 0, and therefore this
condition is never met.
I would suggest you simply propagate the errors and keep this piece
of code the way it is, propagating errors too.
> +
> ctrl->identified = true;
>
> return 0;
> @@ -3756,6 +4043,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> {
> nvme_mpath_stop(ctrl);
> nvme_stop_keep_alive(ctrl);
> + nvme_thermal_zones_unregister(ctrl);
> flush_work(&ctrl->async_event_work);
> cancel_work_sync(&ctrl->fw_act_work);
> }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 802aa19..53f0b24 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -15,6 +15,7 @@
> #include <linux/sed-opal.h>
> #include <linux/fault-inject.h>
> #include <linux/rcupdate.h>
> +#include <linux/thermal.h>
>
> extern unsigned int nvme_io_timeout;
> #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ)
> @@ -248,6 +249,14 @@ struct nvme_ctrl {
>
> struct page *discard_page;
> unsigned long discard_page_busy;
> +
> +#ifdef CONFIG_THERMAL
> + /*
> + * tzdev[0]: composite temperature
> + * tzdev[1-8]: temperature sensor 1 through 8
> + */
> + struct thermal_zone_device *tzdev[9];
> +#endif
> };
>
> enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 658ac75..54f0a13 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -780,6 +780,11 @@ struct nvme_write_zeroes_cmd {
>
> /* Features */
>
> +enum {
> + NVME_TEMP_THRESH_MASK = 0xffff,
> + NVME_TEMP_THRESH_SELECT_SHIFT = 16,
> +};
> +
> struct nvme_feat_auto_pst {
> __le64 entries[32];
> };
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-03 2:36 ` Eduardo Valentin
0 siblings, 0 replies; 40+ messages in thread
From: Eduardo Valentin @ 2019-06-03 2:36 UTC (permalink / raw)
Hey Mita,
On Mon, May 27, 2019@01:29:02AM +0900, Akinobu Mita wrote:
> The NVMe controller reports up to nine temperature values in the SMART /
> Health log page (the composite temperature and temperature sensor 1 through
> temperature sensor 8).
>
> This provides these temperatures via thermal zone devices.
>
> Once the controller is identified, the thermal zone devices are created for
> all implemented temperature sensors including the composite temperature.
>
> /sys/class/thermal/thermal_zone[0-*]:
> |---type: 'nvme<instance>-temp<sensor>'
> |---temp: Temperature
> |---trip_point_0_temp: Over temperature threshold
>
> The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> device.
>
> On the other hand, the following symlinks to the thermal zone devices are
> created in the nvme device sysfs directory.
>
> - temp0: Composite temperature
> - temp1: Temperature sensor 1
> ...
> - temp8: Temperature sensor 8
>
These questions on V2 are still unanswered:
a. Can we get a more descriptive string into tz->type?
b. Can these APIs support DT probing too?
> Cc: Zhang Rui <rui.zhang at intel.com>
> Cc: Eduardo Valentin <edubezval at gmail.com>
> Cc: Daniel Lezcano <daniel.lezcano at linaro.org>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Cc: Minwoo Im <minwoo.im.dev at gmail.com>
> Cc: Kenneth Heitke <kenneth.heitke at intel.com>
> Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>
> Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> ---
> * v3
> - Change the type name of thermal zone devices from 'nvme_temp<sensor>' to
> 'nvme<instance>_temp<sensor>'
> - Pass a NULL to the status argument of nvme_set_feature()
> - Change the name of symbolic link from 'nvme_temp<sensor>' to 'temp<sensor>'
> - Don't make it fatal error if the device provides a response
> - Don't register thermal zone for composite temperature if smart log reports
> zero value
> - Move the thermal zones registration and unregistration into the core module.
>
> drivers/nvme/host/core.c | 288 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h | 9 ++
> include/linux/nvme.h | 5 +
> 3 files changed, 302 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c950916..4c8271a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2200,6 +2200,289 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
> }
> }
>
> +#ifdef CONFIG_THERMAL
> +
> +static int nvme_get_temp(struct nvme_ctrl *ctrl, unsigned int sensor, int *temp)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(log->temp_sensor) + 1 !=
> + ARRAY_SIZE(ctrl->tzdev));
> +
> + if (WARN_ON_ONCE(sensor > ARRAY_SIZE(log->temp_sensor)))
> + return -EINVAL;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return -ENOMEM;
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + ret = ret > 0 ? -EINVAL : ret;
> + goto free_log;
> + }
> +
> + if (sensor)
> + *temp = le16_to_cpu(log->temp_sensor[sensor - 1]);
> + else
> + *temp = get_unaligned_le16(log->temperature);
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +static unsigned int nvme_tz_type_to_sensor(const char *type)
> +{
> + int instance;
> + unsigned int sensor;
> +
> + if (sscanf(type, "nvme%d_temp%u", &instance, &sensor) != 2)
> + return UINT_MAX;
> +
> + return sensor;
> +}
> +
> +#define KELVIN_TO_MILLICELSIUS(t) DECI_KELVIN_TO_MILLICELSIUS((t) * 10)
> +#define MILLICELSIUS_TO_KELVIN(t) ((MILLICELSIUS_TO_DECI_KELVIN(t) + 5) / 10)
> +
> +static int nvme_tz_get_temp(struct thermal_zone_device *tzdev, int *temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> + int ret;
> +
> + ret = nvme_get_temp(ctrl, sensor, temp);
> + if (!ret)
> + *temp = KELVIN_TO_MILLICELSIUS(*temp);
> +
> + return ret;
> +}
> +
> +static int nvme_tz_get_trip_type(struct thermal_zone_device *tzdev,
> + int trip, enum thermal_trip_type *type)
> +{
> + *type = THERMAL_TRIP_ACTIVE;
> +
> + return 0;
> +}
> +
> +static int nvme_get_over_temp_thresh(struct nvme_ctrl *ctrl,
> + unsigned int sensor, int *temp)
> +{
> + unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
> + int status;
> + int ret;
> +
> + if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
> + return -EINVAL;
> +
> + ret = nvme_get_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
> + &status);
> + if (!ret)
> + *temp = status & NVME_TEMP_THRESH_MASK;
> +
> + return ret > 0 ? -EINVAL : ret;
> +}
> +
> +static int nvme_set_over_temp_thresh(struct nvme_ctrl *ctrl,
> + unsigned int sensor, int temp)
> +{
> + unsigned int threshold = sensor << NVME_TEMP_THRESH_SELECT_SHIFT;
> + int ret;
> +
> + if (WARN_ON_ONCE(sensor >= ARRAY_SIZE(ctrl->tzdev)))
> + return -EINVAL;
> +
> + if (temp > NVME_TEMP_THRESH_MASK)
> + return -EINVAL;
> +
> + threshold |= temp & NVME_TEMP_THRESH_MASK;
> +
> + ret = nvme_set_features(ctrl, NVME_FEAT_TEMP_THRESH, threshold, NULL, 0,
> + NULL);
> +
> + return ret > 0 ? -EINVAL : ret;
> +}
> +
> +static int nvme_tz_get_trip_temp(struct thermal_zone_device *tzdev,
> + int trip, int *temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> + int ret;
> +
> + ret = nvme_get_over_temp_thresh(ctrl, sensor, temp);
> + if (!ret)
> + *temp = KELVIN_TO_MILLICELSIUS(*temp);
> +
> + return ret;
> +}
> +
> +static int nvme_tz_set_trip_temp(struct thermal_zone_device *tzdev,
> + int trip, int temp)
> +{
> + unsigned int sensor = nvme_tz_type_to_sensor(tzdev->type);
> + struct nvme_ctrl *ctrl = tzdev->devdata;
> +
> + temp = MILLICELSIUS_TO_KELVIN(temp);
> +
> + return nvme_set_over_temp_thresh(ctrl, sensor, temp);
> +}
> +
> +static struct thermal_zone_device_ops nvme_tz_ops = {
> + .get_temp = nvme_tz_get_temp,
> + .get_trip_type = nvme_tz_get_trip_type,
> + .get_trip_temp = nvme_tz_get_trip_temp,
> + .set_trip_temp = nvme_tz_set_trip_temp,
> +};
> +
> +static struct thermal_zone_params nvme_tz_params = {
> + .governor_name = "user_space",
Also,
Was there any particular reason why defaulting to user_space here?
> + .no_hwmon = true,
> +};
> +
> +static struct thermal_zone_device *
> +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> +{
> + struct thermal_zone_device *tzdev;
> + char name[THERMAL_NAME_LENGTH];
> + int ret;
> +
> + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> +
> + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> + &nvme_tz_params, 0, 0);
> + if (IS_ERR(tzdev)) {
> + dev_err(ctrl->device,
> + "Failed to register thermal zone device: %ld\n",
> + PTR_ERR(tzdev));
> + return tzdev;
> + }
> +
> + snprintf(name, sizeof(name), "temp%d", sensor);
> + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> + name);
> + if (ret)
> + goto device_unregister;
> +
> + ret = sysfs_create_link(&tzdev->device.kobj,
> + &ctrl->ctrl_device.kobj, "device");
> + if (ret)
> + goto remove_link;
> +
> + return tzdev;
> +
> +remove_link:
> + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> +device_unregister:
> + thermal_zone_device_unregister(tzdev);
> +
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * nvme_thermal_zones_register() - register nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function creates up to nine thermal zone devices for all implemented
> + * temperature sensors including the composite temperature.
> + * Each thermal zone device provides a single trip point temperature that is
> + * associated with an over temperature threshold.
> + */
> +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + struct nvme_smart_log *log;
> + int ret;
> + int i;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return 0; /* non-fatal error */
I am not sure about this API design here. I would leave the error
handling and judging if this is fatal or not to the caller.
I mean, if I ask to register a nvme thermal zone and I get
a 0 as response, I would assume the thermal zone exists from
now on, right?
> +
> + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> + log, sizeof(*log), 0);
> + if (ret) {
> + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> + /* If the device provided a response, then it's non-fatal */
> + if (ret > 0)
> + ret = 0;
> + goto free_log;
Once again, hiding errors is never a strategy that scales..
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev;
> + int temp;
> +
> + if (i)
> + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> + else
> + temp = get_unaligned_le16(log->temperature);
> +
> + /*
> + * All implemented temperature sensors report a non-zero value
> + * in temperature sensor fields in the smart log page.
> + */
> + if (!temp)
> + continue;
> + if (ctrl->tzdev[i])
> + continue;
> +
> + tzdev = nvme_thermal_zone_register(ctrl, i);
> + if (!IS_ERR(tzdev))
> + ctrl->tzdev[i] = tzdev;
Here again, I would not hide errors, the API should propagate errors
if something goes wrong.
> + }
> +
> +free_log:
> + kfree(log);
> +
> + return ret;
> +}
> +
> +/**
> + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> + * @ctrl: controller instance
> + *
> + * This function removes the registered thermal zone devices and symlinks.
> + */
> +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> + char name[20];
> +
> + if (!tzdev)
> + continue;
> +
> + sysfs_remove_link(&tzdev->device.kobj, "device");
> +
> + snprintf(name, sizeof(name), "temp%d", i);
> + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> +
> + thermal_zone_device_unregister(tzdev);
> +
> + ctrl->tzdev[i] = NULL;
> + }
> +}
> +
> +#else
> +
> +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> +{
> + return 0;
> +}
> +
> +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> +{
> +}
> +
> +#endif /* CONFIG_THERMAL */
> +
> struct nvme_core_quirk_entry {
> /*
> * NVMe model and firmware strings are padded with spaces. For
> @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> if (ret < 0)
> return ret;
>
> + ret = nvme_thermal_zones_register(ctrl);
> + if (ret < 0)
> + return ret;
I would definitely keep this code the way it is here in
nvme_init_identify(), but if I read this right, your
nvme_thermal_zones_register() will never return < 0, and therefore this
condition is never met.
I would suggest you simply propagate the errors and keep this piece
of code the way it is, propagating errors too.
> +
> ctrl->identified = true;
>
> return 0;
> @@ -3756,6 +4043,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> {
> nvme_mpath_stop(ctrl);
> nvme_stop_keep_alive(ctrl);
> + nvme_thermal_zones_unregister(ctrl);
> flush_work(&ctrl->async_event_work);
> cancel_work_sync(&ctrl->fw_act_work);
> }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 802aa19..53f0b24 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -15,6 +15,7 @@
> #include <linux/sed-opal.h>
> #include <linux/fault-inject.h>
> #include <linux/rcupdate.h>
> +#include <linux/thermal.h>
>
> extern unsigned int nvme_io_timeout;
> #define NVME_IO_TIMEOUT (nvme_io_timeout * HZ)
> @@ -248,6 +249,14 @@ struct nvme_ctrl {
>
> struct page *discard_page;
> unsigned long discard_page_busy;
> +
> +#ifdef CONFIG_THERMAL
> + /*
> + * tzdev[0]: composite temperature
> + * tzdev[1-8]: temperature sensor 1 through 8
> + */
> + struct thermal_zone_device *tzdev[9];
> +#endif
> };
>
> enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 658ac75..54f0a13 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -780,6 +780,11 @@ struct nvme_write_zeroes_cmd {
>
> /* Features */
>
> +enum {
> + NVME_TEMP_THRESH_MASK = 0xffff,
> + NVME_TEMP_THRESH_SELECT_SHIFT = 16,
> +};
> +
> struct nvme_feat_auto_pst {
> __le64 entries[32];
> };
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-03 2:36 ` Eduardo Valentin
@ 2019-06-03 15:20 ` Akinobu Mita
-1 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-03 15:20 UTC (permalink / raw)
To: Eduardo Valentin
Cc: linux-nvme, linux-pm, Zhang Rui, Daniel Lezcano, Keith Busch,
Jens Axboe, Christoph Hellwig, Sagi Grimberg, Minwoo Im,
Kenneth Heitke, Chaitanya Kulkarni
2019年6月3日(月) 11:36 Eduardo Valentin <edubezval@gmail.com>:
>
> Hey Mita,
>
> On Mon, May 27, 2019 at 01:29:02AM +0900, Akinobu Mita wrote:
> > The NVMe controller reports up to nine temperature values in the SMART /
> > Health log page (the composite temperature and temperature sensor 1 through
> > temperature sensor 8).
> >
> > This provides these temperatures via thermal zone devices.
> >
> > Once the controller is identified, the thermal zone devices are created for
> > all implemented temperature sensors including the composite temperature.
> >
> > /sys/class/thermal/thermal_zone[0-*]:
> > |---type: 'nvme<instance>-temp<sensor>'
> > |---temp: Temperature
> > |---trip_point_0_temp: Over temperature threshold
> >
> > The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> > device.
> >
> > On the other hand, the following symlinks to the thermal zone devices are
> > created in the nvme device sysfs directory.
> >
> > - temp0: Composite temperature
> > - temp1: Temperature sensor 1
> > ...
> > - temp8: Temperature sensor 8
> >
>
> These questions on V2 are still unanswered:
> a. Can we get a more descriptive string into tz->type?
As I said in the other thread, the NVMe spec only says a controller
reports the composite temperature and temperature sensor 1 through 8.
What temperature sensor N means is vendor specific.
> b. Can these APIs support DT probing too?
OK. I can try, but I don't have arm or arm64 boards with PCIe for
testing with of-thermal. So it'll be untested.
> > +static struct thermal_zone_params nvme_tz_params = {
> > + .governor_name = "user_space",
>
> Also,
>
> Was there any particular reason why defaulting to user_space here?
I only tested with the user_space governor. There is no cooling device
to bind with this.
> > + .no_hwmon = true,
> > +};
> > +
> > +static struct thermal_zone_device *
> > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > +{
> > + struct thermal_zone_device *tzdev;
> > + char name[THERMAL_NAME_LENGTH];
> > + int ret;
> > +
> > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > +
> > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > + &nvme_tz_params, 0, 0);
> > + if (IS_ERR(tzdev)) {
> > + dev_err(ctrl->device,
> > + "Failed to register thermal zone device: %ld\n",
> > + PTR_ERR(tzdev));
> > + return tzdev;
> > + }
> > +
> > + snprintf(name, sizeof(name), "temp%d", sensor);
> > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > + name);
> > + if (ret)
> > + goto device_unregister;
> > +
> > + ret = sysfs_create_link(&tzdev->device.kobj,
> > + &ctrl->ctrl_device.kobj, "device");
> > + if (ret)
> > + goto remove_link;
> > +
> > + return tzdev;
> > +
> > +remove_link:
> > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > +device_unregister:
> > + thermal_zone_device_unregister(tzdev);
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function creates up to nine thermal zone devices for all implemented
> > + * temperature sensors including the composite temperature.
> > + * Each thermal zone device provides a single trip point temperature that is
> > + * associated with an over temperature threshold.
> > + */
> > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + struct nvme_smart_log *log;
> > + int ret;
> > + int i;
> > +
> > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > + if (!log)
> > + return 0; /* non-fatal error */
>
> I am not sure about this API design here. I would leave the error
> handling and judging if this is fatal or not to the caller.
> I mean, if I ask to register a nvme thermal zone and I get
> a 0 as response, I would assume the thermal zone exists from
> now on, right?
This routine is designed to return error code only when we're unable to
communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
negative value).
We don't want to abandon device initialization just due to thermal zone
failures. Because thermal zone device isn't mandatory to manage the
controllers, and the device like qemu doesn't provide smart log (according
to Keith).
> > +
> > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > + log, sizeof(*log), 0);
> > + if (ret) {
> > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > + /* If the device provided a response, then it's non-fatal */
> > + if (ret > 0)
> > + ret = 0;
> > + goto free_log;
>
> Once again, hiding errors is never a strategy that scales..
>
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev;
> > + int temp;
> > +
> > + if (i)
> > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > + else
> > + temp = get_unaligned_le16(log->temperature);
> > +
> > + /*
> > + * All implemented temperature sensors report a non-zero value
> > + * in temperature sensor fields in the smart log page.
> > + */
> > + if (!temp)
> > + continue;
> > + if (ctrl->tzdev[i])
> > + continue;
> > +
> > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > + if (!IS_ERR(tzdev))
> > + ctrl->tzdev[i] = tzdev;
>
> Here again, I would not hide errors, the API should propagate errors
> if something goes wrong.
>
> > + }
> > +
> > +free_log:
> > + kfree(log);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function removes the registered thermal zone devices and symlinks.
> > + */
> > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > + char name[20];
> > +
> > + if (!tzdev)
> > + continue;
> > +
> > + sysfs_remove_link(&tzdev->device.kobj, "device");
> > +
> > + snprintf(name, sizeof(name), "temp%d", i);
> > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > +
> > + thermal_zone_device_unregister(tzdev);
> > +
> > + ctrl->tzdev[i] = NULL;
> > + }
> > +}
> > +
> > +#else
> > +
> > +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > +}
> > +
> > +#endif /* CONFIG_THERMAL */
> > +
> > struct nvme_core_quirk_entry {
> > /*
> > * NVMe model and firmware strings are padded with spaces. For
> > @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> > if (ret < 0)
> > return ret;
> >
> > + ret = nvme_thermal_zones_register(ctrl);
> > + if (ret < 0)
> > + return ret;
>
>
> I would definitely keep this code the way it is here in
> nvme_init_identify(), but if I read this right, your
> nvme_thermal_zones_register() will never return < 0, and therefore this
> condition is never met.
The nvme_get_log() can return a negative value. Only in this case,
nvme_thermal_zones_register() returns error and abandon the device
initialization.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-03 15:20 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-03 15:20 UTC (permalink / raw)
2019?6?3?(?) 11:36 Eduardo Valentin <edubezval at gmail.com>:
>
> Hey Mita,
>
> On Mon, May 27, 2019@01:29:02AM +0900, Akinobu Mita wrote:
> > The NVMe controller reports up to nine temperature values in the SMART /
> > Health log page (the composite temperature and temperature sensor 1 through
> > temperature sensor 8).
> >
> > This provides these temperatures via thermal zone devices.
> >
> > Once the controller is identified, the thermal zone devices are created for
> > all implemented temperature sensors including the composite temperature.
> >
> > /sys/class/thermal/thermal_zone[0-*]:
> > |---type: 'nvme<instance>-temp<sensor>'
> > |---temp: Temperature
> > |---trip_point_0_temp: Over temperature threshold
> >
> > The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> > device.
> >
> > On the other hand, the following symlinks to the thermal zone devices are
> > created in the nvme device sysfs directory.
> >
> > - temp0: Composite temperature
> > - temp1: Temperature sensor 1
> > ...
> > - temp8: Temperature sensor 8
> >
>
> These questions on V2 are still unanswered:
> a. Can we get a more descriptive string into tz->type?
As I said in the other thread, the NVMe spec only says a controller
reports the composite temperature and temperature sensor 1 through 8.
What temperature sensor N means is vendor specific.
> b. Can these APIs support DT probing too?
OK. I can try, but I don't have arm or arm64 boards with PCIe for
testing with of-thermal. So it'll be untested.
> > +static struct thermal_zone_params nvme_tz_params = {
> > + .governor_name = "user_space",
>
> Also,
>
> Was there any particular reason why defaulting to user_space here?
I only tested with the user_space governor. There is no cooling device
to bind with this.
> > + .no_hwmon = true,
> > +};
> > +
> > +static struct thermal_zone_device *
> > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > +{
> > + struct thermal_zone_device *tzdev;
> > + char name[THERMAL_NAME_LENGTH];
> > + int ret;
> > +
> > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > +
> > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > + &nvme_tz_params, 0, 0);
> > + if (IS_ERR(tzdev)) {
> > + dev_err(ctrl->device,
> > + "Failed to register thermal zone device: %ld\n",
> > + PTR_ERR(tzdev));
> > + return tzdev;
> > + }
> > +
> > + snprintf(name, sizeof(name), "temp%d", sensor);
> > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > + name);
> > + if (ret)
> > + goto device_unregister;
> > +
> > + ret = sysfs_create_link(&tzdev->device.kobj,
> > + &ctrl->ctrl_device.kobj, "device");
> > + if (ret)
> > + goto remove_link;
> > +
> > + return tzdev;
> > +
> > +remove_link:
> > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > +device_unregister:
> > + thermal_zone_device_unregister(tzdev);
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function creates up to nine thermal zone devices for all implemented
> > + * temperature sensors including the composite temperature.
> > + * Each thermal zone device provides a single trip point temperature that is
> > + * associated with an over temperature threshold.
> > + */
> > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + struct nvme_smart_log *log;
> > + int ret;
> > + int i;
> > +
> > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > + if (!log)
> > + return 0; /* non-fatal error */
>
> I am not sure about this API design here. I would leave the error
> handling and judging if this is fatal or not to the caller.
> I mean, if I ask to register a nvme thermal zone and I get
> a 0 as response, I would assume the thermal zone exists from
> now on, right?
This routine is designed to return error code only when we're unable to
communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
negative value).
We don't want to abandon device initialization just due to thermal zone
failures. Because thermal zone device isn't mandatory to manage the
controllers, and the device like qemu doesn't provide smart log (according
to Keith).
> > +
> > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > + log, sizeof(*log), 0);
> > + if (ret) {
> > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > + /* If the device provided a response, then it's non-fatal */
> > + if (ret > 0)
> > + ret = 0;
> > + goto free_log;
>
> Once again, hiding errors is never a strategy that scales..
>
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev;
> > + int temp;
> > +
> > + if (i)
> > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > + else
> > + temp = get_unaligned_le16(log->temperature);
> > +
> > + /*
> > + * All implemented temperature sensors report a non-zero value
> > + * in temperature sensor fields in the smart log page.
> > + */
> > + if (!temp)
> > + continue;
> > + if (ctrl->tzdev[i])
> > + continue;
> > +
> > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > + if (!IS_ERR(tzdev))
> > + ctrl->tzdev[i] = tzdev;
>
> Here again, I would not hide errors, the API should propagate errors
> if something goes wrong.
>
> > + }
> > +
> > +free_log:
> > + kfree(log);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > + * @ctrl: controller instance
> > + *
> > + * This function removes the registered thermal zone devices and symlinks.
> > + */
> > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > + char name[20];
> > +
> > + if (!tzdev)
> > + continue;
> > +
> > + sysfs_remove_link(&tzdev->device.kobj, "device");
> > +
> > + snprintf(name, sizeof(name), "temp%d", i);
> > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > +
> > + thermal_zone_device_unregister(tzdev);
> > +
> > + ctrl->tzdev[i] = NULL;
> > + }
> > +}
> > +
> > +#else
> > +
> > +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > +{
> > +}
> > +
> > +#endif /* CONFIG_THERMAL */
> > +
> > struct nvme_core_quirk_entry {
> > /*
> > * NVMe model and firmware strings are padded with spaces. For
> > @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> > if (ret < 0)
> > return ret;
> >
> > + ret = nvme_thermal_zones_register(ctrl);
> > + if (ret < 0)
> > + return ret;
>
>
> I would definitely keep this code the way it is here in
> nvme_init_identify(), but if I read this right, your
> nvme_thermal_zones_register() will never return < 0, and therefore this
> condition is never met.
The nvme_get_log() can return a negative value. Only in this case,
nvme_thermal_zones_register() returns error and abandon the device
initialization.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-03 15:20 ` Akinobu Mita
@ 2019-06-06 4:05 ` Eduardo Valentin
-1 siblings, 0 replies; 40+ messages in thread
From: Eduardo Valentin @ 2019-06-06 4:05 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Daniel Lezcano, Keith Busch,
Jens Axboe, Christoph Hellwig, Sagi Grimberg, Minwoo Im,
Kenneth Heitke, Chaitanya Kulkarni
On Tue, Jun 04, 2019 at 12:20:19AM +0900, Akinobu Mita wrote:
> 2019年6月3日(月) 11:36 Eduardo Valentin <edubezval@gmail.com>:
> >
> > Hey Mita,
> >
> > On Mon, May 27, 2019 at 01:29:02AM +0900, Akinobu Mita wrote:
> > > The NVMe controller reports up to nine temperature values in the SMART /
> > > Health log page (the composite temperature and temperature sensor 1 through
> > > temperature sensor 8).
> > >
> > > This provides these temperatures via thermal zone devices.
> > >
> > > Once the controller is identified, the thermal zone devices are created for
> > > all implemented temperature sensors including the composite temperature.
> > >
> > > /sys/class/thermal/thermal_zone[0-*]:
> > > |---type: 'nvme<instance>-temp<sensor>'
> > > |---temp: Temperature
> > > |---trip_point_0_temp: Over temperature threshold
> > >
> > > The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> > > device.
> > >
> > > On the other hand, the following symlinks to the thermal zone devices are
> > > created in the nvme device sysfs directory.
> > >
> > > - temp0: Composite temperature
> > > - temp1: Temperature sensor 1
> > > ...
> > > - temp8: Temperature sensor 8
> > >
> >
> > These questions on V2 are still unanswered:
> > a. Can we get a more descriptive string into tz->type?
>
> As I said in the other thread, the NVMe spec only says a controller
> reports the composite temperature and temperature sensor 1 through 8.
> What temperature sensor N means is vendor specific.
I see..
>
> > b. Can these APIs support DT probing too?
>
> OK. I can try, but I don't have arm or arm64 boards with PCIe for
> testing with of-thermal. So it'll be untested.
>
OK. Lets see what comes.
> > > +static struct thermal_zone_params nvme_tz_params = {
> > > + .governor_name = "user_space",
> >
> > Also,
> >
> > Was there any particular reason why defaulting to user_space here?
>
> I only tested with the user_space governor. There is no cooling device
> to bind with this.
>
hmmm.. I see. But was there any particular reason to pick the thermal
subsystem here if you do not have any cooling? Or is there any specific
cooling that can be written in future?
If no cooling is expected, why not a simple hwmon?
> > > + .no_hwmon = true,
> > > +};
> > > +
> > > +static struct thermal_zone_device *
> > > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > > +{
> > > + struct thermal_zone_device *tzdev;
> > > + char name[THERMAL_NAME_LENGTH];
> > > + int ret;
> > > +
> > > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > > +
> > > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > > + &nvme_tz_params, 0, 0);
> > > + if (IS_ERR(tzdev)) {
> > > + dev_err(ctrl->device,
> > > + "Failed to register thermal zone device: %ld\n",
> > > + PTR_ERR(tzdev));
> > > + return tzdev;
> > > + }
> > > +
> > > + snprintf(name, sizeof(name), "temp%d", sensor);
> > > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > > + name);
> > > + if (ret)
> > > + goto device_unregister;
> > > +
> > > + ret = sysfs_create_link(&tzdev->device.kobj,
> > > + &ctrl->ctrl_device.kobj, "device");
> > > + if (ret)
> > > + goto remove_link;
> > > +
> > > + return tzdev;
> > > +
> > > +remove_link:
> > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > +device_unregister:
> > > + thermal_zone_device_unregister(tzdev);
> > > +
> > > + return ERR_PTR(ret);
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function creates up to nine thermal zone devices for all implemented
> > > + * temperature sensors including the composite temperature.
> > > + * Each thermal zone device provides a single trip point temperature that is
> > > + * associated with an over temperature threshold.
> > > + */
> > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + struct nvme_smart_log *log;
> > > + int ret;
> > > + int i;
> > > +
> > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > + if (!log)
> > > + return 0; /* non-fatal error */
> >
> > I am not sure about this API design here. I would leave the error
> > handling and judging if this is fatal or not to the caller.
> > I mean, if I ask to register a nvme thermal zone and I get
> > a 0 as response, I would assume the thermal zone exists from
> > now on, right?
>
> This routine is designed to return error code only when we're unable to
> communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
> negative value).
>
> We don't want to abandon device initialization just due to thermal zone
> failures. Because thermal zone device isn't mandatory to manage the
> controllers, and the device like qemu doesn't provide smart log (according
> to Keith).
That is fair.. it is just really weird to continue your business when
the kernel cannot even allocate memory for you..
>
> > > +
> > > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > > + log, sizeof(*log), 0);
> > > + if (ret) {
> > > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > > + /* If the device provided a response, then it's non-fatal */
> > > + if (ret > 0)
> > > + ret = 0;
> > > + goto free_log;
> >
> > Once again, hiding errors is never a strategy that scales..
> >
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev;
> > > + int temp;
> > > +
> > > + if (i)
> > > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > > + else
> > > + temp = get_unaligned_le16(log->temperature);
> > > +
> > > + /*
> > > + * All implemented temperature sensors report a non-zero value
> > > + * in temperature sensor fields in the smart log page.
> > > + */
> > > + if (!temp)
> > > + continue;
> > > + if (ctrl->tzdev[i])
> > > + continue;
> > > +
> > > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > > + if (!IS_ERR(tzdev))
> > > + ctrl->tzdev[i] = tzdev;
> >
> > Here again, I would not hide errors, the API should propagate errors
> > if something goes wrong.
> >
> > > + }
> > > +
> > > +free_log:
> > > + kfree(log);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function removes the registered thermal zone devices and symlinks.
> > > + */
> > > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > > + char name[20];
> > > +
> > > + if (!tzdev)
> > > + continue;
> > > +
> > > + sysfs_remove_link(&tzdev->device.kobj, "device");
> > > +
> > > + snprintf(name, sizeof(name), "temp%d", i);
> > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > +
> > > + thermal_zone_device_unregister(tzdev);
> > > +
> > > + ctrl->tzdev[i] = NULL;
> > > + }
> > > +}
> > > +
> > > +#else
> > > +
> > > +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > +}
> > > +
> > > +#endif /* CONFIG_THERMAL */
> > > +
> > > struct nvme_core_quirk_entry {
> > > /*
> > > * NVMe model and firmware strings are padded with spaces. For
> > > @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> > > if (ret < 0)
> > > return ret;
> > >
> > > + ret = nvme_thermal_zones_register(ctrl);
> > > + if (ret < 0)
> > > + return ret;
> >
> >
> > I would definitely keep this code the way it is here in
> > nvme_init_identify(), but if I read this right, your
> > nvme_thermal_zones_register() will never return < 0, and therefore this
> > condition is never met.
>
> The nvme_get_log() can return a negative value. Only in this case,
> nvme_thermal_zones_register() returns error and abandon the device
> initialization.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-06 4:05 ` Eduardo Valentin
0 siblings, 0 replies; 40+ messages in thread
From: Eduardo Valentin @ 2019-06-06 4:05 UTC (permalink / raw)
On Tue, Jun 04, 2019@12:20:19AM +0900, Akinobu Mita wrote:
> 2019?6?3?(?) 11:36 Eduardo Valentin <edubezval at gmail.com>:
> >
> > Hey Mita,
> >
> > On Mon, May 27, 2019@01:29:02AM +0900, Akinobu Mita wrote:
> > > The NVMe controller reports up to nine temperature values in the SMART /
> > > Health log page (the composite temperature and temperature sensor 1 through
> > > temperature sensor 8).
> > >
> > > This provides these temperatures via thermal zone devices.
> > >
> > > Once the controller is identified, the thermal zone devices are created for
> > > all implemented temperature sensors including the composite temperature.
> > >
> > > /sys/class/thermal/thermal_zone[0-*]:
> > > |---type: 'nvme<instance>-temp<sensor>'
> > > |---temp: Temperature
> > > |---trip_point_0_temp: Over temperature threshold
> > >
> > > The thermal_zone[0-*] contains a 'device' symlink to the corresponding nvme
> > > device.
> > >
> > > On the other hand, the following symlinks to the thermal zone devices are
> > > created in the nvme device sysfs directory.
> > >
> > > - temp0: Composite temperature
> > > - temp1: Temperature sensor 1
> > > ...
> > > - temp8: Temperature sensor 8
> > >
> >
> > These questions on V2 are still unanswered:
> > a. Can we get a more descriptive string into tz->type?
>
> As I said in the other thread, the NVMe spec only says a controller
> reports the composite temperature and temperature sensor 1 through 8.
> What temperature sensor N means is vendor specific.
I see..
>
> > b. Can these APIs support DT probing too?
>
> OK. I can try, but I don't have arm or arm64 boards with PCIe for
> testing with of-thermal. So it'll be untested.
>
OK. Lets see what comes.
> > > +static struct thermal_zone_params nvme_tz_params = {
> > > + .governor_name = "user_space",
> >
> > Also,
> >
> > Was there any particular reason why defaulting to user_space here?
>
> I only tested with the user_space governor. There is no cooling device
> to bind with this.
>
hmmm.. I see. But was there any particular reason to pick the thermal
subsystem here if you do not have any cooling? Or is there any specific
cooling that can be written in future?
If no cooling is expected, why not a simple hwmon?
> > > + .no_hwmon = true,
> > > +};
> > > +
> > > +static struct thermal_zone_device *
> > > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > > +{
> > > + struct thermal_zone_device *tzdev;
> > > + char name[THERMAL_NAME_LENGTH];
> > > + int ret;
> > > +
> > > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > > +
> > > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > > + &nvme_tz_params, 0, 0);
> > > + if (IS_ERR(tzdev)) {
> > > + dev_err(ctrl->device,
> > > + "Failed to register thermal zone device: %ld\n",
> > > + PTR_ERR(tzdev));
> > > + return tzdev;
> > > + }
> > > +
> > > + snprintf(name, sizeof(name), "temp%d", sensor);
> > > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > > + name);
> > > + if (ret)
> > > + goto device_unregister;
> > > +
> > > + ret = sysfs_create_link(&tzdev->device.kobj,
> > > + &ctrl->ctrl_device.kobj, "device");
> > > + if (ret)
> > > + goto remove_link;
> > > +
> > > + return tzdev;
> > > +
> > > +remove_link:
> > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > +device_unregister:
> > > + thermal_zone_device_unregister(tzdev);
> > > +
> > > + return ERR_PTR(ret);
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function creates up to nine thermal zone devices for all implemented
> > > + * temperature sensors including the composite temperature.
> > > + * Each thermal zone device provides a single trip point temperature that is
> > > + * associated with an over temperature threshold.
> > > + */
> > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + struct nvme_smart_log *log;
> > > + int ret;
> > > + int i;
> > > +
> > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > + if (!log)
> > > + return 0; /* non-fatal error */
> >
> > I am not sure about this API design here. I would leave the error
> > handling and judging if this is fatal or not to the caller.
> > I mean, if I ask to register a nvme thermal zone and I get
> > a 0 as response, I would assume the thermal zone exists from
> > now on, right?
>
> This routine is designed to return error code only when we're unable to
> communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
> negative value).
>
> We don't want to abandon device initialization just due to thermal zone
> failures. Because thermal zone device isn't mandatory to manage the
> controllers, and the device like qemu doesn't provide smart log (according
> to Keith).
That is fair.. it is just really weird to continue your business when
the kernel cannot even allocate memory for you..
>
> > > +
> > > + ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
> > > + log, sizeof(*log), 0);
> > > + if (ret) {
> > > + dev_err(ctrl->device, "Failed to get SMART log: %d\n", ret);
> > > + /* If the device provided a response, then it's non-fatal */
> > > + if (ret > 0)
> > > + ret = 0;
> > > + goto free_log;
> >
> > Once again, hiding errors is never a strategy that scales..
> >
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev;
> > > + int temp;
> > > +
> > > + if (i)
> > > + temp = le16_to_cpu(log->temp_sensor[i - 1]);
> > > + else
> > > + temp = get_unaligned_le16(log->temperature);
> > > +
> > > + /*
> > > + * All implemented temperature sensors report a non-zero value
> > > + * in temperature sensor fields in the smart log page.
> > > + */
> > > + if (!temp)
> > > + continue;
> > > + if (ctrl->tzdev[i])
> > > + continue;
> > > +
> > > + tzdev = nvme_thermal_zone_register(ctrl, i);
> > > + if (!IS_ERR(tzdev))
> > > + ctrl->tzdev[i] = tzdev;
> >
> > Here again, I would not hide errors, the API should propagate errors
> > if something goes wrong.
> >
> > > + }
> > > +
> > > +free_log:
> > > + kfree(log);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nvme_thermal_zones_unregister() - unregister nvme thermal zone devices
> > > + * @ctrl: controller instance
> > > + *
> > > + * This function removes the registered thermal zone devices and symlinks.
> > > + */
> > > +static void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(ctrl->tzdev); i++) {
> > > + struct thermal_zone_device *tzdev = ctrl->tzdev[i];
> > > + char name[20];
> > > +
> > > + if (!tzdev)
> > > + continue;
> > > +
> > > + sysfs_remove_link(&tzdev->device.kobj, "device");
> > > +
> > > + snprintf(name, sizeof(name), "temp%d", i);
> > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > +
> > > + thermal_zone_device_unregister(tzdev);
> > > +
> > > + ctrl->tzdev[i] = NULL;
> > > + }
> > > +}
> > > +
> > > +#else
> > > +
> > > +static inline int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static inline void nvme_thermal_zones_unregister(struct nvme_ctrl *ctrl)
> > > +{
> > > +}
> > > +
> > > +#endif /* CONFIG_THERMAL */
> > > +
> > > struct nvme_core_quirk_entry {
> > > /*
> > > * NVMe model and firmware strings are padded with spaces. For
> > > @@ -2754,6 +3037,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> > > if (ret < 0)
> > > return ret;
> > >
> > > + ret = nvme_thermal_zones_register(ctrl);
> > > + if (ret < 0)
> > > + return ret;
> >
> >
> > I would definitely keep this code the way it is here in
> > nvme_init_identify(), but if I read this right, your
> > nvme_thermal_zones_register() will never return < 0, and therefore this
> > condition is never met.
>
> The nvme_get_log() can return a negative value. Only in this case,
> nvme_thermal_zones_register() returns error and abandon the device
> initialization.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v3 2/3] nvme: add thermal zone devices
2019-06-06 4:05 ` Eduardo Valentin
@ 2019-06-07 15:21 ` Akinobu Mita
-1 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-07 15:21 UTC (permalink / raw)
To: Eduardo Valentin
Cc: linux-nvme, linux-pm, Zhang Rui, Daniel Lezcano, Keith Busch,
Jens Axboe, Christoph Hellwig, Sagi Grimberg, Minwoo Im,
Kenneth Heitke, Chaitanya Kulkarni
2019年6月6日(木) 13:05 Eduardo Valentin <edubezval@gmail.com>:
> > > > +static struct thermal_zone_params nvme_tz_params = {
> > > > + .governor_name = "user_space",
> > >
> > > Also,
> > >
> > > Was there any particular reason why defaulting to user_space here?
> >
> > I only tested with the user_space governor. There is no cooling device
> > to bind with this.
> >
>
> hmmm.. I see. But was there any particular reason to pick the thermal
> subsystem here if you do not have any cooling? Or is there any specific
> cooling that can be written in future?
>
> If no cooling is expected, why not a simple hwmon?
For example, we can use USB port powered cooling fan with USB hub that
supports per-port power switching. It can be turned on and off by user
space tool.
> > > > + .no_hwmon = true,
> > > > +};
> > > > +
> > > > +static struct thermal_zone_device *
> > > > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > > > +{
> > > > + struct thermal_zone_device *tzdev;
> > > > + char name[THERMAL_NAME_LENGTH];
> > > > + int ret;
> > > > +
> > > > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > > > +
> > > > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > > > + &nvme_tz_params, 0, 0);
> > > > + if (IS_ERR(tzdev)) {
> > > > + dev_err(ctrl->device,
> > > > + "Failed to register thermal zone device: %ld\n",
> > > > + PTR_ERR(tzdev));
> > > > + return tzdev;
> > > > + }
> > > > +
> > > > + snprintf(name, sizeof(name), "temp%d", sensor);
> > > > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > > > + name);
> > > > + if (ret)
> > > > + goto device_unregister;
> > > > +
> > > > + ret = sysfs_create_link(&tzdev->device.kobj,
> > > > + &ctrl->ctrl_device.kobj, "device");
> > > > + if (ret)
> > > > + goto remove_link;
> > > > +
> > > > + return tzdev;
> > > > +
> > > > +remove_link:
> > > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > > +device_unregister:
> > > > + thermal_zone_device_unregister(tzdev);
> > > > +
> > > > + return ERR_PTR(ret);
> > > > +}
> > > > +
> > > > +/**
> > > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > > + * @ctrl: controller instance
> > > > + *
> > > > + * This function creates up to nine thermal zone devices for all implemented
> > > > + * temperature sensors including the composite temperature.
> > > > + * Each thermal zone device provides a single trip point temperature that is
> > > > + * associated with an over temperature threshold.
> > > > + */
> > > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > > +{
> > > > + struct nvme_smart_log *log;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > > + if (!log)
> > > > + return 0; /* non-fatal error */
> > >
> > > I am not sure about this API design here. I would leave the error
> > > handling and judging if this is fatal or not to the caller.
> > > I mean, if I ask to register a nvme thermal zone and I get
> > > a 0 as response, I would assume the thermal zone exists from
> > > now on, right?
> >
> > This routine is designed to return error code only when we're unable to
> > communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
> > negative value).
> >
> > We don't want to abandon device initialization just due to thermal zone
> > failures. Because thermal zone device isn't mandatory to manage the
> > controllers, and the device like qemu doesn't provide smart log (according
> > to Keith).
>
> That is fair.. it is just really weird to continue your business when
> the kernel cannot even allocate memory for you..
Right. The system is not unlikely to continue working properly when under
page size allocation with GFP_KERNEL is failed, unless fault injection for
allocation is enabled.
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v3 2/3] nvme: add thermal zone devices
@ 2019-06-07 15:21 ` Akinobu Mita
0 siblings, 0 replies; 40+ messages in thread
From: Akinobu Mita @ 2019-06-07 15:21 UTC (permalink / raw)
2019?6?6?(?) 13:05 Eduardo Valentin <edubezval at gmail.com>:
> > > > +static struct thermal_zone_params nvme_tz_params = {
> > > > + .governor_name = "user_space",
> > >
> > > Also,
> > >
> > > Was there any particular reason why defaulting to user_space here?
> >
> > I only tested with the user_space governor. There is no cooling device
> > to bind with this.
> >
>
> hmmm.. I see. But was there any particular reason to pick the thermal
> subsystem here if you do not have any cooling? Or is there any specific
> cooling that can be written in future?
>
> If no cooling is expected, why not a simple hwmon?
For example, we can use USB port powered cooling fan with USB hub that
supports per-port power switching. It can be turned on and off by user
space tool.
> > > > + .no_hwmon = true,
> > > > +};
> > > > +
> > > > +static struct thermal_zone_device *
> > > > +nvme_thermal_zone_register(struct nvme_ctrl *ctrl, unsigned int sensor)
> > > > +{
> > > > + struct thermal_zone_device *tzdev;
> > > > + char name[THERMAL_NAME_LENGTH];
> > > > + int ret;
> > > > +
> > > > + snprintf(name, sizeof(name), "nvme%d_temp%u", ctrl->instance, sensor);
> > > > +
> > > > + tzdev = thermal_zone_device_register(name, 1, 1, ctrl, &nvme_tz_ops,
> > > > + &nvme_tz_params, 0, 0);
> > > > + if (IS_ERR(tzdev)) {
> > > > + dev_err(ctrl->device,
> > > > + "Failed to register thermal zone device: %ld\n",
> > > > + PTR_ERR(tzdev));
> > > > + return tzdev;
> > > > + }
> > > > +
> > > > + snprintf(name, sizeof(name), "temp%d", sensor);
> > > > + ret = sysfs_create_link(&ctrl->ctrl_device.kobj, &tzdev->device.kobj,
> > > > + name);
> > > > + if (ret)
> > > > + goto device_unregister;
> > > > +
> > > > + ret = sysfs_create_link(&tzdev->device.kobj,
> > > > + &ctrl->ctrl_device.kobj, "device");
> > > > + if (ret)
> > > > + goto remove_link;
> > > > +
> > > > + return tzdev;
> > > > +
> > > > +remove_link:
> > > > + sysfs_remove_link(&ctrl->ctrl_device.kobj, name);
> > > > +device_unregister:
> > > > + thermal_zone_device_unregister(tzdev);
> > > > +
> > > > + return ERR_PTR(ret);
> > > > +}
> > > > +
> > > > +/**
> > > > + * nvme_thermal_zones_register() - register nvme thermal zone devices
> > > > + * @ctrl: controller instance
> > > > + *
> > > > + * This function creates up to nine thermal zone devices for all implemented
> > > > + * temperature sensors including the composite temperature.
> > > > + * Each thermal zone device provides a single trip point temperature that is
> > > > + * associated with an over temperature threshold.
> > > > + */
> > > > +static int nvme_thermal_zones_register(struct nvme_ctrl *ctrl)
> > > > +{
> > > > + struct nvme_smart_log *log;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + log = kzalloc(sizeof(*log), GFP_KERNEL);
> > > > + if (!log)
> > > > + return 0; /* non-fatal error */
> > >
> > > I am not sure about this API design here. I would leave the error
> > > handling and judging if this is fatal or not to the caller.
> > > I mean, if I ask to register a nvme thermal zone and I get
> > > a 0 as response, I would assume the thermal zone exists from
> > > now on, right?
> >
> > This routine is designed to return error code only when we're unable to
> > communicate with the device at all (i.e. nvme_submit_sync_cmd returns a
> > negative value).
> >
> > We don't want to abandon device initialization just due to thermal zone
> > failures. Because thermal zone device isn't mandatory to manage the
> > controllers, and the device like qemu doesn't provide smart log (according
> > to Keith).
>
> That is fair.. it is just really weird to continue your business when
> the kernel cannot even allocate memory for you..
Right. The system is not unlikely to continue working properly when under
page size allocation with GFP_KERNEL is failed, unless fault injection for
allocation is enabled.
^ permalink raw reply [flat|nested] 40+ messages in thread