diff mbox

[1/8] Thermal: Create sensor level APIs

Message ID 1360061183-14137-2-git-send-email-durgadoss.r@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Zhang Rui
Headers show

Commit Message

durgadoss.r@intel.com Feb. 5, 2013, 10:46 a.m. UTC
This patch creates sensor level APIs, in the
generic thermal framework.

A Thermal sensor is a piece of hardware that can report
temperature of the spot in which it is placed. A thermal
sensor driver reads the temperature from this sensor
and reports it out. This kind of driver can be in
any subsystem. If the sensor needs to participate
in platform thermal management, the corresponding
driver can use the APIs introduced in this patch, to
register(or unregister) with the thermal framework.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |  280 +++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |   29 +++++
 2 files changed, 309 insertions(+)

Comments

Zhang Rui Feb. 8, 2013, 7:53 a.m. UTC | #1
On Tue, 2013-02-05 at 16:16 +0530, Durgadoss R wrote:
> This patch creates sensor level APIs, in the
> generic thermal framework.
> 
> A Thermal sensor is a piece of hardware that can report
> temperature of the spot in which it is placed. A thermal
> sensor driver reads the temperature from this sensor
> and reports it out. This kind of driver can be in
> any subsystem. If the sensor needs to participate
> in platform thermal management, the corresponding
> driver can use the APIs introduced in this patch, to
> register(or unregister) with the thermal framework.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |  280 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h       |   29 +++++
>  2 files changed, 309 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 0a1bf6b..cb94497 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -44,13 +44,16 @@ MODULE_LICENSE("GPL");
>  
>  static DEFINE_IDR(thermal_tz_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_cdev_list);
>  static LIST_HEAD(thermal_governor_list);
>  
>  static DEFINE_MUTEX(thermal_list_lock);
> +static DEFINE_MUTEX(sensor_list_lock);
>  static DEFINE_MUTEX(thermal_governor_lock);
>  
>  static struct thermal_governor *__find_governor(const char *name)
> @@ -423,6 +426,103 @@ static void thermal_zone_device_check(struct work_struct *work)
>  #define to_thermal_zone(_dev) \
>  	container_of(_dev, struct thermal_zone_device, device)
>  
> +#define to_thermal_sensor(_dev) \
> +	container_of(_dev, struct thermal_sensor, device)
> +
> +static ssize_t
> +sensor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	return sprintf(buf, "%s\n", ts->name);
> +}
> +
> +static ssize_t
> +sensor_temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	ret = ts->ops->get_temp(ts, &val);
> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t
> +hyst_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
> +		return -EINVAL;
> +
> +	ret = ts->ops->get_hyst(ts, indx, &val);
> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t
> +hyst_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!ts->ops->set_hyst)
> +		return -EPERM;
> +
> +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +
> +	ret = ts->ops->set_hyst(ts, indx, val);
> +
> +	return ret ? ret : count;
> +}
> +
> +static ssize_t
> +threshold_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> +		return -EINVAL;
> +
> +	ret = ts->ops->get_threshold(ts, indx, &val);
> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t
> +threshold_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!ts->ops->set_threshold)
> +		return -EPERM;
> +
> +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +
> +	ret = ts->ops->set_threshold(ts, indx, val);
> +
> +	return ret ? ret : count;
> +}
> +
>  static ssize_t
>  type_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -707,6 +807,10 @@ 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);
>  
> +/* Thermal sensor attributes */
> +static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
> +static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
> +
>  /* sys I/F for cooling device */
>  #define to_cooling_device(_dev)	\
>  	container_of(_dev, struct thermal_cooling_device, device)
> @@ -1493,6 +1597,182 @@ static void remove_trip_attrs(struct thermal_zone_device *tz)
>  }
>  
>  /**
> + * enable_sensor_thresholds - create sysfs nodes for thresholdX

this is a little confusing.
I'd prefer
thermal_sensor_sysfs_add() and create sensor_name and temp_input
attribute in this function as well, and thermal_sensor_sysfs_remove()
to remove the sysfs I/F.
And further more, we can do this for thermal zones and cooling devices.

others look okay to me.

thanks,
rui

--
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
durgadoss.r@intel.com Feb. 8, 2013, 8:26 a.m. UTC | #2
SGkgUnVpLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFpoYW5nLCBS
dWkNCj4gU2VudDogRnJpZGF5LCBGZWJydWFyeSAwOCwgMjAxMyAxOjI0IFBNDQo+IFRvOiBSLCBE
dXJnYWRvc3MNCj4gQ2M6IGxpbnV4LXBtQHZnZXIua2VybmVsLm9yZzsgbGludXgta2VybmVsQHZn
ZXIua2VybmVsLm9yZzsNCj4gZWR1YXJkby52YWxlbnRpbkB0aS5jb207IGhvbmdiby56aGFuZ0Bs
aW5hcm8ub3JnOyB3bmlAbnZpZGlhLmNvbQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDEvOF0gVGhl
cm1hbDogQ3JlYXRlIHNlbnNvciBsZXZlbCBBUElzDQo+IA0KPiBPbiBUdWUsIDIwMTMtMDItMDUg
YXQgMTY6MTYgKzA1MzAsIER1cmdhZG9zcyBSIHdyb3RlOg0KPiA+IFRoaXMgcGF0Y2ggY3JlYXRl
cyBzZW5zb3IgbGV2ZWwgQVBJcywgaW4gdGhlDQo+ID4gZ2VuZXJpYyB0aGVybWFsIGZyYW1ld29y
ay4NCg0KW3NuaXAuXQ0KDQo+ID4gKw0KPiA+ICBzdGF0aWMgc3NpemVfdA0KPiA+ICB0eXBlX3No
b3coc3RydWN0IGRldmljZSAqZGV2LCBzdHJ1Y3QgZGV2aWNlX2F0dHJpYnV0ZSAqYXR0ciwgY2hh
ciAqYnVmKQ0KPiA+ICB7DQo+ID4gQEAgLTcwNyw2ICs4MDcsMTAgQEAgc3RhdGljIERFVklDRV9B
VFRSKG1vZGUsIDA2NDQsIG1vZGVfc2hvdywNCj4gbW9kZV9zdG9yZSk7DQo+ID4gIHN0YXRpYyBE
RVZJQ0VfQVRUUihwYXNzaXZlLCBTX0lSVUdPIHwgU19JV1VTUiwgcGFzc2l2ZV9zaG93LA0KPiBw
YXNzaXZlX3N0b3JlKTsNCj4gPiAgc3RhdGljIERFVklDRV9BVFRSKHBvbGljeSwgU19JUlVHTyB8
IFNfSVdVU1IsIHBvbGljeV9zaG93LA0KPiBwb2xpY3lfc3RvcmUpOw0KPiA+DQo+ID4gKy8qIFRo
ZXJtYWwgc2Vuc29yIGF0dHJpYnV0ZXMgKi8NCj4gPiArc3RhdGljIERFVklDRV9BVFRSKHNlbnNv
cl9uYW1lLCAwNDQ0LCBzZW5zb3JfbmFtZV9zaG93LCBOVUxMKTsNCj4gPiArc3RhdGljIERFVklD
RV9BVFRSKHRlbXBfaW5wdXQsIDA0NDQsIHNlbnNvcl90ZW1wX3Nob3csIE5VTEwpOw0KPiA+ICsN
Cj4gPiAgLyogc3lzIEkvRiBmb3IgY29vbGluZyBkZXZpY2UgKi8NCj4gPiAgI2RlZmluZSB0b19j
b29saW5nX2RldmljZShfZGV2KQlcDQo+ID4gIAljb250YWluZXJfb2YoX2Rldiwgc3RydWN0IHRo
ZXJtYWxfY29vbGluZ19kZXZpY2UsIGRldmljZSkNCj4gPiBAQCAtMTQ5Myw2ICsxNTk3LDE4MiBA
QCBzdGF0aWMgdm9pZCByZW1vdmVfdHJpcF9hdHRycyhzdHJ1Y3QNCj4gdGhlcm1hbF96b25lX2Rl
dmljZSAqdHopDQo+ID4gIH0NCj4gPg0KPiA+ICAvKioNCj4gPiArICogZW5hYmxlX3NlbnNvcl90
aHJlc2hvbGRzIC0gY3JlYXRlIHN5c2ZzIG5vZGVzIGZvciB0aHJlc2hvbGRYDQo+IA0KPiB0aGlz
IGlzIGEgbGl0dGxlIGNvbmZ1c2luZy4NCj4gSSdkIHByZWZlcg0KPiB0aGVybWFsX3NlbnNvcl9z
eXNmc19hZGQoKSBhbmQgY3JlYXRlIHNlbnNvcl9uYW1lIGFuZCB0ZW1wX2lucHV0DQo+IGF0dHJp
YnV0ZSBpbiB0aGlzIGZ1bmN0aW9uIGFzIHdlbGwsIGFuZCB0aGVybWFsX3NlbnNvcl9zeXNmc19y
ZW1vdmUoKQ0KPiB0byByZW1vdmUgdGhlIHN5c2ZzIEkvRi4NCj4gQW5kIGZ1cnRoZXIgbW9yZSwg
d2UgY2FuIGRvIHRoaXMgZm9yIHRoZXJtYWwgem9uZXMgYW5kIGNvb2xpbmcgZGV2aWNlcy4NCg0K
SSBhZ3JlZSwgYW5kIHdlIHdpbGwgY2hhbmdlIHRoaXMuDQpTaW5jZSB0aGlzIGludm9sdmVzIGNo
YW5nZSBmb3IgYWxsIHN0YW5kYXJkIEFQSXMsIEkgdGhpbmsgaXQgaXMgYmV0dGVyIHRvIHN1Ym1p
dA0KYSBzZXBhcmF0ZSBwYXRjaCB0byBkbyB0aGlzLiBXaGF0IGRvIHlvdSB0aGluayA/DQoNCj4g
DQo+IG90aGVycyBsb29rIG9rYXkgdG8gbWUuDQoNClRoYW5rIHlvdSA6LSkNCg0KPiANCj4gdGhh
bmtzLA0KPiBydWkNCg0K
--
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
Zhang Rui Feb. 8, 2013, 9:54 a.m. UTC | #3
On Fri, 2013-02-08 at 01:26 -0700, R, Durgadoss wrote:
> Hi Rui,
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Friday, February 08, 2013 1:24 PM
> > To: R, Durgadoss
> > Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > eduardo.valentin@ti.com; hongbo.zhang@linaro.org; wni@nvidia.com
> > Subject: Re: [PATCH 1/8] Thermal: Create sensor level APIs
> > 
> > On Tue, 2013-02-05 at 16:16 +0530, Durgadoss R wrote:
> > > This patch creates sensor level APIs, in the
> > > generic thermal framework.
> 
> [snip.]
> 
> > > +
> > >  static ssize_t
> > >  type_show(struct device *dev, struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -707,6 +807,10 @@ 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);
> > >
> > > +/* Thermal sensor attributes */
> > > +static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
> > > +static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
> > > +
> > >  /* sys I/F for cooling device */
> > >  #define to_cooling_device(_dev)	\
> > >  	container_of(_dev, struct thermal_cooling_device, device)
> > > @@ -1493,6 +1597,182 @@ static void remove_trip_attrs(struct
> > thermal_zone_device *tz)
> > >  }
> > >
> > >  /**
> > > + * enable_sensor_thresholds - create sysfs nodes for thresholdX
> > 
> > this is a little confusing.
> > I'd prefer
> > thermal_sensor_sysfs_add() and create sensor_name and temp_input
> > attribute in this function as well, and thermal_sensor_sysfs_remove()
> > to remove the sysfs I/F.
> > And further more, we can do this for thermal zones and cooling devices.
> 
> I agree, and we will change this.
> Since this involves change for all standard APIs, I think it is better to submit
> a separate patch to do this. What do you think ?
> 
I'm okay with this.

thanks,
rui
> > 
> > others look okay to me.
> 
> Thank you :-)
> 
> > 
> > thanks,
> > rui
> 


--
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
Eduardo Valentin Feb. 28, 2013, 6:58 p.m. UTC | #4
Durga,

On 05-02-2013 06:46, Durgadoss R wrote:
> This patch creates sensor level APIs, in the
> generic thermal framework.
>
> A Thermal sensor is a piece of hardware that can report
> temperature of the spot in which it is placed. A thermal
> sensor driver reads the temperature from this sensor
> and reports it out. This kind of driver can be in
> any subsystem. If the sensor needs to participate
> in platform thermal management, the corresponding
> driver can use the APIs introduced in this patch, to
> register(or unregister) with the thermal framework.

At first glance, patch seams reasonable. But I have one major concern as 
follows inline, apart from several minor comments.

>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>   drivers/thermal/thermal_sys.c |  280 +++++++++++++++++++++++++++++++++++++++++
>   include/linux/thermal.h       |   29 +++++
>   2 files changed, 309 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 0a1bf6b..cb94497 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -44,13 +44,16 @@ MODULE_LICENSE("GPL");
>
>   static DEFINE_IDR(thermal_tz_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_cdev_list);
>   static LIST_HEAD(thermal_governor_list);
>
>   static DEFINE_MUTEX(thermal_list_lock);
> +static DEFINE_MUTEX(sensor_list_lock);
>   static DEFINE_MUTEX(thermal_governor_lock);
>
>   static struct thermal_governor *__find_governor(const char *name)
> @@ -423,6 +426,103 @@ static void thermal_zone_device_check(struct work_struct *work)
>   #define to_thermal_zone(_dev) \
>   	container_of(_dev, struct thermal_zone_device, device)
>
> +#define to_thermal_sensor(_dev) \
> +	container_of(_dev, struct thermal_sensor, device)
> +
> +static ssize_t
> +sensor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	return sprintf(buf, "%s\n", ts->name);

For security reasons:
s/sprintf/snprintf

> +}
> +
> +static ssize_t
> +sensor_temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	ret = ts->ops->get_temp(ts, &val);
> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);

ditto.

> +}
> +
> +static ssize_t
> +hyst_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))

I'd rather check if it returns 1.

> +		return -EINVAL;
> +
> +	ret = ts->ops->get_hyst(ts, indx, &val);

 From your probe, you won't check for devices registered with 
ops.get_hyst == NULL. This may lead to a NULL pointer access above.

> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);

snprintf.

> +}
> +
> +static ssize_t
> +hyst_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!ts->ops->set_hyst)
> +		return -EPERM;
> +
> +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +
> +	ret = ts->ops->set_hyst(ts, indx, val);

 From your probe, you won't check for devices registered with
ops.set_hyst == NULL. This may lead to a NULL pointer access above.

> +
> +	return ret ? ret : count;
> +}
> +
> +static ssize_t
> +threshold_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> +		return -EINVAL;
> +
> +	ret = ts->ops->get_threshold(ts, indx, &val);
 From your probe, you won't check for devices registered with
ops.get_threshold == NULL. This may lead to a NULL pointer access above.

> +
> +	return ret ? ret : sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t
> +threshold_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int indx, ret;
> +	long val;
> +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> +
> +	if (!ts->ops->set_threshold)
> +		return -EPERM;
> +
> +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +
> +	ret = ts->ops->set_threshold(ts, indx, val);
> +
> +	return ret ? ret : count;
> +}
> +

One may be careful with the above functions. Userland having control on 
these properties may lead to spurious events, depending on the 
programming sequence / value changing order. I believe, it would be 
wiser to disable the interrupt generation prior to changing the thresholds.

>   static ssize_t
>   type_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
> @@ -707,6 +807,10 @@ 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);
>
> +/* Thermal sensor attributes */
> +static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
> +static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
> +
>   /* sys I/F for cooling device */
>   #define to_cooling_device(_dev)	\
>   	container_of(_dev, struct thermal_cooling_device, device)
> @@ -1493,6 +1597,182 @@ static void remove_trip_attrs(struct thermal_zone_device *tz)
>   }
>
>   /**
> + * enable_sensor_thresholds - create sysfs nodes for thresholdX
> + * @ts:		the thermal sensor
> + * @count:	Number of thresholds supported by sensor hardware
> + *
> + * 'Thresholds' are temperatures programmed into the sensor hardware,
> + * on crossing which the sensor generates an interrupt. 'Trip points'
> + * are temperatures which the thermal manager/governor thinks, some
> + * action should be taken when the sensor reaches the value.
> + */
> +static int enable_sensor_thresholds(struct thermal_sensor *ts, int count)
> +{
> +	int i;
> +	int size = sizeof(struct thermal_attr) * count;
> +
> +	ts->thresh_attrs = kzalloc(size, GFP_KERNEL);

how about using devm_ helpers?

> +	if (!ts->thresh_attrs)
> +		return -ENOMEM;
> +
> +	if (ts->ops->get_hyst) {
> +		ts->hyst_attrs = kzalloc(size, GFP_KERNEL);
> +		if (!ts->hyst_attrs) {
> +			kfree(ts->thresh_attrs);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	ts->thresholds = count;
> +
> +	/* Create threshold attributes */
> +	for (i = 0; i < count; i++) {
> +		snprintf(ts->thresh_attrs[i].name, THERMAL_NAME_LENGTH,
> +						 "threshold%d", i);
> +
> +		sysfs_attr_init(&ts->thresh_attrs[i].attr.attr);
> +		ts->thresh_attrs[i].attr.attr.name = ts->thresh_attrs[i].name;
> +		ts->thresh_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> +		ts->thresh_attrs[i].attr.show = threshold_show;
> +		ts->thresh_attrs[i].attr.store = threshold_store;
> +
> +		device_create_file(&ts->device, &ts->thresh_attrs[i].attr);
> +
> +		/* Create threshold_hyst attributes */
> +		if (!ts->ops->get_hyst)
> +			continue;
> +
> +		snprintf(ts->hyst_attrs[i].name, THERMAL_NAME_LENGTH,
> +						 "threshold%d_hyst", i);
> +
> +		sysfs_attr_init(&ts->hyst_attrs[i].attr.attr);
> +		ts->hyst_attrs[i].attr.attr.name = ts->hyst_attrs[i].name;
> +		ts->hyst_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> +		ts->hyst_attrs[i].attr.show = hyst_show;
> +		ts->hyst_attrs[i].attr.store = hyst_store;
> +
> +		device_create_file(&ts->device, &ts->hyst_attrs[i].attr);
> +	}
> +	return 0;
> +}
> +


I know there are people who is in favor of having the thermal policy 
controlled in userland.
Depending on which thermal zone we are talking about, I'd even consider 
it a valid approach.
On the other hand there are thermal zones that controls silicon junction 
temperature which
does not depend on any input from userland (like which kind of use case 
one may be running).


That said, I believe we may want to segregate which sensors we want to 
expose the write access
from userspace to their hyst and threshold values. Exposing all of them 
may lead to easy security leak.


> +/**
> + * thermal_sensor_register - register a new thermal sensor
> + * @name:	name of the thermal sensor
> + * @count:	Number of thresholds supported by hardware
> + * @ops:	standard thermal sensor callbacks
> + * @devdata:	private device data
> + */
> +struct thermal_sensor *thermal_sensor_register(const char *name, int count,
> +			struct thermal_sensor_ops *ops, void *devdata)
> +{
> +	struct thermal_sensor *ts;
> +	int ret;
> +
> +	if (!name || (name && strlen(name) >= THERMAL_NAME_LENGTH))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops || !ops->get_temp)
> +		return ERR_PTR(-EINVAL);
> +

Please consider additional checks (see my comments above)

> +	ts = kzalloc(sizeof(*ts), GFP_KERNEL);

devm_ helpers

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

s/strcpy/strlcpy

> +	ts->ops = ops;
> +	ts->devdata = devdata;
> +	ts->device.class = &thermal_class;
> +
> +	dev_set_name(&ts->device, "sensor%d", ts->id);
> +	ret = device_register(&ts->device);
> +	if (ret)
> +		goto exit_idr;
> +
> +	ret = device_create_file(&ts->device, &dev_attr_sensor_name);
> +	if (ret)
> +		goto exit_unregister;
> +
> +	ret = device_create_file(&ts->device, &dev_attr_temp_input);
> +	if (ret)
> +		goto exit_name;
> +
> +	if (count > 0 && ts->ops->get_threshold) {
> +		ret = enable_sensor_thresholds(ts, count);
> +		if (ret)
> +			goto exit_temp;
> +	}
> +
> +	/* Add this sensor to the global list of sensors */
> +	mutex_lock(&sensor_list_lock);
> +	list_add_tail(&ts->node, &thermal_sensor_list);
> +	mutex_unlock(&sensor_list_lock);
> +
> +	return ts;
> +
> +exit_temp:
> +	device_remove_file(&ts->device, &dev_attr_temp_input);
> +exit_name:
> +	device_remove_file(&ts->device, &dev_attr_sensor_name);
> +exit_unregister:
> +	device_unregister(&ts->device);
> +exit_idr:
> +	release_idr(&thermal_sensor_idr, &thermal_idr_lock, ts->id);
> +exit_free:
> +	kfree(ts);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(thermal_sensor_register);
> +
> +void thermal_sensor_unregister(struct thermal_sensor *ts)
> +{
> +	int i;
> +	struct thermal_sensor *pos, *next;
> +	bool found = false;
> +
> +	if (!ts)
> +		return;
> +
> +	mutex_lock(&sensor_list_lock);
> +	list_for_each_entry_safe(pos, next, &thermal_sensor_list, node) {
> +		if (pos == ts) {
> +			list_del(&ts->node);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&sensor_list_lock);
> +
> +	if (!found)
> +		return;
> +
> +	for (i = 0; i < ts->thresholds; i++) {
> +		device_remove_file(&ts->device, &ts->thresh_attrs[i].attr);
> +		if (ts->ops->get_hyst) {
> +			device_remove_file(&ts->device,
> +					&ts->hyst_attrs[i].attr);
> +		}
> +	}
> +
> +	device_remove_file(&ts->device, &dev_attr_sensor_name);
> +	device_remove_file(&ts->device, &dev_attr_temp_input);
> +
> +	release_idr(&thermal_sensor_idr, &thermal_idr_lock, ts->id);
> +	idr_destroy(&ts->idr);
> +
> +	device_unregister(&ts->device);
> +
> +	kfree(ts);
> +	return;
> +}
> +EXPORT_SYMBOL(thermal_sensor_unregister);
> +
> +/**
>    * thermal_zone_device_register - register a new thermal zone device
>    * @type:	the thermal zone device type
>    * @trips:	the number of trip points the thermal zone support
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 9b78f8c..5470dae 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -55,6 +55,7 @@
>   #define DEFAULT_THERMAL_GOVERNOR       "user_space"
>   #endif
>
> +struct thermal_sensor;
>   struct thermal_zone_device;
>   struct thermal_cooling_device;
>
> @@ -135,6 +136,15 @@ struct thermal_cooling_device_ops {
>   	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
>   };
>
> +struct thermal_sensor_ops {
> +	int (*get_temp) (struct thermal_sensor *, long *);
> +	int (*get_trend) (struct thermal_sensor *, int, enum thermal_trend *);
> +	int (*set_threshold) (struct thermal_sensor *, int, long);
> +	int (*get_threshold) (struct thermal_sensor *, int, long *);
> +	int (*set_hyst) (struct thermal_sensor *, int, long);
> +	int (*get_hyst) (struct thermal_sensor *, int, long *);
> +};
> +
>   struct thermal_cooling_device {
>   	int id;
>   	char type[THERMAL_NAME_LENGTH];
> @@ -152,6 +162,21 @@ struct thermal_attr {
>   	char name[THERMAL_NAME_LENGTH];
>   };
>
> +struct thermal_sensor {
> +	char name[THERMAL_NAME_LENGTH];
> +	int id;
> +	int temp;
> +	int prev_temp;
> +	int thresholds;
> +	void *devdata;
> +	struct idr idr;
> +	struct device device;
> +	struct list_head node;
> +	struct thermal_sensor_ops *ops;
> +	struct thermal_attr *thresh_attrs;
> +	struct thermal_attr *hyst_attrs;
> +};
> +
>   struct thermal_zone_device {
>   	int id;
>   	char type[THERMAL_NAME_LENGTH];
> @@ -245,6 +270,10 @@ void notify_thermal_framework(struct thermal_zone_device *, int);
>   int thermal_register_governor(struct thermal_governor *);
>   void thermal_unregister_governor(struct thermal_governor *);
>
> +struct thermal_sensor *thermal_sensor_register(const char *, int,
> +				struct thermal_sensor_ops *, void *);
> +void thermal_sensor_unregister(struct thermal_sensor *);
> +
>   #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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
durgadoss.r@intel.com March 1, 2013, 5:08 a.m. UTC | #5
Hi Eduardo,

> -----Original Message-----
> From: Eduardo Valentin [mailto:eduardo.valentin@ti.com]
> Sent: Friday, March 01, 2013 12:29 AM
> To: R, Durgadoss
> Cc: Zhang, Rui; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org;
> hongbo.zhang@linaro.org; wni@nvidia.com
> Subject: Re: [PATCH 1/8] Thermal: Create sensor level APIs
> 
> Durga,
> 
> On 05-02-2013 06:46, Durgadoss R wrote:
> > This patch creates sensor level APIs, in the
> > generic thermal framework.
> >
> > A Thermal sensor is a piece of hardware that can report
> > temperature of the spot in which it is placed. A thermal
> > sensor driver reads the temperature from this sensor
> > and reports it out. This kind of driver can be in
> > any subsystem. If the sensor needs to participate
> > in platform thermal management, the corresponding
> > driver can use the APIs introduced in this patch, to
> > register(or unregister) with the thermal framework.
> 
> At first glance, patch seams reasonable. But I have one major concern as
> follows inline, apart from several minor comments.
> 
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >   drivers/thermal/thermal_sys.c |  280
> +++++++++++++++++++++++++++++++++++++++++
> >   include/linux/thermal.h       |   29 +++++
> >   2 files changed, 309 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 0a1bf6b..cb94497 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -44,13 +44,16 @@ MODULE_LICENSE("GPL");
> >
> >   static DEFINE_IDR(thermal_tz_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_cdev_list);
> >   static LIST_HEAD(thermal_governor_list);
> >
> >   static DEFINE_MUTEX(thermal_list_lock);
> > +static DEFINE_MUTEX(sensor_list_lock);
> >   static DEFINE_MUTEX(thermal_governor_lock);
> >
> >   static struct thermal_governor *__find_governor(const char *name)
> > @@ -423,6 +426,103 @@ static void thermal_zone_device_check(struct
> work_struct *work)
> >   #define to_thermal_zone(_dev) \
> >   	container_of(_dev, struct thermal_zone_device, device)
> >
> > +#define to_thermal_sensor(_dev) \
> > +	container_of(_dev, struct thermal_sensor, device)
> > +
> > +static ssize_t
> > +sensor_name_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> > +{
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	return sprintf(buf, "%s\n", ts->name);
> 
> For security reasons:
> s/sprintf/snprintf
> 
> > +}
> > +
> > +static ssize_t
> > +sensor_temp_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> > +{
> > +	int ret;
> > +	long val;
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	ret = ts->ops->get_temp(ts, &val);
> > +
> > +	return ret ? ret : sprintf(buf, "%ld\n", val);
> 
> ditto.
> 
> > +}
> > +
> > +static ssize_t
> > +hyst_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	int indx, ret;
> > +	long val;
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
> 
> I'd rather check if it returns 1.
> 
> > +		return -EINVAL;
> > +
> > +	ret = ts->ops->get_hyst(ts, indx, &val);
> 
>  From your probe, you won't check for devices registered with
> ops.get_hyst == NULL. This may lead to a NULL pointer access above.

if ops.get_hyst is NULL, we don't even create these sysfs interfaces.
This check is in enable_sensor_thresholds function.

> 
> > +
> > +	return ret ? ret : sprintf(buf, "%ld\n", val);
> 
> snprintf.
> 
> > +}
> > +
> > +static ssize_t
> > +hyst_store(struct device *dev, struct device_attribute *attr,
> > +				   const char *buf, size_t count)
> > +{
> > +	int indx, ret;
> > +	long val;
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	if (!ts->ops->set_hyst)
> > +		return -EPERM;
> > +
> > +	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
> > +		return -EINVAL;
> > +
> > +	if (kstrtol(buf, 10, &val))
> > +		return -EINVAL;
> > +
> > +	ret = ts->ops->set_hyst(ts, indx, val);
> 
>  From your probe, you won't check for devices registered with
> ops.set_hyst == NULL. This may lead to a NULL pointer access above.
> 
> > +
> > +	return ret ? ret : count;
> > +}
> > +
> > +static ssize_t
> > +threshold_show(struct device *dev, struct device_attribute *attr, char
> *buf)
> > +{
> > +	int indx, ret;
> > +	long val;
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> > +		return -EINVAL;
> > +
> > +	ret = ts->ops->get_threshold(ts, indx, &val);
>  From your probe, you won't check for devices registered with
> ops.get_threshold == NULL. This may lead to a NULL pointer access above.

It checks for ops.get_threshold, but not for the setter method.
Will take of that in next version.

> 
> > +
> > +	return ret ? ret : sprintf(buf, "%ld\n", val);
> > +}
> > +
> > +static ssize_t
> > +threshold_store(struct device *dev, struct device_attribute *attr,
> > +				   const char *buf, size_t count)
> > +{
> > +	int indx, ret;
> > +	long val;
> > +	struct thermal_sensor *ts = to_thermal_sensor(dev);
> > +
> > +	if (!ts->ops->set_threshold)
> > +		return -EPERM;
> > +
> > +	if (!sscanf(attr->attr.name, "threshold%d", &indx))
> > +		return -EINVAL;
> > +
> > +	if (kstrtol(buf, 10, &val))
> > +		return -EINVAL;
> > +
> > +	ret = ts->ops->set_threshold(ts, indx, val);
> > +
> > +	return ret ? ret : count;
> > +}
> > +
> 
> One may be careful with the above functions. Userland having control on
> these properties may lead to spurious events, depending on the
> programming sequence / value changing order. I believe, it would be
> wiser to disable the interrupt generation prior to changing the thresholds.

Valid case. We call set_threshold from the framework layer, and leave it to the
sensor driver to take care of rest of the things.

> 
> >   static ssize_t
> >   type_show(struct device *dev, struct device_attribute *attr, char *buf)
> >   {
> > @@ -707,6 +807,10 @@ 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);
> >
> > +/* Thermal sensor attributes */
> > +static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
> > +static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
> > +
> >   /* sys I/F for cooling device */
> >   #define to_cooling_device(_dev)	\
> >   	container_of(_dev, struct thermal_cooling_device, device)
> > @@ -1493,6 +1597,182 @@ static void remove_trip_attrs(struct
> thermal_zone_device *tz)
> >   }
> >
> >   /**
> > + * enable_sensor_thresholds - create sysfs nodes for thresholdX
> > + * @ts:		the thermal sensor
> > + * @count:	Number of thresholds supported by sensor hardware
> > + *
> > + * 'Thresholds' are temperatures programmed into the sensor hardware,
> > + * on crossing which the sensor generates an interrupt. 'Trip points'
> > + * are temperatures which the thermal manager/governor thinks, some
> > + * action should be taken when the sensor reaches the value.
> > + */
> > +static int enable_sensor_thresholds(struct thermal_sensor *ts, int count)
> > +{
> > +	int i;
> > +	int size = sizeof(struct thermal_attr) * count;
> > +
> > +	ts->thresh_attrs = kzalloc(size, GFP_KERNEL);
> 
> how about using devm_ helpers?

Yes, I will use.

> 
> > +	if (!ts->thresh_attrs)
> > +		return -ENOMEM;
> > +
> > +	if (ts->ops->get_hyst) {
> > +		ts->hyst_attrs = kzalloc(size, GFP_KERNEL);
> > +		if (!ts->hyst_attrs) {
> > +			kfree(ts->thresh_attrs);
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	ts->thresholds = count;
> > +
> > +	/* Create threshold attributes */
> > +	for (i = 0; i < count; i++) {
> > +		snprintf(ts->thresh_attrs[i].name,
> THERMAL_NAME_LENGTH,
> > +						 "threshold%d", i);
> > +
> > +		sysfs_attr_init(&ts->thresh_attrs[i].attr.attr);
> > +		ts->thresh_attrs[i].attr.attr.name = ts->thresh_attrs[i].name;
> > +		ts->thresh_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> > +		ts->thresh_attrs[i].attr.show = threshold_show;
> > +		ts->thresh_attrs[i].attr.store = threshold_store;
> > +
> > +		device_create_file(&ts->device, &ts->thresh_attrs[i].attr);
> > +
> > +		/* Create threshold_hyst attributes */
> > +		if (!ts->ops->get_hyst)
> > +			continue;
> > +
> > +		snprintf(ts->hyst_attrs[i].name, THERMAL_NAME_LENGTH,
> > +						 "threshold%d_hyst", i);
> > +
> > +		sysfs_attr_init(&ts->hyst_attrs[i].attr.attr);
> > +		ts->hyst_attrs[i].attr.attr.name = ts->hyst_attrs[i].name;
> > +		ts->hyst_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> > +		ts->hyst_attrs[i].attr.show = hyst_show;
> > +		ts->hyst_attrs[i].attr.store = hyst_store;
> > +
> > +		device_create_file(&ts->device, &ts->hyst_attrs[i].attr);
> > +	}
> > +	return 0;
> > +}
> > +
> 
> 
> I know there are people who is in favor of having the thermal policy
> controlled in userland.
> Depending on which thermal zone we are talking about, I'd even consider
> it a valid approach.

agreed.

> On the other hand there are thermal zones that controls silicon junction
> temperature which
> does not depend on any input from userland (like which kind of use case
> one may be running).
> 
> 
> That said, I believe we may want to segregate which sensors we want to
> expose the write access
> from userspace to their hyst and threshold values. Exposing all of them
> may lead to easy security leak.

If the sensor driver does not want this hysteresis to be writeable,
it can provide a NULL pointer in ops.set_hyst. 

Most of sprintf, strcpy I used them for consistency, as rest of the
framework code is already using these APIs.
We can do a separate clean up patch for these functions, IMHO.

That said, I also get hurt by static checkers on these :-)So, I agree with
you that these should be fixed, but not in this series.

Thanks,
Durga
--
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
diff mbox

Patch

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 0a1bf6b..cb94497 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -44,13 +44,16 @@  MODULE_LICENSE("GPL");
 
 static DEFINE_IDR(thermal_tz_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_cdev_list);
 static LIST_HEAD(thermal_governor_list);
 
 static DEFINE_MUTEX(thermal_list_lock);
+static DEFINE_MUTEX(sensor_list_lock);
 static DEFINE_MUTEX(thermal_governor_lock);
 
 static struct thermal_governor *__find_governor(const char *name)
@@ -423,6 +426,103 @@  static void thermal_zone_device_check(struct work_struct *work)
 #define to_thermal_zone(_dev) \
 	container_of(_dev, struct thermal_zone_device, device)
 
+#define to_thermal_sensor(_dev) \
+	container_of(_dev, struct thermal_sensor, device)
+
+static ssize_t
+sensor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	return sprintf(buf, "%s\n", ts->name);
+}
+
+static ssize_t
+sensor_temp_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	int ret;
+	long val;
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	ret = ts->ops->get_temp(ts, &val);
+
+	return ret ? ret : sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t
+hyst_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	int indx, ret;
+	long val;
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
+		return -EINVAL;
+
+	ret = ts->ops->get_hyst(ts, indx, &val);
+
+	return ret ? ret : sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t
+hyst_store(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int indx, ret;
+	long val;
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	if (!ts->ops->set_hyst)
+		return -EPERM;
+
+	if (!sscanf(attr->attr.name, "threshold%d_hyst", &indx))
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &val))
+		return -EINVAL;
+
+	ret = ts->ops->set_hyst(ts, indx, val);
+
+	return ret ? ret : count;
+}
+
+static ssize_t
+threshold_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	int indx, ret;
+	long val;
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	if (!sscanf(attr->attr.name, "threshold%d", &indx))
+		return -EINVAL;
+
+	ret = ts->ops->get_threshold(ts, indx, &val);
+
+	return ret ? ret : sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t
+threshold_store(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int indx, ret;
+	long val;
+	struct thermal_sensor *ts = to_thermal_sensor(dev);
+
+	if (!ts->ops->set_threshold)
+		return -EPERM;
+
+	if (!sscanf(attr->attr.name, "threshold%d", &indx))
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &val))
+		return -EINVAL;
+
+	ret = ts->ops->set_threshold(ts, indx, val);
+
+	return ret ? ret : count;
+}
+
 static ssize_t
 type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -707,6 +807,10 @@  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);
 
