[PATCH 00/15] thermal: sysfs: add locking

[PATCH 00/15] thermal: sysfs: add locking

[Eduardo Valentin]

Hello,

Several thermal sysfs entries are currently being called
from userspace without locking. Data and calls to ops
are accessed deliberated without any care for locking.

This patch series attempts to fix this.

Now that sysfs handlers are on the same place it is easier
to visualize this issue and fix it. The strategy is essentially
to lock any access. Also, functions in thermal core and thermal
helpers are assumed to do the proper locking already.

This patch series is based on the thermal core reorganization.

There is no change in the ABI to userspace. The difference
is that now serialization to data will be done.

For your consideration, I am also adding this to this branch:
  git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal sysfs_locking

Comments are welcome.

BR,

Eduardo Valentin (15):
  thermal: sysfs: lock tz in type_show
  thermal: sysfs: lock tz while on access to mode properties
  thermal: sysfs: lock tz while on trip_point_type properties
  thermal: sysfs: lock tz while on trip_point_temp properties
  thermal: sysfs: lock tz while on trip_point_hyst properties
  thermal: sysfs: lock tz while on passive properties
  thermal: sysfs: lock tz while on policy properties
  thermal: sysfs: improve locking of emul_temp_store()
  thermal: sysfs: lock tz when access sustainable power properties
  thermal: sysfs: lock tz when access tzp properties
  thermal: sysfs: lock cdev while accessing type
  thermal: sysfs: lock cdev while accessing max_state
  thermal: sysfs: lock cdev while accessing cur_state
  thermal: sysfs: serialize access to instances
  thermal: sysfs: add comments describing locking strategy

 drivers/thermal/thermal_sysfs.c | 133 ++++++++++++++++++++++++++++++++++------
 include/linux/thermal.h         |   2 +-
 2 files changed, 115 insertions(+), 20 deletions(-)

-- 
2.1.4

[PATCH 01/15] thermal: sysfs: lock tz in type_show

[Eduardo Valentin]

Avoid race while accessing tz->type.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 4136b71..ee983ca 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -29,8 +29,13 @@ static ssize_t
 type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	char *type;
 
-	return sprintf(buf, "%s\n", tz->type);
+	mutex_lock(&tz->lock);
+	type = tz->type;
+	mutex_unlock(&tz->lock);
+
+	return sprintf(buf, "%s\n", type);
 }
 
 static ssize_t
-- 
2.1.4

[PATCH 02/15] thermal: sysfs: lock tz while on access to mode properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler mode_show()  and mode_store().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index ee983ca..1db2406 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -62,7 +62,9 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf)
 	if (!tz->ops->get_mode)
 		return -EPERM;
 
+	mutex_lock(&tz->lock);
 	result = tz->ops->get_mode(tz, &mode);
+	mutex_unlock(&tz->lock);
 	if (result)
 		return result;
 
@@ -75,17 +77,22 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	   const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	enum thermal_device_mode mode = THERMAL_DEVICE_DISABLED;
 	int result;
 
 	if (!tz->ops->set_mode)
 		return -EPERM;
 
 	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
+		mode = THERMAL_DEVICE_ENABLED;
 	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
+		mode = THERMAL_DEVICE_DISABLED;
 	else
-		result = -EINVAL;
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+	result = tz->ops->set_mode(tz, mode);
+	mutex_unlock(&tz->lock);
 
 	if (result)
 		return result;
-- 
2.1.4

[PATCH 03/15] thermal: sysfs: lock tz while on trip_point_type properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler trip_point_type_show() and trip_point_temp_store().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 1db2406..b69036e 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -114,7 +114,9 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
 	if (sscanf(attr->attr.name, "trip_point_%d_type", &trip) != 1)
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	result = tz->ops->get_trip_type(tz, trip, &type);
+	mutex_unlock(&tz->lock);
 	if (result)
 		return result;
 
-- 
2.1.4

[PATCH 04/15] thermal: sysfs: lock tz while on trip_point_temp properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler trip_point_temp_show() and trip_point_temp_store().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index b69036e..32f410f 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -151,7 +151,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
 	if (kstrtoint(buf, 10, &temperature))
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	ret = tz->ops->set_trip_temp(tz, trip, temperature);
+	mutex_unlock(&tz->lock);
 	if (ret)
 		return ret;
 
@@ -174,7 +176,9 @@ trip_point_temp_show(struct device *dev, struct device_attribute *attr,
 	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != 1)
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	ret = tz->ops->get_trip_temp(tz, trip, &temperature);
+	mutex_unlock(&tz->lock);
 
 	if (ret)
 		return ret;
-- 
2.1.4

[PATCH 05/15] thermal: sysfs: lock tz while on trip_point_hyst properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler trip_point_hyst_store() and
trip_point_hyst_show().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 32f410f..afb42a2 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -208,7 +208,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
 	 * here. The driver implementing 'set_trip_hyst' has to
 	 * take care of this.
 	 */
+	mutex_lock(&tz->lock);
 	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
+	mutex_unlock(&tz->lock);
 
 	return ret ? ret : count;
 }
@@ -227,7 +229,9 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
 	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip) != 1)
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	ret = tz->ops->get_trip_hyst(tz, trip, &temperature);
+	mutex_unlock(&tz->lock);
 
 	return ret ? ret : sprintf(buf, "%d\n", temperature);
 }
-- 
2.1.4

