[PATCH v1 01/11] thermal/core: Relocate the traces definition in thermal directory

[PATCH v1 01/11] thermal/core: Relocate the traces definition in thermal directory

[Daniel Lezcano]

The traces are exported but only local to the thermal core code. On
the other side, the traces take the thermal zone device structure as
argument, thus they have to rely on the exported thermal.h header
file. As we want to move the structure to the private thermal core
header, first we have to relocate those traces to the same place as
many drivers do.

Cc: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/Makefile                                    | 3 ++-
 drivers/thermal/cpufreq_cooling.c                           | 2 +-
 drivers/thermal/devfreq_cooling.c                           | 2 +-
 drivers/thermal/gov_fair_share.c                            | 2 +-
 drivers/thermal/gov_power_allocator.c                       | 2 +-
 drivers/thermal/gov_step_wise.c                             | 2 +-
 drivers/thermal/thermal_core.c                              | 2 +-
 drivers/thermal/thermal_helpers.c                           | 3 +--
 .../events/thermal.h => drivers/thermal/thermal_trace.h     | 6 ++++++
 .../thermal/thermal_trace_ipa.h                             | 6 ++++++
 10 files changed, 21 insertions(+), 9 deletions(-)
 rename include/trace/events/thermal.h => drivers/thermal/thermal_trace.h (97%)
 rename include/trace/events/thermal_power_allocator.h => drivers/thermal/thermal_trace_ipa.h (96%)

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index eed300e83d48..058664bc3ec0 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -2,7 +2,7 @@
 #
 # Makefile for sensor chip drivers.
 #
-
+CFLAGS_thermal_core.o		:= -I$(src)
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 thermal_sys-y			+= thermal_core.o thermal_sysfs.o
 thermal_sys-y			+= thermal_trip.o thermal_helpers.o
@@ -16,6 +16,7 @@ thermal_sys-$(CONFIG_THERMAL_OF)		+= thermal_of.o
 thermal_sys-$(CONFIG_THERMAL_ACPI)		+= thermal_acpi.o
 
 # governors
+CFLAGS_gov_power_allocator.o			:= -I$(src)
 thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE)	+= gov_fair_share.o
 thermal_sys-$(CONFIG_THERMAL_GOV_BANG_BANG)	+= gov_bang_bang.o
 thermal_sys-$(CONFIG_THERMAL_GOV_STEP_WISE)	+= gov_step_wise.o
diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index 9f8b438fcf8f..65ef08b30334 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -23,7 +23,7 @@
 #include <linux/thermal.h>
 #include <linux/units.h>
 
-#include <trace/events/thermal.h>
+#include "thermal_trace.h"
 
 /*
  * Cooling state <-> CPUFreq frequency
diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index 24b474925cd6..262e62ab6cf2 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -20,7 +20,7 @@
 #include <linux/thermal.h>
 #include <linux/units.h>
 
-#include <trace/events/thermal.h>
+#include "thermal_trace.h"
 
 #define SCALE_ERROR_MITIGATION	100
 
diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index aad7d5fe3a14..1ce5692151f5 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -11,7 +11,7 @@
  */
 
 #include <linux/thermal.h>
-#include <trace/events/thermal.h>
+#include "thermal_trace.h" 
 
 #include "thermal_core.h"
 
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0eaf1527d3e3..3df2d440d73d 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -12,7 +12,7 @@
 #include <linux/thermal.h>
 
 #define CREATE_TRACE_POINTS
-#include <trace/events/thermal_power_allocator.h>
+#include "thermal_trace_ipa.h" 
 
 #include "thermal_core.h"
 
diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
index 31235e169c5a..f69c83e2992d 100644
--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -12,7 +12,7 @@
 
 #include <linux/thermal.h>
 #include <linux/minmax.h>
-#include <trace/events/thermal.h>
+#include "thermal_trace.h" 
 
 #include "thermal_core.h"
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 46dedfe061df..cec72c6673a5 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -22,7 +22,7 @@
 #include <linux/suspend.h>
 
 #define CREATE_TRACE_POINTS
-#include <trace/events/thermal.h>
+#include "thermal_trace.h"
 
 #include "thermal_core.h"
 #include "thermal_hwmon.h"
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 9558339f5633..c41b1a5700a5 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -19,9 +19,8 @@
 #include <linux/string.h>
 #include <linux/sysfs.h>
 
-#include <trace/events/thermal.h>
-
 #include "thermal_core.h"
+#include "thermal_trace.h"
 
 int get_tz_trend(struct thermal_zone_device *tz, int trip)
 {
diff --git a/include/trace/events/thermal.h b/drivers/thermal/thermal_trace.h
similarity index 97%
rename from include/trace/events/thermal.h
rename to drivers/thermal/thermal_trace.h
index e58bf3072f32..459c8ce6cf3b 100644
--- a/include/trace/events/thermal.h
+++ b/drivers/thermal/thermal_trace.h
@@ -195,5 +195,11 @@ TRACE_EVENT(thermal_power_devfreq_limit,
 #endif /* CONFIG_DEVFREQ_THERMAL */
 #endif /* _TRACE_THERMAL_H */
 
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE thermal_trace
+
 /* This part must be outside protection */
 #include <trace/define_trace.h>
diff --git a/include/trace/events/thermal_power_allocator.h b/drivers/thermal/thermal_trace_ipa.h
similarity index 96%
rename from include/trace/events/thermal_power_allocator.h
rename to drivers/thermal/thermal_trace_ipa.h
index 1c8fb95544f9..84568db5421b 100644
--- a/include/trace/events/thermal_power_allocator.h
+++ b/drivers/thermal/thermal_trace_ipa.h
@@ -84,5 +84,11 @@ TRACE_EVENT(thermal_power_allocator_pid,
 );
 #endif /* _TRACE_THERMAL_POWER_ALLOCATOR_H */
 
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE thermal_trace_ipa
+
 /* This part must be outside protection */
 #include <trace/define_trace.h>
-- 
2.34.1

[PATCH v1 02/11] thermal/drivers/intel_pch_thermal: Use thermal driver device to write a trace

[Daniel Lezcano]

The pch_critical() callback accesses the thermal zone device structure
internals, it dereferences the thermal zone struct device and the 'type'.

For the former, the driver related device should be use instead and
for the latter an accessor already exists. Use them instead of
accessing the internals.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/intel/intel_pch_thermal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index dce50d239357..0de46057db2a 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -127,7 +127,10 @@ static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp)
 
 static void pch_critical(struct thermal_zone_device *tzd)
 {
-	dev_dbg(&tzd->device, "%s: critical temperature reached\n", tzd->type);
+	struct pch_thermal_device *ptd = thermal_zone_device_priv(tzd);
+
+	dev_dbg(&ptd->pdev->dev, "%s: critical temperature reached\n",
+		thermal_zone_device_type(tzd));
 }
 
 static struct thermal_zone_device_ops tzd_ops = {
-- 
2.34.1

[PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Daniel Lezcano]

The driver hooks the thermal framework sysfs to add some driver
specific information. A debatable approach as that may belong the
device sysfs directory, not the thermal zone directory.

As the driver is accessing the thermal internals, we should provide at
least an API to the thermal framework to add an attribute to the
existing sysfs thermal zone entry.

Before doing that and given the age of the driver (2008) may be it is
worth to double check if these attributes are really needed. So my
first proposal is to remove them if that does not hurt.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
 1 file changed, 193 deletions(-)

diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
index 5a6ad0552311..5a9738a93083 100644
--- a/drivers/thermal/intel/intel_menlow.c
+++ b/drivers/thermal/intel/intel_menlow.c
@@ -230,174 +230,8 @@ struct intel_menlow_attribute {
 static LIST_HEAD(intel_menlow_attr_list);
 static DEFINE_MUTEX(intel_menlow_attr_lock);
 
-/*
- * sensor_get_auxtrip - get the current auxtrip value from sensor
- * @handle: Object handle
- * @index : GET_AUX1/GET_AUX0
- * @value : The address will be fill by the value
- */
-static int sensor_get_auxtrip(acpi_handle handle, int index,
-							unsigned long long *value)
-{
-	acpi_status status;
-
-	if ((index != 0 && index != 1) || !value)
-		return -EINVAL;
-
-	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
-				       NULL, value);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
-	return 0;
-}
-
-/*
- * sensor_set_auxtrip - set the new auxtrip value to sensor
- * @handle: Object handle
- * @index : GET_AUX1/GET_AUX0
- * @value : The value will be set
- */
-static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
-{
-	acpi_status status;
-	union acpi_object arg = {
-		ACPI_TYPE_INTEGER
-	};
-	struct acpi_object_list args = {
-		1, &arg
-	};
-	unsigned long long temp;
-
-	if (index != 0 && index != 1)
-		return -EINVAL;
-
-	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
-				       NULL, &temp);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-	if ((index && value < temp) || (!index && value > temp))
-		return -EINVAL;
-
-	arg.integer.value = value;
-	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
-				       &args, &temp);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
-	/* do we need to check the return value of SAX0/SAX1 ? */
-
-	return 0;
-}
-
-#define to_intel_menlow_attr(_attr)	\
-	container_of(_attr, struct intel_menlow_attribute, attr)
-
-static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
-			char *buf, int idx)
-{
-	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
-	unsigned long long value;
-	int result;
-
-	result = sensor_get_auxtrip(attr->handle, idx, &value);
-	if (result)
-		return result;
-
-	return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
-}
-
-static ssize_t aux0_show(struct device *dev,
-			 struct device_attribute *dev_attr, char *buf)
-{
-	return aux_show(dev, dev_attr, buf, 0);
-}
-
-static ssize_t aux1_show(struct device *dev,
-			 struct device_attribute *dev_attr, char *buf)
-{
-	return aux_show(dev, dev_attr, buf, 1);
-}
-
-static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
-			 const char *buf, size_t count, int idx)
-{
-	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
-	int value;
-	int result;
-
-	/*Sanity check; should be a positive integer */
-	if (!sscanf(buf, "%d", &value))
-		return -EINVAL;
-
-	if (value < 0)
-		return -EINVAL;
-
-	result = sensor_set_auxtrip(attr->handle, idx,
-				    celsius_to_deci_kelvin(value));
-	return result ? result : count;
-}
-
-static ssize_t aux0_store(struct device *dev,
-			  struct device_attribute *dev_attr,
-			  const char *buf, size_t count)
-{
-	return aux_store(dev, dev_attr, buf, count, 0);
-}
-
-static ssize_t aux1_store(struct device *dev,
-			  struct device_attribute *dev_attr,
-			  const char *buf, size_t count)
-{
-	return aux_store(dev, dev_attr, buf, count, 1);
-}
-
 /* BIOS can enable/disable the thermal user application in dabney platform */
 #define BIOS_ENABLED "\\_TZ.GSTS"
-static ssize_t bios_enabled_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	acpi_status status;
-	unsigned long long bios_enabled;
-
-	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
-}
-
-static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
-					  void *store, struct device *dev,
-					  acpi_handle handle)
-{
-	struct intel_menlow_attribute *attr;
-	int result;
-
-	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
-	if (!attr)
-		return -ENOMEM;
-
-	sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
-	attr->attr.attr.name = name;
-	attr->attr.attr.mode = mode;
-	attr->attr.show = show;
-	attr->attr.store = store;
-	attr->device = dev;
-	attr->handle = handle;
-
-	result = device_create_file(dev, &attr->attr);
-	if (result) {
-		kfree(attr);
-		return result;
-	}
-
-	mutex_lock(&intel_menlow_attr_lock);
-	list_add_tail(&attr->node, &intel_menlow_attr_list);
-	mutex_unlock(&intel_menlow_attr_lock);
-
-	return 0;
-}
 
 static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
 						void *context, void **rv)
@@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
 	if (ACPI_FAILURE(status))
 		return (status == AE_NOT_FOUND) ? AE_OK : status;
 
-	result = intel_menlow_add_one_attribute("aux0", 0644,
-						aux0_show, aux0_store,
-						&thermal->device, handle);
-	if (result)
-		return AE_ERROR;
-
 	status = acpi_get_handle(handle, GET_AUX1, &dummy);
 	if (ACPI_FAILURE(status))
 		goto aux1_not_found;
