[PATCH v2 0/2] Support to tune governor in run time

[PATCH v2 0/2] Support to tune governor in run time

[Wei Ni]

This serie can support to turn governor for thermal zone in
run time.
Adds avaiable_policies attribute, so the user can get available
governor policies and change te governor to a new one.
Adds thermal_update_governor() function, so the thermal platform
driver can use it to update governor.

Changes from v1:
1. split avaiable_policies part to the "patch 1/2", and add document
for it.
2. add document for the thermal_update_governor.

Wei Ni (2):
  thermal: add available policies attribut
  thermal: add interface to support tune governor in run-time

 Documentation/thermal/sysfs-api.txt |   11 ++++
 drivers/thermal/thermal_core.c      |  105 ++++++++++++++++++++++++++++++++---
 include/linux/thermal.h             |   12 ++++
 3 files changed, 119 insertions(+), 9 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] thermal: add available policies attribut

[Wei Ni]

The Linux thermal framework support to change governor policy
in run time, but it can't show what available policies supported.

This patch adds available_policies attribut to the thermal
framework, it can list the thermal governors which can be
used for a particular zone. This attribut is read only.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 Documentation/thermal/sysfs-api.txt |    6 ++++++
 drivers/thermal/thermal_core.c      |   25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 87519cb..6a70b55c 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -172,6 +172,7 @@ Thermal zone device sys I/F, created once it's registered:
     |---temp:			Current temperature
     |---mode:			Working mode of the thermal zone
     |---policy:			Thermal governor used for this zone
+    |---available_policies	Available governors for this zone
     |---trip_point_[0-*]_temp:	Trip point temperature
     |---trip_point_[0-*]_type:	Trip point type
     |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
@@ -238,6 +239,10 @@ policy
 	One of the various thermal governors used for a particular zone.
 	RW, Required
 
+available_policies
+	Availabe thermal governors which can be used for a particular zone.
+	RO, Required
+
 trip_point_[0-*]_temp
 	The temperature above which trip point will be fired.
 	Unit: millidegree Celsius
@@ -330,6 +335,7 @@ method, the sys I/F structure will be built like this:
     |---temp:			37000
     |---mode:			enabled
     |---policy:			step_wise
+    |---available_policies:	step_wise fair_share
     |---trip_point_0_temp:	100000
     |---trip_point_0_type:	critical
     |---trip_point_1_temp:	80000
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 338a88b..aab1df8 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -761,6 +761,24 @@ policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
 	return sprintf(buf, "%s\n", tz->governor->name);
 }
 
+static ssize_t
+available_policies_show(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct thermal_governor *pos;
+	ssize_t count = 0;
+
+	mutex_lock(&thermal_governor_lock);
+
+	list_for_each_entry(pos, &thermal_governor_list, governor_list)
+		count += sprintf(buf + count, "%s ", pos->name);
+	count += sprintf(buf + count, "\n");
+
+	mutex_unlock(&thermal_governor_lock);
+
+	return count;
+}
+
 #ifdef CONFIG_THERMAL_EMULATION
 static ssize_t
 emul_temp_store(struct device *dev, struct device_attribute *attr,
@@ -794,6 +812,7 @@ static DEVICE_ATTR(temp, 0444, temp_show, NULL);
 static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
 static DEVICE_ATTR(passive, S_IRUGO | S_IWUSR, passive_show, passive_store);
 static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
+static DEVICE_ATTR(available_policies, S_IRUGO, available_policies_show, NULL);
 
 /* sys I/F for cooling device */
 #define to_cooling_device(_dev)	\
@@ -1527,6 +1546,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	if (result)
 		goto unregister;
 
+	/* Create available_policies attribute */
+	result = device_create_file(&tz->device, &dev_attr_available_policies);
+	if (result)
+		goto unregister;
+
 	/* Update 'this' zone's governor information */
 	mutex_lock(&thermal_governor_lock);
 
@@ -1622,6 +1646,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	if (tz->ops->get_mode)
 		device_remove_file(&tz->device, &dev_attr_mode);
 	device_remove_file(&tz->device, &dev_attr_policy);
+	device_remove_file(&tz->device, &dev_attr_available_policies);
 	remove_trip_attrs(tz);
 	tz->governor = NULL;
 
-- 
1.7.9.5

[PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Wei Ni]

In the of-thermal driver, it register a thermal zone device
without governor, since the governers are not static, we
should be able to update to a new one in run-time, so I add
a new fucntion thermal_update_governor() to do it.

In the future, the governor will be more complex, it may want
to tune governor-specific parameters/configuration for
different thermal zone at different run-time, so I add
start/stop for the governor and add governor_data as governor's
parameters, so that the thermal platform driver can change
the governor with start/stop, and tune the governor parameters
in run-time.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 Documentation/thermal/sysfs-api.txt |    5 +++
 drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
 include/linux/thermal.h             |   12 ++++++
 3 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 6a70b55c..7efd8e6 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
 This function serves as an arbitrator to set the state of a cooling
 device. It sets the cooling device to the deepest cooling state if
 possible.
+
+5.5:thermal_update_governor:
+This function update the thermal zone's governor to a new one and update
+the governor data of the new governor for that thermal zone. Returns 0 if
+success, an erro code otherwise.
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index aab1df8..d922baf 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
 			name = pos->tzp->governor_name;
 		else
 			name = DEFAULT_THERMAL_GOVERNOR;
-		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
+		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
+			if (governor->start) {
+				err = governor->start(pos);
+				if (err < 0)
+					goto exit;
+			}
 			pos->governor = governor;
+		}
 	}
 
+exit:
 	mutex_unlock(&thermal_list_lock);
 	mutex_unlock(&thermal_governor_lock);
 
@@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
 
 	list_for_each_entry(pos, &thermal_tz_list, node) {
 		if (!strnicmp(pos->governor->name, governor->name,
-						THERMAL_NAME_LENGTH))
+						THERMAL_NAME_LENGTH)) {
+			if (pos->governor->stop)
+				pos->governor->stop(pos);
 			pos->governor = NULL;
+		}
 	}
 
 	mutex_unlock(&thermal_list_lock);
@@ -130,6 +140,51 @@ exit:
 	return;
 }
 
