From: "R, Durgadoss" <durgadoss.r@intel.com>
To: Eduardo Valentin <eduardo.valentin@ti.com>
Cc: "Zhang, Rui" <rui.zhang@intel.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"hongbo.zhang@linaro.org" <hongbo.zhang@linaro.org>,
"wni@nvidia.com" <wni@nvidia.com>
Subject: RE: [PATCH 1/8] Thermal: Create sensor level APIs
Date: Fri, 1 Mar 2013 05:08:20 +0000 [thread overview]
Message-ID: <4D68720C2E767A4AA6A8796D42C8EB59292A12@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <512FA8DB.6000704@ti.com>
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
next prev parent reply other threads:[~2013-03-01 5:08 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-05 10:46 [PATCHv3 0/8] Thermal Framework Enhancements Durgadoss R
2013-02-05 10:46 ` [PATCH 1/8] Thermal: Create sensor level APIs Durgadoss R
2013-02-08 7:53 ` Zhang Rui
2013-02-08 8:26 ` R, Durgadoss
2013-02-08 9:54 ` Zhang Rui
2013-02-28 18:58 ` Eduardo Valentin
2013-03-01 5:08 ` R, Durgadoss [this message]
2013-02-05 10:46 ` [PATCH 2/8] Thermal: Create zone " Durgadoss R
2013-02-08 8:11 ` Zhang Rui
2013-02-08 8:54 ` R, Durgadoss
2013-02-08 9:54 ` Zhang Rui
2013-02-08 10:27 ` R, Durgadoss
2013-02-28 19:29 ` Eduardo Valentin
2013-03-01 15:31 ` R, Durgadoss
2013-02-05 10:46 ` [PATCH 3/8] Thermal: Add APIs to bind cdev to new zone structure Durgadoss R
2013-02-08 8:28 ` Zhang Rui
2013-02-28 19:35 ` Eduardo Valentin
2013-03-01 15:33 ` R, Durgadoss
2013-02-05 10:46 ` [PATCH 4/8] Thermal: Add trip point sysfs nodes for sensor Durgadoss R
2013-02-08 8:50 ` Zhang Rui
2013-02-28 19:51 ` Eduardo Valentin
2013-03-01 15:34 ` R, Durgadoss
2013-02-05 10:46 ` [PATCH 5/8] Thermal: Create Thermal map sysfs attributes for a zone Durgadoss R
2013-02-08 9:04 ` Zhang Rui
2013-02-08 9:08 ` R, Durgadoss
2013-02-08 9:55 ` Zhang Rui
2013-02-28 21:30 ` Eduardo Valentin
2013-02-05 10:46 ` [PATCH 6/8] Thermal: Add Documentation to new APIs Durgadoss R
2013-02-08 9:21 ` Zhang Rui
2013-02-05 10:46 ` [PATCH 7/8] Thermal: Add ABI Documentation for sysfs interfaces Durgadoss R
2013-02-05 10:46 ` [PATCH 8/8] Thermal: Dummy driver used for testing Durgadoss R
2013-02-08 7:53 ` [PATCHv3 0/8] Thermal Framework Enhancements Zhang Rui
2013-02-08 9:35 ` Zhang Rui
2013-02-28 21:33 ` Eduardo Valentin
2013-03-01 5:12 ` R, Durgadoss
2013-08-29 19:38 ` Eduardo Valentin
2013-08-30 9:20 ` R, Durgadoss
2013-08-30 12:34 ` Eduardo Valentin
2013-08-30 13:21 ` R, Durgadoss
-- strict thread matches above, loose matches on Subject: below --
2012-12-18 9:29 [PATCH " Durgadoss R
2012-12-18 9:29 ` [PATCH 1/8] Thermal: Create sensor level APIs Durgadoss R
2012-12-18 11:13 ` Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D68720C2E767A4AA6A8796D42C8EB59292A12@BGSMSX101.gar.corp.intel.com \
--to=durgadoss.r@intel.com \
--cc=eduardo.valentin@ti.com \
--cc=hongbo.zhang@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rui.zhang@intel.com \
--cc=wni@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).