@@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
 	if (ACPI_FAILURE(status))
 		goto aux1_not_found;
 
-	result = intel_menlow_add_one_attribute("aux1", 0644,
-						aux1_show, aux1_store,
-						&thermal->device, handle);
-	if (result) {
-		intel_menlow_unregister_sensor();
-		return AE_ERROR;
-	}
-
-	/*
-	 * create the "dabney_enabled" attribute which means the user app
-	 * should be loaded or not
-	 */
-
-	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
-						bios_enabled_show, NULL,
-						&thermal->device, handle);
-	if (result) {
-		intel_menlow_unregister_sensor();
-		return AE_ERROR;
-	}
-
 	return AE_OK;
 
  aux1_not_found:
-- 
2.34.1

[PATCH v1 04/11] thermal/drivers/db8500: Use driver dev instead of tz->device

[Daniel Lezcano]

The db8500 driver uses the thermal zone device instead of the device
attached to it. In order to prevent the drivers to access the thermal
zone device structure, replace the thermal zone device by the driver
to show the debug message.

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/db8500_thermal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
index c0418497520c..de790e526ca5 100644
--- a/drivers/thermal/db8500_thermal.c
+++ b/drivers/thermal/db8500_thermal.c
@@ -53,6 +53,7 @@ static const unsigned long db8500_thermal_points[] = {
 
 struct db8500_thermal_zone {
 	struct thermal_zone_device *tz;
+	struct device *dev;
 	unsigned long interpolated_temp;
 	unsigned int cur_index;
 };
@@ -114,7 +115,7 @@ static irqreturn_t prcmu_low_irq_handler(int irq, void *irq_data)
 	idx -= 1;
 
 	db8500_thermal_update_config(th, idx, next_low, next_high);
-	dev_dbg(&th->tz->device,
+	dev_dbg(th->dev,
 		"PRCMU set max %ld, min %ld\n", next_high, next_low);
 
 	thermal_zone_device_update(th->tz, THERMAL_EVENT_UNSPECIFIED);
@@ -136,7 +137,7 @@ static irqreturn_t prcmu_high_irq_handler(int irq, void *irq_data)
 
 		db8500_thermal_update_config(th, idx, next_low, next_high);
 
-		dev_dbg(&th->tz->device,
+		dev_dbg(th->dev,
 			"PRCMU set max %ld, min %ld\n", next_high, next_low);
 	} else if (idx == num_points - 1)
 		/* So we roof out 1 degree over the max point */
@@ -157,6 +158,8 @@ static int db8500_thermal_probe(struct platform_device *pdev)
 	if (!th)
 		return -ENOMEM;
 
+	th->dev = dev;
+	
 	low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
 	if (low_irq < 0)
 		return low_irq;
-- 
2.34.1

[PATCH v1 05/11] thermal/drivers/stm: Don't set no_hwmon to false

[Daniel Lezcano]

The thermal->tzp->no_hwmon parameter is only used when calling
thermal_zone_device_register().

Setting it to 'false' before calling thermal_add_hwmon_sysfs() has no
effect.

Remove the call and again prevent the drivers to access the thermal
internals.

Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/st/stm_thermal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/thermal/st/stm_thermal.c b/drivers/thermal/st/stm_thermal.c
index 6f2bad8ef82f..903fcf1763f1 100644
--- a/drivers/thermal/st/stm_thermal.c
+++ b/drivers/thermal/st/stm_thermal.c
@@ -558,7 +558,6 @@ static int stm_thermal_probe(struct platform_device *pdev)
 	 * Thermal_zone doesn't enable hwmon as default,
 	 * enable it here
 	 */
-	sensor->th_dev->tzp->no_hwmon = false;
 	ret = thermal_add_hwmon_sysfs(sensor->th_dev);
 	if (ret)
 		goto err_tz;
-- 
2.34.1

[PATCH v1 06/11] thermal/drivers/ti: Use fixed update interval

[Daniel Lezcano]

Currently the TI thermal driver sets the sensor update interval based
on the polling of the thermal zone. In order to get the polling rate,
the code inspects the thermal zone device strcuture internals, thus
breaking the self-encapsulation of the thermal framework core
framework.

On the other side, we see the common polling rates set in the device
tree for the platforms using this driver are 500 or 1000 ms.

Setting the polling rate to 250 ms would be far enough to cover the
combination we found in the device tree.

Instead of accessing the thermal zone device structure polling rate,
let's use a common update interval of 250 ms for the driver.

Cc: Keerthy <j-keerthy@ti.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 0c8914017c18..430c4b43151f 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -23,6 +23,8 @@
 #include "ti-bandgap.h"
 #include "../thermal_hwmon.h"
 
+#define TI_BANDGAP_UPDATE_INTERVAL_MS 250
+
 /* common data structures */
 struct ti_thermal_data {
 	struct cpufreq_policy *policy;
@@ -159,7 +161,6 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
 			     char *domain)
 {
 	struct ti_thermal_data *data;
-	int interval;
 
 	data = ti_bandgap_get_sensor_data(bgp, id);
 
@@ -177,10 +178,9 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
 		return PTR_ERR(data->ti_thermal);
 	}
 
-	interval = jiffies_to_msecs(data->ti_thermal->polling_delay_jiffies);
-
 	ti_bandgap_set_sensor_data(bgp, id, data);
-	ti_bandgap_write_update_interval(bgp, data->sensor_id, interval);
+	ti_bandgap_write_update_interval(bgp, data->sensor_id,
+					 TI_BANDGAP_UPDATE_INTERVAL_MS);
 
 	if (devm_thermal_add_hwmon_sysfs(bgp->dev, data->ti_thermal))
 		dev_warn(bgp->dev, "failed to add hwmon sysfs attributes\n");
-- 
2.34.1

[PATCH v1 07/11] thermal/drivers/bcm2835: Remove buggy call to thermal_of_zone_unregister

[Daniel Lezcano]

The driver is using the devm_thermal_of_zone_device_register().

In the error path of the function calling
devm_thermal_of_zone_device_register(), the function
devm_thermal_of_zone_unregister() should be called instead of
thermal_of_zone_unregister(), otherwise this one will be called twice
when the device is freed.

The same happens for the remove function where the devm_ guarantee the
thermal_of_zone_unregister() will be called, so adding this call in
the remove function will lead to a double free also.

Use devm_ variant in the error path of the probe function.

Remove thermal_of_zone_unregister() in the remove function.

Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/broadcom/bcm2835_thermal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/broadcom/bcm2835_thermal.c b/drivers/thermal/broadcom/bcm2835_thermal.c
index a217d832f24e..ea07152a6d0d 100644
--- a/drivers/thermal/broadcom/bcm2835_thermal.c
+++ b/drivers/thermal/broadcom/bcm2835_thermal.c
@@ -269,13 +269,13 @@ static int bcm2835_thermal_probe(struct platform_device *pdev)
 	 */
 	err = thermal_add_hwmon_sysfs(tz);
 	if (err)
-		goto err_tz;
+		goto err_clk;
 
 	bcm2835_thermal_debugfs(pdev);
 
 	return 0;
 err_tz:
-	thermal_of_zone_unregister(tz);
+	devm_thermal_of_zone_unregister(&pdev->dev, tz);
 err_clk:
 	clk_disable_unprepare(data->clk);
 
@@ -285,10 +285,8 @@ static int bcm2835_thermal_probe(struct platform_device *pdev)
 static int bcm2835_thermal_remove(struct platform_device *pdev)
 {
 	struct bcm2835_thermal_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tz = data->tz;
 
 	debugfs_remove_recursive(data->debugfsdir);
-	thermal_of_zone_unregister(tz);
 	clk_disable_unprepare(data->clk);
 
 	return 0;
-- 
2.34.1

[PATCH v1 08/11] thermal/of: Unexport unused OF functions

[Daniel Lezcano]

The functions thermal_of_zone_register() and
thermal_of_zone_unregister() are no longer needed from the drivers as
the devm_ variant is always used.

Make them static in the C file and remove their declaration from thermal.h

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_of.c |  8 +++-----
 include/linux/thermal.h      | 17 -----------------
 2 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index ff4d12ef51bc..6fb14e521197 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -439,7 +439,7 @@ static int thermal_of_unbind(struct thermal_zone_device *tz,
  *
  * @tz: a pointer to the thermal zone structure
  */
-void thermal_of_zone_unregister(struct thermal_zone_device *tz)
+static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
 {
 	struct thermal_trip *trips = tz->trips;
 	struct thermal_zone_params *tzp = tz->tzp;
@@ -451,7 +451,6 @@ void thermal_of_zone_unregister(struct thermal_zone_device *tz)
 	kfree(tzp);
 	kfree(ops);
 }
-EXPORT_SYMBOL_GPL(thermal_of_zone_unregister);
 
 /**
  * thermal_of_zone_register - Register a thermal zone with device node
@@ -473,8 +472,8 @@ EXPORT_SYMBOL_GPL(thermal_of_zone_unregister);
  *	- ENOMEM: if one structure can not be allocated
  *	- Other negative errors are returned by the underlying called functions
  */
-struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
-						     const struct thermal_zone_device_ops *ops)
+static struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
+							    const struct thermal_zone_device_ops *ops)
 {
 	struct thermal_zone_device *tz;
 	struct thermal_trip *trips;
@@ -550,7 +549,6 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor,
 
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(thermal_of_zone_register);
 
 static void devm_thermal_of_zone_release(struct device *dev, void *res)
 {
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index eb80cee4f64f..8cdf94cdc5ff 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -297,25 +297,12 @@ struct thermal_zone_params {
 
 /* Function declarations */
 #ifdef CONFIG_THERMAL_OF
-struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
-						     const struct thermal_zone_device_ops *ops);
-
 struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
 							  const struct thermal_zone_device_ops *ops);
 
-void thermal_of_zone_unregister(struct thermal_zone_device *tz);
-
 void devm_thermal_of_zone_unregister(struct device *dev, struct thermal_zone_device *tz);
 
-void thermal_of_zone_unregister(struct thermal_zone_device *tz);
-
 #else
-static inline
-struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
-						     const struct thermal_zone_device_ops *ops)
-{
-	return ERR_PTR(-ENOTSUPP);
-}
 
 static inline
 struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
@@ -324,10 +311,6 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
 	return ERR_PTR(-ENOTSUPP);
 }
 
-static inline void thermal_of_zone_unregister(struct thermal_zone_device *tz)
-{
-}
-
 static inline void devm_thermal_of_zone_unregister(struct device *dev,
 						   struct thermal_zone_device *tz)
 {
-- 
2.34.1

[PATCH v1 09/11] thermal/core: Add a linked device parameter

[Daniel Lezcano]

Some drivers want to create a link from the thermal zone to the device
sysfs entry and vice versa. That is the case of the APCI driver.

Having a backpointer from the device to the thermal zone sounds akward
as we can have the same device instantiating multiple thermal zones so
there will be a conflict while creating the second link with the same
name. Moreover, the userspace has enough information to build the
dependency from the thermal zone device link without having this cyclic
link from the device to thermal zone.

Anyway, everything in its time.

This change allows to create a these cyclic links tz <-> device as
ACPI does and will allow to remove the code in the ACPI driver.

The limitation of this change is there can be only a 1:1 relationship
between the device and the thermal zone, otherwise the 'thermal_zone'
link name will conflict with the previous link with the same name.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 16 ++++++++++++++++
 include/linux/thermal.h        |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index cec72c6673a5..ca91189bc441 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1340,6 +1340,18 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	list_add_tail(&tz->node, &thermal_tz_list);
 	mutex_unlock(&thermal_list_lock);
 
+	if (tzp && tzp->linked_dev) {
+		result = sysfs_create_link(&tzp->linked_dev->kobj,
+					   &tz->device.kobj, "thermal_zone");
+		if (result)
+			goto out_list_del;
+
+		result = sysfs_create_link(&tz->device.kobj,
+					   &tzp->linked_dev->kobj, "device");
+		if (result)
+			goto out_del_link;
+	}
+
 	/* Bind cooling devices for this zone */
 	bind_tz(tz);
 
@@ -1354,6 +1366,10 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 
 	return tz;
 
+out_del_link:
+	sysfs_remove_link(&tz->device.kobj, "thermal_zone");
+out_list_del:
+	list_del(&tz->node);
 unregister:
 	device_del(&tz->device);
 release_device:
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 8cdf94cdc5ff..f60d7edf1e5d 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -256,6 +256,13 @@ struct thermal_zone_params {
 	int num_tbps;	/* Number of tbp entries */
 	struct thermal_bind_params *tbp;
 
+	/*
+	 * @linked_dev: Add a cross link from the device to the
+	 * thermal zone and vice versa. They will be named
+	 * respectively 'device' and 'thermal_zone'
+	 */
+	struct device *linked_dev;
+
 	/*
 	 * Sustainable power (heat) that this thermal zone can dissipate in
 	 * mW
-- 
2.34.1

[PATCH v1 10/11] thermal/core: Alloc-copy-free the thermal zone parameters structure

[Daniel Lezcano]

The caller of the function thermal_zone_device_register_with_trips()
can pass a thermal_zone_params structure parameter.

This one is used by the thermal core code until the thermal zone is
destroyed. That forces the caller, so the driver, to keep the pointer
valid until it unregisters the thermal zone if we want to make the
thermal zone device structure private the core code.

As the thermal zone device structure would be private, the driver can
not access to thermal zone device structure to retrieve the tzp field
after it passed it to register the thermal zone.

So instead of forcing the users of the function to deal with the tzp
structure life cycle, make the usage easier by allocating our own
thermal zone params, copying the parameter content and by freeing at
unregister time. The user can then create the parameters on the stack,
pass it to the registering function and forget about it.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index ca91189bc441..6cbda8f4522e 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1263,13 +1263,19 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	if (!tz)
 		return ERR_PTR(-ENOMEM);
 
+	if (tzp) {
+		tz->tzp = kmemdup(tzp, sizeof(*tzp), GFP_KERNEL);
+		if (!tz->tzp)
+			goto free_tz;
+	}
+	
 	INIT_LIST_HEAD(&tz->thermal_instances);
 	ida_init(&tz->ida);
 	mutex_init(&tz->lock);
 	id = ida_alloc(&thermal_tz_ida, GFP_KERNEL);
 	if (id < 0) {
 		result = id;
-		goto free_tz;
+		goto free_tzp;
 	}
 
 	tz->id = id;
@@ -1279,7 +1285,6 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 		ops->critical = thermal_zone_device_critical;
 
 	tz->ops = ops;
-	tz->tzp = tzp;
 	tz->device.class = thermal_class;
 	tz->devdata = devdata;
 	tz->trips = trips;
@@ -1377,6 +1382,8 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	tz = NULL;
 remove_id:
 	ida_free(&thermal_tz_ida, id);
+free_tzp:
+	kfree(tz->tzp);
 free_tz:
 	kfree(tz);
 	return ERR_PTR(result);
@@ -1472,6 +1479,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	device_del(&tz->device);
 	mutex_unlock(&tz->lock);
 
+	kfree(tzp);
+	
 	put_device(&tz->device);
 
 	thermal_notify_tz_delete(tz_id);
-- 
2.34.1

Re: [PATCH v1 06/11] thermal/drivers/ti: Use fixed update interval

[J, KEERTHY]

On 3/7/2023 7:07 PM, Daniel Lezcano wrote:
> Currently the TI thermal driver sets the sensor update interval based
> on the polling of the thermal zone. In order to get the polling rate,
> the code inspects the thermal zone device strcuture internals, thus
> breaking the self-encapsulation of the thermal framework core
> framework.
> 
> On the other side, we see the common polling rates set in the device
> tree for the platforms using this driver are 500 or 1000 ms.
> 
> Setting the polling rate to 250 ms would be far enough to cover the
> combination we found in the device tree.
> 
> Instead of accessing the thermal zone device structure polling rate,
> let's use a common update interval of 250 ms for the driver.

Thanks for the patch.

Acked-by: Keerthy <j-keerthy@ti.com>

> 
> Cc: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 0c8914017c18..430c4b43151f 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -23,6 +23,8 @@
>   #include "ti-bandgap.h"
>   #include "../thermal_hwmon.h"
>   
> +#define TI_BANDGAP_UPDATE_INTERVAL_MS 250
> +
>   /* common data structures */
>   struct ti_thermal_data {
>   	struct cpufreq_policy *policy;
> @@ -159,7 +161,6 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
>   			     char *domain)
>   {
>   	struct ti_thermal_data *data;
> -	int interval;
>   
>   	data = ti_bandgap_get_sensor_data(bgp, id);
>   
> @@ -177,10 +178,9 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
>   		return PTR_ERR(data->ti_thermal);
>   	}
>   
> -	interval = jiffies_to_msecs(data->ti_thermal->polling_delay_jiffies);
> -
>   	ti_bandgap_set_sensor_data(bgp, id, data);
> -	ti_bandgap_write_update_interval(bgp, data->sensor_id, interval);
> +	ti_bandgap_write_update_interval(bgp, data->sensor_id,
> +					 TI_BANDGAP_UPDATE_INTERVAL_MS);
>   
>   	if (devm_thermal_add_hwmon_sysfs(bgp->dev, data->ti_thermal))
>   		dev_warn(bgp->dev, "failed to add hwmon sysfs attributes\n");

Re: [PATCH v1 06/11] thermal/drivers/ti: Use fixed update interval

[Gole, Dhruva]

On 3/7/2023 7:07 PM, Daniel Lezcano wrote:
> Currently the TI thermal driver sets the sensor update interval based
> on the polling of the thermal zone. In order to get the polling rate,
> the code inspects the thermal zone device strcuture internals, thus
> breaking the self-encapsulation of the thermal framework core
> framework.
>
> On the other side, we see the common polling rates set in the device
> tree for the platforms using this driver are 500 or 1000 ms.
>
> Setting the polling rate to 250 ms would be far enough to cover the
> combination we found in the device tree.
>
> Instead of accessing the thermal zone device structure polling rate,
> let's use a common update interval of 250 ms for the driver.
>
> Cc: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 0c8914017c18..430c4b43151f 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -23,6 +23,8 @@
>  #include "ti-bandgap.h"
>  #include "../thermal_hwmon.h"
>  
> +#define TI_BANDGAP_UPDATE_INTERVAL_MS 250
> +
>  /* common data structures */
>  struct ti_thermal_data {
>  	struct cpufreq_policy *policy;
> @@ -159,7 +161,6 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
>  			     char *domain)
>  {
>  	struct ti_thermal_data *data;
> -	int interval;
>  
>  	data = ti_bandgap_get_sensor_data(bgp, id);
>  
> @@ -177,10 +178,9 @@ int ti_thermal_expose_sensor(struct ti_bandgap *bgp, int id,
>  		return PTR_ERR(data->ti_thermal);
>  	}
>  
> -	interval = jiffies_to_msecs(data->ti_thermal->polling_delay_jiffies);
> -
>  	ti_bandgap_set_sensor_data(bgp, id, data);
> -	ti_bandgap_write_update_interval(bgp, data->sensor_id, interval);
> +	ti_bandgap_write_update_interval(bgp, data->sensor_id,
> +					 TI_BANDGAP_UPDATE_INTERVAL_MS);
Reviewed-by: Dhruva Gole <d-gole@ti.com>
>  
>  	if (devm_thermal_add_hwmon_sysfs(bgp->dev, data->ti_thermal))
>  		dev_warn(bgp->dev, "failed to add hwmon sysfs attributes\n");

-- 
Regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v1 01/11] thermal/core: Relocate the traces definition in thermal directory

[Steven Rostedt]

On Tue,  7 Mar 2023 14:37:25 +0100
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> The traces are exported but only local to the thermal core code. On
> the other side, the traces take the thermal zone device structure as
> argument, thus they have to rely on the exported thermal.h header
> file. As we want to move the structure to the private thermal core
> header, first we have to relocate those traces to the same place as
> many drivers do.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  drivers/thermal/Makefile                                    | 3 ++-
>  drivers/thermal/cpufreq_cooling.c                           | 2 +-
>  drivers/thermal/devfreq_cooling.c                           | 2 +-
>  drivers/thermal/gov_fair_share.c                            | 2 +-
>  drivers/thermal/gov_power_allocator.c                       | 2 +-
>  drivers/thermal/gov_step_wise.c                             | 2 +-
>  drivers/thermal/thermal_core.c                              | 2 +-
>  drivers/thermal/thermal_helpers.c                           | 3 +--
>  .../events/thermal.h => drivers/thermal/thermal_trace.h     | 6 ++++++
>  .../thermal/thermal_trace_ipa.h                             | 6 ++++++
>  10 files changed, 21 insertions(+), 9 deletions(-)
>  rename include/trace/events/thermal.h => drivers/thermal/thermal_trace.h (97%)
>  rename include/trace/events/thermal_power_allocator.h => drivers/thermal/thermal_trace_ipa.h (96%)

Re: [PATCH v1 04/11] thermal/drivers/db8500: Use driver dev instead of tz->device

[Linus Walleij]

On Tue, Mar 7, 2023 at 2:37 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> The db8500 driver uses the thermal zone device instead of the device
> attached to it. In order to prevent the drivers to access the thermal
> zone device structure, replace the thermal zone device by the driver
> to show the debug message.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Yep that's the right thing to do.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Daniel Lezcano]

Hi,

is this code removal acceptable ?


On 07/03/2023 14:37, Daniel Lezcano wrote:
> The driver hooks the thermal framework sysfs to add some driver
> specific information. A debatable approach as that may belong the
> device sysfs directory, not the thermal zone directory.
> 
> As the driver is accessing the thermal internals, we should provide at
> least an API to the thermal framework to add an attribute to the
> existing sysfs thermal zone entry.
> 
> Before doing that and given the age of the driver (2008) may be it is
> worth to double check if these attributes are really needed. So my
> first proposal is to remove them if that does not hurt.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>



> ---
>   drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
>   1 file changed, 193 deletions(-)
> 
> diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
> index 5a6ad0552311..5a9738a93083 100644
> --- a/drivers/thermal/intel/intel_menlow.c
> +++ b/drivers/thermal/intel/intel_menlow.c
> @@ -230,174 +230,8 @@ struct intel_menlow_attribute {
>   static LIST_HEAD(intel_menlow_attr_list);
>   static DEFINE_MUTEX(intel_menlow_attr_lock);
>   
> -/*
> - * sensor_get_auxtrip - get the current auxtrip value from sensor
> - * @handle: Object handle
> - * @index : GET_AUX1/GET_AUX0
> - * @value : The address will be fill by the value
> - */
> -static int sensor_get_auxtrip(acpi_handle handle, int index,
> -							unsigned long long *value)
> -{
> -	acpi_status status;
> -
> -	if ((index != 0 && index != 1) || !value)
> -		return -EINVAL;
> -
> -	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> -				       NULL, value);
> -	if (ACPI_FAILURE(status))
> -		return -EIO;
> -
> -	return 0;
> -}
> -
> -/*
> - * sensor_set_auxtrip - set the new auxtrip value to sensor
> - * @handle: Object handle
> - * @index : GET_AUX1/GET_AUX0
> - * @value : The value will be set
> - */
> -static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> -{
> -	acpi_status status;
> -	union acpi_object arg = {
> -		ACPI_TYPE_INTEGER
> -	};
> -	struct acpi_object_list args = {
> -		1, &arg
> -	};
> -	unsigned long long temp;
> -
> -	if (index != 0 && index != 1)
> -		return -EINVAL;
> -
> -	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> -				       NULL, &temp);
> -	if (ACPI_FAILURE(status))
> -		return -EIO;
> -	if ((index && value < temp) || (!index && value > temp))
> -		return -EINVAL;
> -
> -	arg.integer.value = value;
> -	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> -				       &args, &temp);
> -	if (ACPI_FAILURE(status))
> -		return -EIO;
> -
> -	/* do we need to check the return value of SAX0/SAX1 ? */
> -
> -	return 0;
> -}
> -
> -#define to_intel_menlow_attr(_attr)	\
> -	container_of(_attr, struct intel_menlow_attribute, attr)
> -
> -static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
> -			char *buf, int idx)
> -{
> -	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> -	unsigned long long value;
> -	int result;
> -
> -	result = sensor_get_auxtrip(attr->handle, idx, &value);
> -	if (result)
> -		return result;
> -
> -	return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
> -}
> -
> -static ssize_t aux0_show(struct device *dev,
> -			 struct device_attribute *dev_attr, char *buf)
> -{
> -	return aux_show(dev, dev_attr, buf, 0);
> -}
> -
> -static ssize_t aux1_show(struct device *dev,
> -			 struct device_attribute *dev_attr, char *buf)
> -{
> -	return aux_show(dev, dev_attr, buf, 1);
> -}
> -
> -static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
> -			 const char *buf, size_t count, int idx)
> -{
> -	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> -	int value;
> -	int result;
> -
> -	/*Sanity check; should be a positive integer */
> -	if (!sscanf(buf, "%d", &value))
> -		return -EINVAL;
> -
> -	if (value < 0)
> -		return -EINVAL;
> -
> -	result = sensor_set_auxtrip(attr->handle, idx,
> -				    celsius_to_deci_kelvin(value));
> -	return result ? result : count;
> -}
> -
> -static ssize_t aux0_store(struct device *dev,
> -			  struct device_attribute *dev_attr,
> -			  const char *buf, size_t count)
> -{
> -	return aux_store(dev, dev_attr, buf, count, 0);
> -}
> -
> -static ssize_t aux1_store(struct device *dev,
> -			  struct device_attribute *dev_attr,
> -			  const char *buf, size_t count)
> -{
> -	return aux_store(dev, dev_attr, buf, count, 1);
> -}
> -
>   /* BIOS can enable/disable the thermal user application in dabney platform */
>   #define BIOS_ENABLED "\\_TZ.GSTS"
> -static ssize_t bios_enabled_show(struct device *dev,
> -				 struct device_attribute *attr, char *buf)
> -{
> -	acpi_status status;
> -	unsigned long long bios_enabled;
> -
> -	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
> -	if (ACPI_FAILURE(status))
> -		return -ENODEV;
> -
> -	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> -}
> -
> -static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> -					  void *store, struct device *dev,
> -					  acpi_handle handle)
> -{
> -	struct intel_menlow_attribute *attr;
> -	int result;
> -
> -	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
> -	sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
> -	attr->attr.attr.name = name;
> -	attr->attr.attr.mode = mode;
> -	attr->attr.show = show;
> -	attr->attr.store = store;
> -	attr->device = dev;
> -	attr->handle = handle;
> -
> -	result = device_create_file(dev, &attr->attr);
> -	if (result) {
> -		kfree(attr);
> -		return result;
> -	}
> -
> -	mutex_lock(&intel_menlow_attr_lock);
> -	list_add_tail(&attr->node, &intel_menlow_attr_list);
> -	mutex_unlock(&intel_menlow_attr_lock);
> -
> -	return 0;
> -}
>   
>   static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>   						void *context, void **rv)
> @@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>   	if (ACPI_FAILURE(status))
>   		return (status == AE_NOT_FOUND) ? AE_OK : status;
>   
> -	result = intel_menlow_add_one_attribute("aux0", 0644,
> -						aux0_show, aux0_store,
> -						&thermal->device, handle);
> -	if (result)
> -		return AE_ERROR;
> -
>   	status = acpi_get_handle(handle, GET_AUX1, &dummy);
>   	if (ACPI_FAILURE(status))
>   		goto aux1_not_found;
> @@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>   	if (ACPI_FAILURE(status))
>   		goto aux1_not_found;
>   
> -	result = intel_menlow_add_one_attribute("aux1", 0644,
> -						aux1_show, aux1_store,
> -						&thermal->device, handle);
> -	if (result) {
> -		intel_menlow_unregister_sensor();
> -		return AE_ERROR;
> -	}
> -
> -	/*
> -	 * create the "dabney_enabled" attribute which means the user app
> -	 * should be loaded or not
> -	 */
> -
> -	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> -						bios_enabled_show, NULL,
> -						&thermal->device, handle);
> -	if (result) {
> -		intel_menlow_unregister_sensor();
> -		return AE_ERROR;
> -	}
> -
>   	return AE_OK;
>   
>    aux1_not_found:

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Rafael J. Wysocki]

