| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Oct 2011 17:45:11 +0100
From: Jonathan Cameron <jic23@....ac.uk>
To: Guenter Roeck <guenter.roeck@...csson.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linus.ml.walleij@...il.com" <linus.ml.walleij@...il.com>,
"zdevai@...il.com" <zdevai@...il.com>,
"linux@....linux.org.uk" <linux@....linux.org.uk>,
"arnd@...db.de" <arnd@...db.de>,
"broonie@...nsource.wolfsonmicro.com"
<broonie@...nsource.wolfsonmicro.com>,
"gregkh@...e.de" <gregkh@...e.de>,
"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
"khali@...ux-fr.org" <khali@...ux-fr.org>
Subject: Re: [PATCH 5/6] IIO:hwmon interface client driver.
On 10/19/11 17:38, Guenter Roeck wrote:
> On Wed, Oct 19, 2011 at 10:47:07AM -0400, Jonathan Cameron wrote:
>> Should move to drivers/hwmon once people are happy with it.
>>
>> Minimal support of simple in, curr and temp attributes
>> so far.
>>
>> Signed-off-by: Jonathan Cameron <jic23@....ac.uk>
>
> Couple of comments below.
Thanks. All excellent points. Will be fixed in V3.
Jonathan
>
>> ---
>> drivers/iio/Kconfig | 8 ++
>> drivers/iio/Makefile | 1 +
>> drivers/iio/iio_hwmon.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 223 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 308bc97..fb6d0a1 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -11,6 +11,14 @@ menuconfig IIO
>>
>> if IIO
>>
>> +config IIO_HWMON
>> + tristate "Hwmon driver that uses channels specified via iio maps"
>> + help
>> + This is a platform driver that in combination with a suitable
>> + map allows IIO devices to provide basic hwmon functionality
>> + for those channels specified in the map.
>> +
> Should probably also depend on HWMON
>
>> +
>> source "drivers/iio/adc/Kconfig"
>> source "drivers/iio/imu/Kconfig"
>> source "drivers/iio/light/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index cfb588a..5f9c01a 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -6,6 +6,7 @@ obj-y = inkern.o
>> obj-$(CONFIG_IIO) += iio.o
>> industrialio-y := core.o
>>
>> +obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o
>> obj-y += adc/
>> obj-y += imu/
>> obj-y += light/
>> diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c
>> new file mode 100644
>> index 0000000..6eeceeb
>> --- /dev/null
>> +++ b/drivers/iio/iio_hwmon.c
>> @@ -0,0 +1,214 @@
>> +/* Hwmon client for industrial I/O devices
>> + *
>> + * Copyright (c) 2011 Jonathan Cameron
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * Limited functionality currently supported.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/iio/inkern.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +/**
>> + * struct iio_hwmon_state - device instance state
>> + * @channels: filled with null terminated array of channels from iio
>> + * @num_channels: number of channels in channels (saves counting twice)
>> + * @hwmon_dev: associated hwmon device
>> + * @attr_group: the group of attributes
>> + * @attrs: null terminated array of attribute pointers.
>> + */
>> +struct iio_hwmon_state {
>> + struct iio_channel **channels;
>> + int num_channels;
>> + struct device *hwmon_dev;
>> + struct attribute_group attr_group;
>> + struct attribute **attrs;
>> +};
>> +
>> +/*
>> + * Assumes that IIO and hwmon operate in the same base units.
>> + * This is supposed to be true, but needs verification for
>> + * new channel types.
>> + */
>> +static ssize_t iio_hwmon_read_val(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + long result;
>> + int val, ret, scaleint, scalepart;
>> + struct sensor_device_attribute *sattr =
>> + to_sensor_dev_attr(attr);
>
> Does this need more than one line ?
>
>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +
>> + /*
>> + * No locking between this pair, so theoretically possible
>> + * the scale has changed.
>> + */
>> + ret = iio_read_channel_raw(state->channels[sattr->index],
>> + &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = iio_read_channel_scale(state->channels[sattr->index],
>> + &scaleint, &scalepart);
>> + if (ret < 0)
>> + return ret;
>> + switch (ret) {
>> + case IIO_VAL_INT:
>> + result = val * scaleint;
>> + break;
>> + case IIO_VAL_INT_PLUS_MICRO:
>> + result = val * (scaleint * 1000000 + scalepart)/1000000;
>> + break;
>> + case IIO_VAL_INT_PLUS_NANO:
>> + result = val * (scaleint * 1000000000 + scalepart)/1000000000;
>
> I think you might want to use long or even long long for all variables here,
> or at least typecast to long or long long. Concern is that for example
> "scaleint * 1000000000" may be calculated as int and would easily overflow;
> even as long it would easily overflow.
>
> There should be blanks around '/' (why doesn't checkpatch complain about this anymore ?).
>
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return sprintf(buf, "%ld\n", result);
>> +}
>> +
>> +static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
>> +{
>> + int i;
>> + struct sensor_device_attribute *a;
>> + for (i = 0; i < st->num_channels; i++)
>> + if (st->attrs[i]) {
>> + a = to_sensor_dev_attr(
>> + container_of(st->attrs[i],
>> + struct device_attribute,
>> + attr));
>> + kfree(a);
>> + }
>> +}
>> +
>> +static int __devinit iio_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct iio_hwmon_state *st;
>> + struct sensor_device_attribute *a;
>> + int ret, i = 0;
>> + int in_i = 1, temp_i = 1, curr_i = 1;
>> +
>> + st = kzalloc(sizeof(*st), GFP_KERNEL);
>> + if (st == NULL) {
>> + ret = -ENOMEM;
>> + goto error_ret;
>> + }
>> +
>> + st->channels = iio_channel_get_all(&pdev->dev, NULL);
>> + if (IS_ERR(st->channels)) {
>> + ret = PTR_ERR(st->channels);
>> + goto error_free_state;
>> + }
>> +
>> + /* count how many attributes we have */
>> + while (st->channels[st->num_channels])
>> + st->num_channels++;
>> +
>> + st->attrs = kzalloc(sizeof(st->attrs)*(i+1), GFP_KERNEL);
>
> Blanks around '*' and '+'.
> i == 0 here, so what is the point, and what are you trying to do ?
> Should it be st->num_channels instead of i + 1 ?
>
>> + if (st->attrs == NULL) {
>> + ret = -ENOMEM;
>> + goto error_release_channels;
>> + }
>> + for (i = 0; i < st->num_channels; i++) {
>> + a = kzalloc(sizeof(*a), GFP_KERNEL);
>
> What if this fails ?
>
>> + sysfs_attr_init(&a->dev_attr.attr);
>> + switch (iio_get_channel_type(st->channels[i])) {
>> + case IIO_VOLTAGE:
>> + a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> + "in%d_input",
>> + in_i++);
>> + break;
>> + case IIO_TEMP:
>> + a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> + "temp%d_input",
>> + temp_i++);
>> + break;
>> + case IIO_CURRENT:
>> + a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> + "curr%d_input",
>> + curr_i++);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + goto error_free_attrs;
>> + }
>> + if (a->dev_attr.attr.name == NULL) {
>> + ret = -ENOMEM;
>> + goto error_free_attrs;
>> + }
> I think this and the above EINVAL will result in leaking "a" since you did not store it
> in st->attrs[i] yet.
>
>> + a->dev_attr.show = iio_hwmon_read_val;
>> + a->dev_attr.attr.mode = S_IRUGO;
>> + a->index = i;
>> + st->attrs[i] = &a->dev_attr.attr;
>> + }
>> +
>> + st->attr_group.attrs = st->attrs;
>> + st->hwmon_dev = hwmon_device_register(&pdev->dev);
>> +
> Would be better to do this after successfully creating the sysfs attributes.
>
>> + ret = sysfs_create_group(&pdev->dev.kobj, &st->attr_group);
>> + if (ret < 0)
>> + goto error_free_attrs;
>> +
>> + platform_set_drvdata(pdev, st);
>> +
> You need to do this prior to creating the sysfs entries, since the read functions
> expect it to be there.
>
>> + return 0;
>> +
>> +error_free_attrs:
>
> This doesn't unregister the hwmon device.
>
>> + iio_hwmon_free_attrs(st);
>> + kfree(st->attrs);
>> +error_release_channels:
>> + iio_channel_release_all(st->channels);
>> +error_free_state:
>> + kfree(st);
>> +error_ret:
>> + return ret;
>> +}
>> +
>> +static int __devexit iio_hwmon_remove(struct platform_device *pdev)
>> +{
>> + struct iio_hwmon_state *st = platform_get_drvdata(pdev);
>> +
>> + sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
>> + iio_hwmon_free_attrs(st);
>> + kfree(st->attrs);
>> + hwmon_device_unregister(st->hwmon_dev);
>> + iio_channel_release_all(st->channels);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver __refdata iio_hwmon_driver = {
>> + .driver = {
>> + .name = "iio_hwmon",
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = iio_hwmon_probe,
>> + .remove = __devexit_p(iio_hwmon_remove),
>> +};
>> +
>> +static int iio_inkern_init(void)
>> +{
>> + return platform_driver_register(&iio_hwmon_driver);
>> +}
>> +module_init(iio_inkern_init);
>> +
>> +static void iio_inkern_exit(void)
>> +{
>> + platform_driver_unregister(&iio_hwmon_driver);
>> +}
>> +module_exit(iio_inkern_exit);
>> +
>> +MODULE_AUTHOR("Jonathan Cameron <jic23@....ac.uk>");
>> +MODULE_DESCRIPTION("IIO to hwmon driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.7
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists