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

From: Eduardo Valentin <eduardo.valentin@ti.com>
To: Zhang Rui <rui.zhang@intel.com>
Cc: Wei Ni <wni@nvidia.com>,
	eduardo.valentin@ti.com, mark.rutland@arm.com,
	durgadoss.r@intel.com, MLongnecker@nvidia.com,
	swarren@wwwdotorg.org, linux-pm@vger.kernel.org,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH v2 2/2] thermal: add interface to support tune governor in run-time
Date: Fri, 17 Jan 2014 12:01:05 -0400	[thread overview]
Message-ID: <52D953C1.9050908@ti.com> (raw)
In-Reply-To: <1389946970.5918.46.camel@rzhang1-mobl4>

[-- 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 --]