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

Groups > linux.kernel > #1233615 > unrolled thread

[PATCH 3/3] Thermal: do thermal zone update after a cooling device registered

Back to article view | Back to linux.kernel

Contents

  [PATCH 3/3] Thermal: do thermal zone update after a cooling device registered Chen Yu <yu.c.chen@intel.com> - 2015-09-27 07:50 +0200
    Re: [PATCH 3/3] Thermal: do thermal zone update after a cooling  device registered Javi Merino <javi.merino@arm.com> - 2015-09-28 16:30 +0200
      RE: [PATCH 3/3] Thermal: do thermal zone update after a cooling  device registered "Chen, Yu C" <yu.c.chen@intel.com> - 2015-09-28 20:00 +0200
        Re: [PATCH 3/3] Thermal: do thermal zone update after a cooling  device registered Javi Merino <javi.merino@arm.com> - 2015-09-29 18:10 +0200

#1233615 — [PATCH 3/3] Thermal: do thermal zone update after a cooling device registered

From: Zhang Rui <rui.zhang@intel.com>

When a new cooling device is registered, we need to update the
thermal zone to set the new registered cooling device to a proper
state.

This fixes a problem that the system is cool, while the fan devices
are left running on full speed after boot, if fan device is registered
after thermal zone device.

CC: <stable@vger.kernel.org> #3.18+
Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431
Tested-by: Manuel Krause <manuelkrause@netscape.net>
Tested-by: szegad <szegadlo@poczta.onet.pl>
Tested-by: prash <prash.n.rao@gmail.com>
Tested-by: amish <ammdispose-arch@yahoo.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/thermal/thermal_core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c3bdb48..09c78a4 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1450,6 +1450,7 @@ __thermal_cooling_device_register(struct device_node *np,
 				  const struct thermal_cooling_device_ops *ops)
 {
 	struct thermal_cooling_device *cdev;
+	struct thermal_instance *pos, *next;
 	int result;
 
 	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
@@ -1494,6 +1495,15 @@ __thermal_cooling_device_register(struct device_node *np,
 	/* Update binding information for 'this' new cdev */
 	bind_cdev(cdev);
 
+	list_for_each_entry_safe(pos, next, &cdev->thermal_instances, cdev_node) {
+			if (next->cdev_node.next == &cdev->thermal_instances) {
+				thermal_zone_device_update(next->tz);
+				break;
+			}
+			if (pos->tz != next->tz)
+				thermal_zone_device_update(pos->tz);
+	}
+
 	return cdev;
 }
 
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1234199 — Re: [PATCH 3/3] Thermal: do thermal zone update after a cooling device registered

On Sun, Sep 27, 2015 at 06:48:44AM +0100, Chen Yu wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> 
> When a new cooling device is registered, we need to update the
> thermal zone to set the new registered cooling device to a proper
> state.
> 
> This fixes a problem that the system is cool, while the fan devices
> are left running on full speed after boot, if fan device is registered
> after thermal zone device.
> 
> CC: <stable@vger.kernel.org> #3.18+
> Reference:https://bugzilla.kernel.org/show_bug.cgi?id=92431
> Tested-by: Manuel Krause <manuelkrause@netscape.net>
> Tested-by: szegad <szegadlo@poczta.onet.pl>
> Tested-by: prash <prash.n.rao@gmail.com>
> Tested-by: amish <ammdispose-arch@yahoo.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/thermal/thermal_core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index c3bdb48..09c78a4 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1450,6 +1450,7 @@ __thermal_cooling_device_register(struct device_node *np,
>  				  const struct thermal_cooling_device_ops *ops)
>  {
>  	struct thermal_cooling_device *cdev;
> +	struct thermal_instance *pos, *next;
>  	int result;
>  
>  	if (type && strlen(type) >= THERMAL_NAME_LENGTH)
> @@ -1494,6 +1495,15 @@ __thermal_cooling_device_register(struct device_node *np,
>  	/* Update binding information for 'this' new cdev */
>  	bind_cdev(cdev);
>  

I think you need to hold cdev->lock here, to make sure that no thermal
zone is added or removed from cdev->thermal_instances while you are looping.

> +	list_for_each_entry_safe(pos, next, &cdev->thermal_instances, cdev_node) {

Why list_for_each_entry_safe() ?  You are not going to remove any
entry, so you can just use list_for_each_entry()

> +			if (next->cdev_node.next == &cdev->thermal_instances) {
> +				thermal_zone_device_update(next->tz);
> +				break;
> +			}
> +			if (pos->tz != next->tz)
> +				thermal_zone_device_update(pos->tz);
> +	}

Why is this so complicated?  Can't you just do:

	list_for_each_entry(pos, &cdev->thermal_instances, cdev_node)
        	thermal_zone_device_update(pos->tz);

Cheers,
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1234330 — RE: [PATCH 3/3] Thermal: do thermal zone update after a cooling device registered

SGksIEphdmksDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogSmF2aSBN
ZXJpbm8gW21haWx0bzpqYXZpLm1lcmlub0Bhcm0uY29tXQ0KPiBTZW50OiBNb25kYXksIFNlcHRl
bWJlciAyOCwgMjAxNSAxMDoyOSBQTQ0KPiBUbzogQ2hlbiwgWXUgQw0KPiBDYzogbGludXgtcG1A
dmdlci5rZXJuZWwub3JnOyBlZHViZXp2YWxAZ21haWwuY29tOyBaaGFuZywgUnVpOyBsaW51eC0N
Cj4ga2VybmVsQHZnZXIua2VybmVsLm9yZzsgc3RhYmxlQHZnZXIua2VybmVsLm9yZw0KPiBTdWJq
ZWN0OiBSZTogW1BBVENIIDMvM10gVGhlcm1hbDogZG8gdGhlcm1hbCB6b25lIHVwZGF0ZSBhZnRl
ciBhIGNvb2xpbmcNCj4gZGV2aWNlIHJlZ2lzdGVyZWQNCj4gDQo+IE9uIFN1biwgU2VwIDI3LCAy
MDE1IGF0IDA2OjQ4OjQ0QU0gKzAxMDAsIENoZW4gWXUgd3JvdGU6DQo+ID4gRnJvbTogWmhhbmcg
UnVpIDxydWkuemhhbmdAaW50ZWwuY29tPg0KPiA+DQo+ID4NCj4gDQo+IEkgdGhpbmsgeW91IG5l
ZWQgdG8gaG9sZCBjZGV2LT5sb2NrIGhlcmUsIHRvIG1ha2Ugc3VyZSB0aGF0IG5vIHRoZXJtYWwg
em9uZQ0KPiBpcyBhZGRlZCBvciByZW1vdmVkIGZyb20gY2Rldi0+dGhlcm1hbF9pbnN0YW5jZXMg
d2hpbGUgeW91IGFyZSBsb29waW5nLg0KPiANCkFoIHJpZ2h0LCB3aWxsIGFkZC4gSWYgSSBhZGQg
dGhlIGNkZXYgLT5sb2NrIGhlcmUsIHdpbGwgdGhlcmUgYmUgYSBBQi1CQSBsb2NrIHdpdGggDQp0
aGVybWFsX3pvbmVfdW5iaW5kX2Nvb2xpbmdfZGV2aWNlPw0KDQo+IA0KPiBXaHkgbGlzdF9mb3Jf
ZWFjaF9lbnRyeV9zYWZlKCkgPyAgWW91IGFyZSBub3QgZ29pbmcgdG8gcmVtb3ZlIGFueSBlbnRy
eSwgc28NCj4geW91IGNhbiBqdXN0IHVzZSBsaXN0X2Zvcl9lYWNoX2VudHJ5KCkNCj4gDQo+IA0K
PiBXaHkgaXMgdGhpcyBzbyBjb21wbGljYXRlZD8gIENhbid0IHlvdSBqdXN0IGRvOg0KPiANCj4g
CWxpc3RfZm9yX2VhY2hfZW50cnkocG9zLCAmY2Rldi0+dGhlcm1hbF9pbnN0YW5jZXMsIGNkZXZf
bm9kZSkNCj4gICAgICAgICAJdGhlcm1hbF96b25lX2RldmljZV91cGRhdGUocG9zLT50eik7DQo+
IA0KDQpUaGlzIGlzIGFuIG9wdGltaXphdGlvbiBoZXJlOg0KSWdub3JlIHRoZXJtYWwgaW5zdGFu
Y2UgdGhhdCByZWZlcnMgdG8gdGhlIHNhbWUgdGhlcm1hbCB6b25lIGluIHRoaXMgbG9vcCwNCnRo
aXMgd29ya3MgYmVjYXVzZSBiaW5kX2NkZXYoKSBhbHdheXMgYmluZHMgdGhlIGNvb2xpbmcgZGV2
aWNlIHRvIG9uZSANCnRoZXJtYWwgem9uZSBmaXJzdCwgYW5kIHRoZW4gYmluZHMgdG8gdGhlIG5l
eHQgdGhlcm1hbCB6b25lLg0KDQoNCkJlc3QgUmVnYXJkcywNCll1DQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1235353 — Re: [PATCH 3/3] Thermal: do thermal zone update after a cooling device registered

Hi Yu,

On Mon, Sep 28, 2015 at 06:52:00PM +0100, Chen, Yu C wrote:
> Hi, Javi,
> 
> > -----Original Message-----
> > From: Javi Merino [mailto:javi.merino@arm.com]
> > Sent: Monday, September 28, 2015 10:29 PM
> > To: Chen, Yu C
> > Cc: linux-pm@vger.kernel.org; edubezval@gmail.com; Zhang, Rui; linux-
> > kernel@vger.kernel.org; stable@vger.kernel.org
> > Subject: Re: [PATCH 3/3] Thermal: do thermal zone update after a cooling
> > device registered
> > 
> > On Sun, Sep 27, 2015 at 06:48:44AM +0100, Chen Yu wrote:
> > > From: Zhang Rui <rui.zhang@intel.com>
> > >
> > >
> > 
> > I think you need to hold cdev->lock here, to make sure that no thermal zone
> > is added or removed from cdev->thermal_instances while you are looping.
> > 
> Ah right, will add. If I add the cdev ->lock here, will there be a AB-BA lock with 
> thermal_zone_unbind_cooling_device?

You're right, it could lead to a deadlock.  The locks can't be
swapped because that won't work in step_wise.

The best way that I can think of accessing thermal_instances
atomically is by making it RCU protected instead of with mutexes.
What do you think?


> > Why list_for_each_entry_safe() ?  You are not going to remove any entry, so
> > you can just use list_for_each_entry()
> > 
> > 
> > Why is this so complicated?  Can't you just do:
> > 
> > 	list_for_each_entry(pos, &cdev->thermal_instances, cdev_node)
> >         	thermal_zone_device_update(pos->tz);
> > 
> 
> This is an optimization here:
> Ignore thermal instance that refers to the same thermal zone in this loop,
> this works because bind_cdev() always binds the cooling device to one 
> thermal zone first, and then binds to the next thermal zone.

It has taken me a while to understand this optimization.  Please
document both "if"s in the code.  For the first "if" maybe you can use
list_is_last() to make it easier to understand that you're looking for
the last element in the list:

		if (list_is_last(&pos->cdev_node, &cdev->thermal_instances)) {
			thermal_zone_device_update(pos->tz);

For the second "if" you can say that you only need to run
thermal_zone_device_update() once per thermal zone, even though
multiple thermal instances may refer to the same thermal zone.

Cheers,
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web