On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi,
>
> is this code removal acceptable ?

I'll let you know later this week.

> On 07/03/2023 14:37, Daniel Lezcano wrote:
> > The driver hooks the thermal framework sysfs to add some driver
> > specific information. A debatable approach as that may belong the
> > device sysfs directory, not the thermal zone directory.
> >
> > As the driver is accessing the thermal internals, we should provide at
> > least an API to the thermal framework to add an attribute to the
> > existing sysfs thermal zone entry.
> >
> > Before doing that and given the age of the driver (2008) may be it is
> > worth to double check if these attributes are really needed. So my
> > first proposal is to remove them if that does not hurt.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
>
>
> > ---
> >   drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
> >   1 file changed, 193 deletions(-)
> >
> > diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
> > index 5a6ad0552311..5a9738a93083 100644
> > --- a/drivers/thermal/intel/intel_menlow.c
> > +++ b/drivers/thermal/intel/intel_menlow.c
> > @@ -230,174 +230,8 @@ struct intel_menlow_attribute {
> >   static LIST_HEAD(intel_menlow_attr_list);
> >   static DEFINE_MUTEX(intel_menlow_attr_lock);
> >
> > -/*
> > - * sensor_get_auxtrip - get the current auxtrip value from sensor
> > - * @handle: Object handle
> > - * @index : GET_AUX1/GET_AUX0
> > - * @value : The address will be fill by the value
> > - */
> > -static int sensor_get_auxtrip(acpi_handle handle, int index,
> > -                                                     unsigned long long *value)
> > -{
> > -     acpi_status status;
> > -
> > -     if ((index != 0 && index != 1) || !value)
> > -             return -EINVAL;
> > -
> > -     status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> > -                                    NULL, value);
> > -     if (ACPI_FAILURE(status))
> > -             return -EIO;
> > -
> > -     return 0;
> > -}
> > -
> > -/*
> > - * sensor_set_auxtrip - set the new auxtrip value to sensor
> > - * @handle: Object handle
> > - * @index : GET_AUX1/GET_AUX0
> > - * @value : The value will be set
> > - */
> > -static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> > -{
> > -     acpi_status status;
> > -     union acpi_object arg = {
> > -             ACPI_TYPE_INTEGER
> > -     };
> > -     struct acpi_object_list args = {
> > -             1, &arg
> > -     };
> > -     unsigned long long temp;
> > -
> > -     if (index != 0 && index != 1)
> > -             return -EINVAL;
> > -
> > -     status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> > -                                    NULL, &temp);
> > -     if (ACPI_FAILURE(status))
> > -             return -EIO;
> > -     if ((index && value < temp) || (!index && value > temp))
> > -             return -EINVAL;
> > -
> > -     arg.integer.value = value;
> > -     status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> > -                                    &args, &temp);
> > -     if (ACPI_FAILURE(status))
> > -             return -EIO;
> > -
> > -     /* do we need to check the return value of SAX0/SAX1 ? */
> > -
> > -     return 0;
> > -}
> > -
> > -#define to_intel_menlow_attr(_attr)  \
> > -     container_of(_attr, struct intel_menlow_attribute, attr)
> > -
> > -static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
> > -                     char *buf, int idx)
> > -{
> > -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> > -     unsigned long long value;
> > -     int result;
> > -
> > -     result = sensor_get_auxtrip(attr->handle, idx, &value);
> > -     if (result)
> > -             return result;
> > -
> > -     return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
> > -}
> > -
> > -static ssize_t aux0_show(struct device *dev,
> > -                      struct device_attribute *dev_attr, char *buf)
> > -{
> > -     return aux_show(dev, dev_attr, buf, 0);
> > -}
> > -
> > -static ssize_t aux1_show(struct device *dev,
> > -                      struct device_attribute *dev_attr, char *buf)
> > -{
> > -     return aux_show(dev, dev_attr, buf, 1);
> > -}
> > -
> > -static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
> > -                      const char *buf, size_t count, int idx)
> > -{
> > -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> > -     int value;
> > -     int result;
> > -
> > -     /*Sanity check; should be a positive integer */
> > -     if (!sscanf(buf, "%d", &value))
> > -             return -EINVAL;
> > -
> > -     if (value < 0)
> > -             return -EINVAL;
> > -
> > -     result = sensor_set_auxtrip(attr->handle, idx,
> > -                                 celsius_to_deci_kelvin(value));
> > -     return result ? result : count;
> > -}
> > -
> > -static ssize_t aux0_store(struct device *dev,
> > -                       struct device_attribute *dev_attr,
> > -                       const char *buf, size_t count)
> > -{
> > -     return aux_store(dev, dev_attr, buf, count, 0);
> > -}
> > -
> > -static ssize_t aux1_store(struct device *dev,
> > -                       struct device_attribute *dev_attr,
> > -                       const char *buf, size_t count)
> > -{
> > -     return aux_store(dev, dev_attr, buf, count, 1);
> > -}
> > -
> >   /* BIOS can enable/disable the thermal user application in dabney platform */
> >   #define BIOS_ENABLED "\\_TZ.GSTS"
> > -static ssize_t bios_enabled_show(struct device *dev,
> > -                              struct device_attribute *attr, char *buf)
> > -{
> > -     acpi_status status;
> > -     unsigned long long bios_enabled;
> > -
> > -     status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
> > -     if (ACPI_FAILURE(status))
> > -             return -ENODEV;
> > -
> > -     return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> > -}
> > -
> > -static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> > -                                       void *store, struct device *dev,
> > -                                       acpi_handle handle)
> > -{
> > -     struct intel_menlow_attribute *attr;
> > -     int result;
> > -
> > -     attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> > -     if (!attr)
> > -             return -ENOMEM;
> > -
> > -     sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
> > -     attr->attr.attr.name = name;
> > -     attr->attr.attr.mode = mode;
> > -     attr->attr.show = show;
> > -     attr->attr.store = store;
> > -     attr->device = dev;
> > -     attr->handle = handle;
> > -
> > -     result = device_create_file(dev, &attr->attr);
> > -     if (result) {
> > -             kfree(attr);
> > -             return result;
> > -     }
> > -
> > -     mutex_lock(&intel_menlow_attr_lock);
> > -     list_add_tail(&attr->node, &intel_menlow_attr_list);
> > -     mutex_unlock(&intel_menlow_attr_lock);
> > -
> > -     return 0;
> > -}
> >
> >   static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >                                               void *context, void **rv)
> > @@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >       if (ACPI_FAILURE(status))
> >               return (status == AE_NOT_FOUND) ? AE_OK : status;
> >
> > -     result = intel_menlow_add_one_attribute("aux0", 0644,
> > -                                             aux0_show, aux0_store,
> > -                                             &thermal->device, handle);
> > -     if (result)
> > -             return AE_ERROR;
> > -
> >       status = acpi_get_handle(handle, GET_AUX1, &dummy);
> >       if (ACPI_FAILURE(status))
> >               goto aux1_not_found;
> > @@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >       if (ACPI_FAILURE(status))
> >               goto aux1_not_found;
> >
> > -     result = intel_menlow_add_one_attribute("aux1", 0644,
> > -                                             aux1_show, aux1_store,
> > -                                             &thermal->device, handle);
> > -     if (result) {
> > -             intel_menlow_unregister_sensor();
> > -             return AE_ERROR;
> > -     }
> > -
> > -     /*
> > -      * create the "dabney_enabled" attribute which means the user app
> > -      * should be loaded or not
> > -      */
> > -
> > -     result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> > -                                             bios_enabled_show, NULL,
> > -                                             &thermal->device, handle);
> > -     if (result) {
> > -             intel_menlow_unregister_sensor();
> > -             return AE_ERROR;
> > -     }
> > -
> >       return AE_OK;
> >
> >    aux1_not_found:
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Daniel Lezcano]

