| Message ID | 1360061183-14137-2-git-send-email-durgadoss.r@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
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
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
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
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
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 --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);
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(+)