+/**
+ * thermal_update_governor() - update the thermal zone device's governor
+ * to a new one.
+ * @tzd: pointer of thermal zone device, which need to update governor.
+ * @name: new governor name.
+ * @gov_data: governor data for the new governor if needed.
+ *
+ * Return: On success returns 0, an error code otherwise
+ */
+int thermal_update_governor(struct thermal_zone_device *tzd,
+			    const char *name, void *gov_data)
+{
+	struct thermal_governor *old_gov, *new_gov;
+	int ret = 0;
+
+	mutex_lock(&thermal_governor_lock);
+
+	new_gov = __find_governor(name);
+	if (!new_gov) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	old_gov = tzd->governor;
+
+	if (old_gov && old_gov->stop)
+		old_gov->stop(tzd);
+
+	if (new_gov->start) {
+		ret = new_gov->start(tzd);
+		if (ret < 0) {
+			if (old_gov && old_gov->start)
+				old_gov->start(tzd);
+			goto exit;
+		}
+	}
+
+	tzd->governor = new_gov;
+	tzd->governor_data = gov_data;
+
+exit:
+	mutex_unlock(&thermal_governor_lock);
+	return ret;
+}
+
 static int get_idr(struct idr *idr, struct mutex *lock, int *id)
 {
 	int ret;
@@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
 {
 	int ret = -EINVAL;
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_governor *gov;
 	char name[THERMAL_NAME_LENGTH];
 
 	snprintf(name, sizeof(name), "%s", buf);
 
-	mutex_lock(&thermal_governor_lock);
-
-	gov = __find_governor(strim(name));
-	if (!gov)
+	ret = thermal_update_governor(tz, strim(name), NULL);
+	if (ret < 0)
 		goto exit;
 
-	tz->governor = gov;
 	ret = count;
 
 exit:
-	mutex_unlock(&thermal_governor_lock);
 	return ret;
 }
 
@@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	else
 		tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
 
+	if (tz->governor->start) {
+		result = tz->governor->start(tz);
+		if (result < 0)
+			tz->governor = NULL;
+	}
+
 	mutex_unlock(&thermal_governor_lock);
 
 	if (!tz->tzp || !tz->tzp->no_hwmon) {
@@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 		return tz;
 
 unregister:
+	if (tz->governor && tz->governor->stop)
+		tz->governor->stop(tz);
+	tz->governor = NULL;
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
 	device_unregister(&tz->device);
 	return ERR_PTR(result);
@@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	device_remove_file(&tz->device, &dev_attr_policy);
 	device_remove_file(&tz->device, &dev_attr_available_policies);
 	remove_trip_attrs(tz);
+
+	if (tz->governor && tz->governor->stop)
+		tz->governor->stop(tz);
 	tz->governor = NULL;
 
 	thermal_remove_hwmon_sysfs(tz);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f7e11c7..a473736 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -177,6 +177,7 @@ struct thermal_zone_device {
 	struct thermal_zone_device_ops *ops;
 	const struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
+	void *governor_data;
 	struct list_head thermal_instances;
 	struct idr idr;
 	struct mutex lock; /* protect thermal_instances list */
@@ -187,6 +188,15 @@ struct thermal_zone_device {
 /* Structure that holds thermal governor information */
 struct thermal_governor {
 	char name[THERMAL_NAME_LENGTH];
+
+	/*
+	 * The start and stop operations will be called when thermal zone is
+	 * registered and when change governor via sysfs or driver in running
+	 * time.
+	 */
+	int (*start)(struct thermal_zone_device *tz);
+	void (*stop)(struct thermal_zone_device *tz);
+
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
 	struct list_head	governor_list;
 };
@@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
 		struct thermal_cooling_device *, int);
 void thermal_cdev_update(struct thermal_cooling_device *);
 void thermal_notify_framework(struct thermal_zone_device *, int);
+int thermal_update_governor(struct thermal_zone_device *, const char *,
+			    void *);
 
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
-- 
1.7.9.5

Re: [PATCH v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

Wei,

Thanks for your patches. Please help me out to properly understand your
intentions here.

On 13-01-2014 07:17, Wei Ni wrote:
> This serie can support to turn governor for thermal zone in
> run time.

Can you please explain why this is needed? Are you facing troubles with
current way to switch governors? If yes, can you please report them?

> Adds avaiable_policies attribute, so the user can get available
> governor policies and change te governor to a new one.

OK. This one I understand the need and you are explaining its motivation.

> Adds thermal_update_governor() function, so the thermal platform
> driver can use it to update governor.

Here I cannot see why the platform driver would need to update a
governor, instead of a zone. Platform drivers are not supposed to be
aware of governors. For updating a zone we already have an API for that,
please check documentation.

> 
> Changes from v1:
> 1. split avaiable_policies part to the "patch 1/2", and add document
> for it.
> 2. add document for the thermal_update_governor.
> 
> Wei Ni (2):
>   thermal: add available policies attribut
>   thermal: add interface to support tune governor in run-time
> 
>  Documentation/thermal/sysfs-api.txt |   11 ++++
>  drivers/thermal/thermal_core.c      |  105 ++++++++++++++++++++++++++++++++---
>  include/linux/thermal.h             |   12 ++++
>  3 files changed, 119 insertions(+), 9 deletions(-)
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 1/2] thermal: add available policies attribut

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4505 bytes --]

On 13-01-2014 07:17, Wei Ni wrote:
> The Linux thermal framework support to change governor policy
> in run time, but it can't show what available policies supported.
> 
> This patch adds available_policies attribut to the thermal

s/attribut/attribute/g

> framework, it can list the thermal governors which can be
> used for a particular zone. This attribut is read only.
> 
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/thermal/sysfs-api.txt |    6 ++++++
>  drivers/thermal/thermal_core.c      |   25 +++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 87519cb..6a70b55c 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -172,6 +172,7 @@ Thermal zone device sys I/F, created once it's registered:
>      |---temp:			Current temperature
>      |---mode:			Working mode of the thermal zone
>      |---policy:			Thermal governor used for this zone
> +    |---available_policies	Available governors for this zone

I would prefer:
"Available thermal governors for this zone"

>      |---trip_point_[0-*]_temp:	Trip point temperature
>      |---trip_point_[0-*]_type:	Trip point type
>      |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
> @@ -238,6 +239,10 @@ policy
>  	One of the various thermal governors used for a particular zone.
>  	RW, Required
>  
> +available_policies
> +	Availabe thermal governors which can be used for a particular zone.
> +	RO, Required
> +
>  trip_point_[0-*]_temp
>  	The temperature above which trip point will be fired.
>  	Unit: millidegree Celsius
> @@ -330,6 +335,7 @@ method, the sys I/F structure will be built like this:
>      |---temp:			37000
>      |---mode:			enabled
>      |---policy:			step_wise
> +    |---available_policies:	step_wise fair_share
>      |---trip_point_0_temp:	100000
>      |---trip_point_0_type:	critical
>      |---trip_point_1_temp:	80000
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 338a88b..aab1df8 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -761,6 +761,24 @@ policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
>  	return sprintf(buf, "%s\n", tz->governor->name);
>  }
>  
> +static ssize_t
> +available_policies_show(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct thermal_governor *pos;
> +	ssize_t count = 0;
> +
> +	mutex_lock(&thermal_governor_lock);
> +
> +	list_for_each_entry(pos, &thermal_governor_list, governor_list)
> +		count += sprintf(buf + count, "%s ", pos->name);

I would prefer if you'd used scnprintf

> +	count += sprintf(buf + count, "\n");

ditto.


It is unlikely, but What if count if greater than PAGE_SIZE?

> +
> +	mutex_unlock(&thermal_governor_lock);
> +
> +	return count;
> +}
> +
>  #ifdef CONFIG_THERMAL_EMULATION
>  static ssize_t
>  emul_temp_store(struct device *dev, struct device_attribute *attr,
> @@ -794,6 +812,7 @@ static DEVICE_ATTR(temp, 0444, temp_show, NULL);
>  static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
>  static DEVICE_ATTR(passive, S_IRUGO | S_IWUSR, passive_show, passive_store);
>  static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
> +static DEVICE_ATTR(available_policies, S_IRUGO, available_policies_show, NULL);
>  
>  /* sys I/F for cooling device */
>  #define to_cooling_device(_dev)	\
> @@ -1527,6 +1546,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	if (result)
>  		goto unregister;
>  
> +	/* Create available_policies attribute */
> +	result = device_create_file(&tz->device, &dev_attr_available_policies);
> +	if (result)
> +		goto unregister;
> +
>  	/* Update 'this' zone's governor information */
>  	mutex_lock(&thermal_governor_lock);
>  
> @@ -1622,6 +1646,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  	if (tz->ops->get_mode)
>  		device_remove_file(&tz->device, &dev_attr_mode);
>  	device_remove_file(&tz->device, &dev_attr_policy);
> +	device_remove_file(&tz->device, &dev_attr_available_policies);
>  	remove_trip_attrs(tz);
>  	tz->governor = NULL;
>  
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 0/2] Support to tune governor in run time

[Matthew Longnecker]

On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>> This serie can support to turn governor for thermal zone in
>> run time.
>
> Can you please explain why this is needed? Are you facing troubles with
> current way to switch governors? If yes, can you please report them?
>

....

>> Adds thermal_update_governor() function, so the thermal platform
>> driver can use it to update governor.
>
> Here I cannot see why the platform driver would need to update a
> governor, instead of a zone. Platform drivers are not supposed to be
> aware of governors. For updating a zone we already have an API for that,
> please check documentation.
>

I think we have a miscommunication. The purpose of 
thermal_update_governor is to *switch* governors at runtime (from within 
the kernel).

Wei has used the term "update" in the sense of switch rather than 
"update" in the sense used by thermal_zone_device_update.

Eduardo, what is your recommended technique for setting the governor of 
a thermal zone device created via device tree?

thanks,
Matt

Re: [PATCH v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

On 13-01-2014 15:01, Matthew Longnecker wrote:
> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>> This serie can support to turn governor for thermal zone in
>>> run time.
>>
>> Can you please explain why this is needed? Are you facing troubles with
>> current way to switch governors? If yes, can you please report them?

Looks like there is a need to switch governors from within kernel code,
but not explanation of use cases is provided.

>>
> 
> ....
> 
>>> Adds thermal_update_governor() function, so the thermal platform
>>> driver can use it to update governor.
>>
>> Here I cannot see why the platform driver would need to update a
>> governor, instead of a zone. Platform drivers are not supposed to be
>> aware of governors. For updating a zone we already have an API for that,
>> please check documentation.
>>
> 
> I think we have a miscommunication. The purpose of
> thermal_update_governor is to *switch* governors at runtime (from within
> the kernel).
> 
> Wei has used the term "update" in the sense of switch rather than
> "update" in the sense used by thermal_zone_device_update.

Fine, but why do you need it?

> 
> Eduardo, what is your recommended technique for setting the governor of
> a thermal zone device created via device tree?

So far the recommended (and existing) way is by user(land) decision.

> 
> thanks,
> Matt
> 
> -- 
> 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
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 0/2] Support to tune governor in run time

[Wei Ni]

On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On 13-01-2014 15:01, Matthew Longnecker wrote:
>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>> This serie can support to turn governor for thermal zone in
>>>> run time.
>>>
>>> Can you please explain why this is needed? Are you facing troubles with
>>> current way to switch governors? If yes, can you please report them?
> 
> Looks like there is a need to switch governors from within kernel code,
> but not explanation of use cases is provided.

In fact, with the of-thermal framework, the driver can't initialize to
any governor, even default governor, it may be a bug. As we discussed in
other mail list, we should not set governor in DT, and I think the
platform driver should be able to set to any governors which it want, so
I this patch, the driver can call the thermal_update_governor() to
switch to new governor in kernel space.
For example, there have two thermal driver, using of-thermal to register
thermal zone device. One want to set to step_wise governor, another want
fair_share, how can we do? I think after they calling the
thermal_zone_of_sensor_register(), they can call the
thermal_update_governor() to set the governor which they want.

> 
>>>
>>
>> ....
>>
>>>> Adds thermal_update_governor() function, so the thermal platform
>>>> driver can use it to update governor.
>>>
>>> Here I cannot see why the platform driver would need to update a
>>> governor, instead of a zone. Platform drivers are not supposed to be
>>> aware of governors. For updating a zone we already have an API for that,
>>> please check documentation.
>>>
>>
>> I think we have a miscommunication. The purpose of
>> thermal_update_governor is to *switch* governors at runtime (from within
>> the kernel).
>>
>> Wei has used the term "update" in the sense of switch rather than
>> "update" in the sense used by thermal_zone_device_update.
> 
> Fine, but why do you need it?

Yes, I have consider to use "switch", but As my commit in the patch, in
the future, the governor may be more complex, it may need governor
parameter/configuration, and need to be tuned in run-time or in
initialization.
In fact we develop a new governor, based on PID logic. It will need some
parameters, such as proportional, integral and derivative. these value
will be initialized in different values for different thermal zone, so
we want to use the thermal_zone_device_update() to switch governor or
update the governor's parameters for different thermal zone.

> 
>>
>> Eduardo, what is your recommended technique for setting the governor of
>> a thermal zone device created via device tree?
> 
> So far the recommended (and existing) way is by user(land) decision.
> 
>>
>> thanks,
>> Matt
>>
>> -- 
>> 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 v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4014 bytes --]

Hello Wei,

On 14-01-2014 00:17, Wei Ni wrote:
> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>> This serie can support to turn governor for thermal zone in
>>>>> run time.
>>>>
>>>> Can you please explain why this is needed? Are you facing troubles with
>>>> current way to switch governors? If yes, can you please report them?
>>
>> Looks like there is a need to switch governors from within kernel code,
>> but not explanation of use cases is provided.
> 
> In fact, with the of-thermal framework, the driver can't initialize to
> any governor, even default governor, it may be a bug. As we discussed in

I see. Using default governor is not a bug. The thermal zone shall not
be defined in way that it is specific to a policy. We must remember that
DT is not used for specifying policies, but only describing hardware.

> other mail list, we should not set governor in DT, and I think the
> platform driver should be able to set to any governors which it want, so

OK. But still, because you believe the platform driver has to set the
governor, this is not a bug. Defining which policy is used to control a
zone can be a later decision in the boot process. Specially because the
critical requirements are not dealt by the governors anyway.

> I this patch, the driver can call the thermal_update_governor() to
> switch to new governor in kernel space.

I see.

> For example, there have two thermal driver, using of-thermal to register
> thermal zone device. One want to set to step_wise governor, another want
> fair_share, how can we do? I think after they calling the
> thermal_zone_of_sensor_register(), they can call the
> thermal_update_governor() to set the governor which they want.

OK. Now I get a better view of what you are trying to achieve with this
series. However, why can't you hand this decision off to userland,
during, say very early boot sequence?


> 
>>
>>>>
>>>
>>> ....
>>>
>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>> driver can use it to update governor.
>>>>
>>>> Here I cannot see why the platform driver would need to update a
>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>> aware of governors. For updating a zone we already have an API for that,
>>>> please check documentation.
>>>>
>>>
>>> I think we have a miscommunication. The purpose of
>>> thermal_update_governor is to *switch* governors at runtime (from within
>>> the kernel).
>>>
>>> Wei has used the term "update" in the sense of switch rather than
>>> "update" in the sense used by thermal_zone_device_update.
>>
>> Fine, but why do you need it?
> 
> Yes, I have consider to use "switch", but As my commit in the patch, in
> the future, the governor may be more complex, it may need governor
> parameter/configuration, and need to be tuned in run-time or in
> initialization.
> In fact we develop a new governor, based on PID logic. It will need some
> parameters, such as proportional, integral and derivative. these value
> will be initialized in different values for different thermal zone, so
> we want to use the thermal_zone_device_update() to switch governor or
> update the governor's parameters for different thermal zone.
> 
>>
>>>
>>> Eduardo, what is your recommended technique for setting the governor of
>>> a thermal zone device created via device tree?
>>
>> So far the recommended (and existing) way is by user(land) decision.
>>
>>>
>>> thanks,
>>> Matt
>>>
>>> -- 
>>> 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
>>>
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]

On 14-01-2014 00:17, Wei Ni wrote:
> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>> This serie can support to turn governor for thermal zone in
>>>>> run time.
>>>>
>>>> Can you please explain why this is needed? Are you facing troubles with
>>>> current way to switch governors? If yes, can you please report them?
>>
>> Looks like there is a need to switch governors from within kernel code,
>> but not explanation of use cases is provided.
> 
> In fact, with the of-thermal framework, the driver can't initialize to
> any governor, even default governor, it may be a bug. As we discussed in
> other mail list, we should not set governor in DT, and I think the
> platform driver should be able to set to any governors which it want, so
> I this patch, the driver can call the thermal_update_governor() to
> switch to new governor in kernel space.
> For example, there have two thermal driver, using of-thermal to register
> thermal zone device. One want to set to step_wise governor, another want
> fair_share, how can we do? I think after they calling the
> thermal_zone_of_sensor_register(), they can call the
> thermal_update_governor() to set the governor which they want.
> 
>>
>>>>
>>>
>>> ....
>>>
>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>> driver can use it to update governor.
>>>>
>>>> Here I cannot see why the platform driver would need to update a
>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>> aware of governors. For updating a zone we already have an API for that,
>>>> please check documentation.
>>>>
>>>
>>> I think we have a miscommunication. The purpose of
>>> thermal_update_governor is to *switch* governors at runtime (from within
>>> the kernel).
>>>
>>> Wei has used the term "update" in the sense of switch rather than
>>> "update" in the sense used by thermal_zone_device_update.
>>
>> Fine, but why do you need it?
> 
> Yes, I have consider to use "switch", but As my commit in the patch, in
> the future, the governor may be more complex, it may need governor
> parameter/configuration, and need to be tuned in run-time or in
> initialization.