On 13/03/2023 13:26, Rafael J. Wysocki wrote:
> On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>>
>> Hi,
>>
>> is this code removal acceptable ?
> 
> I'll let you know later this week.

Great, thank you


>> On 07/03/2023 14:37, Daniel Lezcano wrote:
>>> The driver hooks the thermal framework sysfs to add some driver
>>> specific information. A debatable approach as that may belong the
>>> device sysfs directory, not the thermal zone directory.
>>>
>>> As the driver is accessing the thermal internals, we should provide at
>>> least an API to the thermal framework to add an attribute to the
>>> existing sysfs thermal zone entry.
>>>
>>> Before doing that and given the age of the driver (2008) may be it is
>>> worth to double check if these attributes are really needed. So my
>>> first proposal is to remove them if that does not hurt.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>>
>>
>>> ---
>>>    drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
>>>    1 file changed, 193 deletions(-)
>>>
>>> diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
>>> index 5a6ad0552311..5a9738a93083 100644
>>> --- a/drivers/thermal/intel/intel_menlow.c
>>> +++ b/drivers/thermal/intel/intel_menlow.c
>>> @@ -230,174 +230,8 @@ struct intel_menlow_attribute {
>>>    static LIST_HEAD(intel_menlow_attr_list);
>>>    static DEFINE_MUTEX(intel_menlow_attr_lock);
>>>
>>> -/*
>>> - * sensor_get_auxtrip - get the current auxtrip value from sensor
>>> - * @handle: Object handle
>>> - * @index : GET_AUX1/GET_AUX0
>>> - * @value : The address will be fill by the value
>>> - */
>>> -static int sensor_get_auxtrip(acpi_handle handle, int index,
>>> -                                                     unsigned long long *value)
>>> -{
>>> -     acpi_status status;
>>> -
>>> -     if ((index != 0 && index != 1) || !value)
>>> -             return -EINVAL;
>>> -
>>> -     status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
>>> -                                    NULL, value);
>>> -     if (ACPI_FAILURE(status))
>>> -             return -EIO;
>>> -
>>> -     return 0;
>>> -}
>>> -
>>> -/*
>>> - * sensor_set_auxtrip - set the new auxtrip value to sensor
>>> - * @handle: Object handle
>>> - * @index : GET_AUX1/GET_AUX0
>>> - * @value : The value will be set
>>> - */
>>> -static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
>>> -{
>>> -     acpi_status status;
>>> -     union acpi_object arg = {
>>> -             ACPI_TYPE_INTEGER
>>> -     };
>>> -     struct acpi_object_list args = {
>>> -             1, &arg
>>> -     };
>>> -     unsigned long long temp;
>>> -
>>> -     if (index != 0 && index != 1)
>>> -             return -EINVAL;
>>> -
>>> -     status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
>>> -                                    NULL, &temp);
>>> -     if (ACPI_FAILURE(status))
>>> -             return -EIO;
>>> -     if ((index && value < temp) || (!index && value > temp))
>>> -             return -EINVAL;
>>> -
>>> -     arg.integer.value = value;
>>> -     status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
>>> -                                    &args, &temp);
>>> -     if (ACPI_FAILURE(status))
>>> -             return -EIO;
>>> -
>>> -     /* do we need to check the return value of SAX0/SAX1 ? */
>>> -
>>> -     return 0;
>>> -}
>>> -
>>> -#define to_intel_menlow_attr(_attr)  \
>>> -     container_of(_attr, struct intel_menlow_attribute, attr)
>>> -
>>> -static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
>>> -                     char *buf, int idx)
>>> -{
>>> -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
>>> -     unsigned long long value;
>>> -     int result;
>>> -
>>> -     result = sensor_get_auxtrip(attr->handle, idx, &value);
>>> -     if (result)
>>> -             return result;
>>> -
>>> -     return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
>>> -}
>>> -
>>> -static ssize_t aux0_show(struct device *dev,
>>> -                      struct device_attribute *dev_attr, char *buf)
>>> -{
>>> -     return aux_show(dev, dev_attr, buf, 0);
>>> -}
>>> -
>>> -static ssize_t aux1_show(struct device *dev,
>>> -                      struct device_attribute *dev_attr, char *buf)
>>> -{
>>> -     return aux_show(dev, dev_attr, buf, 1);
>>> -}
>>> -
>>> -static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
>>> -                      const char *buf, size_t count, int idx)
>>> -{
>>> -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
>>> -     int value;
>>> -     int result;
>>> -
>>> -     /*Sanity check; should be a positive integer */
>>> -     if (!sscanf(buf, "%d", &value))
>>> -             return -EINVAL;
>>> -
>>> -     if (value < 0)
>>> -             return -EINVAL;
>>> -
>>> -     result = sensor_set_auxtrip(attr->handle, idx,
>>> -                                 celsius_to_deci_kelvin(value));
>>> -     return result ? result : count;
>>> -}
>>> -
>>> -static ssize_t aux0_store(struct device *dev,
>>> -                       struct device_attribute *dev_attr,
>>> -                       const char *buf, size_t count)
>>> -{
>>> -     return aux_store(dev, dev_attr, buf, count, 0);
>>> -}
>>> -
>>> -static ssize_t aux1_store(struct device *dev,
>>> -                       struct device_attribute *dev_attr,
>>> -                       const char *buf, size_t count)
>>> -{
>>> -     return aux_store(dev, dev_attr, buf, count, 1);
>>> -}
>>> -
>>>    /* BIOS can enable/disable the thermal user application in dabney platform */
>>>    #define BIOS_ENABLED "\\_TZ.GSTS"
>>> -static ssize_t bios_enabled_show(struct device *dev,
>>> -                              struct device_attribute *attr, char *buf)
>>> -{
>>> -     acpi_status status;
>>> -     unsigned long long bios_enabled;
>>> -
>>> -     status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
>>> -     if (ACPI_FAILURE(status))
>>> -             return -ENODEV;
>>> -
>>> -     return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
>>> -}
>>> -
>>> -static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
>>> -                                       void *store, struct device *dev,
>>> -                                       acpi_handle handle)
>>> -{
>>> -     struct intel_menlow_attribute *attr;
>>> -     int result;
>>> -
>>> -     attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
>>> -     if (!attr)
>>> -             return -ENOMEM;
>>> -
>>> -     sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
>>> -     attr->attr.attr.name = name;
>>> -     attr->attr.attr.mode = mode;
>>> -     attr->attr.show = show;
>>> -     attr->attr.store = store;
>>> -     attr->device = dev;
>>> -     attr->handle = handle;
>>> -
>>> -     result = device_create_file(dev, &attr->attr);
>>> -     if (result) {
>>> -             kfree(attr);
>>> -             return result;
>>> -     }
>>> -
>>> -     mutex_lock(&intel_menlow_attr_lock);
>>> -     list_add_tail(&attr->node, &intel_menlow_attr_list);
>>> -     mutex_unlock(&intel_menlow_attr_lock);
>>> -
>>> -     return 0;
>>> -}
>>>
>>>    static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>>>                                                void *context, void **rv)
>>> @@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>>>        if (ACPI_FAILURE(status))
>>>                return (status == AE_NOT_FOUND) ? AE_OK : status;
>>>
>>> -     result = intel_menlow_add_one_attribute("aux0", 0644,
>>> -                                             aux0_show, aux0_store,
>>> -                                             &thermal->device, handle);
>>> -     if (result)
>>> -             return AE_ERROR;
>>> -
>>>        status = acpi_get_handle(handle, GET_AUX1, &dummy);
>>>        if (ACPI_FAILURE(status))
>>>                goto aux1_not_found;
>>> @@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
>>>        if (ACPI_FAILURE(status))
>>>                goto aux1_not_found;
>>>
>>> -     result = intel_menlow_add_one_attribute("aux1", 0644,
>>> -                                             aux1_show, aux1_store,
>>> -                                             &thermal->device, handle);
>>> -     if (result) {
>>> -             intel_menlow_unregister_sensor();
>>> -             return AE_ERROR;
>>> -     }
>>> -
>>> -     /*
>>> -      * create the "dabney_enabled" attribute which means the user app
>>> -      * should be loaded or not
>>> -      */
>>> -
>>> -     result = intel_menlow_add_one_attribute("bios_enabled", 0444,
>>> -                                             bios_enabled_show, NULL,
>>> -                                             &thermal->device, handle);
>>> -     if (result) {
>>> -             intel_menlow_unregister_sensor();
>>> -             return AE_ERROR;
>>> -     }
>>> -
>>>        return AE_OK;
>>>
>>>     aux1_not_found:
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 10/11] thermal/core: Alloc-copy-free the thermal zone parameters structure

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.3-rc2 next-20230315]
[cannot apply to rafael-pm/thermal]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-drivers-intel_pch_thermal-Use-thermal-driver-device-to-write-a-trace/20230307-223759
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230307133735.90772-11-daniel.lezcano%40linaro.org
patch subject: [PATCH v1 10/11] thermal/core: Alloc-copy-free the thermal zone parameters structure
config: i386-randconfig-a011-20230313 (https://download.01.org/0day-ci/archive/20230315/202303152025.rVv9btko-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a9813bacf3531491cb02c13aafe358e6b5423aa0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Lezcano/thermal-drivers-intel_pch_thermal-Use-thermal-driver-device-to-write-a-trace/20230307-223759
        git checkout a9813bacf3531491cb02c13aafe358e6b5423aa0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/thermal/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303152025.rVv9btko-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/thermal/thermal_core.c:1267:7: warning: variable 'result' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                   if (!tz->tzp)
                       ^~~~~~~~
   drivers/thermal/thermal_core.c:1388:17: note: uninitialized use occurs here
           return ERR_PTR(result);
                          ^~~~~~
   drivers/thermal/thermal_core.c:1267:3: note: remove the 'if' if its condition is always false
                   if (!tz->tzp)
                   ^~~~~~~~~~~~~
   drivers/thermal/thermal_core.c:1217:12: note: initialize the variable 'result' to silence this warning
           int result;
                     ^
                      = 0
   1 warning generated.


vim +1267 drivers/thermal/thermal_core.c

  1183	
  1184	/**
  1185	 * thermal_zone_device_register_with_trips() - register a new thermal zone device
  1186	 * @type:	the thermal zone device type
  1187	 * @trips:	a pointer to an array of thermal trips
  1188	 * @num_trips:	the number of trip points the thermal zone support
  1189	 * @mask:	a bit string indicating the writeablility of trip points
  1190	 * @devdata:	private device data
  1191	 * @ops:	standard thermal zone device callbacks
  1192	 * @tzp:	thermal zone platform parameters
  1193	 * @passive_delay: number of milliseconds to wait between polls when
  1194	 *		   performing passive cooling
  1195	 * @polling_delay: number of milliseconds to wait between polls when checking
  1196	 *		   whether trip points have been crossed (0 for interrupt
  1197	 *		   driven systems)
  1198	 *
  1199	 * This interface function adds a new thermal zone device (sensor) to
  1200	 * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
  1201	 * thermal cooling devices registered at the same time.
  1202	 * thermal_zone_device_unregister() must be called when the device is no
  1203	 * longer needed. The passive cooling depends on the .get_trend() return value.
  1204	 *
  1205	 * Return: a pointer to the created struct thermal_zone_device or an
  1206	 * in case of error, an ERR_PTR. Caller must check return value with
  1207	 * IS_ERR*() helpers.
  1208	 */
  1209	struct thermal_zone_device *
  1210	thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
  1211						void *devdata, struct thermal_zone_device_ops *ops,
  1212						struct thermal_zone_params *tzp, int passive_delay,
  1213						int polling_delay)
  1214	{
  1215		struct thermal_zone_device *tz;
  1216		int id;
  1217		int result;
  1218		int count;
  1219		struct thermal_governor *governor;
  1220	
  1221		if (!type || strlen(type) == 0) {
  1222			pr_err("No thermal zone type defined\n");
  1223			return ERR_PTR(-EINVAL);
  1224		}
  1225	
  1226		if (strlen(type) >= THERMAL_NAME_LENGTH) {
  1227			pr_err("Thermal zone name (%s) too long, should be under %d chars\n",
  1228			       type, THERMAL_NAME_LENGTH);
  1229			return ERR_PTR(-EINVAL);
  1230		}
  1231	
  1232		/*
  1233		 * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
  1234		 * For example, shifting by 32 will result in compiler warning:
  1235		 * warning: right shift count >= width of type [-Wshift-count- overflow]
  1236		 *
  1237		 * Also "mask >> num_trips" will always be true with 32 bit shift.
  1238		 * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
  1239		 * mask >> 32 = 0x80000000
  1240		 * This will result in failure for the below condition.
  1241		 *
  1242		 * Check will be true when the bit 31 of the mask is set.
  1243		 * 32 bit shift will cause overflow of 4 byte integer.
  1244		 */
  1245		if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
  1246			pr_err("Incorrect number of thermal trips\n");
  1247			return ERR_PTR(-EINVAL);
  1248		}
  1249	
  1250		if (!ops) {
  1251			pr_err("Thermal zone device ops not defined\n");
  1252			return ERR_PTR(-EINVAL);
  1253		}
  1254	
  1255		if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
  1256			return ERR_PTR(-EINVAL);
  1257	
  1258		if (!thermal_class)
  1259			return ERR_PTR(-ENODEV);
  1260	
  1261		tz = kzalloc(sizeof(*tz), GFP_KERNEL);
  1262		if (!tz)
  1263			return ERR_PTR(-ENOMEM);
  1264	
  1265		if (tzp) {
  1266			tz->tzp = kmemdup(tzp, sizeof(*tzp), GFP_KERNEL);
> 1267			if (!tz->tzp)
  1268				goto free_tz;
  1269		}
  1270		
  1271		INIT_LIST_HEAD(&tz->thermal_instances);
  1272		ida_init(&tz->ida);
  1273		mutex_init(&tz->lock);
  1274		id = ida_alloc(&thermal_tz_ida, GFP_KERNEL);
  1275		if (id < 0) {
  1276			result = id;
  1277			goto free_tzp;
  1278		}
  1279	
  1280		tz->id = id;
  1281		strscpy(tz->type, type, sizeof(tz->type));
  1282	
  1283		if (!ops->critical)
  1284			ops->critical = thermal_zone_device_critical;
  1285	
  1286		tz->ops = ops;
  1287		tz->device.class = thermal_class;
  1288		tz->devdata = devdata;
  1289		tz->trips = trips;
  1290		tz->num_trips = num_trips;
  1291	
  1292		thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
  1293		thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
  1294	
  1295		/* sys I/F */
  1296		/* Add nodes that are always present via .groups */
  1297		result = thermal_zone_create_device_groups(tz, mask);
  1298		if (result)
  1299			goto remove_id;
  1300	
  1301		/* A new thermal zone needs to be updated anyway. */
  1302		atomic_set(&tz->need_update, 1);
  1303	
  1304		result = dev_set_name(&tz->device, "thermal_zone%d", tz->id);
  1305		if (result) {
  1306			thermal_zone_destroy_device_groups(tz);
  1307			goto remove_id;
  1308		}
  1309		result = device_register(&tz->device);
  1310		if (result)
  1311			goto release_device;
  1312	
  1313		for (count = 0; count < num_trips; count++) {
  1314			struct thermal_trip trip;
  1315	
  1316			result = thermal_zone_get_trip(tz, count, &trip);
  1317			if (result)
  1318				set_bit(count, &tz->trips_disabled);
  1319		}
  1320	
  1321		/* Update 'this' zone's governor information */
  1322		mutex_lock(&thermal_governor_lock);
  1323	
  1324		if (tz->tzp)
  1325			governor = __find_governor(tz->tzp->governor_name);
  1326		else
  1327			governor = def_governor;
  1328	
  1329		result = thermal_set_governor(tz, governor);
  1330		if (result) {
  1331			mutex_unlock(&thermal_governor_lock);
  1332			goto unregister;
  1333		}
  1334	
  1335		mutex_unlock(&thermal_governor_lock);
  1336	
  1337		if (!tz->tzp || !tz->tzp->no_hwmon) {
  1338			result = thermal_add_hwmon_sysfs(tz);
  1339			if (result)
  1340				goto unregister;
  1341		}
  1342	
  1343		mutex_lock(&thermal_list_lock);
  1344		list_add_tail(&tz->node, &thermal_tz_list);
  1345		mutex_unlock(&thermal_list_lock);
  1346	
  1347		if (tzp && tzp->linked_dev) {
  1348			result = sysfs_create_link(&tzp->linked_dev->kobj,
  1349						   &tz->device.kobj, "thermal_zone");
  1350			if (result)
  1351				goto out_list_del;
  1352	
  1353			result = sysfs_create_link(&tz->device.kobj,
  1354						   &tzp->linked_dev->kobj, "device");
  1355			if (result)
  1356				goto out_del_link;
  1357		}
  1358	
  1359		/* Bind cooling devices for this zone */
  1360		bind_tz(tz);
  1361	
  1362		INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
  1363	
  1364		thermal_zone_device_init(tz);
  1365		/* Update the new thermal zone and mark it as already updated. */
  1366		if (atomic_cmpxchg(&tz->need_update, 1, 0))
  1367			thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
  1368	
  1369		thermal_notify_tz_create(tz->id, tz->type);
  1370	
  1371		return tz;
  1372	
  1373	out_del_link:
  1374		sysfs_remove_link(&tz->device.kobj, "thermal_zone");
  1375	out_list_del:
  1376		list_del(&tz->node);
  1377	unregister:
  1378		device_del(&tz->device);
  1379	release_device:
  1380		put_device(&tz->device);
  1381		tz = NULL;
  1382	remove_id:
  1383		ida_free(&thermal_tz_ida, id);
  1384	free_tzp:
  1385		kfree(tz->tzp);
  1386	free_tz:
  1387		kfree(tz);
  1388		return ERR_PTR(result);
  1389	}
  1390	EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
  1391	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Rafael J. Wysocki]

