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@xxxxxxxxxxxxx>
> > >Cc: Len Brown <lenb@xxxxxxxxxx>
> > >Cc: linux-acpi@xxxxxxxxxxxxxxx
> > >Cc: "Lee, Chun-Yi" <jlee@xxxxxxxx>
> > >Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > >Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > >Cc: Keerthy <j-keerthy@xxxxxx>
> > >Cc: linux-kernel@xxxxxxxxxxxxxxx
> > >Cc: linux-omap@xxxxxxxxxxxxxxx
> > >Cc: platform-driver-x86@xxxxxxxxxxxxxxx
> > >Cc: linux-pm@xxxxxxxxxxxxxxx
> > >Signed-off-by: Eduardo Valentin <edubezval@xxxxxxxxx>
> > >---
> > >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);
> > >
> > >