[PATCH 06/15] thermal: sysfs: lock tz while on passive properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler passive_store() and
passive_show().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index afb42a2..dcaeb17 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -252,18 +252,24 @@ passive_store(struct device *dev, struct device_attribute *attr,
 	if (state && state < 1000)
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	if (state && !tz->forced_passive) {
 		if (!tz->passive_delay)
 			tz->passive_delay = 1000;
+		mutex_unlock(&tz->lock);
 		thermal_zone_device_rebind_exception(tz, "Processor",
 						     sizeof("Processor"));
+		mutex_lock(&tz->lock);
 	} else if (!state && tz->forced_passive) {
 		tz->passive_delay = 0;
+		mutex_unlock(&tz->lock);
 		thermal_zone_device_unbind_exception(tz, "Processor",
 						     sizeof("Processor"));
+		mutex_lock(&tz->lock);
 	}
 
 	tz->forced_passive = state;
+	mutex_unlock(&tz->lock);
 
 	thermal_zone_device_update(tz);
 
@@ -275,8 +281,13 @@ passive_show(struct device *dev, struct device_attribute *attr,
 	     char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	unsigned int passive;
 
-	return sprintf(buf, "%d\n", tz->forced_passive);
+	mutex_lock(&tz->lock);
+	passive = tz->forced_passive;
+	mutex_unlock(&tz->lock);
+
+	return sprintf(buf, "%u\n", passive);
 }
 
 static ssize_t
@@ -494,7 +505,9 @@ static umode_t thermal_zone_passive_is_visible(struct kobject *kobj,
 	tz = container_of(dev, struct thermal_zone_device, device);
 
 	for (count = 0; count < tz->trips; count++) {
+		mutex_lock(&tz->lock);
 		tz->ops->get_trip_type(tz, count, &trip_type);
+		mutex_unlock(&tz->lock);
 
 		if (trip_type == THERMAL_TRIP_PASSIVE)
 			return attr->mode;
-- 
2.1.4

[PATCH 07/15] thermal: sysfs: lock tz while on policy properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler policy_show().
policy_store() is already locked by
the thermal core.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index dcaeb17..234eb18 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -311,8 +311,14 @@ static ssize_t
 policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	char *name;
 
-	return sprintf(buf, "%s\n", tz->governor->name);
+	/* locking the zone because governor->name does not change */
+	mutex_lock(&tz->lock);
+	name = tz->governor->name;
+	mutex_unlock(&tz->lock);
+
+	return sprintf(buf, "%s\n", name);
 }
 
 static ssize_t
-- 
2.1.4

[PATCH 08/15] thermal: sysfs: improve locking of emul_temp_store()

[Eduardo Valentin]

emul_temp_store locks only for setting temperature,
but it must lock also for calls to tz.ops.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 234eb18..32ffadf 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -340,13 +340,12 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
 	if (kstrtoint(buf, 10, &temperature))
 		return -EINVAL;
 
-	if (!tz->ops->set_emul_temp) {
-		mutex_lock(&tz->lock);
+	mutex_lock(&tz->lock);
+	if (!tz->ops->set_emul_temp)
 		tz->emul_temperature = temperature;
-		mutex_unlock(&tz->lock);
-	} else {
+	else
 		ret = tz->ops->set_emul_temp(tz, temperature);
-	}
+	mutex_unlock(&tz->lock);
 
 	if (!ret)
 		thermal_zone_device_update(tz);
-- 
2.1.4

[PATCH 09/15] thermal: sysfs: lock tz when access sustainable power properties

[Eduardo Valentin]

Lock tz when in user facing sysfs handlers to expose
sustainable power properties. This includes:
sustainable_power_show()
sustainable_power_store()

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 32ffadf..5b08d64 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -360,9 +360,14 @@ sustainable_power_show(struct device *dev, struct device_attribute *devattr,
 		       char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	unsigned int sustainable_power;
+
+	mutex_lock(&tz->lock);
+	sustainable_power = tz->tzp->sustainable_power;
+	mutex_unlock(&tz->lock);
 
 	if (tz->tzp)
-		return sprintf(buf, "%u\n", tz->tzp->sustainable_power);
+		return sprintf(buf, "%u\n", sustainable_power);
 	else
 		return -EIO;
 }
@@ -380,7 +385,9 @@ sustainable_power_store(struct device *dev, struct device_attribute *devattr,
 	if (kstrtou32(buf, 10, &sustainable_power))
 		return -EINVAL;
 
+	mutex_lock(&tz->lock);
 	tz->tzp->sustainable_power = sustainable_power;
+	mutex_unlock(&tz->lock);
 
 	return count;
 }
-- 
2.1.4

[PATCH 10/15] thermal: sysfs: lock tz when access tzp properties

[Eduardo Valentin]

Lock tz when in user facing sysfs handlers to expose
tzp properties. This includes changing the macro
create_s32_tzp_attr(), which is used  in the
tzp properties:
k_po, k_pu, k_i, k_d, integral_cutoff, slope, offset.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 5b08d64..86a77cd 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -398,11 +398,16 @@ sustainable_power_store(struct device *dev, struct device_attribute *devattr,
 		char *buf)						\
 	{								\
 	struct thermal_zone_device *tz = to_thermal_zone(dev);		\
+	int value;							\
 									\
-	if (tz->tzp)							\
-		return sprintf(buf, "%d\n", tz->tzp->name);		\
-	else								\
+	if (!tz->tzp)							\
 		return -EIO;						\
+									\
+	mutex_lock(&tz->lock);						\
+	value = tz->tzp->name;						\
+	mutex_unlock(&tz->lock);					\
+									\
+	return sprintf(buf, "%d\n", value);				\
 	}								\
 									\
 	static ssize_t							\
@@ -418,7 +423,9 @@ sustainable_power_store(struct device *dev, struct device_attribute *devattr,
 		if (kstrtos32(buf, 10, &value))				\
 			return -EINVAL;					\
 									\
+		mutex_lock(&tz->lock);					\
 		tz->tzp->name = value;					\
+		mutex_unlock(&tz->lock);				\
 									\
 		return count;						\
 	}								\
-- 
2.1.4

[PATCH 11/15] thermal: sysfs: lock cdev while accessing type

[Eduardo Valentin]

Serialized data access to cdev->type in user facing
sysfs handler thermal_cooling_device_type_show().

The existing cdev lock is used. Updating comment
on the lock definition.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 7 ++++++-
 include/linux/thermal.h         | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 86a77cd..044090a 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -682,8 +682,13 @@ thermal_cooling_device_type_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
+	char *type;
+
+	mutex_lock(&cdev->lock);
+	type = cdev->type;
+	mutex_unlock(&cdev->lock);
 
-	return sprintf(buf, "%s\n", cdev->type);
+	return sprintf(buf, "%s\n", type);
 }
 
 static ssize_t
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index fa2c2be..7b2c014 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -137,7 +137,7 @@ struct thermal_cooling_device {
 	void *devdata;
 	const struct thermal_cooling_device_ops *ops;
 	bool updated; /* true if the cooling device does not need update */
-	struct mutex lock; /* protect thermal_instances list */
+	struct mutex lock; /* protect thermal_cooling_device */
 	struct list_head thermal_instances;
 	struct list_head node;
 };
-- 
2.1.4

[PATCH 12/15] thermal: sysfs: lock cdev while accessing max_state

[Eduardo Valentin]

Serialized calls to cdev->ops in user facing
sysfs handler thermal_cooling_device_max_state_show().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 044090a..3eaa081 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -699,7 +699,9 @@ thermal_cooling_device_max_state_show(struct device *dev,
 	unsigned long state;
 	int ret;
 
+	mutex_lock(&cdev->lock);
 	ret = cdev->ops->get_max_state(cdev, &state);
+	mutex_unlock(&cdev->lock);
 	if (ret)
 		return ret;
 	return sprintf(buf, "%ld\n", state);
-- 
2.1.4

[PATCH 13/15] thermal: sysfs: lock cdev while accessing cur_state

[Eduardo Valentin]

Serialized calls to cdev->ops in user facing
sysfs handlers thermal_cooling_device_cur_state_show()
and thermal_cooling_device_cur_state_store().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 3eaa081..ab78e08 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -715,7 +715,9 @@ thermal_cooling_device_cur_state_show(struct device *dev,
 	unsigned long state;
 	int ret;
 
+	mutex_lock(&cdev->lock);
 	ret = cdev->ops->get_cur_state(cdev, &state);
+	mutex_unlock(&cdev->lock);
 	if (ret)
 		return ret;
 	return sprintf(buf, "%ld\n", state);
@@ -736,7 +738,9 @@ thermal_cooling_device_cur_state_store(struct device *dev,
 	if ((long)state < 0)
 		return -EINVAL;
 
+	mutex_lock(&cdev->lock);
 	result = cdev->ops->set_cur_state(cdev, state);
+	mutex_unlock(&cdev->lock);
 	if (result)
 		return result;
 	return count;
-- 
2.1.4

[PATCH 14/15] thermal: sysfs: serialize access to instances

[Eduardo Valentin]

Because instances are the result of a matching
between cdev and tz, we need to lock both
in order to access the instance reliably.

This patch locks both tz and cdev in user
facing sysfs handlers when accessing thermal
zone instance.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index ab78e08..31314be 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -781,14 +781,20 @@ thermal_cooling_device_trip_point_show(struct device *dev,
 				       struct device_attribute *attr, char *buf)
 {
 	struct thermal_instance *instance;
+	int trip;
 
 	instance =
 	    container_of(attr, struct thermal_instance, attr);
 
+	mutex_lock(&instance->tz->lock);
+	mutex_lock(&instance->cdev->lock);
+	trip = instance->trip;
+	mutex_unlock(&instance->cdev->lock);
+	mutex_unlock(&instance->tz->lock);
 	if (instance->trip == THERMAL_TRIPS_NONE)
 		return sprintf(buf, "-1\n");
 	else
-		return sprintf(buf, "%d\n", instance->trip);
+		return sprintf(buf, "%d\n", trip);
 }
 
 ssize_t
@@ -796,10 +802,16 @@ thermal_cooling_device_weight_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct thermal_instance *instance;
+	int weight;
 
 	instance = container_of(attr, struct thermal_instance, weight_attr);
+	mutex_lock(&instance->tz->lock);
+	mutex_lock(&instance->cdev->lock);
+	weight = instance->weight;
+	mutex_unlock(&instance->cdev->lock);
+	mutex_unlock(&instance->tz->lock);
 
-	return sprintf(buf, "%d\n", instance->weight);
+	return sprintf(buf, "%d\n", weight);
 }
 
 ssize_t
@@ -815,7 +827,11 @@ thermal_cooling_device_weight_store(struct device *dev,
 		return ret;
 
 	instance = container_of(attr, struct thermal_instance, weight_attr);
+	mutex_lock(&instance->tz->lock);
+	mutex_lock(&instance->cdev->lock);
 	instance->weight = weight;
+	mutex_unlock(&instance->cdev->lock);
+	mutex_unlock(&instance->tz->lock);
 
 	return count;
 }
-- 
2.1.4

[PATCH 15/15] thermal: sysfs: add comments describing locking strategy

[Eduardo Valentin]

The locking strategy of sysfs entries in thermal subsystem
is to hold a lock to serialize access to struct thermal_zone_device
and struct thermal_cooling_device data. Besides, the respective
locks will also be held if calls to ops need to be done to fetch
data or to trigger driver actions.

The exception of the locking strategy are functions called
during initialization of cdev and tz.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 31314be..743df50 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -23,7 +23,14 @@
 
 #include "thermal_core.h"
 
-/* sys I/F for thermal zone */
+/*
+ * sys I/F for thermal zone
+ *
+ * Note on locking. The sysfs interface will always first
+ * lock the zone to serialize data access and ops calls.
+ * All calls to thermal_core and thermal_helpers are assumed
+ * to handle locking properly.
+ */
 
 static ssize_t
 type_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -555,6 +562,9 @@ static const struct attribute_group *thermal_zone_attribute_groups[] = {
  * helper function to instantiate sysfs entries for every trip
  * point and its properties of a struct thermal_zone_device.
  *
+ * This function is assumed to be called only during probe,
+ * and therefore no locking in the thermal zone device is done.
+ *
  * Return: 0 on success, the proper error value otherwise.
  */
 static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
@@ -770,6 +780,10 @@ static const struct attribute_group *cooling_device_attr_groups[] = {
 	NULL,
 };
 
+/*
+ * Assumed to be called at the creation of the cooling device
+ * and for this reason, no locking is done
+ */
 void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *cdev)
 {
 	cdev->device.groups = cooling_device_attr_groups;
-- 
2.1.4

Re: [PATCH 00/15] thermal: sysfs: add locking

[Keerthy]

On Tuesday 31 May 2016 12:01 PM, Eduardo Valentin wrote:
> Hello,
>
> Several thermal sysfs entries are currently being called
> from userspace without locking. Data and calls to ops
> are accessed deliberated without any care for locking.
>
> This patch series attempts to fix this.
>
> Now that sysfs handlers are on the same place it is easier
> to visualize this issue and fix it. The strategy is essentially
> to lock any access. Also, functions in thermal core and thermal
> helpers are assumed to do the proper locking already.
>
> This patch series is based on the thermal core reorganization.
>
> There is no change in the ABI to userspace. The difference
> is that now serialization to data will be done.
>
> For your consideration, I am also adding this to this branch:
>    git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal sysfs_locking

Tested and checked for reading temperatures from all the thermal zones
on DRA7/DRA72 and AM57XX-BEAGLE-X15 boards.

Also tested emulation using a rebased version of
https://lkml.org/lkml/2016/5/10/346

For DRA7/DRA72 and BEAGLE-X15:
Tested-by: Keerthy <j-keerthy@ti.com>

>
> Comments are welcome.
>
> BR,
>
> Eduardo Valentin (15):
>    thermal: sysfs: lock tz in type_show
>    thermal: sysfs: lock tz while on access to mode properties
>    thermal: sysfs: lock tz while on trip_point_type properties
>    thermal: sysfs: lock tz while on trip_point_temp properties
>    thermal: sysfs: lock tz while on trip_point_hyst properties
>    thermal: sysfs: lock tz while on passive properties
>    thermal: sysfs: lock tz while on policy properties
>    thermal: sysfs: improve locking of emul_temp_store()
>    thermal: sysfs: lock tz when access sustainable power properties
>    thermal: sysfs: lock tz when access tzp properties
>    thermal: sysfs: lock cdev while accessing type
>    thermal: sysfs: lock cdev while accessing max_state
>    thermal: sysfs: lock cdev while accessing cur_state
>    thermal: sysfs: serialize access to instances
>    thermal: sysfs: add comments describing locking strategy
>
>   drivers/thermal/thermal_sysfs.c | 133 ++++++++++++++++++++++++++++++++++------
>   include/linux/thermal.h         |   2 +-
>   2 files changed, 115 insertions(+), 20 deletions(-)
>

Re: [PATCH 02/15] thermal: sysfs: lock tz while on access to mode properties

[Keerthy]

Hi Eduardo,

On Tuesday 31 May 2016 12:01 PM, Eduardo Valentin wrote:
> Serialized calls to tz.ops in user facing
> sysfs handler mode_show()  and mode_store().

This seems to be causing a deadlock at boot time during the ending 
stages of boot:

http://pastebin.ubuntu.com/17085291/

It took a while to git bisect on linux-next.

Seems like you introduced new locking at the sysfs layer which causes 
this deadlock as the underlying code again tries to acquire the same 
tz->lock.

Regards,
Keerthy

>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>   drivers/thermal/thermal_sysfs.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index ee983ca..1db2406 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -62,7 +62,9 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf)
>   	if (!tz->ops->get_mode)
>   		return -EPERM;
>
> +	mutex_lock(&tz->lock);
>   	result = tz->ops->get_mode(tz, &mode);
> +	mutex_unlock(&tz->lock);
>   	if (result)
>   		return result;
>
> @@ -75,17 +77,22 @@ mode_store(struct device *dev, struct device_attribute *attr,
>   	   const char *buf, size_t count)
>   {
>   	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	enum thermal_device_mode mode = THERMAL_DEVICE_DISABLED;
>   	int result;
>
>   	if (!tz->ops->set_mode)
>   		return -EPERM;
>
>   	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
> +		mode = THERMAL_DEVICE_ENABLED;
>   	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
> +		mode = THERMAL_DEVICE_DISABLED;
>   	else
> -		result = -EINVAL;
> +		return -EINVAL;
> +
> +	mutex_lock(&tz->lock);
> +	result = tz->ops->set_mode(tz, mode);
> +	mutex_unlock(&tz->lock);
>
>   	if (result)
>   		return result;
>

Re: [PATCH 02/15] thermal: sysfs: lock tz while on access to mode properties

[Keerthy]

On Tuesday 07 June 2016 02:38 PM, Keerthy wrote:
> Hi Eduardo,
>
> On Tuesday 31 May 2016 12:01 PM, Eduardo Valentin wrote:
>> Serialized calls to tz.ops in user facing
>> sysfs handler mode_show()  and mode_store().
>
> This seems to be causing a deadlock at boot time during the ending
> stages of boot:
>
> http://pastebin.ubuntu.com/17085291/
>
> It took a while to git bisect on linux-next.
>
> Seems like you introduced new locking at the sysfs layer which causes
> this deadlock as the underlying code again tries to acquire the same
> tz->lock.

I confirm reverting this patch helps me boot on linux-next. This patch 
can be dropped as the lower layer functions are already acquiring tz->lock.

Thanks Vignesh for reporting the deadlock.

Regards,
Keerthy

>
> Regards,
> Keerthy
>
>>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: linux-pm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
>> ---
>>   drivers/thermal/thermal_sysfs.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_sysfs.c
>> b/drivers/thermal/thermal_sysfs.c
>> index ee983ca..1db2406 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -62,7 +62,9 @@ mode_show(struct device *dev, struct
>> device_attribute *attr, char *buf)
>>       if (!tz->ops->get_mode)
>>           return -EPERM;
>>
>> +    mutex_lock(&tz->lock);
>>       result = tz->ops->get_mode(tz, &mode);
>> +    mutex_unlock(&tz->lock);
>>       if (result)
>>           return result;
>>
>> @@ -75,17 +77,22 @@ mode_store(struct device *dev, struct
>> device_attribute *attr,
>>          const char *buf, size_t count)
>>   {
>>       struct thermal_zone_device *tz = to_thermal_zone(dev);
>> +    enum thermal_device_mode mode = THERMAL_DEVICE_DISABLED;
>>       int result;
>>
>>       if (!tz->ops->set_mode)
>>           return -EPERM;
>>
>>       if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
>> -        result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
>> +        mode = THERMAL_DEVICE_ENABLED;
>>       else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
>> -        result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
>> +        mode = THERMAL_DEVICE_DISABLED;
>>       else
>> -        result = -EINVAL;
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&tz->lock);
>> +    result = tz->ops->set_mode(tz, mode);
>> +    mutex_unlock(&tz->lock);
>>
>>       if (result)
>>           return result;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/15] thermal: sysfs: lock tz while on access to mode properties

[Zhang Rui]

On 二, 2016-06-07 at 14:52 +0530, Keerthy wrote:
> 
> On Tuesday 07 June 2016 02:38 PM, Keerthy wrote:
> > 
> > Hi Eduardo,
> > 
> > On Tuesday 31 May 2016 12:01 PM, Eduardo Valentin wrote:
> > > 
> > > Serialized calls to tz.ops in user facing
> > > sysfs handler mode_show()  and mode_store().
> > This seems to be causing a deadlock at boot time during the ending
> > stages of boot:
> > 
> > http://pastebin.ubuntu.com/17085291/
> > 
> > It took a while to git bisect on linux-next.
> > 
> > Seems like you introduced new locking at the sysfs layer which
> > causes
> > this deadlock as the underlying code again tries to acquire the
> > same
> > tz->lock.
> I confirm reverting this patch helps me boot on linux-next. This
> patch 
> can be dropped as the lower layer functions are already acquiring tz-
> >lock.
> 
> Thanks Vignesh for reporting the deadlock.
> 
the root cause of the deadlock is that some platform driver invokes
thermal_zone_device_update() in .set_mode(), after mode changed.

If we wants to lock tz->ops->set_mode(), we should cleanup all the
platform thermal drivers, and deliver a thermal_zone_device_update()
in mode_store(), at the same time.

thanks,
rui

> Regards,
> Keerthy
> 
> > 
> > 
> > Regards,
> > Keerthy
> > 
> > > 
> > > 
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > Cc: linux-pm@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> > > ---
> > >   drivers/thermal/thermal_sysfs.c | 13 ++++++++++---
> > >   1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/thermal_sysfs.c
> > > b/drivers/thermal/thermal_sysfs.c
> > > index ee983ca..1db2406 100644
> > > --- a/drivers/thermal/thermal_sysfs.c
> > > +++ b/drivers/thermal/thermal_sysfs.c
> > > @@ -62,7 +62,9 @@ mode_show(struct device *dev, struct
> > > device_attribute *attr, char *buf)
> > >       if (!tz->ops->get_mode)
> > >           return -EPERM;
> > > 
> > > +    mutex_lock(&tz->lock);
> > >       result = tz->ops->get_mode(tz, &mode);
> > > +    mutex_unlock(&tz->lock);
> > >       if (result)
> > >           return result;
> > > 
> > > @@ -75,17 +77,22 @@ mode_store(struct device *dev, struct
> > > device_attribute *attr,
> > >          const char *buf, size_t count)
> > >   {
> > >       struct thermal_zone_device *tz = to_thermal_zone(dev);
> > > +    enum thermal_device_mode mode = THERMAL_DEVICE_DISABLED;
> > >       int result;
> > > 
> > >       if (!tz->ops->set_mode)
> > >           return -EPERM;
> > > 
> > >       if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
> > > -        result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
> > > +        mode = THERMAL_DEVICE_ENABLED;
> > >       else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
> > > -        result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
> > > +        mode = THERMAL_DEVICE_DISABLED;
> > >       else
> > > -        result = -EINVAL;
> > > +        return -EINVAL;
> > > +
> > > +    mutex_lock(&tz->lock);
> > > +    result = tz->ops->set_mode(tz, mode);
> > > +    mutex_unlock(&tz->lock);
> > > 
> > >       if (result)
> > >           return result;
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" 
> > in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

Because several drivers do the following pattern:
.set_mode()
   ...
   local_data->mode = new_mode;
   thermal_zone_device_update(tz);

makes sense to simply do the thermal_zone_device_update()
in thermal core, after setting the new mode.

Also, this patch also remove deadlocks on drivers that
call thermal_zone_device_update() on .set_mode(),
as .set_mode()  is now called always with tz->lock held.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
Rui, Keerthy,

I think this patch should take care of the introduced deadlock.

Let me know if solves on your end.

BR,

Eduardo
---

 drivers/acpi/thermal.c                             |  2 --
 drivers/platform/x86/acerhdf.c                     |  1 -
 drivers/thermal/imx_thermal.c                      |  1 -
 drivers/thermal/of-thermal.c                       |  8 ++---
 drivers/thermal/thermal_core.c                     | 41 +++++++++++++++++-----
 drivers/thermal/thermal_sysfs.c                    |  1 +
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |  1 -
 7 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 82707f9..8582b88 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
 
 	if (!tz->tz_enabled)
 		return;
-
-	thermal_zone_device_update(tz->thermal_zone);
 }
 
 /* sys I/F for generic thermal sysfs support */
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 460fa67..aee33ba 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
 	kernelmode = 1;
 
 	thz_dev->polling_delay = interval*1000;
-	thermal_zone_device_update(thz_dev);
 	pr_notice("kernel mode fan control ON\n");
 }
 
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c5547bd..a413eb6 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 	}
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..b44c102 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -181,9 +181,6 @@ static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	if (!data->ops || !data->ops->set_emul_temp)
-		return -EINVAL;
-
 	return data->ops->set_emul_temp(data->sensor_data, temp);
 }
 