On Mon, Mar 13, 2023 at 1:35 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 13/03/2023 13:26, Rafael J. Wysocki wrote:
> > On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >>
> >> Hi,
> >>
> >> is this code removal acceptable ?
> >
> > I'll let you know later this week.
>
> Great, thank you

So it would be acceptable if it had no users, but that's somewhat hard
to establish.

As I wrote in a reply to the RFC version of this, I'd rather make
these attributes depend on a Kconfig option or a module parameter
before removing them completely.

> >> On 07/03/2023 14:37, Daniel Lezcano wrote:
> >>> The driver hooks the thermal framework sysfs to add some driver
> >>> specific information. A debatable approach as that may belong the
> >>> device sysfs directory, not the thermal zone directory.
> >>>
> >>> As the driver is accessing the thermal internals, we should provide at
> >>> least an API to the thermal framework to add an attribute to the
> >>> existing sysfs thermal zone entry.
> >>>
> >>> Before doing that and given the age of the driver (2008) may be it is
> >>> worth to double check if these attributes are really needed. So my
> >>> first proposal is to remove them if that does not hurt.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>
> >>
> >>
> >>> ---
> >>>    drivers/thermal/intel/intel_menlow.c | 193 ---------------------------
> >>>    1 file changed, 193 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
> >>> index 5a6ad0552311..5a9738a93083 100644
> >>> --- a/drivers/thermal/intel/intel_menlow.c
> >>> +++ b/drivers/thermal/intel/intel_menlow.c
> >>> @@ -230,174 +230,8 @@ struct intel_menlow_attribute {
> >>>    static LIST_HEAD(intel_menlow_attr_list);
> >>>    static DEFINE_MUTEX(intel_menlow_attr_lock);
> >>>
> >>> -/*
> >>> - * sensor_get_auxtrip - get the current auxtrip value from sensor
> >>> - * @handle: Object handle
> >>> - * @index : GET_AUX1/GET_AUX0
> >>> - * @value : The address will be fill by the value
> >>> - */
> >>> -static int sensor_get_auxtrip(acpi_handle handle, int index,
> >>> -                                                     unsigned long long *value)
> >>> -{
> >>> -     acpi_status status;
> >>> -
> >>> -     if ((index != 0 && index != 1) || !value)
> >>> -             return -EINVAL;
> >>> -
> >>> -     status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> >>> -                                    NULL, value);
> >>> -     if (ACPI_FAILURE(status))
> >>> -             return -EIO;
> >>> -
> >>> -     return 0;
> >>> -}
> >>> -
> >>> -/*
> >>> - * sensor_set_auxtrip - set the new auxtrip value to sensor
> >>> - * @handle: Object handle
> >>> - * @index : GET_AUX1/GET_AUX0
> >>> - * @value : The value will be set
> >>> - */
> >>> -static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> >>> -{
> >>> -     acpi_status status;
> >>> -     union acpi_object arg = {
> >>> -             ACPI_TYPE_INTEGER
> >>> -     };
> >>> -     struct acpi_object_list args = {
> >>> -             1, &arg
> >>> -     };
> >>> -     unsigned long long temp;
> >>> -
> >>> -     if (index != 0 && index != 1)
> >>> -             return -EINVAL;
> >>> -
> >>> -     status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> >>> -                                    NULL, &temp);
> >>> -     if (ACPI_FAILURE(status))
> >>> -             return -EIO;
> >>> -     if ((index && value < temp) || (!index && value > temp))
> >>> -             return -EINVAL;
> >>> -
> >>> -     arg.integer.value = value;
> >>> -     status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> >>> -                                    &args, &temp);
> >>> -     if (ACPI_FAILURE(status))
> >>> -             return -EIO;
> >>> -
> >>> -     /* do we need to check the return value of SAX0/SAX1 ? */
> >>> -
> >>> -     return 0;
> >>> -}
> >>> -
> >>> -#define to_intel_menlow_attr(_attr)  \
> >>> -     container_of(_attr, struct intel_menlow_attribute, attr)
> >>> -
> >>> -static ssize_t aux_show(struct device *dev, struct device_attribute *dev_attr,
> >>> -                     char *buf, int idx)
> >>> -{
> >>> -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> >>> -     unsigned long long value;
> >>> -     int result;
> >>> -
> >>> -     result = sensor_get_auxtrip(attr->handle, idx, &value);
> >>> -     if (result)
> >>> -             return result;
> >>> -
> >>> -     return sprintf(buf, "%lu", deci_kelvin_to_celsius(value));
> >>> -}
> >>> -
> >>> -static ssize_t aux0_show(struct device *dev,
> >>> -                      struct device_attribute *dev_attr, char *buf)
> >>> -{
> >>> -     return aux_show(dev, dev_attr, buf, 0);
> >>> -}
> >>> -
> >>> -static ssize_t aux1_show(struct device *dev,
> >>> -                      struct device_attribute *dev_attr, char *buf)
> >>> -{
> >>> -     return aux_show(dev, dev_attr, buf, 1);
> >>> -}
> >>> -
> >>> -static ssize_t aux_store(struct device *dev, struct device_attribute *dev_attr,
> >>> -                      const char *buf, size_t count, int idx)
> >>> -{
> >>> -     struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> >>> -     int value;
> >>> -     int result;
> >>> -
> >>> -     /*Sanity check; should be a positive integer */
> >>> -     if (!sscanf(buf, "%d", &value))
> >>> -             return -EINVAL;
> >>> -
> >>> -     if (value < 0)
> >>> -             return -EINVAL;
> >>> -
> >>> -     result = sensor_set_auxtrip(attr->handle, idx,
> >>> -                                 celsius_to_deci_kelvin(value));
> >>> -     return result ? result : count;
> >>> -}
> >>> -
> >>> -static ssize_t aux0_store(struct device *dev,
> >>> -                       struct device_attribute *dev_attr,
> >>> -                       const char *buf, size_t count)
> >>> -{
> >>> -     return aux_store(dev, dev_attr, buf, count, 0);
> >>> -}
> >>> -
> >>> -static ssize_t aux1_store(struct device *dev,
> >>> -                       struct device_attribute *dev_attr,
> >>> -                       const char *buf, size_t count)
> >>> -{
> >>> -     return aux_store(dev, dev_attr, buf, count, 1);
> >>> -}
> >>> -
> >>>    /* BIOS can enable/disable the thermal user application in dabney platform */
> >>>    #define BIOS_ENABLED "\\_TZ.GSTS"
> >>> -static ssize_t bios_enabled_show(struct device *dev,
> >>> -                              struct device_attribute *attr, char *buf)
> >>> -{
> >>> -     acpi_status status;
> >>> -     unsigned long long bios_enabled;
> >>> -
> >>> -     status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
> >>> -     if (ACPI_FAILURE(status))
> >>> -             return -ENODEV;
> >>> -
> >>> -     return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> >>> -}
> >>> -
> >>> -static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> >>> -                                       void *store, struct device *dev,
> >>> -                                       acpi_handle handle)
> >>> -{
> >>> -     struct intel_menlow_attribute *attr;
> >>> -     int result;
> >>> -
> >>> -     attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> >>> -     if (!attr)
> >>> -             return -ENOMEM;
> >>> -
> >>> -     sysfs_attr_init(&attr->attr.attr); /* That is consistent naming :D */
> >>> -     attr->attr.attr.name = name;
> >>> -     attr->attr.attr.mode = mode;
> >>> -     attr->attr.show = show;
> >>> -     attr->attr.store = store;
> >>> -     attr->device = dev;
> >>> -     attr->handle = handle;
> >>> -
> >>> -     result = device_create_file(dev, &attr->attr);
> >>> -     if (result) {
> >>> -             kfree(attr);
> >>> -             return result;
> >>> -     }
> >>> -
> >>> -     mutex_lock(&intel_menlow_attr_lock);
> >>> -     list_add_tail(&attr->node, &intel_menlow_attr_list);
> >>> -     mutex_unlock(&intel_menlow_attr_lock);
> >>> -
> >>> -     return 0;
> >>> -}
> >>>
> >>>    static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>>                                                void *context, void **rv)
> >>> @@ -420,12 +254,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>>        if (ACPI_FAILURE(status))
> >>>                return (status == AE_NOT_FOUND) ? AE_OK : status;
> >>>
> >>> -     result = intel_menlow_add_one_attribute("aux0", 0644,
> >>> -                                             aux0_show, aux0_store,
> >>> -                                             &thermal->device, handle);
> >>> -     if (result)
> >>> -             return AE_ERROR;
> >>> -
> >>>        status = acpi_get_handle(handle, GET_AUX1, &dummy);
> >>>        if (ACPI_FAILURE(status))
> >>>                goto aux1_not_found;
> >>> @@ -434,27 +262,6 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> >>>        if (ACPI_FAILURE(status))
> >>>                goto aux1_not_found;
> >>>
> >>> -     result = intel_menlow_add_one_attribute("aux1", 0644,
> >>> -                                             aux1_show, aux1_store,
> >>> -                                             &thermal->device, handle);
> >>> -     if (result) {
> >>> -             intel_menlow_unregister_sensor();
> >>> -             return AE_ERROR;
> >>> -     }
> >>> -
> >>> -     /*
> >>> -      * create the "dabney_enabled" attribute which means the user app
> >>> -      * should be loaded or not
> >>> -      */
> >>> -
> >>> -     result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> >>> -                                             bios_enabled_show, NULL,
> >>> -                                             &thermal->device, handle);
> >>> -     if (result) {
> >>> -             intel_menlow_unregister_sensor();
> >>> -             return AE_ERROR;
> >>> -     }
> >>> -
> >>>        return AE_OK;
> >>>
> >>>     aux1_not_found:
> >>
> >> --
> >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> >> <http://twitter.com/#!/linaroorg> Twitter |
> >> <http://www.linaro.org/linaro-blog/> Blog
> >>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH v1 10/11] thermal/core: Alloc-copy-free the thermal zone parameters structure

