Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1428364 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 02/15] thermal: sysfs: lock tz while on access to mode  properties Zhang Rui <rui.zhang@intel.com> - 2016-06-22 04:50 +0200
    [PATCH 1/1] thermal: core: call thermal_zone_device_update() after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 07:10 +0200
      Re: [PATCH 1/1] thermal: core: call thermal_zone_device_update()  after mode update "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-23 14:30 +0200
        Re: [PATCH 1/1] thermal: core: call thermal_zone_device_update()  after mode update Keerthy <a0393675@ti.com> - 2016-06-23 14:40 +0200
    [PATCHv2 1/1] thermal: core: call thermal_zone_device_update() after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 07:20 +0200
      Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update()  after mode update Keerthy <a0393675@ti.com> - 2016-06-22 11:40 +0200
        Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update()  after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 16:40 +0200
          Re: [PATCHv2 1/1] thermal: core: call thermal_zone_device_update()  after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 17:10 +0200
    [PATCHv3 1/1] thermal: core: call thermal_zone_device_update() after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 16:40 +0200
    [PATCHv4 1/1] thermal: core: call thermal_zone_device_update() after mode update Eduardo Valentin <edubezval@gmail.com> - 2016-06-22 17:10 +0200
      Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update()  after mode update Keerthy <a0393675@ti.com> - 2016-06-23 06:40 +0200
      Re: [PATCHv4 1/1] thermal: core: call thermal_zone_device_update()  after mode update Darren Hart <dvhart@infradead.org> - 2016-06-23 07:00 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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);
>
>

[toc] | [prev] | [next] | [standalone]

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

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);
> >
> >

[toc] | [prev] | [next] | [standalone]

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

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);
> > >
> > >

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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);
>
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web