OK, I see you have a very complex governor coming. But you know,
prediction is very difficult, especially if it's about the future. :-)

It would be very constructive if you could share the governor together
with the proposed change. It is not fair to introduce a new API for a
non-existing user, don't you agree?

> In fact we develop a new governor, based on PID logic. It will need some
> parameters, such as proportional, integral and derivative. these value
> will be initialized in different values for different thermal zone, so
> we want to use the thermal_zone_device_update() to switch governor or
> update the governor's parameters for different thermal zone.

The parameters are a zone specific data right? Why should it be bound to
governor switch? The governor should be able to fetch the zone data when
needed.

Again, if you share the user of the API you are proposing we can work
together to make a better fit between API and users.

For the existing governors, I don't see a need to have the stop and
start callbacks.

> 
>>
>>>
>>> Eduardo, what is your recommended technique for setting the governor of
>>> a thermal zone device created via device tree?
>>
>> So far the recommended (and existing) way is by user(land) decision.
>>
>>>
>>> thanks,
>>> Matt
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 1/2] thermal: add available policies attribut

[Wei Ni]

On 01/13/2014 11:58 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On 13-01-2014 07:17, Wei Ni wrote:
>> The Linux thermal framework support to change governor policy
>> in run time, but it can't show what available policies supported.
>>
>> This patch adds available_policies attribut to the thermal
> 
> s/attribut/attribute/g

oh, thanks, I will fix it.

> 
>> framework, it can list the thermal governors which can be
>> used for a particular zone. This attribut is read only.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  Documentation/thermal/sysfs-api.txt |    6 ++++++
>>  drivers/thermal/thermal_core.c      |   25 +++++++++++++++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> index 87519cb..6a70b55c 100644
>> --- a/Documentation/thermal/sysfs-api.txt
>> +++ b/Documentation/thermal/sysfs-api.txt
>> @@ -172,6 +172,7 @@ Thermal zone device sys I/F, created once it's registered:
>>      |---temp:			Current temperature
>>      |---mode:			Working mode of the thermal zone
>>      |---policy:			Thermal governor used for this zone
>> +    |---available_policies	Available governors for this zone
> 
> I would prefer:
> "Available thermal governors for this zone"

Ok.

> 
>>      |---trip_point_[0-*]_temp:	Trip point temperature
>>      |---trip_point_[0-*]_type:	Trip point type
>>      |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
>> @@ -238,6 +239,10 @@ policy
>>  	One of the various thermal governors used for a particular zone.
>>  	RW, Required
>>  
>> +available_policies
>> +	Availabe thermal governors which can be used for a particular zone.
>> +	RO, Required
>> +
>>  trip_point_[0-*]_temp
>>  	The temperature above which trip point will be fired.
>>  	Unit: millidegree Celsius
>> @@ -330,6 +335,7 @@ method, the sys I/F structure will be built like this:
>>      |---temp:			37000
>>      |---mode:			enabled
>>      |---policy:			step_wise
>> +    |---available_policies:	step_wise fair_share
>>      |---trip_point_0_temp:	100000
>>      |---trip_point_0_type:	critical
>>      |---trip_point_1_temp:	80000
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 338a88b..aab1df8 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -761,6 +761,24 @@ policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
>>  	return sprintf(buf, "%s\n", tz->governor->name);
>>  }
>>  
>> +static ssize_t
>> +available_policies_show(struct device *dev, struct device_attribute *devattr,
>> +			char *buf)
>> +{
>> +	struct thermal_governor *pos;
>> +	ssize_t count = 0;
>> +
>> +	mutex_lock(&thermal_governor_lock);
>> +
>> +	list_for_each_entry(pos, &thermal_governor_list, governor_list)
>> +		count += sprintf(buf + count, "%s ", pos->name);
> 
> I would prefer if you'd used scnprintf
> 
>> +	count += sprintf(buf + count, "\n");
> 
> ditto.
> 
> 
> It is unlikely, but What if count if greater than PAGE_SIZE?

hmm, I checked the sysfs driver, in funciton sysfs_kf_seq_show(), it
seems it can handle the count greater than PAGE_SIZE.
Or we can return error here directly.
What do you think about it?

> 
>> +
>> +	mutex_unlock(&thermal_governor_lock);
>> +
>> +	return count;
>> +}
>> +
>>  #ifdef CONFIG_THERMAL_EMULATION
>>  static ssize_t
>>  emul_temp_store(struct device *dev, struct device_attribute *attr,
>> @@ -794,6 +812,7 @@ static DEVICE_ATTR(temp, 0444, temp_show, NULL);
>>  static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
>>  static DEVICE_ATTR(passive, S_IRUGO | S_IWUSR, passive_show, passive_store);
>>  static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
>> +static DEVICE_ATTR(available_policies, S_IRUGO, available_policies_show, NULL);
>>  
>>  /* sys I/F for cooling device */
>>  #define to_cooling_device(_dev)	\
>> @@ -1527,6 +1546,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>  	if (result)
>>  		goto unregister;
>>  
>> +	/* Create available_policies attribute */
>> +	result = device_create_file(&tz->device, &dev_attr_available_policies);
>> +	if (result)
>> +		goto unregister;
>> +
>>  	/* Update 'this' zone's governor information */
>>  	mutex_lock(&thermal_governor_lock);
>>  
>> @@ -1622,6 +1646,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>  	if (tz->ops->get_mode)
>>  		device_remove_file(&tz->device, &dev_attr_mode);
>>  	device_remove_file(&tz->device, &dev_attr_policy);
>> +	device_remove_file(&tz->device, &dev_attr_available_policies);
>>  	remove_trip_attrs(tz);
>>  	tz->governor = NULL;
>>  
>>
> 
> 

Re: [PATCH v2 0/2] Support to tune governor in run time

[Wei Ni]

On 01/15/2014 01:44 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> Hello Wei,
> 
> On 14-01-2014 00:17, Wei Ni wrote:
>> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>>> This serie can support to turn governor for thermal zone in
>>>>>> run time.
>>>>>
>>>>> Can you please explain why this is needed? Are you facing troubles with
>>>>> current way to switch governors? If yes, can you please report them?
>>>
>>> Looks like there is a need to switch governors from within kernel code,
>>> but not explanation of use cases is provided.
>>
>> In fact, with the of-thermal framework, the driver can't initialize to
>> any governor, even default governor, it may be a bug. As we discussed in
> 
> I see. Using default governor is not a bug. The thermal zone shall not
> be defined in way that it is specific to a policy. We must remember that
> DT is not used for specifying policies, but only describing hardware.
> 
>> other mail list, we should not set governor in DT, and I think the
>> platform driver should be able to set to any governors which it want, so
> 
> OK. But still, because you believe the platform driver has to set the
> governor, this is not a bug. Defining which policy is used to control a
> zone can be a later decision in the boot process. Specially because the
> critical requirements are not dealt by the governors anyway.
> 
>> I this patch, the driver can call the thermal_update_governor() to
>> switch to new governor in kernel space.
> 
> I see.
> 
>> For example, there have two thermal driver, using of-thermal to register
>> thermal zone device. One want to set to step_wise governor, another want
>> fair_share, how can we do? I think after they calling the
>> thermal_zone_of_sensor_register(), they can call the
>> thermal_update_governor() to set the governor which they want.
> 
> OK. Now I get a better view of what you are trying to achieve with this
> series. However, why can't you hand this decision off to userland,
> during, say very early boot sequence?

I think the thermal_zone_device_register() provide a way to set governor
when register a thermal zone, and this function is exposed to all
driver, so I supposed the thermal framework can support to handle the
governor in kernel.

Anyway, let's consider to switch the governor in user space.

> 
> 
>>
>>>
>>>>>
>>>>
>>>> ....
>>>>
>>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>>> driver can use it to update governor.
>>>>>
>>>>> Here I cannot see why the platform driver would need to update a
>>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>>> aware of governors. For updating a zone we already have an API for that,
>>>>> please check documentation.
>>>>>
>>>>
>>>> I think we have a miscommunication. The purpose of
>>>> thermal_update_governor is to *switch* governors at runtime (from within
>>>> the kernel).
>>>>
>>>> Wei has used the term "update" in the sense of switch rather than
>>>> "update" in the sense used by thermal_zone_device_update.
>>>
>>> Fine, but why do you need it?
>>
>> Yes, I have consider to use "switch", but As my commit in the patch, in
>> the future, the governor may be more complex, it may need governor
>> parameter/configuration, and need to be tuned in run-time or in
>> initialization.
>> In fact we develop a new governor, based on PID logic. It will need some
>> parameters, such as proportional, integral and derivative. these value
>> will be initialized in different values for different thermal zone, so
>> we want to use the thermal_zone_device_update() to switch governor or
>> update the governor's parameters for different thermal zone.
>>
>>>
>>>>
>>>> Eduardo, what is your recommended technique for setting the governor of
>>>> a thermal zone device created via device tree?
>>>
>>> So far the recommended (and existing) way is by user(land) decision.
>>>
>>>>
>>>> thanks,
>>>> Matt
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
>>
>>
> 
> 

Re: [PATCH v2 0/2] Support to tune governor in run time

[Wei Ni]

On 01/15/2014 01:50 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On 14-01-2014 00:17, Wei Ni wrote:
>> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>>> This serie can support to turn governor for thermal zone in
>>>>>> run time.
>>>>>
>>>>> Can you please explain why this is needed? Are you facing troubles with
>>>>> current way to switch governors? If yes, can you please report them?
>>>
>>> Looks like there is a need to switch governors from within kernel code,
>>> but not explanation of use cases is provided.
>>
>> In fact, with the of-thermal framework, the driver can't initialize to
>> any governor, even default governor, it may be a bug. As we discussed in
>> other mail list, we should not set governor in DT, and I think the
>> platform driver should be able to set to any governors which it want, so
>> I this patch, the driver can call the thermal_update_governor() to
>> switch to new governor in kernel space.
>> For example, there have two thermal driver, using of-thermal to register
>> thermal zone device. One want to set to step_wise governor, another want
>> fair_share, how can we do? I think after they calling the
>> thermal_zone_of_sensor_register(), they can call the
>> thermal_update_governor() to set the governor which they want.
>>
>>>
>>>>>
>>>>
>>>> ....
>>>>
>>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>>> driver can use it to update governor.
>>>>>
>>>>> Here I cannot see why the platform driver would need to update a
>>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>>> aware of governors. For updating a zone we already have an API for that,
>>>>> please check documentation.
>>>>>
>>>>
>>>> I think we have a miscommunication. The purpose of
>>>> thermal_update_governor is to *switch* governors at runtime (from within
>>>> the kernel).
>>>>
>>>> Wei has used the term "update" in the sense of switch rather than
>>>> "update" in the sense used by thermal_zone_device_update.
>>>
>>> Fine, but why do you need it?
>>
>> Yes, I have consider to use "switch", but As my commit in the patch, in
>> the future, the governor may be more complex, it may need governor
>> parameter/configuration, and need to be tuned in run-time or in
>> initialization.
> 
> OK, I see you have a very complex governor coming. But you know,
> prediction is very difficult, especially if it's about the future. :-)

Yes, absolutely agree.

> 
> It would be very constructive if you could share the governor together
> with the proposed change. It is not fair to introduce a new API for a
> non-existing user, don't you agree?
> 
>> In fact we develop a new governor, based on PID logic. It will need some
>> parameters, such as proportional, integral and derivative. these value
>> will be initialized in different values for different thermal zone, so
>> we want to use the thermal_zone_device_update() to switch governor or
>> update the governor's parameters for different thermal zone.
> 
> The parameters are a zone specific data right? Why should it be bound to
> governor switch? The governor should be able to fetch the zone data when
> needed.

Let me consider it carefully again.

> 
> Again, if you share the user of the API you are proposing we can work
> together to make a better fit between API and users.
> 
> For the existing governors, I don't see a need to have the stop and
> start callbacks.
> 
>>
>>>
>>>>
>>>> Eduardo, what is your recommended technique for setting the governor of
>>>> a thermal zone device created via device tree?
>>>
>>> So far the recommended (and existing) way is by user(land) decision.
>>>
>>>>
>>>> thanks,
>>>> Matt
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
>>
>>
> 
> 

Re: [PATCH v2 1/2] thermal: add available policies attribut

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 5666 bytes --]