+/* Thermal sensor attributes */
+static DEVICE_ATTR(sensor_name, 0444, sensor_name_show, NULL);
+static DEVICE_ATTR(temp_input, 0444, sensor_temp_show, NULL);
+
 /* sys I/F for cooling device */
 #define to_cooling_device(_dev)	\
 	container_of(_dev, struct thermal_cooling_device, device)
@@ -1493,6 +1597,182 @@  static void remove_trip_attrs(struct thermal_zone_device *tz)
 }
 
 /**
+ * enable_sensor_thresholds - create sysfs nodes for thresholdX
+ * @ts:		the thermal sensor
+ * @count:	Number of thresholds supported by sensor hardware
+ *
+ * 'Thresholds' are temperatures programmed into the sensor hardware,
+ * on crossing which the sensor generates an interrupt. 'Trip points'
+ * are temperatures which the thermal manager/governor thinks, some
+ * action should be taken when the sensor reaches the value.
+ */
+static int enable_sensor_thresholds(struct thermal_sensor *ts, int count)
+{
+	int i;
+	int size = sizeof(struct thermal_attr) * count;
+
+	ts->thresh_attrs = kzalloc(size, GFP_KERNEL);
+	if (!ts->thresh_attrs)
+		return -ENOMEM;
+
+	if (ts->ops->get_hyst) {
+		ts->hyst_attrs = kzalloc(size, GFP_KERNEL);
+		if (!ts->hyst_attrs) {
+			kfree(ts->thresh_attrs);
+			return -ENOMEM;
+		}
+	}
+
+	ts->thresholds = count;
+
+	/* Create threshold attributes */
+	for (i = 0; i < count; i++) {
+		snprintf(ts->thresh_attrs[i].name, THERMAL_NAME_LENGTH,
+						 "threshold%d", i);
+
+		sysfs_attr_init(&ts->thresh_attrs[i].attr.attr);
+		ts->thresh_attrs[i].attr.attr.name = ts->thresh_attrs[i].name;
+		ts->thresh_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
+		ts->thresh_attrs[i].attr.show = threshold_show;
+		ts->thresh_attrs[i].attr.store = threshold_store;
+
+		device_create_file(&ts->device, &ts->thresh_attrs[i].attr);
+
+		/* Create threshold_hyst attributes */
+		if (!ts->ops->get_hyst)
+			continue;
+
+		snprintf(ts->hyst_attrs[i].name, THERMAL_NAME_LENGTH,
+						 "threshold%d_hyst", i);
+
+		sysfs_attr_init(&ts->hyst_attrs[i].attr.attr);
+		ts->hyst_attrs[i].attr.attr.name = ts->hyst_attrs[i].name;
+		ts->hyst_attrs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
+		ts->hyst_attrs[i].attr.show = hyst_show;
+		ts->hyst_attrs[i].attr.store = hyst_store;
+
+		device_create_file(&ts->device, &ts->hyst_attrs[i].attr);
+	}
+	return 0;
+}
+
+/**
+ * thermal_sensor_register - register a new thermal sensor
+ * @name:	name of the thermal sensor
+ * @count:	Number of thresholds supported by hardware
+ * @ops:	standard thermal sensor callbacks
+ * @devdata:	private device data
+ */
+struct thermal_sensor *thermal_sensor_register(const char *name, int count,
+			struct thermal_sensor_ops *ops, void *devdata)
+{
+	struct thermal_sensor *ts;
+	int ret;
+
+	if (!name || (name && strlen(name) >= THERMAL_NAME_LENGTH))
+		return ERR_PTR(-EINVAL);
+
+	if (!ops || !ops->get_temp)
+		return ERR_PTR(-EINVAL);
+
+	ts = kzalloc(sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return ERR_PTR(-ENOMEM);
+
+	idr_init(&ts->idr);
+	ret = get_idr(&thermal_sensor_idr, &thermal_idr_lock, &ts->id);
+	if (ret)
+		goto exit_free;
+
+	strcpy(ts->name, name);
+	ts->ops = ops;
+	ts->devdata = devdata;
+	ts->device.class = &thermal_class;
+
+	dev_set_name(&ts->device, "sensor%d", ts->id);
+	ret = device_register(&ts->device);
+	if (ret)
+		goto exit_idr;
+
+	ret = device_create_file(&ts->device, &dev_attr_sensor_name);
+	if (ret)
+		goto exit_unregister;
+
+	ret = device_create_file(&ts->device, &dev_attr_temp_input);
+	if (ret)
+		goto exit_name;
+
+	if (count > 0 && ts->ops->get_threshold) {
+		ret = enable_sensor_thresholds(ts, count);
+		if (ret)
+			goto exit_temp;
+	}
+
+	/* Add this sensor to the global list of sensors */
+	mutex_lock(&sensor_list_lock);
+	list_add_tail(&ts->node, &thermal_sensor_list);
+	mutex_unlock(&sensor_list_lock);
+
+	return ts;
+
+exit_temp:
+	device_remove_file(&ts->device, &dev_attr_temp_input);
+exit_name:
+	device_remove_file(&ts->device, &dev_attr_sensor_name);
+exit_unregister:
+	device_unregister(&ts->device);
+exit_idr:
+	release_idr(&thermal_sensor_idr, &thermal_idr_lock, ts->id);
+exit_free:
+	kfree(ts);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(thermal_sensor_register);
+
+void thermal_sensor_unregister(struct thermal_sensor *ts)
+{
+	int i;
+	struct thermal_sensor *pos, *next;
+	bool found = false;
+
+	if (!ts)
+		return;
+
+	mutex_lock(&sensor_list_lock);
+	list_for_each_entry_safe(pos, next, &thermal_sensor_list, node) {
+		if (pos == ts) {
+			list_del(&ts->node);
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&sensor_list_lock);
+
+	if (!found)
+		return;
+
+	for (i = 0; i < ts->thresholds; i++) {
+		device_remove_file(&ts->device, &ts->thresh_attrs[i].attr);
+		if (ts->ops->get_hyst) {
+			device_remove_file(&ts->device,
+					&ts->hyst_attrs[i].attr);
+		}
+	}
+
+	device_remove_file(&ts->device, &dev_attr_sensor_name);
+	device_remove_file(&ts->device, &dev_attr_temp_input);
+
+	release_idr(&thermal_sensor_idr, &thermal_idr_lock, ts->id);
+	idr_destroy(&ts->idr);
+
+	device_unregister(&ts->device);
+
+	kfree(ts);
+	return;
+}
+EXPORT_SYMBOL(thermal_sensor_unregister);
+
+/**
  * thermal_zone_device_register - register a new thermal zone device
  * @type:	the thermal zone device type
  * @trips:	the number of trip points the thermal zone support
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 9b78f8c..5470dae 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -55,6 +55,7 @@ 
 #define DEFAULT_THERMAL_GOVERNOR       "user_space"
 #endif
 
+struct thermal_sensor;
 struct thermal_zone_device;
 struct thermal_cooling_device;
 
@@ -135,6 +136,15 @@  struct thermal_cooling_device_ops {
 	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
 };
 
+struct thermal_sensor_ops {
+	int (*get_temp) (struct thermal_sensor *, long *);
+	int (*get_trend) (struct thermal_sensor *, int, enum thermal_trend *);
+	int (*set_threshold) (struct thermal_sensor *, int, long);
+	int (*get_threshold) (struct thermal_sensor *, int, long *);
+	int (*set_hyst) (struct thermal_sensor *, int, long);
+	int (*get_hyst) (struct thermal_sensor *, int, long *);
+};
+
 struct thermal_cooling_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -152,6 +162,21 @@  struct thermal_attr {
 	char name[THERMAL_NAME_LENGTH];
 };
 
+struct thermal_sensor {
+	char name[THERMAL_NAME_LENGTH];
+	int id;
+	int temp;
+	int prev_temp;
+	int thresholds;
+	void *devdata;
+	struct idr idr;
+	struct device device;
+	struct list_head node;
+	struct thermal_sensor_ops *ops;
+	struct thermal_attr *thresh_attrs;
+	struct thermal_attr *hyst_attrs;
+};
+
 struct thermal_zone_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -245,6 +270,10 @@  void notify_thermal_framework(struct thermal_zone_device *, int);
 int thermal_register_governor(struct thermal_governor *);
 void thermal_unregister_governor(struct thermal_governor *);
 
+struct thermal_sensor *thermal_sensor_register(const char *, int,
+				struct thermal_sensor_ops *, void *);
+void thermal_sensor_unregister(struct thermal_sensor *);
+
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
 						enum events event);