@@ -292,7 +289,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
 	mutex_unlock(&tz->lock);
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
@@ -427,7 +423,9 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
-	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
+	if (ops->set_emul_temp)
+		tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
+
 	mutex_unlock(&tzd->lock);
 
 	return tzd;
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 09da955..bb1ede7 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -323,6 +323,21 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
 		       def_governor->throttle(tz, trip);
 }
 
+static void thermal_zone_update_stats(struct thermal_zone_device *tz,
+				      int last_trip, int cur_trip)
+{
+	unsigned long long cur_time = get_jiffies_64();
+	struct thermal_zone_stats *cur, *last;
+
+	cur = tz->stats_table[cur_trip];
+	last = tz->stats_table[last_trip];
+
+	cur->counter++;
+	cur->last_time = cur_time;
+
+	last->time_in += cur_time - last->last_time;
+}
+
 static void thermal_tripped_notify(struct thermal_zone_device *tz,
 				   int trip, enum thermal_trip_type trip_type,
 				   int trip_temp)
@@ -333,6 +348,7 @@ static void thermal_tripped_notify(struct thermal_zone_device *tz,
 			NULL };
 	int upper_trip_hyst, upper_trip_temp, trip_hyst = 0;
 	int ret = 0;
+	int cur_trip, last_trip;
 
 	snprintf(tuv_name, sizeof(tuv_name), "THERMAL_ZONE=%s", tz->type);
 	snprintf(tuv_temp, sizeof(tuv_temp), "TEMP=%d", tz->temperature);