On 15-01-2014 07:26, Wei Ni wrote:
> On 01/13/2014 11:58 PM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 13-01-2014 07:17, Wei Ni wrote:
>>> The Linux thermal framework support to change governor policy
>>> in run time, but it can't show what available policies supported.
>>>
>>> This patch adds available_policies attribut to the thermal
>>
>> s/attribut/attribute/g
> 
> oh, thanks, I will fix it.
> 
>>
>>> framework, it can list the thermal governors which can be
>>> used for a particular zone. This attribut is read only.
>>>
>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  Documentation/thermal/sysfs-api.txt |    6 ++++++
>>>  drivers/thermal/thermal_core.c      |   25 +++++++++++++++++++++++++
>>>  2 files changed, 31 insertions(+)
>>>
>>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>> index 87519cb..6a70b55c 100644
>>> --- a/Documentation/thermal/sysfs-api.txt
>>> +++ b/Documentation/thermal/sysfs-api.txt
>>> @@ -172,6 +172,7 @@ Thermal zone device sys I/F, created once it's registered:
>>>      |---temp:			Current temperature
>>>      |---mode:			Working mode of the thermal zone
>>>      |---policy:			Thermal governor used for this zone
>>> +    |---available_policies	Available governors for this zone
>>
>> I would prefer:
>> "Available thermal governors for this zone"
> 
> Ok.
> 
>>
>>>      |---trip_point_[0-*]_temp:	Trip point temperature
>>>      |---trip_point_[0-*]_type:	Trip point type
>>>      |---trip_point_[0-*]_hyst:	Hysteresis value for this trip point
>>> @@ -238,6 +239,10 @@ policy
>>>  	One of the various thermal governors used for a particular zone.
>>>  	RW, Required
>>>  
>>> +available_policies
>>> +	Availabe thermal governors which can be used for a particular zone.
>>> +	RO, Required
>>> +
>>>  trip_point_[0-*]_temp
>>>  	The temperature above which trip point will be fired.
>>>  	Unit: millidegree Celsius
>>> @@ -330,6 +335,7 @@ method, the sys I/F structure will be built like this:
>>>      |---temp:			37000
>>>      |---mode:			enabled
>>>      |---policy:			step_wise
>>> +    |---available_policies:	step_wise fair_share
>>>      |---trip_point_0_temp:	100000
>>>      |---trip_point_0_type:	critical
>>>      |---trip_point_1_temp:	80000
>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>> index 338a88b..aab1df8 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -761,6 +761,24 @@ policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
>>>  	return sprintf(buf, "%s\n", tz->governor->name);
>>>  }
>>>  
>>> +static ssize_t
>>> +available_policies_show(struct device *dev, struct device_attribute *devattr,
>>> +			char *buf)
>>> +{
>>> +	struct thermal_governor *pos;
>>> +	ssize_t count = 0;
>>> +
>>> +	mutex_lock(&thermal_governor_lock);
>>> +
>>> +	list_for_each_entry(pos, &thermal_governor_list, governor_list)
>>> +		count += sprintf(buf + count, "%s ", pos->name);
>>
>> I would prefer if you'd used scnprintf
>>
>>> +	count += sprintf(buf + count, "\n");
>>
>> ditto.
>>
>>
>> It is unlikely, but What if count if greater than PAGE_SIZE?
> 
> hmm, I checked the sysfs driver, in funciton sysfs_kf_seq_show(), it
> seems it can handle the count greater than PAGE_SIZE.
> Or we can return error here directly.
> What do you think about it?

Just to be aware that it will truncate your buffer:
	/*
	 * The code works fine with PAGE_SIZE return but it's likely to
	 * indicate truncated result or overflow in normal use cases.
	 */
	if (count >= (ssize_t)PAGE_SIZE) {
		print_symbol("fill_read_buffer: %s returned bad count\n",
			(unsigned long)ops->show);
		/* Try to struggle along */
		count = PAGE_SIZE - 1;
	}


So, you either truncate here or leave to sysfs_seq_show(). Leave at
least a comment in case you leave to sysfs layer.

> 
>>
>>> +
>>> +	mutex_unlock(&thermal_governor_lock);
>>> +
>>> +	return count;
>>> +}
>>> +
>>>  #ifdef CONFIG_THERMAL_EMULATION
>>>  static ssize_t
>>>  emul_temp_store(struct device *dev, struct device_attribute *attr,
>>> @@ -794,6 +812,7 @@ static DEVICE_ATTR(temp, 0444, temp_show, NULL);
>>>  static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
>>>  static DEVICE_ATTR(passive, S_IRUGO | S_IWUSR, passive_show, passive_store);
>>>  static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
>>> +static DEVICE_ATTR(available_policies, S_IRUGO, available_policies_show, NULL);
>>>  
>>>  /* sys I/F for cooling device */
>>>  #define to_cooling_device(_dev)	\
>>> @@ -1527,6 +1546,11 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>>  	if (result)
>>>  		goto unregister;
>>>  
>>> +	/* Create available_policies attribute */
>>> +	result = device_create_file(&tz->device, &dev_attr_available_policies);
>>> +	if (result)
>>> +		goto unregister;
>>> +
>>>  	/* Update 'this' zone's governor information */
>>>  	mutex_lock(&thermal_governor_lock);
>>>  
>>> @@ -1622,6 +1646,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>>  	if (tz->ops->get_mode)
>>>  		device_remove_file(&tz->device, &dev_attr_mode);
>>>  	device_remove_file(&tz->device, &dev_attr_policy);
>>> +	device_remove_file(&tz->device, &dev_attr_available_policies);
>>>  	remove_trip_attrs(tz);
>>>  	tz->governor = NULL;
>>>  
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4819 bytes --]

On 15-01-2014 07:43, Wei Ni wrote:
> On 01/15/2014 01:50 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 14-01-2014 00:17, Wei Ni wrote:
>>> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>>>> This serie can support to turn governor for thermal zone in
>>>>>>> run time.
>>>>>>
>>>>>> Can you please explain why this is needed? Are you facing troubles with
>>>>>> current way to switch governors? If yes, can you please report them?
>>>>
>>>> Looks like there is a need to switch governors from within kernel code,
>>>> but not explanation of use cases is provided.
>>>
>>> In fact, with the of-thermal framework, the driver can't initialize to
>>> any governor, even default governor, it may be a bug. As we discussed in
>>> other mail list, we should not set governor in DT, and I think the
>>> platform driver should be able to set to any governors which it want, so
>>> I this patch, the driver can call the thermal_update_governor() to
>>> switch to new governor in kernel space.
>>> For example, there have two thermal driver, using of-thermal to register
>>> thermal zone device. One want to set to step_wise governor, another want
>>> fair_share, how can we do? I think after they calling the
>>> thermal_zone_of_sensor_register(), they can call the
>>> thermal_update_governor() to set the governor which they want.
>>>
>>>>
>>>>>>
>>>>>
>>>>> ....
>>>>>
>>>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>>>> driver can use it to update governor.
>>>>>>
>>>>>> Here I cannot see why the platform driver would need to update a
>>>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>>>> aware of governors. For updating a zone we already have an API for that,
>>>>>> please check documentation.
>>>>>>
>>>>>
>>>>> I think we have a miscommunication. The purpose of
>>>>> thermal_update_governor is to *switch* governors at runtime (from within
>>>>> the kernel).
>>>>>
>>>>> Wei has used the term "update" in the sense of switch rather than
>>>>> "update" in the sense used by thermal_zone_device_update.
>>>>
>>>> Fine, but why do you need it?
>>>
>>> Yes, I have consider to use "switch", but As my commit in the patch, in
>>> the future, the governor may be more complex, it may need governor
>>> parameter/configuration, and need to be tuned in run-time or in
>>> initialization.
>>
>> OK, I see you have a very complex governor coming. But you know,
>> prediction is very difficult, especially if it's about the future. :-)
> 
> Yes, absolutely agree.
> 
>>
>> It would be very constructive if you could share the governor together
>> with the proposed change. It is not fair to introduce a new API for a
>> non-existing user, don't you agree?
>>
>>> In fact we develop a new governor, based on PID logic. It will need some
>>> parameters, such as proportional, integral and derivative. these value
>>> will be initialized in different values for different thermal zone, so
>>> we want to use the thermal_zone_device_update() to switch governor or
>>> update the governor's parameters for different thermal zone.
>>
>> The parameters are a zone specific data right? Why should it be bound to
>> governor switch? The governor should be able to fetch the zone data when
>> needed.
> 
> Let me consider it carefully again.

Let me make clear, I am not against your proposal for change, as long as
you provide enough info so that we can design the API properly. For new
APIs, it is hard to design it, see its benefits and understand why
existing APIs are not enough if we do not see at least one user of the
new API. In other words, yes, I am open to changes, as long as I can
properly understand it.

> 
>>
>> Again, if you share the user of the API you are proposing we can work
>> together to make a better fit between API and users.
>>
>> For the existing governors, I don't see a need to have the stop and
>> start callbacks.
>>
>>>
>>>>
>>>>>
>>>>> Eduardo, what is your recommended technique for setting the governor of
>>>>> a thermal zone device created via device tree?
>>>>
>>>> So far the recommended (and existing) way is by user(land) decision.
>>>>
>>>>>
>>>>> thanks,
>>>>> Matt
>>>>>
>>>>> -- 
>>>>> 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
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 0/2] Support to tune governor in run time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4955 bytes --]

