Message-ID: <512FB02C.3090802@ti.com>
Date:	Thu, 28 Feb 2013 15:29:48 -0400
From:	Eduardo Valentin <eduardo.valentin@...com>
To:	Durgadoss R <durgadoss.r@...el.com>
CC:	<rui.zhang@...el.com>, <linux-pm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <hongbo.zhang@...aro.org>,
	<wni@...dia.com>
Subject: Re: [PATCH 2/8] Thermal: Create zone level APIs

On 05-02-2013 06:46, Durgadoss R wrote:
> This patch adds a new thermal_zone structure to
> thermal.h. Also, adds zone level APIs to the thermal
> framework.
>
> A thermal zone is a hot spot on the platform, which
> can have one or more sensors and cooling devices attached
> to it. These sensors can be mapped to a set of cooling
> devices, which when throttled, can help to bring down
> the temperature of the hot spot.
>
> Signed-off-by: Durgadoss R <durgadoss.r@...el.com>
> ---
>   drivers/thermal/thermal_sys.c |  196 +++++++++++++++++++++++++++++++++++++++++
>   include/linux/thermal.h       |   22 +++++
>   2 files changed, 218 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index cb94497..838d4fb 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -43,19 +43,46 @@ MODULE_DESCRIPTION("Generic thermal management sysfs support");
>   MODULE_LICENSE("GPL");
>
>   static DEFINE_IDR(thermal_tz_idr);
> +static DEFINE_IDR(thermal_zone_idr);
>   static DEFINE_IDR(thermal_cdev_idr);
>   static DEFINE_IDR(thermal_sensor_idr);
>   static DEFINE_MUTEX(thermal_idr_lock);
>
>   static LIST_HEAD(thermal_tz_list);
>   static LIST_HEAD(thermal_sensor_list);
> +static LIST_HEAD(thermal_zone_list);
>   static LIST_HEAD(thermal_cdev_list);
>   static LIST_HEAD(thermal_governor_list);
>
>   static DEFINE_MUTEX(thermal_list_lock);
>   static DEFINE_MUTEX(sensor_list_lock);
> +static DEFINE_MUTEX(zone_list_lock);
>   static DEFINE_MUTEX(thermal_governor_lock);
>
> +#define for_each_thermal_sensor(pos) \
> +	list_for_each_entry(pos, &thermal_sensor_list, node)
> +
> +#define for_each_thermal_zone(pos) \
> +	list_for_each_entry(pos, &thermal_zone_list, node)
> +
> +#define GET_INDEX(tz, ptr, type)			\

Why do you need type? You seam to be using it only for sensors. It 
becomes a bit cryptic :-)