@@ -344,8 +360,11 @@ static void thermal_tripped_notify(struct thermal_zone_device *tz,
 	mutex_lock(&tz->lock);
 
 	/* crossing up */
-	if (tz->last_temperature < trip_temp && trip_temp < tz->temperature)
+	if (tz->last_temperature < trip_temp && trip_temp < tz->temperature) {
 		kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, msg);
+		last_trip = trip - 1;
+		cur_trip = trip;
+	}
 
 	if (tz->ops->get_trip_hyst)
 		tz->ops->get_trip_hyst(tz, trip, &trip_hyst);
@@ -355,6 +374,8 @@ static void thermal_tripped_notify(struct thermal_zone_device *tz,
 	if (tz->last_temperature > trip_temp && trip_temp > tz->temperature) {
 		snprintf(tuv_trip, sizeof(tuv_trip), "TRIP=%d", trip - 1);
 		kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, msg);
+		last_trip = trip;
+		cur_trip = trip - 1;
 	}
 
 	ret = tz->ops->get_trip_temp(tz, trip + 1, &upper_trip_temp);
@@ -369,19 +390,15 @@ static void thermal_tripped_notify(struct thermal_zone_device *tz,
 	    upper_trip_temp > tz->temperature)
 		kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, msg);
 
+	thermal_zone_device_update_stats(tz, last_trip, cur_trip);
 unlock:
 	mutex_unlock(&tz->lock);
 }
 
 static void handle_critical_trips(struct thermal_zone_device *tz,
-				  int trip, enum thermal_trip_type trip_type)
+				  int trip, enum thermal_trip_type trip_type,
+				  int trip_temp)
 {
-	int trip_temp;
-
-	tz->ops->get_trip_temp(tz, trip, &trip_temp);
-
-	thermal_tripped_notify(tz, trip, trip_type, trip_temp);
-
 	/* If we have not crossed the trip_temp, we do not care. */
 	if (trip_temp <= 0 || tz->temperature < trip_temp)
 		return;
@@ -402,15 +419,21 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 {
 	enum thermal_trip_type type;
+	int trip_temp;
+
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
 
 	/* Ignore disabled trip points */
 	if (test_bit(trip, &tz->trips_disabled))
 		return;
 
 	tz->ops->get_trip_type(tz, trip, &type);
+	tz->ops->get_trip_type(tz, trip, &trip_temp);
+
+	thermal_tripped_notify(tz, trip, trip_type, trip_temp);
 
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
-		handle_critical_trips(tz, trip, type);
+		handle_critical_trips(tz, trip, type, trip_temp);
 	else
 		handle_non_critical_trips(tz, trip, type);
 }
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 743df50..3d0dc30 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&tz->lock);
 	result = tz->ops->set_mode(tz, mode);
 	mutex_unlock(&tz->lock);
+	thermal_zone_device_update(tz);
 
 	if (result)
 		return result;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..9a5a3a3 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
 	data->mode = mode;
 	ti_bandgap_write_update_interval(bgp, data->sensor_id,
 					data->ti_thermal->polling_delay);
-	thermal_zone_device_update(data->ti_thermal);
 	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
 		data->ti_thermal->polling_delay);
 
-- 
2.1.4

[PATCHv2 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

Because several drivers do the following pattern:
.set_mode()
   ...
   local_data->mode = new_mode;
   thermal_zone_device_update(tz);

makes sense to simply do the thermal_zone_device_update()
in thermal core, after setting the new mode.

Also, this patch also remove deadlocks on drivers that
call thermal_zone_device_update() on .set_mode(),
as .set_mode()  is now called always with tz->lock held.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
Please ignore last version.

V1-> V2:
Cleaned the patch and remove unrelated changes.
---

 drivers/acpi/thermal.c                             | 2 --
 drivers/platform/x86/acerhdf.c                     | 1 -
 drivers/thermal/imx_thermal.c                      | 1 -
 drivers/thermal/of-thermal.c                       | 1 -
 drivers/thermal/thermal_sysfs.c                    | 1 +
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 1 -
 6 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 82707f9..8582b88 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
 
 	if (!tz->tz_enabled)
 		return;
-
-	thermal_zone_device_update(tz->thermal_zone);
 }
 
 /* sys I/F for generic thermal sysfs support */
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 460fa67..aee33ba 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
 	kernelmode = 1;
 
 	thz_dev->polling_delay = interval*1000;
-	thermal_zone_device_update(thz_dev);
 	pr_notice("kernel mode fan control ON\n");
 }
 
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c5547bd..a413eb6 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 	}
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..4602e2e 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -292,7 +292,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
 	mutex_unlock(&tz->lock);
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 743df50..3d0dc30 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&tz->lock);
 	result = tz->ops->set_mode(tz, mode);
 	mutex_unlock(&tz->lock);
+	thermal_zone_device_update(tz);
 
 	if (result)
 		return result;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..9a5a3a3 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
 	data->mode = mode;
 	ti_bandgap_write_update_interval(bgp, data->sensor_id,
 					data->ti_thermal->polling_delay);
-	thermal_zone_device_update(data->ti_thermal);
 	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
 		data->ti_thermal->polling_delay);
 
-- 
2.1.4

Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Keerthy]

On Wednesday 22 June 2016 10:45 AM, Eduardo Valentin wrote:
> Because several drivers do the following pattern:
> .set_mode()
>     ...
>     local_data->mode = new_mode;
>     thermal_zone_device_update(tz);
>
> makes sense to simply do the thermal_zone_device_update()
> in thermal core, after setting the new mode.

http://pastebin.ubuntu.com/17687601/

linux-next + current v2 of the patch.
The back trace still comes.

Regards,
Keerthy

>
> Also, this patch also remove deadlocks on drivers that
> call thermal_zone_device_update() on .set_mode(),
> as .set_mode()  is now called always with tz->lock held.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> Cc: "Lee, Chun-Yi" <jlee@suse.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
> Please ignore last version.
>
> V1-> V2:
> Cleaned the patch and remove unrelated changes.
> ---
>
>   drivers/acpi/thermal.c                             | 2 --
>   drivers/platform/x86/acerhdf.c                     | 1 -
>   drivers/thermal/imx_thermal.c                      | 1 -
>   drivers/thermal/of-thermal.c                       | 1 -
>   drivers/thermal/thermal_sysfs.c                    | 1 +
>   drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 1 -
>   6 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 82707f9..8582b88 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
>
>   	if (!tz->tz_enabled)
>   		return;
> -
> -	thermal_zone_device_update(tz->thermal_zone);
>   }
>
>   /* sys I/F for generic thermal sysfs support */
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 460fa67..aee33ba 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
>   	kernelmode = 1;
>
>   	thz_dev->polling_delay = interval*1000;
> -	thermal_zone_device_update(thz_dev);
>   	pr_notice("kernel mode fan control ON\n");
>   }
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index c5547bd..a413eb6 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
>   	}
>
>   	data->mode = mode;
> -	thermal_zone_device_update(tz);
>
>   	return 0;
>   }
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..4602e2e 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -292,7 +292,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>   	mutex_unlock(&tz->lock);
>
>   	data->mode = mode;
> -	thermal_zone_device_update(tz);
>
>   	return 0;
>   }
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 743df50..3d0dc30 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
>   	mutex_lock(&tz->lock);
>   	result = tz->ops->set_mode(tz, mode);
>   	mutex_unlock(&tz->lock);
> +	thermal_zone_device_update(tz);
>
>   	if (result)
>   		return result;
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 15c0a9a..9a5a3a3 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
>   	data->mode = mode;
>   	ti_bandgap_write_update_interval(bgp, data->sensor_id,
>   					data->ti_thermal->polling_delay);
> -	thermal_zone_device_update(data->ti_thermal);
>   	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
>   		data->ti_thermal->polling_delay);
>
>

[PATCHv3 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

Because several drivers do the following pattern:
.set_mode()
   ...
   local_data->mode = new_mode;
   thermal_zone_device_update(tz);

makes sense to simply do the thermal_zone_device_update()
in thermal core, after setting the new mode.

Also, this patch also remove deadlocks on drivers that
call thermal_zone_device_update() on .set_mode(),
as .set_mode()  is now called always with tz->lock held.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---

Hey,


V2--> V3: Remove locking from OF thermal set mode.
The locking is done from thermal core already.

BR,

Eduardo

 drivers/acpi/thermal.c                             | 2 --
 drivers/platform/x86/acerhdf.c                     | 1 -
 drivers/thermal/imx_thermal.c                      | 1 -
 drivers/thermal/of-thermal.c                       | 5 -----
 drivers/thermal/thermal_sysfs.c                    | 1 +
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 1 -
 6 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 82707f9..8582b88 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
 
 	if (!tz->tz_enabled)
 		return;
-
-	thermal_zone_device_update(tz->thermal_zone);
 }
 
 /* sys I/F for generic thermal sysfs support */
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 460fa67..aee33ba 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
 	kernelmode = 1;
 
 	thz_dev->polling_delay = interval*1000;
-	thermal_zone_device_update(thz_dev);
 	pr_notice("kernel mode fan control ON\n");
 }
 
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c5547bd..a413eb6 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 	}
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..95c47da 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	mutex_lock(&tz->lock);
-
 	if (mode == THERMAL_DEVICE_ENABLED)
 		tz->polling_delay = data->polling_delay;
 	else
 		tz->polling_delay = 0;
 
-	mutex_unlock(&tz->lock);
-
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 743df50..3d0dc30 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&tz->lock);
 	result = tz->ops->set_mode(tz, mode);
 	mutex_unlock(&tz->lock);
+	thermal_zone_device_update(tz);
 
 	if (result)
 		return result;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..9a5a3a3 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
 	data->mode = mode;
 	ti_bandgap_write_update_interval(bgp, data->sensor_id,
 					data->ti_thermal->polling_delay);
-	thermal_zone_device_update(data->ti_thermal);
 	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
 		data->ti_thermal->polling_delay);
 
-- 
2.1.4

Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