On 15-01-2014 07:35, Wei Ni wrote:
> On 01/15/2014 01:44 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> Hello Wei,
>>
>> On 14-01-2014 00:17, Wei Ni wrote:
>>> On 01/14/2014 05:33 AM, Eduardo Valentin wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On 13-01-2014 15:01, Matthew Longnecker wrote:
>>>>> On 1/13/2014 7:42 AM, Eduardo Valentin wrote:
>>>>>>> This serie can support to turn governor for thermal zone in
>>>>>>> run time.
>>>>>>
>>>>>> Can you please explain why this is needed? Are you facing troubles with
>>>>>> current way to switch governors? If yes, can you please report them?
>>>>
>>>> Looks like there is a need to switch governors from within kernel code,
>>>> but not explanation of use cases is provided.
>>>
>>> In fact, with the of-thermal framework, the driver can't initialize to
>>> any governor, even default governor, it may be a bug. As we discussed in
>>
>> I see. Using default governor is not a bug. The thermal zone shall not
>> be defined in way that it is specific to a policy. We must remember that
>> DT is not used for specifying policies, but only describing hardware.
>>
>>> other mail list, we should not set governor in DT, and I think the
>>> platform driver should be able to set to any governors which it want, so
>>
>> OK. But still, because you believe the platform driver has to set the
>> governor, this is not a bug. Defining which policy is used to control a
>> zone can be a later decision in the boot process. Specially because the
>> critical requirements are not dealt by the governors anyway.
>>
>>> I this patch, the driver can call the thermal_update_governor() to
>>> switch to new governor in kernel space.
>>
>> I see.
>>
>>> For example, there have two thermal driver, using of-thermal to register
>>> thermal zone device. One want to set to step_wise governor, another want
>>> fair_share, how can we do? I think after they calling the
>>> thermal_zone_of_sensor_register(), they can call the
>>> thermal_update_governor() to set the governor which they want.
>>
>> OK. Now I get a better view of what you are trying to achieve with this
>> series. However, why can't you hand this decision off to userland,
>> during, say very early boot sequence?
> 
> I think the thermal_zone_device_register() provide a way to set governor
> when register a thermal zone, and this function is exposed to all
> driver, so I supposed the thermal framework can support to handle the
> governor in kernel.

In fact, we lost that link if going via DT definitions. I will consider
your point. On the other hand, the thermal framework does not allow for
platform drivers to switch governors after registration, only via user
request.

> 
> Anyway, let's consider to switch the governor in user space.

OK.

> 
>>
>>
>>>
>>>>
>>>>>>
>>>>>
>>>>> ....
>>>>>
>>>>>>> Adds thermal_update_governor() function, so the thermal platform
>>>>>>> driver can use it to update governor.
>>>>>>
>>>>>> Here I cannot see why the platform driver would need to update a
>>>>>> governor, instead of a zone. Platform drivers are not supposed to be
>>>>>> aware of governors. For updating a zone we already have an API for that,
>>>>>> please check documentation.
>>>>>>
>>>>>
>>>>> I think we have a miscommunication. The purpose of
>>>>> thermal_update_governor is to *switch* governors at runtime (from within
>>>>> the kernel).
>>>>>
>>>>> Wei has used the term "update" in the sense of switch rather than
>>>>> "update" in the sense used by thermal_zone_device_update.
>>>>
>>>> Fine, but why do you need it?
>>>
>>> Yes, I have consider to use "switch", but As my commit in the patch, in
>>> the future, the governor may be more complex, it may need governor
>>> parameter/configuration, and need to be tuned in run-time or in
>>> initialization.
>>> In fact we develop a new governor, based on PID logic. It will need some
>>> parameters, such as proportional, integral and derivative. these value
>>> will be initialized in different values for different thermal zone, so
>>> we want to use the thermal_zone_device_update() to switch governor or
>>> update the governor's parameters for different thermal zone.
>>>
>>>>
>>>>>
>>>>> Eduardo, what is your recommended technique for setting the governor of
>>>>> a thermal zone device created via device tree?
>>>>
>>>> So far the recommended (and existing) way is by user(land) decision.
>>>>
>>>>>
>>>>> thanks,
>>>>> Matt
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Zhang Rui]

On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
> In the of-thermal driver, it register a thermal zone device
> without governor, since the governers are not static, we
> should be able to update to a new one in run-time, so I add
> a new fucntion thermal_update_governor() to do it.
> 
> In the future, the governor will be more complex, it may want
> to tune governor-specific parameters/configuration for
> different thermal zone at different run-time, so I add
> start/stop for the governor and add governor_data as governor's
> parameters, so that the thermal platform driver can change
> the governor with start/stop, and tune the governor parameters
> in run-time.

are you going to introduce a new governor with .start/.stop implemented
or are you going to introduce .start/.stop callbacks for the current
governors?

I'd like to see if there is any real request for this.
> 
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/thermal/sysfs-api.txt |    5 +++
>  drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
>  include/linux/thermal.h             |   12 ++++++
>  3 files changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index 6a70b55c..7efd8e6 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
>  This function serves as an arbitrator to set the state of a cooling
>  device. It sets the cooling device to the deepest cooling state if
>  possible.
> +
> +5.5:thermal_update_governor:
> +This function update the thermal zone's governor to a new one and update
> +the governor data of the new governor for that thermal zone. Returns 0 if
> +success, an erro code otherwise.
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index aab1df8..d922baf 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
>  			name = pos->tzp->governor_name;
>  		else
>  			name = DEFAULT_THERMAL_GOVERNOR;
> -		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
> +		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
> +			if (governor->start) {
> +				err = governor->start(pos);
> +				if (err < 0)
> +					goto exit;
> +			}
>  			pos->governor = governor;
> +		}
>  	}
>  
> +exit:
>  	mutex_unlock(&thermal_list_lock);
>  	mutex_unlock(&thermal_governor_lock);
>  
> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
>  
>  	list_for_each_entry(pos, &thermal_tz_list, node) {
>  		if (!strnicmp(pos->governor->name, governor->name,
> -						THERMAL_NAME_LENGTH))
> +						THERMAL_NAME_LENGTH)) {
> +			if (pos->governor->stop)
> +				pos->governor->stop(pos);
>  			pos->governor = NULL;
> +		}
>  	}
>  
>  	mutex_unlock(&thermal_list_lock);
> @@ -130,6 +140,51 @@ exit:
>  	return;
>  }
>  
> +/**
> + * thermal_update_governor() - update the thermal zone device's governor
> + * to a new one.
> + * @tzd: pointer of thermal zone device, which need to update governor.
> + * @name: new governor name.
> + * @gov_data: governor data for the new governor if needed.
> + *
> + * Return: On success returns 0, an error code otherwise
> + */
> +int thermal_update_governor(struct thermal_zone_device *tzd,
> +			    const char *name, void *gov_data)
> +{
> +	struct thermal_governor *old_gov, *new_gov;
> +	int ret = 0;
> +
> +	mutex_lock(&thermal_governor_lock);
> +
> +	new_gov = __find_governor(name);
> +	if (!new_gov) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	old_gov = tzd->governor;
> +
> +	if (old_gov && old_gov->stop)
> +		old_gov->stop(tzd);
> +
> +	if (new_gov->start) {
> +		ret = new_gov->start(tzd);
> +		if (ret < 0) {
> +			if (old_gov && old_gov->start)
> +				old_gov->start(tzd);
> +			goto exit;
> +		}
> +	}
> +
> +	tzd->governor = new_gov;
> +	tzd->governor_data = gov_data;
> +
> +exit:
> +	mutex_unlock(&thermal_governor_lock);
> +	return ret;
> +}
> +
>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int ret;
> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
>  {
>  	int ret = -EINVAL;
>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
> -	struct thermal_governor *gov;
>  	char name[THERMAL_NAME_LENGTH];
>  
>  	snprintf(name, sizeof(name), "%s", buf);
>  
> -	mutex_lock(&thermal_governor_lock);
> -
> -	gov = __find_governor(strim(name));
> -	if (!gov)
> +	ret = thermal_update_governor(tz, strim(name), NULL);
> +	if (ret < 0)
>  		goto exit;
>  
> -	tz->governor = gov;
>  	ret = count;
>  
>  exit:
> -	mutex_unlock(&thermal_governor_lock);
>  	return ret;
>  }
>  
> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	else
>  		tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
>  
> +	if (tz->governor->start) {

Note that tz->governor may be NULL at this time.
although this seems like a bug to me.

> +		result = tz->governor->start(tz);
> +		if (result < 0)
> +			tz->governor = NULL;
> +	}
> +
why not invoke thermal_update_governor() directly here?

thanks,
rui
>  	mutex_unlock(&thermal_governor_lock);
>  
>  	if (!tz->tzp || !tz->tzp->no_hwmon) {
> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  		return tz;
>  
>  unregister:
> +	if (tz->governor && tz->governor->stop)
> +		tz->governor->stop(tz);
> +	tz->governor = NULL;
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>  	device_unregister(&tz->device);
>  	return ERR_PTR(result);
> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  	device_remove_file(&tz->device, &dev_attr_policy);
>  	device_remove_file(&tz->device, &dev_attr_available_policies);
>  	remove_trip_attrs(tz);
> +
> +	if (tz->governor && tz->governor->stop)
> +		tz->governor->stop(tz);
>  	tz->governor = NULL;
>  

>  	thermal_remove_hwmon_sysfs(tz);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f7e11c7..a473736 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -177,6 +177,7 @@ struct thermal_zone_device {
>  	struct thermal_zone_device_ops *ops;
>  	const struct thermal_zone_params *tzp;
>  	struct thermal_governor *governor;
> +	void *governor_data;
>  	struct list_head thermal_instances;
>  	struct idr idr;
>  	struct mutex lock; /* protect thermal_instances list */
> @@ -187,6 +188,15 @@ struct thermal_zone_device {
>  /* Structure that holds thermal governor information */
>  struct thermal_governor {
>  	char name[THERMAL_NAME_LENGTH];
> +
> +	/*
> +	 * The start and stop operations will be called when thermal zone is
> +	 * registered and when change governor via sysfs or driver in running
> +	 * time.
> +	 */
> +	int (*start)(struct thermal_zone_device *tz);
> +	void (*stop)(struct thermal_zone_device *tz);
> +
>  	int (*throttle)(struct thermal_zone_device *tz, int trip);
>  	struct list_head	governor_list;
>  };
> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>  		struct thermal_cooling_device *, int);
>  void thermal_cdev_update(struct thermal_cooling_device *);
>  void thermal_notify_framework(struct thermal_zone_device *, int);
> +int thermal_update_governor(struct thermal_zone_device *, const char *,
> +			    void *);
>  
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,

Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Wei Ni]

On 01/17/2014 04:22 PM, Zhang Rui wrote:
> On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
>> In the of-thermal driver, it register a thermal zone device
>> without governor, since the governers are not static, we
>> should be able to update to a new one in run-time, so I add
>> a new fucntion thermal_update_governor() to do it.
>>
>> In the future, the governor will be more complex, it may want
>> to tune governor-specific parameters/configuration for
>> different thermal zone at different run-time, so I add
>> start/stop for the governor and add governor_data as governor's
>> parameters, so that the thermal platform driver can change
>> the governor with start/stop, and tune the governor parameters
>> in run-time.
> 
> are you going to introduce a new governor with .start/.stop implemented
> or are you going to introduce .start/.stop callbacks for the current
> governors?

We developed a new governor, based on PID logic. It will need some
parameters, such as proportional, integral and derivative. These values
will be initialized in different values for different thermal zone.
When the thermal driver use the pid governor, the thermal framework will
call the pid governor's .start(thz), and in the .start(), it will
allocate structures for those parameters, which are in the thermal zone,
something like:
int pid_thermal_gov_start(struct thermal_zone_device *tz)
{
...
gov = kzalloc(sizeof(struct pid_thermal_governor), GFP_KERNEL);
gov->parameter_1 = tz->governor_params->parameter_1;
gov->parameter_2 = tz->governor_params->parameter_2;
...
tz->governor_data = gov;

/* and also will create ABI for the pid governor in this thermal zone */
}
So when run .throttle(), it can get those parameters from the
tz->governor_data to run.
And in the .stop, it will free those structures.

I'm preparing the patches for the pid governor, and need to be reviewed
internal first, and then I will try to upstream them :)