> +({							\
> +	int i, ret = -EINVAL;				\
> +	do {						\
> +		if (!tz || !ptr)			\
> +			break;				\
> +		mutex_lock(&type##_list_lock);		\
> +		for (i = 0; i < tz->type##_indx; i++) {	\
> +			if (tz->type##s[i] == ptr) {	\
> +				ret = i;		\
> +				break;			\
> +			}				\
> +		}					\
> +		mutex_unlock(&type##_list_lock);	\
> +	} while (0);					\
> +	ret;						\
> +})
> +
>   static struct thermal_governor *__find_governor(const char *name)
>   {
>   	struct thermal_governor *pos;
> @@ -421,15 +448,44 @@ static void thermal_zone_device_check(struct work_struct *work)
>   	thermal_zone_device_update(tz);
>   }
>
> +static void remove_sensor_from_zone(struct thermal_zone *tz,
> +				struct thermal_sensor *ts)
> +{
> +	int j, indx;
> +
> +	indx = GET_INDEX(tz, ts, sensor);
> +	if (indx < 0)
> +		return;
> +
> +	sysfs_remove_link(&tz->device.kobj, kobject_name(&ts->device.kobj));
> +
> +	/* Shift the entries in the tz->sensors array */
> +	for (j = indx; j < MAX_SENSORS_PER_ZONE - 1; j++)
> +		tz->sensors[j] = tz->sensors[j + 1];
> +
> +	tz->sensor_indx--;
> +}
> +
>   /* sys I/F for thermal zone */
>
>   #define to_thermal_zone(_dev) \
>   	container_of(_dev, struct thermal_zone_device, device)
>
> +#define to_zone(_dev) \
> +	container_of(_dev, struct thermal_zone, device)
> +
>   #define to_thermal_sensor(_dev) \
>   	container_of(_dev, struct thermal_sensor, device)
>
>   static ssize_t
> +zone_name_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct thermal_zone *tz = to_zone(dev);
> +
> +	return sprintf(buf, "%s\n", tz->name);
snprintf

> +}
> +
> +static ssize_t
>   sensor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
>   	struct thermal_sensor *ts = to_thermal_sensor(dev);
> @@ -811,6 +867,8 @@ static DEVICE_ATTR(policy, S_IRUGO | S_IWUSR, policy_show, policy_store);
>   static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
>   static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
>
> +static DEVICE_ATTR(zone_name, 0444, zone_name_show, NULL);
> +
>   /* sys I/F for cooling device */
>   #define to_cooling_device(_dev)	\
>   	container_of(_dev, struct thermal_cooling_device, device)
> @@ -1656,6 +1714,136 @@ static int enable_sensor_thresholds(struct thermal_sensor *ts, int count)
>   	return 0;
>   }
>
> +struct thermal_zone *create_thermal_zone(const char *name, void *devdata)
> +{
> +	struct thermal_zone *tz;
> +	int ret;
> +
> +	if (!name || (name && strlen(name) >= THERMAL_NAME_LENGTH))
> +		return ERR_PTR(-EINVAL);
> +
> +	tz = kzalloc(sizeof(*tz), GFP_KERNEL);

devm_ helpers

> +	if (!tz)
> +		return ERR_PTR(-ENOMEM);
> +
> +	idr_init(&tz->idr);
> +	ret = get_idr(&thermal_zone_idr, &thermal_idr_lock, &tz->id);
> +	if (ret)
> +		goto exit_free;
> +
> +	strcpy(tz->name, name);

strlcpy

> +	tz->devdata = devdata;
> +	tz->device.class = &thermal_class;
> +
> +	dev_set_name(&tz->device, "zone%d", tz->id);
> +	ret = device_register(&tz->device);
> +	if (ret)
> +		goto exit_idr;
> +
> +	ret = device_create_file(&tz->device, &dev_attr_zone_name);
> +	if (ret)
> +		goto exit_unregister;
> +
> +	/* Add this zone to the global list of thermal zones */
> +	mutex_lock(&zone_list_lock);
> +	list_add_tail(&tz->node, &thermal_zone_list);
> +	mutex_unlock(&zone_list_lock);
> +	return tz;
> +
> +exit_unregister:
> +	device_unregister(&tz->device);
> +exit_idr:
> +	release_idr(&thermal_zone_idr, &thermal_idr_lock, tz->id);
> +exit_free:
> +	kfree(tz);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(create_thermal_zone);
> +
> +void remove_thermal_zone(struct thermal_zone *tz)
> +{
> +	struct thermal_zone *pos, *next;
> +	bool found = false;
> +
> +	if (!tz)
> +		return;
> +
> +	mutex_lock(&zone_list_lock);
> +
> +	list_for_each_entry_safe(pos, next, &thermal_zone_list, node) {
> +		if (pos == tz) {
> +			list_del(&tz->node);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		goto exit;
> +
> +	device_remove_file(&tz->device, &dev_attr_zone_name);
> +
> +	release_idr(&thermal_zone_idr, &thermal_idr_lock, tz->id);
> +	idr_destroy(&tz->idr);
> +
> +	device_unregister(&tz->device);
> +	kfree(tz);
> +exit:
> +	mutex_unlock(&zone_list_lock);
> +	return;
> +}
> +EXPORT_SYMBOL(remove_thermal_zone);

Do we need to impose removal ordering here? Meaning, what happens if the 
above is called with sensors registered?

> +
> +struct thermal_sensor *get_sensor_by_name(const char *name)
Why do you need this function? does not seam to be used anywhere in this 
patch. Besides it is unrelated to what this patch proposes itself to do.

> +{
> +	struct thermal_sensor *pos;
> +	struct thermal_sensor *ts = NULL;
> +
> +	mutex_lock(&sensor_list_lock);
> +	for_each_thermal_sensor(pos) {
> +		if (!strnicmp(pos->name, name, THERMAL_NAME_LENGTH)) {
> +			ts = pos;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&sensor_list_lock);
> +	return ts;
> +}
> +EXPORT_SYMBOL(get_sensor_by_name);
> +
> +int add_sensor_to_zone(struct thermal_zone *tz, struct thermal_sensor *ts)
> +{
> +	int ret;
> +
> +	if (!tz || !ts)
> +		return -EINVAL;
> +
> +	mutex_lock(&zone_list_lock);
> +
> +	/* Ensure we are not adding the same sensor again!! */
> +	ret = GET_INDEX(tz, ts, sensor);
> +	if (ret >= 0) {
> +		ret = -EEXIST;
> +		goto exit_zone;
> +	}
> +
> +	mutex_lock(&sensor_list_lock);
> +
> +	ret = sysfs_create_link(&tz->device.kobj, &ts->device.kobj,
> +				kobject_name(&ts->device.kobj));
> +	if (ret)
> +		goto exit_sensor;
> +
> +	tz->sensors[tz->sensor_indx++] = ts;

you may overflow your sensors buffer with the above implementation.

you may have a contention on sensors/sensor_indx.

Consider using linked lists.

> +
> +exit_sensor:
> +	mutex_unlock(&sensor_list_lock);
> +exit_zone:
> +	mutex_unlock(&zone_list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(add_sensor_to_zone);
> +
>   /**
>    * thermal_sensor_register - register a new thermal sensor
>    * @name:	name of the thermal sensor
> @@ -1732,6 +1920,7 @@ EXPORT_SYMBOL(thermal_sensor_register);
>   void thermal_sensor_unregister(struct thermal_sensor *ts)
>   {
>   	int i;
> +	struct thermal_zone *tz;
>   	struct thermal_sensor *pos, *next;
>   	bool found = false;
>
> @@ -1751,6 +1940,13 @@ void thermal_sensor_unregister(struct thermal_sensor *ts)
>   	if (!found)
>   		return;
>
> +	mutex_lock(&zone_list_lock);
> +
> +	for_each_thermal_zone(tz)
> +		remove_sensor_from_zone(tz, ts);
> +
> +	mutex_unlock(&zone_list_lock);
> +
>   	for (i = 0; i < ts->thresholds; i++) {
>   		device_remove_file(&ts->device, &ts->thresh_attrs[i].attr);
>   		if (ts->ops->get_hyst) {
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 5470dae..2194519 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -55,6 +55,8 @@
>   #define DEFAULT_THERMAL_GOVERNOR       "user_space"
>   #endif
>
> +#define MAX_SENSORS_PER_ZONE		5


Why not making it a linked list? Why does it need to be static? Just 
trying to avoid to maintain this number sane, as we cannot predict what 
happens in future :-)

> +
>   struct thermal_sensor;
>   struct thermal_zone_device;
>   struct thermal_cooling_device;
> @@ -202,6 +204,21 @@ struct thermal_zone_device {
>   	struct delayed_work poll_queue;
>   };
>
> +struct thermal_zone {
> +	char name[THERMAL_NAME_LENGTH];
> +	int id;
> +	void *devdata;
> +	struct thermal_zone *ops;
> +	struct thermal_governor *governor;
> +	struct idr idr;
> +	struct device device;
> +	struct list_head node;
> +
> +	/* Sensor level information */
> +	int sensor_indx; /* index into 'sensors' array */
> +	struct thermal_sensor *sensors[MAX_SENSORS_PER_ZONE];
> +};


Could you please add documentation for the above structure? Why it does 
not need locking? what is *ops?
> +
>   /* Structure that holds thermal governor information */
>   struct thermal_governor {
>   	char name[THERMAL_NAME_LENGTH];
> @@ -274,6 +291,11 @@ struct thermal_sensor *thermal_sensor_register(const char *, int,
>   				struct thermal_sensor_ops *, void *);
>   void thermal_sensor_unregister(struct thermal_sensor *);
>
> +struct thermal_zone *create_thermal_zone(const char *, void *);
> +void remove_thermal_zone(struct thermal_zone *);
> +int add_sensor_to_zone(struct thermal_zone *, struct thermal_sensor *);
> +struct thermal_sensor *get_sensor_by_name(const char *);
> +

I believe we need a better naming for this API. First suggestion is to 
use same prefix on all of them. Probably this comment applies not only 
to this specific patch.

Besides, for all functions above, you may want to add comments 
documenting them and their parameters?

>   #ifdef CONFIG_NET
>   extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
>   						enum events event);
>

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

Powered by blists - more mailing lists