On Wed, Jun 22, 2016 at 03:03:45PM +0530, Keerthy wrote:
> 
> 
> On Wednesday 22 June 2016 10:45 AM, Eduardo Valentin wrote:
> >Because several drivers do the following pattern:
> >.set_mode()
> >    ...
> >    local_data->mode = new_mode;
> >    thermal_zone_device_update(tz);
> >
> >makes sense to simply do the thermal_zone_device_update()
> >in thermal core, after setting the new mode.
> 
> http://pastebin.ubuntu.com/17687601/
> 
> linux-next + current v2 of the patch.
> The back trace still comes.

I confirm this. Please check V3 I just sent. On V2 I focused
on thermal_zone_device_update(), but for OF thermal specifically,
we also hold the lock for protecting other accesses. Now they
are removed.

> 
> Regards,
> Keerthy
> 
> >
> >Also, this patch also remove deadlocks on drivers that
> >call thermal_zone_device_update() on .set_mode(),
> >as .set_mode()  is now called always with tz->lock held.
> >
> >Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >Cc: Len Brown <lenb@kernel.org>
> >Cc: linux-acpi@vger.kernel.org
> >Cc: "Lee, Chun-Yi" <jlee@suse.com>
> >Cc: Darren Hart <dvhart@infradead.org>
> >Cc: Zhang Rui <rui.zhang@intel.com>
> >Cc: Keerthy <j-keerthy@ti.com>
> >Cc: linux-kernel@vger.kernel.org
> >Cc: linux-omap@vger.kernel.org
> >Cc: platform-driver-x86@vger.kernel.org
> >Cc: linux-pm@vger.kernel.org
> >Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> >---
> >Please ignore last version.
> >
> >V1-> V2:
> >Cleaned the patch and remove unrelated changes.
> >---
> >
> >  drivers/acpi/thermal.c                             | 2 --
> >  drivers/platform/x86/acerhdf.c                     | 1 -
> >  drivers/thermal/imx_thermal.c                      | 1 -
> >  drivers/thermal/of-thermal.c                       | 1 -
> >  drivers/thermal/thermal_sysfs.c                    | 1 +
> >  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 1 -
> >  6 files changed, 1 insertion(+), 6 deletions(-)
> >
> >diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> >index 82707f9..8582b88 100644
> >--- a/drivers/acpi/thermal.c
> >+++ b/drivers/acpi/thermal.c
> >@@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
> >
> >  	if (!tz->tz_enabled)
> >  		return;
> >-
> >-	thermal_zone_device_update(tz->thermal_zone);
> >  }
> >
> >  /* sys I/F for generic thermal sysfs support */
> >diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> >index 460fa67..aee33ba 100644
> >--- a/drivers/platform/x86/acerhdf.c
> >+++ b/drivers/platform/x86/acerhdf.c
> >@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
> >  	kernelmode = 1;
> >
> >  	thz_dev->polling_delay = interval*1000;
> >-	thermal_zone_device_update(thz_dev);
> >  	pr_notice("kernel mode fan control ON\n");
> >  }
> >
> >diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> >index c5547bd..a413eb6 100644
> >--- a/drivers/thermal/imx_thermal.c
> >+++ b/drivers/thermal/imx_thermal.c
> >@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
> >  	}
> >
> >  	data->mode = mode;
> >-	thermal_zone_device_update(tz);
> >
> >  	return 0;
> >  }
> >diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >index b8e509c..4602e2e 100644
> >--- a/drivers/thermal/of-thermal.c
> >+++ b/drivers/thermal/of-thermal.c
> >@@ -292,7 +292,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
> >  	mutex_unlock(&tz->lock);
> >
> >  	data->mode = mode;
> >-	thermal_zone_device_update(tz);
> >
> >  	return 0;
> >  }
> >diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> >index 743df50..3d0dc30 100644
> >--- a/drivers/thermal/thermal_sysfs.c
> >+++ b/drivers/thermal/thermal_sysfs.c
> >@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
> >  	mutex_lock(&tz->lock);
> >  	result = tz->ops->set_mode(tz, mode);
> >  	mutex_unlock(&tz->lock);
> >+	thermal_zone_device_update(tz);
> >
> >  	if (result)
> >  		return result;
> >diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >index 15c0a9a..9a5a3a3 100644
> >--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >@@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
> >  	data->mode = mode;
> >  	ti_bandgap_write_update_interval(bgp, data->sensor_id,
> >  					data->ti_thermal->polling_delay);
> >-	thermal_zone_device_update(data->ti_thermal);
> >  	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
> >  		data->ti_thermal->polling_delay);
> >
> >

[PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

Because several drivers do the following pattern:
.set_mode()
   ...
   local_data->mode = new_mode;
   thermal_zone_device_update(tz);

makes sense to simply do the thermal_zone_device_update()
in thermal core, after setting the new mode.

Also, this patch also remove deadlocks on drivers that
call thermal_zone_device_update() on .set_mode(),
as .set_mode()  is now called always with tz->lock held.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
Hello,

V3->V4:
- ti-soc: Removed extra locking from TI SoC in set_mode
- ACPI: Kept the update on check as it is called on other places

BR,

Eduardo

 drivers/acpi/thermal.c                             | 1 -
 drivers/platform/x86/acerhdf.c                     | 1 -
 drivers/thermal/imx_thermal.c                      | 1 -
 drivers/thermal/of-thermal.c                       | 5 -----
 drivers/thermal/thermal_sysfs.c                    | 1 +
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
 6 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 82707f9..03c3460 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -581,7 +581,6 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			"%s kernel ACPI thermal control\n",
 			tz->tz_enabled ? "Enable" : "Disable"));
-		acpi_thermal_check(tz);
 	}
 	return 0;
 }
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 460fa67..aee33ba 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
 	kernelmode = 1;
 
 	thz_dev->polling_delay = interval*1000;
-	thermal_zone_device_update(thz_dev);
 	pr_notice("kernel mode fan control ON\n");
 }
 
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c5547bd..a413eb6 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 	}
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..95c47da 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	mutex_lock(&tz->lock);
-
 	if (mode == THERMAL_DEVICE_ENABLED)
 		tz->polling_delay = data->polling_delay;
 	else
 		tz->polling_delay = 0;
 
-	mutex_unlock(&tz->lock);
-
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 743df50..3d0dc30 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	mutex_lock(&tz->lock);
 	result = tz->ops->set_mode(tz, mode);
 	mutex_unlock(&tz->lock);
+	thermal_zone_device_update(tz);
 
 	if (result)
 		return result;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..5dce053 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
 		return 0;
 	}
 
-	mutex_lock(&data->ti_thermal->lock);
-
 	if (mode == THERMAL_DEVICE_ENABLED)
 		data->ti_thermal->polling_delay = FAST_TEMP_MONITORING_RATE;
 	else
 		data->ti_thermal->polling_delay = 0;
 
-	mutex_unlock(&data->ti_thermal->lock);
-
 	data->mode = mode;
 	ti_bandgap_write_update_interval(bgp, data->sensor_id,
 					data->ti_thermal->polling_delay);
-	thermal_zone_device_update(data->ti_thermal);
 	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
 		data->ti_thermal->polling_delay);
 
-- 
2.1.4

Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

On Wed, Jun 22, 2016 at 07:36:11AM -0700, Eduardo Valentin wrote:
> On Wed, Jun 22, 2016 at 03:03:45PM +0530, Keerthy wrote:
> > 
> > 
> > On Wednesday 22 June 2016 10:45 AM, Eduardo Valentin wrote:
> > >Because several drivers do the following pattern:
> > >.set_mode()
> > >    ...
> > >    local_data->mode = new_mode;
> > >    thermal_zone_device_update(tz);
> > >
> > >makes sense to simply do the thermal_zone_device_update()
> > >in thermal core, after setting the new mode.
> > 
> > http://pastebin.ubuntu.com/17687601/
> > 
> > linux-next + current v2 of the patch.
> > The back trace still comes.
> 
> I confirm this. Please check V3 I just sent. On V2 I focused
> on thermal_zone_device_update(), but for OF thermal specifically,
> we also hold the lock for protecting other accesses. Now they
> are removed.

Please use v4. I removed extra non needed locking in ti SoC
driver for case which boots without DT (not sure if this is still used
though). Also, improved acpi thermal, as it was remove the update
from other needed cases.