[Dan Carpenter]

Hi Daniel,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Lezcano/thermal-drivers-intel_pch_thermal-Use-thermal-driver-device-to-write-a-trace/20230307-223759
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230307133735.90772-11-daniel.lezcano%40linaro.org
patch subject: [PATCH v1 10/11] thermal/core: Alloc-copy-free the thermal zone parameters structure
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20230316/202303160005.1BnLbm4F-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303160005.1BnLbm4F-lkp@intel.com/

smatch warnings:
drivers/thermal/thermal_core.c:1388 thermal_zone_device_register_with_trips() error: uninitialized symbol 'result'.

vim +/result +1388 drivers/thermal/thermal_core.c

eb7be329bd93b7 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1209  struct thermal_zone_device *
fae11de507f0e4 drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1210  thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
eb7be329bd93b7 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1211  					void *devdata, struct thermal_zone_device_ops *ops,
eb7be329bd93b7 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1212  					struct thermal_zone_params *tzp, int passive_delay,
eb7be329bd93b7 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1213  					int polling_delay)
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1214  {
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1215  	struct thermal_zone_device *tz;
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1216  	int id;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1217  	int result;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1218  	int count;
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1219  	struct thermal_governor *governor;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1220  
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1221  	if (!type || strlen(type) == 0) {
3f95ac324535ea drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1222  		pr_err("No thermal zone type defined\n");
54fa38cc2eda43 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1223  		return ERR_PTR(-EINVAL);
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1224  	}
54fa38cc2eda43 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1225  
c71d8035f1b77d drivers/thermal/thermal_core.c Lad Prabhakar       2022-09-09  1226  	if (strlen(type) >= THERMAL_NAME_LENGTH) {
3f95ac324535ea drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1227  		pr_err("Thermal zone name (%s) too long, should be under %d chars\n",
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1228  		       type, THERMAL_NAME_LENGTH);
3e6fda5c115982 drivers/thermal/thermal.c      Thomas Sujith       2008-02-15  1229  		return ERR_PTR(-EINVAL);
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1230  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1231  
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1232  	/*
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1233  	 * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1234  	 * For example, shifting by 32 will result in compiler warning:
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1235  	 * warning: right shift count >= width of type [-Wshift-count- overflow]
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1236  	 *
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1237  	 * Also "mask >> num_trips" will always be true with 32 bit shift.
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1238  	 * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1239  	 * mask >> 32 = 0x80000000
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1240  	 * This will result in failure for the below condition.
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1241  	 *
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1242  	 * Check will be true when the bit 31 of the mask is set.
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1243  	 * 32 bit shift will cause overflow of 4 byte integer.
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1244  	 */
82b1ec794d7014 drivers/thermal/thermal_core.c Sumeet Pawnikar     2022-09-27  1245  	if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
3f95ac324535ea drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1246  		pr_err("Incorrect number of thermal trips\n");
3e6fda5c115982 drivers/thermal/thermal.c      Thomas Sujith       2008-02-15  1247  		return ERR_PTR(-EINVAL);
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1248  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1249  
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1250  	if (!ops) {
3f95ac324535ea drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1251  		pr_err("Thermal zone device ops not defined\n");
3e6fda5c115982 drivers/thermal/thermal.c      Thomas Sujith       2008-02-15  1252  		return ERR_PTR(-EINVAL);
67eed44b8a8ae7 drivers/thermal/thermal_core.c Amit Kucheria       2019-07-12  1253  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1254  
7c3d5c20dc169e drivers/thermal/thermal_core.c Daniel Lezcano      2022-10-03  1255  	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
6b2aa51d698492 drivers/thermal/thermal_sys.c  Eduardo Valentin    2013-01-02  1256  		return ERR_PTR(-EINVAL);
6b2aa51d698492 drivers/thermal/thermal_sys.c  Eduardo Valentin    2013-01-02  1257  
9e0a9be24bdd61 drivers/thermal/thermal_core.c Rafael J. Wysocki   2023-01-23  1258  	if (!thermal_class)
9e0a9be24bdd61 drivers/thermal/thermal_core.c Rafael J. Wysocki   2023-01-23  1259  		return ERR_PTR(-ENODEV);
9e0a9be24bdd61 drivers/thermal/thermal_core.c Rafael J. Wysocki   2023-01-23  1260  
95e3ed1513494a drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1261  	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1262  	if (!tz)
3e6fda5c115982 drivers/thermal/thermal.c      Thomas Sujith       2008-02-15  1263  		return ERR_PTR(-ENOMEM);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1264  
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1265  	if (tzp) {
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1266  		tz->tzp = kmemdup(tzp, sizeof(*tzp), GFP_KERNEL);
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1267  		if (!tz->tzp)
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1268  			goto free_tz;

result = -ENOMEM;

a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1269  	}
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1270  	
2d374139d5b0b5 drivers/thermal/thermal_sys.c  Zhang Rui           2012-06-27  1271  	INIT_LIST_HEAD(&tz->thermal_instances);
b31ef8285b19ec drivers/thermal/thermal_core.c Matthew Wilcox      2016-12-21  1272  	ida_init(&tz->ida);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1273  	mutex_init(&tz->lock);
5a5b7d8d541684 drivers/thermal/thermal_core.c keliu               2022-05-27  1274  	id = ida_alloc(&thermal_tz_ida, GFP_KERNEL);
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1275  	if (id < 0) {
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1276  		result = id;
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1277  		goto free_tzp;
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1278  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1279  
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1280  	tz->id = id;
1e6c8fb8b8d3e9 drivers/thermal/thermal_core.c Wolfram Sang        2022-08-18  1281  	strscpy(tz->type, type, sizeof(tz->type));
d7203eedf4f68e drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-10  1282  
d7203eedf4f68e drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-10  1283  	if (!ops->critical)
d7203eedf4f68e drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-10  1284  		ops->critical = thermal_zone_device_critical;
d7203eedf4f68e drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-10  1285  
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1286  	tz->ops = ops;
9e0a9be24bdd61 drivers/thermal/thermal_core.c Rafael J. Wysocki   2023-01-23  1287  	tz->device.class = thermal_class;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1288  	tz->devdata = devdata;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1289  	tz->trips = trips;
e5bfcd30f88fdb drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1290  	tz->num_trips = num_trips;
1c600861fa6fd8 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1291  
17d399cd9c8936 drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-16  1292  	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
17d399cd9c8936 drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-16  1293  	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
17d399cd9c8936 drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-16  1294  
4d0fe7490d7f4d drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1295  	/* sys I/F */
1c600861fa6fd8 drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1296  	/* Add nodes that are always present via .groups */
4d0fe7490d7f4d drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1297  	result = thermal_zone_create_device_groups(tz, mask);
4d0fe7490d7f4d drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1298  	if (result)
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08  1299  		goto remove_id;
4d0fe7490d7f4d drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1300  
4511f7166a2deb drivers/thermal/thermal_core.c Chen Yu             2015-10-30  1301  	/* A new thermal zone needs to be updated anyway. */
4511f7166a2deb drivers/thermal/thermal_core.c Chen Yu             2015-10-30  1302  	atomic_set(&tz->need_update, 1);
b1569e99c795bf drivers/thermal/thermal_sys.c  Matthew Garrett     2008-12-03  1303  
4748f9687caaee drivers/thermal/thermal_core.c Yang Yingliang      2022-11-15  1304  	result = dev_set_name(&tz->device, "thermal_zone%d", tz->id);
4748f9687caaee drivers/thermal/thermal_core.c Yang Yingliang      2022-11-15  1305  	if (result) {
4748f9687caaee drivers/thermal/thermal_core.c Yang Yingliang      2022-11-15  1306  		thermal_zone_destroy_device_groups(tz);
4748f9687caaee drivers/thermal/thermal_core.c Yang Yingliang      2022-11-15  1307  		goto remove_id;
4748f9687caaee drivers/thermal/thermal_core.c Yang Yingliang      2022-11-15  1308  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1309  	result = device_register(&tz->device);
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08  1310  	if (result)
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1311  		goto release_device;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1312  
e5bfcd30f88fdb drivers/thermal/thermal_core.c Daniel Lezcano      2022-07-22  1313  	for (count = 0; count < num_trips; count++) {
7c3d5c20dc169e drivers/thermal/thermal_core.c Daniel Lezcano      2022-10-03  1314  		struct thermal_trip trip;
7c3d5c20dc169e drivers/thermal/thermal_core.c Daniel Lezcano      2022-10-03  1315  
7c3d5c20dc169e drivers/thermal/thermal_core.c Daniel Lezcano      2022-10-03  1316  		result = thermal_zone_get_trip(tz, count, &trip);
7c3d5c20dc169e drivers/thermal/thermal_core.c Daniel Lezcano      2022-10-03  1317  		if (result)
81ad4276b505e9 drivers/thermal/thermal_core.c Zhang Rui           2016-03-18  1318  			set_bit(count, &tz->trips_disabled);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1319  	}
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1320  
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1321  	/* Update 'this' zone's governor information */
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1322  	mutex_lock(&thermal_governor_lock);
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1323  
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1324  	if (tz->tzp)
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1325  		governor = __find_governor(tz->tzp->governor_name);
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1326  	else
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1327  		governor = def_governor;
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1328  
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1329  	result = thermal_set_governor(tz, governor);
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1330  	if (result) {
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1331  		mutex_unlock(&thermal_governor_lock);
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1332  		goto unregister;
e33df1d2f3a014 drivers/thermal/thermal_core.c Javi Merino         2015-02-26  1333  	}
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1334  
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1335  	mutex_unlock(&thermal_governor_lock);
a4a15485fbba44 drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1336  
ccba4ffd9eff61 drivers/thermal/thermal_core.c Eduardo Valentin    2013-08-15  1337  	if (!tz->tzp || !tz->tzp->no_hwmon) {
e68b16abd91dca drivers/thermal/thermal.c      Zhang Rui           2008-04-21  1338  		result = thermal_add_hwmon_sysfs(tz);
e68b16abd91dca drivers/thermal/thermal.c      Zhang Rui           2008-04-21  1339  		if (result)
e68b16abd91dca drivers/thermal/thermal.c      Zhang Rui           2008-04-21  1340  			goto unregister;
ccba4ffd9eff61 drivers/thermal/thermal_core.c Eduardo Valentin    2013-08-15  1341  	}
e68b16abd91dca drivers/thermal/thermal.c      Zhang Rui           2008-04-21  1342  
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1343  	mutex_lock(&thermal_list_lock);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1344  	list_add_tail(&tz->node, &thermal_tz_list);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1345  	mutex_unlock(&thermal_list_lock);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1346  
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1347  	if (tzp && tzp->linked_dev) {
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1348  		result = sysfs_create_link(&tzp->linked_dev->kobj,
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1349  					   &tz->device.kobj, "thermal_zone");
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1350  		if (result)
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1351  			goto out_list_del;
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1352  
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1353  		result = sysfs_create_link(&tz->device.kobj,
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1354  					   &tzp->linked_dev->kobj, "device");
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1355  		if (result)
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1356  			goto out_del_link;
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1357  	}
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1358  
7e8ee1e9d7561f drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1359  	/* Bind cooling devices for this zone */
7e8ee1e9d7561f drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1360  	bind_tz(tz);
7e8ee1e9d7561f drivers/thermal/thermal_sys.c  Durgadoss R         2012-09-18  1361  
b659a30d7bdd5d drivers/thermal/thermal_core.c Eduardo Valentin    2016-11-07  1362  	INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
b1569e99c795bf drivers/thermal/thermal_sys.c  Matthew Garrett     2008-12-03  1363  
d0df264fbd3c53 drivers/thermal/thermal_core.c Daniel Lezcano      2020-12-22  1364  	thermal_zone_device_init(tz);
4511f7166a2deb drivers/thermal/thermal_core.c Chen Yu             2015-10-30  1365  	/* Update the new thermal zone and mark it as already updated. */
4511f7166a2deb drivers/thermal/thermal_core.c Chen Yu             2015-10-30  1366  	if (atomic_cmpxchg(&tz->need_update, 1, 0))
0e70f466fb910a drivers/thermal/thermal_core.c Srinivas Pandruvada 2016-08-26  1367  		thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
b1569e99c795bf drivers/thermal/thermal_sys.c  Matthew Garrett     2008-12-03  1368  
55cdf0a283b876 drivers/thermal/thermal_core.c Daniel Lezcano      2020-07-06  1369  	thermal_notify_tz_create(tz->id, tz->type);
55cdf0a283b876 drivers/thermal/thermal_core.c Daniel Lezcano      2020-07-06  1370  
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1371  	return tz;
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1372  
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1373  out_del_link:
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1374  	sysfs_remove_link(&tz->device.kobj, "thermal_zone");
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1375  out_list_del:
70edc3ef596502 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1376  	list_del(&tz->node);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1377  unregister:
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1378  	device_del(&tz->device);
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1379  release_device:
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1380  	put_device(&tz->device);
adc8749b150c51 drivers/thermal/thermal_core.c Yue Hu              2019-08-07  1381  	tz = NULL;
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08  1382  remove_id:
5a5b7d8d541684 drivers/thermal/thermal_core.c keliu               2022-05-27  1383  	ida_free(&thermal_tz_ida, id);
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1384  free_tzp:
a9813bacf35314 drivers/thermal/thermal_core.c Daniel Lezcano      2023-03-07  1385  	kfree(tz->tzp);
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08  1386  free_tz:
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08  1387  	kfree(tz);
9d9ca1f9f04cf1 drivers/thermal/thermal_core.c Christophe Jaillet  2017-08-08 @1388  	return ERR_PTR(result);
203d3d4aa48233 drivers/thermal/thermal.c      Zhang Rui           2008-01-17  1389  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v1 09/11] thermal/core: Add a linked device parameter

[Rafael J. Wysocki]

On Tue, Mar 7, 2023 at 2:38 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> Some drivers want to create a link from the thermal zone to the device
> sysfs entry and vice versa.

Which device is this, exactly?

> That is the case of the APCI driver.
>
> Having a backpointer from the device to the thermal zone sounds akward
> as we can have the same device instantiating multiple thermal zones so
> there will be a conflict while creating the second link with the same
> name. Moreover, the userspace has enough information to build the
> dependency from the thermal zone device link without having this cyclic
> link from the device to thermal zone.
>
> Anyway, everything in its time.
>
> This change allows to create a these cyclic links tz <-> device as
> ACPI does and will allow to remove the code in the ACPI driver.

Well, I'd rather have it in the driver than in the core TBH.

If ACPI is the only user of this, let it do the dirty thing by itself.

There are two cases which would justify making this change:
1. There will be more users of it going forward (seems unlikely from
the description).
2. It gets in the way of some other changes somehow.

I kind of expect 2. to be the case, so how does it get in the way?

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Daniel Lezcano]

On 17/03/2023 19:18, Rafael J. Wysocki wrote:
> On Mon, Mar 13, 2023 at 1:35 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 13/03/2023 13:26, Rafael J. Wysocki wrote:
>>> On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> is this code removal acceptable ?
>>>
>>> I'll let you know later this week.
>>
>> Great, thank you
> 
> So it would be acceptable if it had no users, but that's somewhat hard
> to establish.
> 
> As I wrote in a reply to the RFC version of this, I'd rather make
> these attributes depend on a Kconfig option or a module parameter
> before removing them completely.

Do you mean we set the default to false and see if there are complaints? 
If not after awhile, we remove the code ?

[ ... ]

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 03/11] thermal/drivers/intel_menlow: Remove add_one_attribute

[Rafael J. Wysocki]

On Tue, Apr 4, 2023 at 8:13 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 17/03/2023 19:18, Rafael J. Wysocki wrote:
> > On Mon, Mar 13, 2023 at 1:35 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 13/03/2023 13:26, Rafael J. Wysocki wrote:
> >>> On Mon, Mar 13, 2023 at 11:55 AM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> is this code removal acceptable ?
> >>>
> >>> I'll let you know later this week.
> >>
> >> Great, thank you
> >
> > So it would be acceptable if it had no users, but that's somewhat hard
> > to establish.
> >
> > As I wrote in a reply to the RFC version of this, I'd rather make
> > these attributes depend on a Kconfig option or a module parameter
> > before removing them completely.
>
> Do you mean we set the default to false and see if there are complaints?
> If not after awhile, we remove the code ?

Yes.  That's the idea at least.

Re: [PATCH v1 09/11] thermal/core: Add a linked device parameter

[Daniel Lezcano]

On 27/03/2023 18:16, Rafael J. Wysocki wrote:
> On Tue, Mar 7, 2023 at 2:38 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> Some drivers want to create a link from the thermal zone to the device
>> sysfs entry and vice versa.
> 
> Which device is this, exactly?

ls -alh /sys/bus/acpi/drivers/thermal/LNXTHERM:00/

[ ... ]

lrwxrwxrwx    1         0 thermal_zone -> 
../../../virtual/thermal/thermal_zone0

[ ... ]

The ACPI driver is the only one doing this AFAICT.


>> That is the case of the APCI driver.
>>
>> Having a backpointer from the device to the thermal zone sounds akward
>> as we can have the same device instantiating multiple thermal zones so
>> there will be a conflict while creating the second link with the same
>> name. Moreover, the userspace has enough information to build the
>> dependency from the thermal zone device link without having this cyclic
>> link from the device to thermal zone.
>>
>> Anyway, everything in its time.
>>
>> This change allows to create a these cyclic links tz <-> device as
>> ACPI does and will allow to remove the code in the ACPI driver.
> 
> Well, I'd rather have it in the driver than in the core TBH.
> 
> If ACPI is the only user of this, let it do the dirty thing by itself.
> 
> There are two cases which would justify making this change:
> 1. There will be more users of it going forward (seems unlikely from
> the description).
> 2. It gets in the way of some other changes somehow.
> 
> I kind of expect 2. to be the case, so how does it get in the way?

Shall we do the same approach than 'Menlow' and add an option to remove 
the thermal zone link in the acpi sysfs directory ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 09/11] thermal/core: Add a linked device parameter

[Rafael J. Wysocki]

On Tue, Apr 4, 2023 at 9:01 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 27/03/2023 18:16, Rafael J. Wysocki wrote:
> > On Tue, Mar 7, 2023 at 2:38 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> Some drivers want to create a link from the thermal zone to the device
> >> sysfs entry and vice versa.
> >
> > Which device is this, exactly?
>
> ls -alh /sys/bus/acpi/drivers/thermal/LNXTHERM:00/
>
> [ ... ]
>
> lrwxrwxrwx    1         0 thermal_zone ->
> ../../../virtual/thermal/thermal_zone0
>
> [ ... ]
>
> The ACPI driver is the only one doing this AFAICT.
>
>
> >> That is the case of the APCI driver.
> >>
> >> Having a backpointer from the device to the thermal zone sounds akward
> >> as we can have the same device instantiating multiple thermal zones so
> >> there will be a conflict while creating the second link with the same
> >> name. Moreover, the userspace has enough information to build the
> >> dependency from the thermal zone device link without having this cyclic
> >> link from the device to thermal zone.
> >>
> >> Anyway, everything in its time.
> >>
> >> This change allows to create a these cyclic links tz <-> device as
> >> ACPI does and will allow to remove the code in the ACPI driver.
> >
> > Well, I'd rather have it in the driver than in the core TBH.
> >
> > If ACPI is the only user of this, let it do the dirty thing by itself.
> >
> > There are two cases which would justify making this change:
> > 1. There will be more users of it going forward (seems unlikely from
> > the description).
> > 2. It gets in the way of some other changes somehow.
> >
> > I kind of expect 2. to be the case, so how does it get in the way?
>
> Shall we do the same approach than 'Menlow' and add an option to remove
> the thermal zone link in the acpi sysfs directory ?

That depends on the answer to the question above.