> 
> I'd like to see if there is any real request for this.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  Documentation/thermal/sysfs-api.txt |    5 +++
>>  drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
>>  include/linux/thermal.h             |   12 ++++++
>>  3 files changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> index 6a70b55c..7efd8e6 100644
>> --- a/Documentation/thermal/sysfs-api.txt
>> +++ b/Documentation/thermal/sysfs-api.txt
>> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
>>  This function serves as an arbitrator to set the state of a cooling
>>  device. It sets the cooling device to the deepest cooling state if
>>  possible.
>> +
>> +5.5:thermal_update_governor:
>> +This function update the thermal zone's governor to a new one and update
>> +the governor data of the new governor for that thermal zone. Returns 0 if
>> +success, an erro code otherwise.
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index aab1df8..d922baf 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
>>  			name = pos->tzp->governor_name;
>>  		else
>>  			name = DEFAULT_THERMAL_GOVERNOR;
>> -		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
>> +		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
>> +			if (governor->start) {
>> +				err = governor->start(pos);
>> +				if (err < 0)
>> +					goto exit;
>> +			}
>>  			pos->governor = governor;
>> +		}
>>  	}
>>  
>> +exit:
>>  	mutex_unlock(&thermal_list_lock);
>>  	mutex_unlock(&thermal_governor_lock);
>>  
>> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
>>  
>>  	list_for_each_entry(pos, &thermal_tz_list, node) {
>>  		if (!strnicmp(pos->governor->name, governor->name,
>> -						THERMAL_NAME_LENGTH))
>> +						THERMAL_NAME_LENGTH)) {
>> +			if (pos->governor->stop)
>> +				pos->governor->stop(pos);
>>  			pos->governor = NULL;
>> +		}
>>  	}
>>  
>>  	mutex_unlock(&thermal_list_lock);
>> @@ -130,6 +140,51 @@ exit:
>>  	return;
>>  }
>>  
>> +/**
>> + * thermal_update_governor() - update the thermal zone device's governor
>> + * to a new one.
>> + * @tzd: pointer of thermal zone device, which need to update governor.
>> + * @name: new governor name.
>> + * @gov_data: governor data for the new governor if needed.
>> + *
>> + * Return: On success returns 0, an error code otherwise
>> + */
>> +int thermal_update_governor(struct thermal_zone_device *tzd,
>> +			    const char *name, void *gov_data)
>> +{
>> +	struct thermal_governor *old_gov, *new_gov;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&thermal_governor_lock);
>> +
>> +	new_gov = __find_governor(name);
>> +	if (!new_gov) {
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>> +	old_gov = tzd->governor;
>> +
>> +	if (old_gov && old_gov->stop)
>> +		old_gov->stop(tzd);
>> +
>> +	if (new_gov->start) {
>> +		ret = new_gov->start(tzd);
>> +		if (ret < 0) {
>> +			if (old_gov && old_gov->start)
>> +				old_gov->start(tzd);
>> +			goto exit;
>> +		}
>> +	}
>> +
>> +	tzd->governor = new_gov;
>> +	tzd->governor_data = gov_data;
>> +
>> +exit:
>> +	mutex_unlock(&thermal_governor_lock);
>> +	return ret;
>> +}
>> +
>>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>>  {
>>  	int ret;
>> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
>>  {
>>  	int ret = -EINVAL;
>>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
>> -	struct thermal_governor *gov;
>>  	char name[THERMAL_NAME_LENGTH];
>>  
>>  	snprintf(name, sizeof(name), "%s", buf);
>>  
>> -	mutex_lock(&thermal_governor_lock);
>> -
>> -	gov = __find_governor(strim(name));
>> -	if (!gov)
>> +	ret = thermal_update_governor(tz, strim(name), NULL);
>> +	if (ret < 0)
>>  		goto exit;
>>  
>> -	tz->governor = gov;
>>  	ret = count;
>>  
>>  exit:
>> -	mutex_unlock(&thermal_governor_lock);
>>  	return ret;
>>  }
>>  
>> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>  	else
>>  		tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
>>  
>> +	if (tz->governor->start) {
> 
> Note that tz->governor may be NULL at this time.
> although this seems like a bug to me.

Yes, I have noticed this bug.
BTW, Eduardo have submitted a new patch to set the governor to default
governor [PATCH 1/1] thermal: fix default governor assignment .

Anyway, in here, I should fix it as:
if (tz->governor && tz->governor->start)


Thanks.
Wei.

> 
>> +		result = tz->governor->start(tz);
>> +		if (result < 0)
>> +			tz->governor = NULL;
>> +	}
>> +
> why not invoke thermal_update_governor() directly here?

Oh, yes, we can invoke it here.

> 
> thanks,
> rui
>>  	mutex_unlock(&thermal_governor_lock);
>>  
>>  	if (!tz->tzp || !tz->tzp->no_hwmon) {
>> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>  		return tz;
>>  
>>  unregister:
>> +	if (tz->governor && tz->governor->stop)
>> +		tz->governor->stop(tz);
>> +	tz->governor = NULL;
>>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>>  	device_unregister(&tz->device);
>>  	return ERR_PTR(result);
>> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>  	device_remove_file(&tz->device, &dev_attr_policy);
>>  	device_remove_file(&tz->device, &dev_attr_available_policies);
>>  	remove_trip_attrs(tz);
>> +
>> +	if (tz->governor && tz->governor->stop)
>> +		tz->governor->stop(tz);
>>  	tz->governor = NULL;
>>  
> 
>>  	thermal_remove_hwmon_sysfs(tz);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index f7e11c7..a473736 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -177,6 +177,7 @@ struct thermal_zone_device {
>>  	struct thermal_zone_device_ops *ops;
>>  	const struct thermal_zone_params *tzp;
>>  	struct thermal_governor *governor;
>> +	void *governor_data;
>>  	struct list_head thermal_instances;
>>  	struct idr idr;
>>  	struct mutex lock; /* protect thermal_instances list */
>> @@ -187,6 +188,15 @@ struct thermal_zone_device {
>>  /* Structure that holds thermal governor information */
>>  struct thermal_governor {
>>  	char name[THERMAL_NAME_LENGTH];
>> +
>> +	/*
>> +	 * The start and stop operations will be called when thermal zone is
>> +	 * registered and when change governor via sysfs or driver in running
>> +	 * time.
>> +	 */
>> +	int (*start)(struct thermal_zone_device *tz);
>> +	void (*stop)(struct thermal_zone_device *tz);
>> +
>>  	int (*throttle)(struct thermal_zone_device *tz, int trip);
>>  	struct list_head	governor_list;
>>  };
>> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>>  		struct thermal_cooling_device *, int);
>>  void thermal_cdev_update(struct thermal_cooling_device *);
>>  void thermal_notify_framework(struct thermal_zone_device *, int);
>> +int thermal_update_governor(struct thermal_zone_device *, const char *,
>> +			    void *);
>>  
>>  #ifdef CONFIG_NET
>>  extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
> 
> 

Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 8471 bytes --]

On 17-01-2014 04:22, Zhang Rui wrote:
> On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
>> In the of-thermal driver, it register a thermal zone device
>> without governor, since the governers are not static, we
>> should be able to update to a new one in run-time, so I add
>> a new fucntion thermal_update_governor() to do it.
>>
>> In the future, the governor will be more complex, it may want
>> to tune governor-specific parameters/configuration for
>> different thermal zone at different run-time, so I add
>> start/stop for the governor and add governor_data as governor's
>> parameters, so that the thermal platform driver can change
>> the governor with start/stop, and tune the governor parameters
>> in run-time.
> 
> are you going to introduce a new governor with .start/.stop implemented
> or are you going to introduce .start/.stop callbacks for the current
> governors?

This is what I have been questioning.

> 
> I'd like to see if there is any real request for this.

ditto. I am for introducing these APIs when the real user of them
appears, say the new governor or the changes in the existing ones.

>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  Documentation/thermal/sysfs-api.txt |    5 +++
>>  drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
>>  include/linux/thermal.h             |   12 ++++++
>>  3 files changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> index 6a70b55c..7efd8e6 100644
>> --- a/Documentation/thermal/sysfs-api.txt
>> +++ b/Documentation/thermal/sysfs-api.txt
>> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
>>  This function serves as an arbitrator to set the state of a cooling
>>  device. It sets the cooling device to the deepest cooling state if
>>  possible.
>> +
>> +5.5:thermal_update_governor:
>> +This function update the thermal zone's governor to a new one and update
>> +the governor data of the new governor for that thermal zone. Returns 0 if
>> +success, an erro code otherwise.
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index aab1df8..d922baf 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
>>  			name = pos->tzp->governor_name;
>>  		else
>>  			name = DEFAULT_THERMAL_GOVERNOR;
>> -		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
>> +		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
>> +			if (governor->start) {
>> +				err = governor->start(pos);
>> +				if (err < 0)
>> +					goto exit;
>> +			}
>>  			pos->governor = governor;
>> +		}
>>  	}
>>  
>> +exit:
>>  	mutex_unlock(&thermal_list_lock);
>>  	mutex_unlock(&thermal_governor_lock);
>>  
>> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
>>  
>>  	list_for_each_entry(pos, &thermal_tz_list, node) {
>>  		if (!strnicmp(pos->governor->name, governor->name,
>> -						THERMAL_NAME_LENGTH))
>> +						THERMAL_NAME_LENGTH)) {
>> +			if (pos->governor->stop)
>> +				pos->governor->stop(pos);
>>  			pos->governor = NULL;
>> +		}
>>  	}
>>  
>>  	mutex_unlock(&thermal_list_lock);
>> @@ -130,6 +140,51 @@ exit:
>>  	return;
>>  }
>>  
>> +/**
>> + * thermal_update_governor() - update the thermal zone device's governor
>> + * to a new one.
>> + * @tzd: pointer of thermal zone device, which need to update governor.
>> + * @name: new governor name.
>> + * @gov_data: governor data for the new governor if needed.
>> + *
>> + * Return: On success returns 0, an error code otherwise
>> + */
>> +int thermal_update_governor(struct thermal_zone_device *tzd,
>> +			    const char *name, void *gov_data)
>> +{
>> +	struct thermal_governor *old_gov, *new_gov;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&thermal_governor_lock);
>> +
>> +	new_gov = __find_governor(name);
>> +	if (!new_gov) {
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>> +	old_gov = tzd->governor;
>> +
>> +	if (old_gov && old_gov->stop)
>> +		old_gov->stop(tzd);
>> +
>> +	if (new_gov->start) {
>> +		ret = new_gov->start(tzd);
>> +		if (ret < 0) {
>> +			if (old_gov && old_gov->start)
>> +				old_gov->start(tzd);
>> +			goto exit;
>> +		}
>> +	}
>> +
>> +	tzd->governor = new_gov;
>> +	tzd->governor_data = gov_data;
>> +
>> +exit:
>> +	mutex_unlock(&thermal_governor_lock);
>> +	return ret;
>> +}
>> +
>>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>>  {
>>  	int ret;
>> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
>>  {
>>  	int ret = -EINVAL;
>>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
>> -	struct thermal_governor *gov;
>>  	char name[THERMAL_NAME_LENGTH];
>>  
>>  	snprintf(name, sizeof(name), "%s", buf);
>>  
>> -	mutex_lock(&thermal_governor_lock);
>> -
>> -	gov = __find_governor(strim(name));
>> -	if (!gov)
>> +	ret = thermal_update_governor(tz, strim(name), NULL);
>> +	if (ret < 0)
>>  		goto exit;
>>  
>> -	tz->governor = gov;
>>  	ret = count;
>>  
>>  exit:
>> -	mutex_unlock(&thermal_governor_lock);
>>  	return ret;
>>  }
>>  
>> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>  	else
>>  		tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
>>  
>> +	if (tz->governor->start) {
> 
> Note that tz->governor may be NULL at this time.
> although this seems like a bug to me.
> 
>> +		result = tz->governor->start(tz);
>> +		if (result < 0)
>> +			tz->governor = NULL;
>> +	}
>> +
> why not invoke thermal_update_governor() directly here?
> 
> thanks,
> rui
>>  	mutex_unlock(&thermal_governor_lock);
>>  
>>  	if (!tz->tzp || !tz->tzp->no_hwmon) {
>> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>  		return tz;
>>  
>>  unregister:
>> +	if (tz->governor && tz->governor->stop)
>> +		tz->governor->stop(tz);
>> +	tz->governor = NULL;
>>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>>  	device_unregister(&tz->device);
>>  	return ERR_PTR(result);
>> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>  	device_remove_file(&tz->device, &dev_attr_policy);
>>  	device_remove_file(&tz->device, &dev_attr_available_policies);
>>  	remove_trip_attrs(tz);
>> +
>> +	if (tz->governor && tz->governor->stop)
>> +		tz->governor->stop(tz);
>>  	tz->governor = NULL;
>>  
> 
>>  	thermal_remove_hwmon_sysfs(tz);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index f7e11c7..a473736 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -177,6 +177,7 @@ struct thermal_zone_device {
>>  	struct thermal_zone_device_ops *ops;
>>  	const struct thermal_zone_params *tzp;
>>  	struct thermal_governor *governor;
>> +	void *governor_data;
>>  	struct list_head thermal_instances;
>>  	struct idr idr;
>>  	struct mutex lock; /* protect thermal_instances list */
>> @@ -187,6 +188,15 @@ struct thermal_zone_device {
>>  /* Structure that holds thermal governor information */
>>  struct thermal_governor {
>>  	char name[THERMAL_NAME_LENGTH];
>> +
>> +	/*
>> +	 * The start and stop operations will be called when thermal zone is
>> +	 * registered and when change governor via sysfs or driver in running
>> +	 * time.
>> +	 */
>> +	int (*start)(struct thermal_zone_device *tz);
>> +	void (*stop)(struct thermal_zone_device *tz);
>> +
>>  	int (*throttle)(struct thermal_zone_device *tz, int trip);
>>  	struct list_head	governor_list;
>>  };
>> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>>  		struct thermal_cooling_device *, int);
>>  void thermal_cdev_update(struct thermal_cooling_device *);
>>  void thermal_notify_framework(struct thermal_zone_device *, int);
>> +int thermal_update_governor(struct thermal_zone_device *, const char *,
>> +			    void *);
>>  
>>  #ifdef CONFIG_NET
>>  extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
> 
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Zhang Rui]

On Fri, 2014-01-17 at 17:32 +0800, Wei Ni wrote:
> On 01/17/2014 04:22 PM, Zhang Rui wrote:
> > On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
> >> In the of-thermal driver, it register a thermal zone device
> >> without governor, since the governers are not static, we
> >> should be able to update to a new one in run-time, so I add
> >> a new fucntion thermal_update_governor() to do it.
> >>
> >> In the future, the governor will be more complex, it may want
> >> to tune governor-specific parameters/configuration for
> >> different thermal zone at different run-time, so I add
> >> start/stop for the governor and add governor_data as governor's
> >> parameters, so that the thermal platform driver can change
> >> the governor with start/stop, and tune the governor parameters
> >> in run-time.
> > 
> > are you going to introduce a new governor with .start/.stop implemented
> > or are you going to introduce .start/.stop callbacks for the current
> > governors?
> 
> We developed a new governor, based on PID logic. It will need some
> parameters, such as proportional, integral and derivative. These values
> will be initialized in different values for different thermal zone.
> When the thermal driver use the pid governor, the thermal framework will
> call the pid governor's .start(thz), and in the .start(), it will
> allocate structures for those parameters, which are in the thermal zone,
> something like:
> int pid_thermal_gov_start(struct thermal_zone_device *tz)
> {
> ...
> gov = kzalloc(sizeof(struct pid_thermal_governor), GFP_KERNEL);
> gov->parameter_1 = tz->governor_params->parameter_1;
> gov->parameter_2 = tz->governor_params->parameter_2;
> ...
> tz->governor_data = gov;
> 
> /* and also will create ABI for the pid governor in this thermal zone */
> }
> So when run .throttle(), it can get those parameters from the
> tz->governor_data to run.
> And in the .stop, it will free those structures.
> 
> I'm preparing the patches for the pid governor, and need to be reviewed
> internal first, and then I will try to upstream them :)
> 
sounds interesting.

but the problem is still that you're doing two things in this patch,
one is to introduce runtime-governor tuning support,
another is to introduce two new callbacks for thermal governor.

For me, I'd like to take a patch that does the first thing ONLY, right
now, and I'd also like to see a patch that does the second thing,
together with the new governor patches.

thanks,
rui
> > 
> > I'd like to see if there is any real request for this.
> >>
> >> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  Documentation/thermal/sysfs-api.txt |    5 +++
> >>  drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
> >>  include/linux/thermal.h             |   12 ++++++
> >>  3 files changed, 88 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> >> index 6a70b55c..7efd8e6 100644
> >> --- a/Documentation/thermal/sysfs-api.txt
> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
> >>  This function serves as an arbitrator to set the state of a cooling
> >>  device. It sets the cooling device to the deepest cooling state if
> >>  possible.
> >> +
> >> +5.5:thermal_update_governor:
> >> +This function update the thermal zone's governor to a new one and update
> >> +the governor data of the new governor for that thermal zone. Returns 0 if
> >> +success, an erro code otherwise.
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index aab1df8..d922baf 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
> >>  			name = pos->tzp->governor_name;
> >>  		else
> >>  			name = DEFAULT_THERMAL_GOVERNOR;
> >> -		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
> >> +		if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
> >> +			if (governor->start) {
> >> +				err = governor->start(pos);
> >> +				if (err < 0)
> >> +					goto exit;
> >> +			}
> >>  			pos->governor = governor;
> >> +		}
> >>  	}
> >>  
> >> +exit:
> >>  	mutex_unlock(&thermal_list_lock);
> >>  	mutex_unlock(&thermal_governor_lock);
> >>  
> >> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
> >>  
> >>  	list_for_each_entry(pos, &thermal_tz_list, node) {
> >>  		if (!strnicmp(pos->governor->name, governor->name,
> >> -						THERMAL_NAME_LENGTH))
> >> +						THERMAL_NAME_LENGTH)) {
> >> +			if (pos->governor->stop)
> >> +				pos->governor->stop(pos);
> >>  			pos->governor = NULL;
> >> +		}
> >>  	}
> >>  
> >>  	mutex_unlock(&thermal_list_lock);
> >> @@ -130,6 +140,51 @@ exit:
> >>  	return;
> >>  }
> >>  
> >> +/**
> >> + * thermal_update_governor() - update the thermal zone device's governor
> >> + * to a new one.
> >> + * @tzd: pointer of thermal zone device, which need to update governor.
> >> + * @name: new governor name.
> >> + * @gov_data: governor data for the new governor if needed.
> >> + *
> >> + * Return: On success returns 0, an error code otherwise
> >> + */
> >> +int thermal_update_governor(struct thermal_zone_device *tzd,
> >> +			    const char *name, void *gov_data)
> >> +{
> >> +	struct thermal_governor *old_gov, *new_gov;
> >> +	int ret = 0;
> >> +
> >> +	mutex_lock(&thermal_governor_lock);
> >> +
> >> +	new_gov = __find_governor(name);
> >> +	if (!new_gov) {
> >> +		ret = -EINVAL;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	old_gov = tzd->governor;
> >> +
> >> +	if (old_gov && old_gov->stop)
> >> +		old_gov->stop(tzd);
> >> +
> >> +	if (new_gov->start) {
> >> +		ret = new_gov->start(tzd);
> >> +		if (ret < 0) {
> >> +			if (old_gov && old_gov->start)
> >> +				old_gov->start(tzd);
> >> +			goto exit;
> >> +		}
> >> +	}
> >> +
> >> +	tzd->governor = new_gov;
> >> +	tzd->governor_data = gov_data;
> >> +
> >> +exit:
> >> +	mutex_unlock(&thermal_governor_lock);
> >> +	return ret;
> >> +}
> >> +
> >>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
> >>  {
> >>  	int ret;
> >> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
> >>  {
> >>  	int ret = -EINVAL;
> >>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
> >> -	struct thermal_governor *gov;
> >>  	char name[THERMAL_NAME_LENGTH];
> >>  
> >>  	snprintf(name, sizeof(name), "%s", buf);
> >>  
> >> -	mutex_lock(&thermal_governor_lock);
> >> -
> >> -	gov = __find_governor(strim(name));
> >> -	if (!gov)
> >> +	ret = thermal_update_governor(tz, strim(name), NULL);
> >> +	if (ret < 0)
> >>  		goto exit;
> >>  
> >> -	tz->governor = gov;
> >>  	ret = count;
> >>  
> >>  exit:
> >> -	mutex_unlock(&thermal_governor_lock);
> >>  	return ret;
> >>  }
> >>  
> >> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> >>  	else
> >>  		tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
> >>  
> >> +	if (tz->governor->start) {
> > 
> > Note that tz->governor may be NULL at this time.
> > although this seems like a bug to me.
> 
> Yes, I have noticed this bug.
> BTW, Eduardo have submitted a new patch to set the governor to default
> governor [PATCH 1/1] thermal: fix default governor assignment .
> 
> Anyway, in here, I should fix it as:
> if (tz->governor && tz->governor->start)
> 
> 
> Thanks.
> Wei.
> 
> > 
> >> +		result = tz->governor->start(tz);
> >> +		if (result < 0)
> >> +			tz->governor = NULL;
> >> +	}
> >> +
> > why not invoke thermal_update_governor() directly here?
> 
> Oh, yes, we can invoke it here.
> 
> > 
> > thanks,
> > rui
> >>  	mutex_unlock(&thermal_governor_lock);
> >>  
> >>  	if (!tz->tzp || !tz->tzp->no_hwmon) {
> >> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> >>  		return tz;
> >>  
> >>  unregister:
> >> +	if (tz->governor && tz->governor->stop)
> >> +		tz->governor->stop(tz);
> >> +	tz->governor = NULL;
> >>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> >>  	device_unregister(&tz->device);
> >>  	return ERR_PTR(result);
> >> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> >>  	device_remove_file(&tz->device, &dev_attr_policy);
> >>  	device_remove_file(&tz->device, &dev_attr_available_policies);
> >>  	remove_trip_attrs(tz);
> >> +
> >> +	if (tz->governor && tz->governor->stop)
> >> +		tz->governor->stop(tz);
> >>  	tz->governor = NULL;
> >>  
> > 
> >>  	thermal_remove_hwmon_sysfs(tz);
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index f7e11c7..a473736 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -177,6 +177,7 @@ struct thermal_zone_device {
> >>  	struct thermal_zone_device_ops *ops;
> >>  	const struct thermal_zone_params *tzp;
> >>  	struct thermal_governor *governor;
> >> +	void *governor_data;
> >>  	struct list_head thermal_instances;
> >>  	struct idr idr;
> >>  	struct mutex lock; /* protect thermal_instances list */
> >> @@ -187,6 +188,15 @@ struct thermal_zone_device {
> >>  /* Structure that holds thermal governor information */
> >>  struct thermal_governor {
> >>  	char name[THERMAL_NAME_LENGTH];
> >> +
> >> +	/*
> >> +	 * The start and stop operations will be called when thermal zone is
> >> +	 * registered and when change governor via sysfs or driver in running
> >> +	 * time.
> >> +	 */
> >> +	int (*start)(struct thermal_zone_device *tz);
> >> +	void (*stop)(struct thermal_zone_device *tz);
> >> +
> >>  	int (*throttle)(struct thermal_zone_device *tz, int trip);
> >>  	struct list_head	governor_list;
> >>  };
> >> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
> >>  		struct thermal_cooling_device *, int);
> >>  void thermal_cdev_update(struct thermal_cooling_device *);
> >>  void thermal_notify_framework(struct thermal_zone_device *, int);
> >> +int thermal_update_governor(struct thermal_zone_device *, const char *,
> >> +			    void *);
> >>  
> >>  #ifdef CONFIG_NET
> >>  extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
> > 
> > 
> 

Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time

[Wei Ni]

On 01/20/2014 09:41 AM, Zhang Rui wrote:
> On Fri, 2014-01-17 at 17:32 +0800, Wei Ni wrote:
>> On 01/17/2014 04:22 PM, Zhang Rui wrote:
>>> On Mon, 2014-01-13 at 19:17 +0800, Wei Ni wrote:
>>>> In the of-thermal driver, it register a thermal zone device
>>>> without governor, since the governers are not static, we
>>>> should be able to update to a new one in run-time, so I add
>>>> a new fucntion thermal_update_governor() to do it.
>>>>
>>>> In the future, the governor will be more complex, it may want
>>>> to tune governor-specific parameters/configuration for
>>>> different thermal zone at different run-time, so I add
>>>> start/stop for the governor and add governor_data as governor's
>>>> parameters, so that the thermal platform driver can change
>>>> the governor with start/stop, and tune the governor parameters
>>>> in run-time.
>>>
>>> are you going to introduce a new governor with .start/.stop implemented
>>> or are you going to introduce .start/.stop callbacks for the current
>>> governors?
>>
>> We developed a new governor, based on PID logic. It will need some
>> parameters, such as proportional, integral and derivative. These values
>> will be initialized in different values for different thermal zone.
>> When the thermal driver use the pid governor, the thermal framework will
>> call the pid governor's .start(thz), and in the .start(), it will
>> allocate structures for those parameters, which are in the thermal zone,
>> something like:
>> int pid_thermal_gov_start(struct thermal_zone_device *tz)
>> {
>> ...
>> gov = kzalloc(sizeof(struct pid_thermal_governor), GFP_KERNEL);
>> gov->parameter_1 = tz->governor_params->parameter_1;
>> gov->parameter_2 = tz->governor_params->parameter_2;
>> ...
>> tz->governor_data = gov;
>>
>> /* and also will create ABI for the pid governor in this thermal zone */
>> }
>> So when run .throttle(), it can get those parameters from the
>> tz->governor_data to run.
>> And in the .stop, it will free those structures.
>>
>> I'm preparing the patches for the pid governor, and need to be reviewed
>> internal first, and then I will try to upstream them :)
>>
> sounds interesting.
> 
> but the problem is still that you're doing two things in this patch,
> one is to introduce runtime-governor tuning support,
> another is to introduce two new callbacks for thermal governor.
> 
> For me, I'd like to take a patch that does the first thing ONLY, right
> now, and I'd also like to see a patch that does the second thing,
> together with the new governor patches.

Ok, I will send the patch just for the thermal_update_governor() first.

Thanks.
Wei.

> 
> thanks,
> rui
>>>
>>> I'd like to see if there is any real request for this.
>>>>
>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>> ---
>>>>  Documentation/thermal/sysfs-api.txt |    5 +++
>>>>  drivers/thermal/thermal_core.c      |   80 +++++++++++++++++++++++++++++++----
>>>>  include/linux/thermal.h             |   12 ++++++
>>>>  3 files changed, 88 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>>> index 6a70b55c..7efd8e6 100644
>>>> --- a/Documentation/thermal/sysfs-api.txt
>>>> +++ b/Documentation/thermal/sysfs-api.txt
>>>> @@ -405,3 +405,8 @@ platform data is provided, this uses the step_wise throttling policy.
>>>>  This function serves as an arbitrator to set the state of a cooling
>>>>  device. It sets the cooling device to the deepest cooling state if
>>>>  possible.
>>>> +
>>>> +5.5:thermal_update_governor:
>>>> +This function update the thermal zone's governor to a new one and update
>>>> +the governor data of the new governor for that thermal zone. Returns 0 if
>>>> +success, an erro code otherwise.
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index aab1df8..d922baf 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -93,10 +93,17 @@ int thermal_register_governor(struct thermal_governor *governor)
>>>>                    name = pos->tzp->governor_name;
>>>>            else
>>>>                    name = DEFAULT_THERMAL_GOVERNOR;
>>>> -          if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH))
>>>> +          if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) {
>>>> +                  if (governor->start) {
>>>> +                          err = governor->start(pos);
>>>> +                          if (err < 0)
>>>> +                                  goto exit;
>>>> +                  }
>>>>                    pos->governor = governor;
>>>> +          }
>>>>    }
>>>>
>>>> +exit:
>>>>    mutex_unlock(&thermal_list_lock);
>>>>    mutex_unlock(&thermal_governor_lock);
>>>>
>>>> @@ -119,8 +126,11 @@ void thermal_unregister_governor(struct thermal_governor *governor)
>>>>
>>>>    list_for_each_entry(pos, &thermal_tz_list, node) {
>>>>            if (!strnicmp(pos->governor->name, governor->name,
>>>> -                                          THERMAL_NAME_LENGTH))
>>>> +                                          THERMAL_NAME_LENGTH)) {
>>>> +                  if (pos->governor->stop)
>>>> +                          pos->governor->stop(pos);
>>>>                    pos->governor = NULL;
>>>> +          }
>>>>    }
>>>>
>>>>    mutex_unlock(&thermal_list_lock);
>>>> @@ -130,6 +140,51 @@ exit:
>>>>    return;
>>>>  }
>>>>
>>>> +/**
>>>> + * thermal_update_governor() - update the thermal zone device's governor
>>>> + * to a new one.
>>>> + * @tzd: pointer of thermal zone device, which need to update governor.
>>>> + * @name: new governor name.
>>>> + * @gov_data: governor data for the new governor if needed.
>>>> + *
>>>> + * Return: On success returns 0, an error code otherwise
>>>> + */
>>>> +int thermal_update_governor(struct thermal_zone_device *tzd,
>>>> +                      const char *name, void *gov_data)
>>>> +{
>>>> +  struct thermal_governor *old_gov, *new_gov;
>>>> +  int ret = 0;
>>>> +
>>>> +  mutex_lock(&thermal_governor_lock);
>>>> +
>>>> +  new_gov = __find_governor(name);
>>>> +  if (!new_gov) {
>>>> +          ret = -EINVAL;
>>>> +          goto exit;
>>>> +  }
>>>> +
>>>> +  old_gov = tzd->governor;
>>>> +
>>>> +  if (old_gov && old_gov->stop)
>>>> +          old_gov->stop(tzd);
>>>> +
>>>> +  if (new_gov->start) {
>>>> +          ret = new_gov->start(tzd);
>>>> +          if (ret < 0) {
>>>> +                  if (old_gov && old_gov->start)
>>>> +                          old_gov->start(tzd);
>>>> +                  goto exit;
>>>> +          }
>>>> +  }
>>>> +
>>>> +  tzd->governor = new_gov;
>>>> +  tzd->governor_data = gov_data;
>>>> +
>>>> +exit:
>>>> +  mutex_unlock(&thermal_governor_lock);
>>>> +  return ret;
>>>> +}
>>>> +
>>>>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>>>>  {
>>>>    int ret;
>>>> @@ -734,22 +789,17 @@ policy_store(struct device *dev, struct device_attribute *attr,
>>>>  {
>>>>    int ret = -EINVAL;
>>>>    struct thermal_zone_device *tz = to_thermal_zone(dev);
>>>> -  struct thermal_governor *gov;
>>>>    char name[THERMAL_NAME_LENGTH];
>>>>
>>>>    snprintf(name, sizeof(name), "%s", buf);
>>>>
>>>> -  mutex_lock(&thermal_governor_lock);
>>>> -
>>>> -  gov = __find_governor(strim(name));
>>>> -  if (!gov)
>>>> +  ret = thermal_update_governor(tz, strim(name), NULL);
>>>> +  if (ret < 0)
>>>>            goto exit;
>>>>
>>>> -  tz->governor = gov;
>>>>    ret = count;
>>>>
>>>>  exit:
>>>> -  mutex_unlock(&thermal_governor_lock);
>>>>    return ret;
>>>>  }
>>>>
>>>> @@ -1559,6 +1609,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>>>    else
>>>>            tz->governor = __find_governor(DEFAULT_THERMAL_GOVERNOR);
>>>>
>>>> +  if (tz->governor->start) {
>>>
>>> Note that tz->governor may be NULL at this time.
>>> although this seems like a bug to me.
>>
>> Yes, I have noticed this bug.
>> BTW, Eduardo have submitted a new patch to set the governor to default
>> governor [PATCH 1/1] thermal: fix default governor assignment .
>>
>> Anyway, in here, I should fix it as:
>> if (tz->governor && tz->governor->start)
>>
>>
>> Thanks.
>> Wei.
>>
>>>
>>>> +          result = tz->governor->start(tz);
>>>> +          if (result < 0)
>>>> +                  tz->governor = NULL;
>>>> +  }
>>>> +
>>> why not invoke thermal_update_governor() directly here?
>>
>> Oh, yes, we can invoke it here.
>>
>>>
>>> thanks,
>>> rui
>>>>    mutex_unlock(&thermal_governor_lock);
>>>>
>>>>    if (!tz->tzp || !tz->tzp->no_hwmon) {
>>>> @@ -1585,6 +1641,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>>>            return tz;
>>>>
>>>>  unregister:
>>>> +  if (tz->governor && tz->governor->stop)
>>>> +          tz->governor->stop(tz);
>>>> +  tz->governor = NULL;
>>>>    release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>>>>    device_unregister(&tz->device);
>>>>    return ERR_PTR(result);
>>>> @@ -1648,6 +1707,9 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>>>    device_remove_file(&tz->device, &dev_attr_policy);
>>>>    device_remove_file(&tz->device, &dev_attr_available_policies);
>>>>    remove_trip_attrs(tz);
>>>> +
>>>> +  if (tz->governor && tz->governor->stop)
>>>> +          tz->governor->stop(tz);
>>>>    tz->governor = NULL;
>>>>
>>>
>>>>    thermal_remove_hwmon_sysfs(tz);
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index f7e11c7..a473736 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -177,6 +177,7 @@ struct thermal_zone_device {
>>>>    struct thermal_zone_device_ops *ops;
>>>>    const struct thermal_zone_params *tzp;
>>>>    struct thermal_governor *governor;
>>>> +  void *governor_data;
>>>>    struct list_head thermal_instances;
>>>>    struct idr idr;
>>>>    struct mutex lock; /* protect thermal_instances list */
>>>> @@ -187,6 +188,15 @@ struct thermal_zone_device {
>>>>  /* Structure that holds thermal governor information */
>>>>  struct thermal_governor {
>>>>    char name[THERMAL_NAME_LENGTH];
>>>> +
>>>> +  /*
>>>> +   * The start and stop operations will be called when thermal zone is
>>>> +   * registered and when change governor via sysfs or driver in running
>>>> +   * time.
>>>> +   */
>>>> +  int (*start)(struct thermal_zone_device *tz);
>>>> +  void (*stop)(struct thermal_zone_device *tz);
>>>> +
>>>>    int (*throttle)(struct thermal_zone_device *tz, int trip);
>>>>    struct list_head        governor_list;
>>>>  };
>>>> @@ -293,6 +303,8 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>>>>            struct thermal_cooling_device *, int);
>>>>  void thermal_cdev_update(struct thermal_cooling_device *);
>>>>  void thermal_notify_framework(struct thermal_zone_device *, int);
>>>> +int thermal_update_governor(struct thermal_zone_device *, const char *,
>>>> +                      void *);
>>>>
>>>>  #ifdef CONFIG_NET
>>>>  extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
>>>
>>>
>>
> 
> 


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------