> 
> > 
> > Regards,
> > Keerthy
> > 
> > >
> > >Also, this patch also remove deadlocks on drivers that
> > >call thermal_zone_device_update() on .set_mode(),
> > >as .set_mode()  is now called always with tz->lock held.
> > >
> > >Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > >Cc: Len Brown <lenb@kernel.org>
> > >Cc: linux-acpi@vger.kernel.org
> > >Cc: "Lee, Chun-Yi" <jlee@suse.com>
> > >Cc: Darren Hart <dvhart@infradead.org>
> > >Cc: Zhang Rui <rui.zhang@intel.com>
> > >Cc: Keerthy <j-keerthy@ti.com>
> > >Cc: linux-kernel@vger.kernel.org
> > >Cc: linux-omap@vger.kernel.org
> > >Cc: platform-driver-x86@vger.kernel.org
> > >Cc: linux-pm@vger.kernel.org
> > >Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> > >---
> > >Please ignore last version.
> > >
> > >V1-> V2:
> > >Cleaned the patch and remove unrelated changes.
> > >---
> > >
> > >  drivers/acpi/thermal.c                             | 2 --
> > >  drivers/platform/x86/acerhdf.c                     | 1 -
> > >  drivers/thermal/imx_thermal.c                      | 1 -
> > >  drivers/thermal/of-thermal.c                       | 1 -
> > >  drivers/thermal/thermal_sysfs.c                    | 1 +
> > >  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 1 -
> > >  6 files changed, 1 insertion(+), 6 deletions(-)
> > >
> > >diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > >index 82707f9..8582b88 100644
> > >--- a/drivers/acpi/thermal.c
> > >+++ b/drivers/acpi/thermal.c
> > >@@ -519,8 +519,6 @@ static void acpi_thermal_check(void *data)
> > >
> > >  	if (!tz->tz_enabled)
> > >  		return;
> > >-
> > >-	thermal_zone_device_update(tz->thermal_zone);
> > >  }
> > >
> > >  /* sys I/F for generic thermal sysfs support */
> > >diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> > >index 460fa67..aee33ba 100644
> > >--- a/drivers/platform/x86/acerhdf.c
> > >+++ b/drivers/platform/x86/acerhdf.c
> > >@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
> > >  	kernelmode = 1;
> > >
> > >  	thz_dev->polling_delay = interval*1000;
> > >-	thermal_zone_device_update(thz_dev);
> > >  	pr_notice("kernel mode fan control ON\n");
> > >  }
> > >
> > >diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > >index c5547bd..a413eb6 100644
> > >--- a/drivers/thermal/imx_thermal.c
> > >+++ b/drivers/thermal/imx_thermal.c
> > >@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
> > >  	}
> > >
> > >  	data->mode = mode;
> > >-	thermal_zone_device_update(tz);
> > >
> > >  	return 0;
> > >  }
> > >diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> > >index b8e509c..4602e2e 100644
> > >--- a/drivers/thermal/of-thermal.c
> > >+++ b/drivers/thermal/of-thermal.c
> > >@@ -292,7 +292,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
> > >  	mutex_unlock(&tz->lock);
> > >
> > >  	data->mode = mode;
> > >-	thermal_zone_device_update(tz);
> > >
> > >  	return 0;
> > >  }
> > >diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > >index 743df50..3d0dc30 100644
> > >--- a/drivers/thermal/thermal_sysfs.c
> > >+++ b/drivers/thermal/thermal_sysfs.c
> > >@@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
> > >  	mutex_lock(&tz->lock);
> > >  	result = tz->ops->set_mode(tz, mode);
> > >  	mutex_unlock(&tz->lock);
> > >+	thermal_zone_device_update(tz);
> > >
> > >  	if (result)
> > >  		return result;
> > >diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > >index 15c0a9a..9a5a3a3 100644
> > >--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > >+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > >@@ -205,7 +205,6 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
> > >  	data->mode = mode;
> > >  	ti_bandgap_write_update_interval(bgp, data->sensor_id,
> > >  					data->ti_thermal->polling_delay);
> > >-	thermal_zone_device_update(data->ti_thermal);
> > >  	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
> > >  		data->ti_thermal->polling_delay);
> > >
> > >

Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Keerthy]

On Wednesday 22 June 2016 08:33 PM, Eduardo Valentin wrote:
> Because several drivers do the following pattern:
> .set_mode()
>     ...
>     local_data->mode = new_mode;
>     thermal_zone_device_update(tz);
>
> makes sense to simply do the thermal_zone_device_update()
> in thermal core, after setting the new mode.
>
> Also, this patch also remove deadlocks on drivers that
> call thermal_zone_device_update() on .set_mode(),
> as .set_mode()  is now called always with tz->lock held.

I do not see the issue anymore. Boot Tested multiple times on DRA72-EVM.

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

>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> Cc: "Lee, Chun-Yi" <jlee@suse.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
> Hello,
>
> V3->V4:
> - ti-soc: Removed extra locking from TI SoC in set_mode
> - ACPI: Kept the update on check as it is called on other places
>
> BR,
>
> Eduardo
>
>   drivers/acpi/thermal.c                             | 1 -
>   drivers/platform/x86/acerhdf.c                     | 1 -
>   drivers/thermal/imx_thermal.c                      | 1 -
>   drivers/thermal/of-thermal.c                       | 5 -----
>   drivers/thermal/thermal_sysfs.c                    | 1 +
>   drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
>   6 files changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 82707f9..03c3460 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -581,7 +581,6 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>   		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>   			"%s kernel ACPI thermal control\n",
>   			tz->tz_enabled ? "Enable" : "Disable"));
> -		acpi_thermal_check(tz);
>   	}
>   	return 0;
>   }
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 460fa67..aee33ba 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
>   	kernelmode = 1;
>
>   	thz_dev->polling_delay = interval*1000;
> -	thermal_zone_device_update(thz_dev);
>   	pr_notice("kernel mode fan control ON\n");
>   }
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index c5547bd..a413eb6 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
>   	}
>
>   	data->mode = mode;
> -	thermal_zone_device_update(tz);
>
>   	return 0;
>   }
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..95c47da 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>   {
>   	struct __thermal_zone *data = tz->devdata;
>
> -	mutex_lock(&tz->lock);
> -
>   	if (mode == THERMAL_DEVICE_ENABLED)
>   		tz->polling_delay = data->polling_delay;
>   	else
>   		tz->polling_delay = 0;
>
> -	mutex_unlock(&tz->lock);
> -
>   	data->mode = mode;
> -	thermal_zone_device_update(tz);
>
>   	return 0;
>   }
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 743df50..3d0dc30 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
>   	mutex_lock(&tz->lock);
>   	result = tz->ops->set_mode(tz, mode);
>   	mutex_unlock(&tz->lock);
> +	thermal_zone_device_update(tz);
>
>   	if (result)
>   		return result;
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 15c0a9a..5dce053 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
>   		return 0;
>   	}
>
> -	mutex_lock(&data->ti_thermal->lock);
> -
>   	if (mode == THERMAL_DEVICE_ENABLED)
>   		data->ti_thermal->polling_delay = FAST_TEMP_MONITORING_RATE;
>   	else
>   		data->ti_thermal->polling_delay = 0;
>
> -	mutex_unlock(&data->ti_thermal->lock);
> -
>   	data->mode = mode;
>   	ti_bandgap_write_update_interval(bgp, data->sensor_id,
>   					data->ti_thermal->polling_delay);
> -	thermal_zone_device_update(data->ti_thermal);
>   	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
>   		data->ti_thermal->polling_delay);
>
>

Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Darren Hart]

On Wed, Jun 22, 2016 at 08:03:18AM -0700, Eduardo Valentin wrote:
> Because several drivers do the following pattern:
> .set_mode()
>    ...
>    local_data->mode = new_mode;
>    thermal_zone_device_update(tz);
> 
> makes sense to simply do the thermal_zone_device_update()
> in thermal core, after setting the new mode.
> 
> Also, this patch also remove deadlocks on drivers that
> call thermal_zone_device_update() on .set_mode(),
> as .set_mode()  is now called always with tz->lock held.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> Cc: "Lee, Chun-Yi" <jlee@suse.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>

+Peter Feuerer <peter@piie.net>

Peter, any concerns regarding the acerhdf driver?

> ---
> Hello,
> 
> V3->V4:
> - ti-soc: Removed extra locking from TI SoC in set_mode
> - ACPI: Kept the update on check as it is called on other places
> 
> BR,
> 
> Eduardo
> 
>  drivers/acpi/thermal.c                             | 1 -
>  drivers/platform/x86/acerhdf.c                     | 1 -
>  drivers/thermal/imx_thermal.c                      | 1 -
>  drivers/thermal/of-thermal.c                       | 5 -----
>  drivers/thermal/thermal_sysfs.c                    | 1 +
>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
>  6 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 82707f9..03c3460 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -581,7 +581,6 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			"%s kernel ACPI thermal control\n",
>  			tz->tz_enabled ? "Enable" : "Disable"));
> -		acpi_thermal_check(tz);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 460fa67..aee33ba 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
>  	kernelmode = 1;
>  
>  	thz_dev->polling_delay = interval*1000;
> -	thermal_zone_device_update(thz_dev);
>  	pr_notice("kernel mode fan control ON\n");
>  }
>  
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index c5547bd..a413eb6 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
>  	}
>  
>  	data->mode = mode;
> -	thermal_zone_device_update(tz);
>  
>  	return 0;
>  }
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b8e509c..95c47da 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>  {
>  	struct __thermal_zone *data = tz->devdata;
>  
> -	mutex_lock(&tz->lock);
> -
>  	if (mode == THERMAL_DEVICE_ENABLED)
>  		tz->polling_delay = data->polling_delay;
>  	else
>  		tz->polling_delay = 0;
>  
> -	mutex_unlock(&tz->lock);
> -
>  	data->mode = mode;
> -	thermal_zone_device_update(tz);
>  
>  	return 0;
>  }
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 743df50..3d0dc30 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
>  	mutex_lock(&tz->lock);
>  	result = tz->ops->set_mode(tz, mode);
>  	mutex_unlock(&tz->lock);
> +	thermal_zone_device_update(tz);
>  
>  	if (result)
>  		return result;
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 15c0a9a..5dce053 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
>  		return 0;
>  	}
>  
> -	mutex_lock(&data->ti_thermal->lock);
> -
>  	if (mode == THERMAL_DEVICE_ENABLED)
>  		data->ti_thermal->polling_delay = FAST_TEMP_MONITORING_RATE;
>  	else
>  		data->ti_thermal->polling_delay = 0;
>  
> -	mutex_unlock(&data->ti_thermal->lock);
> -
>  	data->mode = mode;
>  	ti_bandgap_write_update_interval(bgp, data->sensor_id,
>  					data->ti_thermal->polling_delay);
> -	thermal_zone_device_update(data->ti_thermal);
>  	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
>  		data->ti_thermal->polling_delay);
>  
> -- 
> 2.1.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Rafael J. Wysocki]

On Wed, Jun 22, 2016 at 7:06 AM, Eduardo Valentin <edubezval@gmail.com> wrote:
> Because several drivers do the following pattern:
> .set_mode()
>    ...
>    local_data->mode = new_mode;
>    thermal_zone_device_update(tz);
>
> makes sense to simply do the thermal_zone_device_update()
> in thermal core, after setting the new mode.
>
> Also, this patch also remove deadlocks on drivers that
> call thermal_zone_device_update() on .set_mode(),
> as .set_mode()  is now called always with tz->lock held.

To me, this part of the patch is way more important than the
optimization mentioned before.

Apparently, the problem is that drivers deadlock, because the
thermal_zone_device_update() invoked from ->set_mode() is called under
tz->lock.

So to address that problem you make the core call
thermal_zone_device_update() after ->set_mode() outside of tz->lock
and the drivers don't have to do it any more.

Is that correct?

Thanks,
Rafael

Re: [PATCH 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Keerthy]

On Thursday 23 June 2016 05:57 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 22, 2016 at 7:06 AM, Eduardo Valentin <edubezval@gmail.com> wrote:
>> Because several drivers do the following pattern:
>> .set_mode()
>>     ...
>>     local_data->mode = new_mode;
>>     thermal_zone_device_update(tz);
>>
>> makes sense to simply do the thermal_zone_device_update()
>> in thermal core, after setting the new mode.
>>
>> Also, this patch also remove deadlocks on drivers that
>> call thermal_zone_device_update() on .set_mode(),
>> as .set_mode()  is now called always with tz->lock held.
>
> To me, this part of the patch is way more important than the
> optimization mentioned before.
>
> Apparently, the problem is that drivers deadlock, because the
> thermal_zone_device_update() invoked from ->set_mode() is called under
> tz->lock.
>
> So to address that problem you make the core call
> thermal_zone_device_update() after ->set_mode() outside of tz->lock
> and the drivers don't have to do it any more.
>
> Is that correct?

Rafael,

On my set up, mode_store locks tz->lock and eventually ends up calling 
of_thermal_set_mode before releasing tz->lock which again tries to lock 
tz->lock and ends up in a deadlock.

http://pastebin.ubuntu.com/17687601/

Regards,
Keerthy
>
> Thanks,
> Rafael
>

Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Zhang Rui]

Hi, Eduardo,

as the locking patch set has not been in upstream yet, you should
either fold this fix into patch 02/15, or rebase your locking patch
series on top of this fix.

thanks,
rui

On 三, 2016-06-22 at 08:03 -0700, Eduardo Valentin wrote:
> Because several drivers do the following pattern:
> .set_mode()
>    ...
>    local_data->mode = new_mode;
>    thermal_zone_device_update(tz);
> 
> makes sense to simply do the thermal_zone_device_update()
> in thermal core, after setting the new mode.
> 
> Also, this patch also remove deadlocks on drivers that
> call thermal_zone_device_update() on .set_mode(),
> as .set_mode()  is now called always with tz->lock held.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> Cc: "Lee, Chun-Yi" <jlee@suse.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
> Hello,
> 
> V3->V4:
> - ti-soc: Removed extra locking from TI SoC in set_mode
> - ACPI: Kept the update on check as it is called on other places
> 
> BR,
> 
> Eduardo
> 
>  drivers/acpi/thermal.c                             | 1 -
>  drivers/platform/x86/acerhdf.c                     | 1 -
>  drivers/thermal/imx_thermal.c                      | 1 -
>  drivers/thermal/of-thermal.c                       | 5 -----
>  drivers/thermal/thermal_sysfs.c                    | 1 +
>  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
>  6 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 82707f9..03c3460 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -581,7 +581,6 @@ static int thermal_set_mode(struct
> thermal_zone_device *thermal,
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			"%s kernel ACPI thermal control\n",
>  			tz->tz_enabled ? "Enable" : "Disable"));
> -		acpi_thermal_check(tz);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/acerhdf.c
> b/drivers/platform/x86/acerhdf.c
> index 460fa67..aee33ba 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -405,7 +405,6 @@ static inline void
> acerhdf_enable_kernelmode(void)
>  	kernelmode = 1;
>  
>  	thz_dev->polling_delay = interval*1000;
> -	thermal_zone_device_update(thz_dev);
>  	pr_notice("kernel mode fan control ON\n");
>  }
>  
> diff --git a/drivers/thermal/imx_thermal.c
> b/drivers/thermal/imx_thermal.c
> index c5547bd..a413eb6 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -246,7 +246,6 @@ static int imx_set_mode(struct
> thermal_zone_device *tz,
>  	}
>  
>  	data->mode = mode;
> -	thermal_zone_device_update(tz);
>  
>  	return 0;
>  }
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
> thermal.c
> index b8e509c..95c47da 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct
> thermal_zone_device *tz,
>  {
>  	struct __thermal_zone *data = tz->devdata;
>  
> -	mutex_lock(&tz->lock);
> -
>  	if (mode == THERMAL_DEVICE_ENABLED)
>  		tz->polling_delay = data->polling_delay;
>  	else
>  		tz->polling_delay = 0;
>  
> -	mutex_unlock(&tz->lock);
> -
>  	data->mode = mode;
> -	thermal_zone_device_update(tz);
>  
>  	return 0;
>  }
> diff --git a/drivers/thermal/thermal_sysfs.c
> b/drivers/thermal/thermal_sysfs.c
> index 743df50..3d0dc30 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct
> device_attribute *attr,
>  	mutex_lock(&tz->lock);
>  	result = tz->ops->set_mode(tz, mode);
>  	mutex_unlock(&tz->lock);
> +	thermal_zone_device_update(tz);
>  
>  	if (result)
>  		return result;
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 15c0a9a..5dce053 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct
> thermal_zone_device *thermal,
>  		return 0;
>  	}
>  
> -	mutex_lock(&data->ti_thermal->lock);
> -
>  	if (mode == THERMAL_DEVICE_ENABLED)
>  		data->ti_thermal->polling_delay =
> FAST_TEMP_MONITORING_RATE;
>  	else
>  		data->ti_thermal->polling_delay = 0;
>  
> -	mutex_unlock(&data->ti_thermal->lock);
> -
>  	data->mode = mode;
>  	ti_bandgap_write_update_interval(bgp, data->sensor_id,
>  					data->ti_thermal-
> >polling_delay);
> -	thermal_zone_device_update(data->ti_thermal);
>  	dev_dbg(&thermal->device, "thermal polling set for
> duration=%d msec\n",
>  		data->ti_thermal->polling_delay);
>  

Re: [PATCH 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

On Thu, Jun 23, 2016 at 02:27:12PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 22, 2016 at 7:06 AM, Eduardo Valentin <edubezval@gmail.com> wrote:
> > Because several drivers do the following pattern:
> > .set_mode()
> >    ...
> >    local_data->mode = new_mode;
> >    thermal_zone_device_update(tz);
> >
> > makes sense to simply do the thermal_zone_device_update()
> > in thermal core, after setting the new mode.
> >
> > Also, this patch also remove deadlocks on drivers that
> > call thermal_zone_device_update() on .set_mode(),
> > as .set_mode()  is now called always with tz->lock held.
> 
> To me, this part of the patch is way more important than the
> optimization mentioned before.
> 
> Apparently, the problem is that drivers deadlock, because the
> thermal_zone_device_update() invoked from ->set_mode() is called under
> tz->lock.
> 
> So to address that problem you make the core call
> thermal_zone_device_update() after ->set_mode() outside of tz->lock
> and the drivers don't have to do it any more.
> 
> Is that correct?

Yes this is correct. The optimization is simply a consequence of the bug
fix, as reported by Keerthy.

> 
> Thanks,
> Rafael

Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Eduardo Valentin]

On Wed, Jun 29, 2016 at 02:23:22PM +0800, Zhang Rui wrote:
> Hi, Eduardo,
> 
> as the locking patch set has not been in upstream yet, you should
> either fold this fix into patch 02/15, or rebase your locking patch
> series on top of this fix.

I am folding into 02/15 and resending only the new version of that
patch. Are you OK to pick it up like that?

> 
> thanks,
> rui
> 
> On 三, 2016-06-22 at 08:03 -0700, Eduardo Valentin wrote:
> > Because several drivers do the following pattern:
> > .set_mode()
> >    ...
> >    local_data->mode = new_mode;
> >    thermal_zone_device_update(tz);
> > 
> > makes sense to simply do the thermal_zone_device_update()
> > in thermal core, after setting the new mode.
> > 
> > Also, this patch also remove deadlocks on drivers that
> > call thermal_zone_device_update() on .set_mode(),
> > as .set_mode()  is now called always with tz->lock held.
> > 
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: linux-acpi@vger.kernel.org
> > Cc: "Lee, Chun-Yi" <jlee@suse.com>
> > Cc: Darren Hart <dvhart@infradead.org>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Keerthy <j-keerthy@ti.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-omap@vger.kernel.org
> > Cc: platform-driver-x86@vger.kernel.org
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> > ---
> > Hello,
> > 
> > V3->V4:
> > - ti-soc: Removed extra locking from TI SoC in set_mode
> > - ACPI: Kept the update on check as it is called on other places
> > 
> > BR,
> > 
> > Eduardo
> > 
> >  drivers/acpi/thermal.c                             | 1 -
> >  drivers/platform/x86/acerhdf.c                     | 1 -
> >  drivers/thermal/imx_thermal.c                      | 1 -
> >  drivers/thermal/of-thermal.c                       | 5 -----
> >  drivers/thermal/thermal_sysfs.c                    | 1 +
> >  drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
> >  6 files changed, 1 insertion(+), 13 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 82707f9..03c3460 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -581,7 +581,6 @@ static int thermal_set_mode(struct
> > thermal_zone_device *thermal,
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  			"%s kernel ACPI thermal control\n",
> >  			tz->tz_enabled ? "Enable" : "Disable"));
> > -		acpi_thermal_check(tz);
> >  	}
> >  	return 0;
> >  }
> > diff --git a/drivers/platform/x86/acerhdf.c
> > b/drivers/platform/x86/acerhdf.c
> > index 460fa67..aee33ba 100644
> > --- a/drivers/platform/x86/acerhdf.c
> > +++ b/drivers/platform/x86/acerhdf.c
> > @@ -405,7 +405,6 @@ static inline void
> > acerhdf_enable_kernelmode(void)
> >  	kernelmode = 1;
> >  
> >  	thz_dev->polling_delay = interval*1000;
> > -	thermal_zone_device_update(thz_dev);
> >  	pr_notice("kernel mode fan control ON\n");
> >  }
> >  
> > diff --git a/drivers/thermal/imx_thermal.c
> > b/drivers/thermal/imx_thermal.c
> > index c5547bd..a413eb6 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -246,7 +246,6 @@ static int imx_set_mode(struct
> > thermal_zone_device *tz,
> >  	}
> >  
> >  	data->mode = mode;
> > -	thermal_zone_device_update(tz);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-
> > thermal.c
> > index b8e509c..95c47da 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct
> > thermal_zone_device *tz,
> >  {
> >  	struct __thermal_zone *data = tz->devdata;
> >  
> > -	mutex_lock(&tz->lock);
> > -
> >  	if (mode == THERMAL_DEVICE_ENABLED)
> >  		tz->polling_delay = data->polling_delay;
> >  	else
> >  		tz->polling_delay = 0;
> >  
> > -	mutex_unlock(&tz->lock);
> > -
> >  	data->mode = mode;
> > -	thermal_zone_device_update(tz);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/thermal/thermal_sysfs.c
> > b/drivers/thermal/thermal_sysfs.c
> > index 743df50..3d0dc30 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct
> > device_attribute *attr,
> >  	mutex_lock(&tz->lock);
> >  	result = tz->ops->set_mode(tz, mode);
> >  	mutex_unlock(&tz->lock);
> > +	thermal_zone_device_update(tz);
> >  
> >  	if (result)
> >  		return result;
> > diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > index 15c0a9a..5dce053 100644
> > --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > @@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct
> > thermal_zone_device *thermal,
> >  		return 0;
> >  	}
> >  
> > -	mutex_lock(&data->ti_thermal->lock);
> > -
> >  	if (mode == THERMAL_DEVICE_ENABLED)
> >  		data->ti_thermal->polling_delay =
> > FAST_TEMP_MONITORING_RATE;
> >  	else
> >  		data->ti_thermal->polling_delay = 0;
> >  
> > -	mutex_unlock(&data->ti_thermal->lock);
> > -
> >  	data->mode = mode;
> >  	ti_bandgap_write_update_interval(bgp, data->sensor_id,
> >  					data->ti_thermal-
> > >polling_delay);
> > -	thermal_zone_device_update(data->ti_thermal);
> >  	dev_dbg(&thermal->device, "thermal polling set for
> > duration=%d msec\n",
> >  		data->ti_thermal->polling_delay);
> >  

[PATCHv2 1/1] thermal: sysfs: lock tz while on access to mode properties

[Eduardo Valentin]

Serialized calls to tz.ops in user facing
sysfs handler mode_show()  and mode_store().

Because several drivers do the following pattern:
.set_mode()
   ...
   local_data->mode = new_mode;
   thermal_zone_device_update(tz);

makes sense to simply do the thermal_zone_device_update()
in thermal core, after setting the new mode.

as .set_mode()  is now called always with tz->lock held.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/acpi/thermal.c                             |  1 -
 drivers/platform/x86/acerhdf.c                     |  1 -
 drivers/thermal/imx_thermal.c                      |  1 -
 drivers/thermal/of-thermal.c                       |  5 -----
 drivers/thermal/thermal_sysfs.c                    | 14 +++++++++++---
 drivers/thermal/ti-soc-thermal/ti-thermal-common.c |  5 -----
 6 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 82707f9..03c3460 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -581,7 +581,6 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			"%s kernel ACPI thermal control\n",
 			tz->tz_enabled ? "Enable" : "Disable"));
-		acpi_thermal_check(tz);
 	}
 	return 0;
 }
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 460fa67..aee33ba 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
 	kernelmode = 1;
 
 	thz_dev->polling_delay = interval*1000;
-	thermal_zone_device_update(thz_dev);
 	pr_notice("kernel mode fan control ON\n");
 }
 
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c5547bd..a413eb6 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
 	}
 
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b8e509c..95c47da 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	mutex_lock(&tz->lock);
-
 	if (mode == THERMAL_DEVICE_ENABLED)
 		tz->polling_delay = data->polling_delay;
 	else
 		tz->polling_delay = 0;
 
-	mutex_unlock(&tz->lock);
-
 	data->mode = mode;
-	thermal_zone_device_update(tz);
 
 	return 0;
 }
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index ee983ca..1e223d4 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -62,7 +62,9 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf)
 	if (!tz->ops->get_mode)
 		return -EPERM;
 
+	mutex_lock(&tz->lock);
 	result = tz->ops->get_mode(tz, &mode);
+	mutex_unlock(&tz->lock);
 	if (result)
 		return result;
 
@@ -75,17 +77,23 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	   const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	enum thermal_device_mode mode = THERMAL_DEVICE_DISABLED;
 	int result;
 
 	if (!tz->ops->set_mode)
 		return -EPERM;
 
 	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
+		mode = THERMAL_DEVICE_ENABLED;
 	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
+		mode = THERMAL_DEVICE_DISABLED;
 	else
-		result = -EINVAL;
+		return -EINVAL;
+
+	mutex_lock(&tz->lock);
+	result = tz->ops->set_mode(tz, mode);
+	mutex_unlock(&tz->lock);
+	thermal_zone_device_update(tz);
 
 	if (result)
 		return result;
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 15c0a9a..5dce053 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
 		return 0;
 	}
 
-	mutex_lock(&data->ti_thermal->lock);
-
 	if (mode == THERMAL_DEVICE_ENABLED)
 		data->ti_thermal->polling_delay = FAST_TEMP_MONITORING_RATE;
 	else
 		data->ti_thermal->polling_delay = 0;
 
-	mutex_unlock(&data->ti_thermal->lock);
-
 	data->mode = mode;
 	ti_bandgap_write_update_interval(bgp, data->sensor_id,
 					data->ti_thermal->polling_delay);
-	thermal_zone_device_update(data->ti_thermal);
 	dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
 		data->ti_thermal->polling_delay);
 
-- 
2.1.4

Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update

[Peter Feuerer]

Hi,

23. Juni 2016 06:52 Uhr, "Darren Hart" <dvhart@infradead.org> schrieb:
> On Wed, Jun 22, 2016 at 08:03:18AM -0700, Eduardo Valentin wrote:
> 
>> Because several drivers do the following pattern:
>> .set_mode()
>> ...
>> local_data->mode = new_mode;
>> thermal_zone_device_update(tz);
>> 
>> makes sense to simply do the thermal_zone_device_update()
>> in thermal core, after setting the new mode.
>> 
>> Also, this patch also remove deadlocks on drivers that
>> call thermal_zone_device_update() on .set_mode(),
>> as .set_mode() is now called always with tz->lock held.
>> 
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-acpi@vger.kernel.org
>> Cc: "Lee, Chun-Yi" <jlee@suse.com>
>> Cc: Darren Hart <dvhart@infradead.org>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Keerthy <j-keerthy@ti.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-omap@vger.kernel.org
>> Cc: platform-driver-x86@vger.kernel.org
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> 
> +Peter Feuerer <peter@piie.net>
> 
> Peter, any concerns regarding the acerhdf driver?

Yes, please see below.



>> ---
>> Hello,
>> 
>> V3->V4:
>> - ti-soc: Removed extra locking from TI SoC in set_mode
>> - ACPI: Kept the update on check as it is called on other places
>> 
>> BR,
>> 
>> Eduardo
>> 
>> drivers/acpi/thermal.c | 1 -
>> drivers/platform/x86/acerhdf.c | 1 -
>> drivers/thermal/imx_thermal.c | 1 -
>> drivers/thermal/of-thermal.c | 5 -----
>> drivers/thermal/thermal_sysfs.c | 1 +

This file does not exist in Linus git master branch.  Which git repo / branch do you base your changes on?  Or are there any patches I need to apply first?


>> drivers/thermal/ti-soc-thermal/ti-thermal-common.c | 5 -----
>> 6 files changed, 1 insertion(+), 13 deletions(-)
>> 
>> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> index 82707f9..03c3460 100644
>> --- a/drivers/acpi/thermal.c
>> +++ b/drivers/acpi/thermal.c
>> @@ -581,7 +581,6 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "%s kernel ACPI thermal control\n",
>> tz->tz_enabled ? "Enable" : "Disable"));
>> - acpi_thermal_check(tz);
>> }
>> return 0;
>> }
>> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
>> index 460fa67..aee33ba 100644
>> --- a/drivers/platform/x86/acerhdf.c
>> +++ b/drivers/platform/x86/acerhdf.c
>> @@ -405,7 +405,6 @@ static inline void acerhdf_enable_kernelmode(void)
>> kernelmode = 1;
>> 
>> thz_dev->polling_delay = interval*1000;
>> - thermal_zone_device_update(thz_dev);

This call is used to inform the thermal zone about the changed polling_delay from 0 (polling disabled) to some user defined interval at runtime - not initialization time.
>From just reading your patch I don't understand, how this is intended to work afterwards.  - I'm clearly missing some further info / patches to give my ok.


>> pr_notice("kernel mode fan control ON\n");
>> }
>> 
>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>> index c5547bd..a413eb6 100644
>> --- a/drivers/thermal/imx_thermal.c
>> +++ b/drivers/thermal/imx_thermal.c
>> @@ -246,7 +246,6 @@ static int imx_set_mode(struct thermal_zone_device *tz,
>> }
>> 
>> data->mode = mode;
>> - thermal_zone_device_update(tz);
>> 
>> return 0;
>> }
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index b8e509c..95c47da 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -282,17 +282,12 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>> {
>> struct __thermal_zone *data = tz->devdata;
>> 
>> - mutex_lock(&tz->lock);
>> -
>> if (mode == THERMAL_DEVICE_ENABLED)
>> tz->polling_delay = data->polling_delay;
>> else
>> tz->polling_delay = 0;
>> 
>> - mutex_unlock(&tz->lock);
>> -
>> data->mode = mode;
>> - thermal_zone_device_update(tz);
>> 
>> return 0;
>> }
>> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
>> index 743df50..3d0dc30 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -100,6 +100,7 @@ mode_store(struct device *dev, struct device_attribute *attr,
>> mutex_lock(&tz->lock);
>> result = tz->ops->set_mode(tz, mode);
>> mutex_unlock(&tz->lock);
>> + thermal_zone_device_update(tz);
>> 
>> if (result)
>> return result;
>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> index 15c0a9a..5dce053 100644
>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> @@ -193,19 +193,14 @@ static int ti_thermal_set_mode(struct thermal_zone_device *thermal,
>> return 0;
>> }
>> 
>> - mutex_lock(&data->ti_thermal->lock);
>> -
>> if (mode == THERMAL_DEVICE_ENABLED)
>> data->ti_thermal->polling_delay = FAST_TEMP_MONITORING_RATE;
>> else
>> data->ti_thermal->polling_delay = 0;
>> 
>> - mutex_unlock(&data->ti_thermal->lock);
>> -
>> data->mode = mode;
>> ti_bandgap_write_update_interval(bgp, data->sensor_id,
>> data->ti_thermal->polling_delay);
>> - thermal_zone_device_update(data->ti_thermal);
>> dev_dbg(&thermal->device, "thermal polling set for duration=%d msec\n",
>> data->ti_thermal->polling_delay);
>> 
>> --
>> 2.1.4

-- 
--peter;