| 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
| ||
|
Message-ID: <534B3F1A.60003@linux.intel.com>
Date: Sun, 13 Apr 2014 18:51:22 -0700
From: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Patch v3 2/6] IIO: core: Introduce read_raw_multi
On 04/12/2014 09:52 AM, Jonathan Cameron wrote:
> On 09/04/14 01:56, Srinivas Pandruvada wrote:
>> This callback is introduced to overcome some limitations of existing
>> read_raw callback. The functionality of both existing read_raw and
>> read_raw_multi is similar, both are used to request values from the
>> device. The current read_raw callback allows only two return values.
>> The new read_raw_multi allows returning multiple values. Instead of
>> passing just address of val and val2, it passes length and pointer
>> to values. Depending on the type and length of passed buffer, iio
>> client drivers can return multiple values.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Hi Srinivas.
>
> This has come together pretty much how I thought it would. Very nice.
> Only comment inline is that I'd prefer we took care now with possiblity
> of really long sets of values so that we don't get bitten by it sometime
> in the future.
>
I was thinking of using snprintf, but buf had no length passed. If we
assume PAGE_SIZE as max length
then I can do what you suggested below,
Thanks,
Srinivas
> If you want to drop the reference to 0 having special meaning in the
> comment as well, thats fine by me.
>
> Jonathan
>> ---
>> drivers/iio/iio_core.h | 2 +-
>> drivers/iio/industrialio-core.c | 65
>> ++++++++++++++++++++++++++--------------
>> drivers/iio/industrialio-event.c | 6 ++--
>> drivers/iio/inkern.c | 16 ++++++++--
>> include/linux/iio/iio.h | 17 +++++++++++
>> include/linux/iio/types.h | 1 +
>> 6 files changed, 80 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
>> index f6db6af..30327ad 100644
>> --- a/drivers/iio/iio_core.h
>> +++ b/drivers/iio/iio_core.h
>> @@ -35,7 +35,7 @@ int __iio_add_chan_devattr(const char *postfix,
>> struct list_head *attr_list);
>> void iio_free_chan_devattr_list(struct list_head *attr_list);
>>
>> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int
>> val2);
>> +ssize_t iio_format_value(char *buf, unsigned int type, int size, int
>> *val);
>>
>> /* Event interface flags */
>> #define IIO_BUSY_BIT_POS 1
>> diff --git a/drivers/iio/industrialio-core.c
>> b/drivers/iio/industrialio-core.c
>> index ede16aec..3bd565c 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -373,41 +373,53 @@ EXPORT_SYMBOL_GPL(iio_enum_write);
>> * @buf: The buffer to which the formated value gets written
>> * @type: One of the IIO_VAL_... constants. This decides how the
>> val and val2
>> * parameters are formatted.
>> - * @val: First part of the value, exact meaning depends on the type
>> parameter.
>> - * @val2: Second part of the value, exact meaning depends on the
>> type parameter.
>> + * @vals: pointer to the values, exact meaning depends on the type
>> parameter.
>> */
>> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int
>> val2)
>> +ssize_t iio_format_value(char *buf, unsigned int type, int size, int
>> *vals)
>> {
>> unsigned long long tmp;
>> bool scale_db = false;
>>
>> switch (type) {
>> case IIO_VAL_INT:
>> - return sprintf(buf, "%d\n", val);
>> + return sprintf(buf, "%d\n", vals[0]);
>> case IIO_VAL_INT_PLUS_MICRO_DB:
>> scale_db = true;
>> case IIO_VAL_INT_PLUS_MICRO:
>> - if (val2 < 0)
>> - return sprintf(buf, "-%ld.%06u%s\n", abs(val), -val2,
>> + if (vals[1] < 0)
>> + return sprintf(buf, "-%ld.%06u%s\n", abs(vals[0]),
>> + -vals[1],
>> scale_db ? " dB" : "");
>> else
>> - return sprintf(buf, "%d.%06u%s\n", val, val2,
>> + return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1],
>> scale_db ? " dB" : "");
>> case IIO_VAL_INT_PLUS_NANO:
>> - if (val2 < 0)
>> - return sprintf(buf, "-%ld.%09u\n", abs(val), -val2);
>> + if (vals[1] < 0)
>> + return sprintf(buf, "-%ld.%09u\n", abs(vals[0]),
>> + -vals[1]);
>> else
>> - return sprintf(buf, "%d.%09u\n", val, val2);
>> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>> case IIO_VAL_FRACTIONAL:
>> - tmp = div_s64((s64)val * 1000000000LL, val2);
>> - val2 = do_div(tmp, 1000000000LL);
>> - val = tmp;
>> - return sprintf(buf, "%d.%09u\n", val, val2);
>> + tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
>> + vals[1] = do_div(tmp, 1000000000LL);
>> + vals[0] = tmp;
>> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>> case IIO_VAL_FRACTIONAL_LOG2:
>> - tmp = (s64)val * 1000000000LL >> val2;
>> - val2 = do_div(tmp, 1000000000LL);
>> - val = tmp;
>> - return sprintf(buf, "%d.%09u\n", val, val2);
>> + tmp = (s64)vals[0] * 1000000000LL >> vals[1];
>> + vals[1] = do_div(tmp, 1000000000LL);
>> + vals[0] = tmp;
>> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>> + case IIO_VAL_INT_MULTIPLE:
>> + {
>> + int i;
>> + int len = 0;
>> +
>> + for (i = 0; i < size; ++i)
>> + len += sprintf(&buf[len], "%d ", vals[i]);
>> + buf[len++] = '\n';
>> + buf[len++] = '\0';
> Whilst we know this is of a fixed maxium length, I think we
> want to take more care to ensure that we don't overrun the maximum
> sysfs length. I'd also prefer specific use of snprintf for the new
> line as well to make sure that doesn't cause trouble. i.e.
>
> for (i = 0; i < size; ++i)
> len += snprintf(&buf[len], PAGE_SIZE - len, "%d ", vals[i]);
> len += snprintf(&buf[len], PAGE_SIZE - len, "/n");
>
> The reasoning being that we could easily mess something up and hit these
> limits sometime in the future. Also not using the explicit character
> settting, but doing it with snprintf is easier to read (slightly ;)
>
>
>> + return len;
>> + }
>> default:
>> return 0;
>> }
>> @@ -419,14 +431,23 @@ static ssize_t iio_read_channel_info(struct
>> device *dev,
>> {
>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> - int val, val2;
>> - int ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
>> - &val, &val2, this_attr->address);
>> + int vals[INDIO_MAX_RAW_ELEMENTS];
>> + int ret;
>> + int val_len = 2;
>> +
>> + if (indio_dev->info->read_raw_multi)
>> + ret = indio_dev->info->read_raw_multi(indio_dev, this_attr->c,
>> + INDIO_MAX_RAW_ELEMENTS,
>> + vals, &val_len,
>> + this_attr->address);
>> + else
>> + ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
>> + &vals[0], &vals[1], this_attr->address);
>>
>> if (ret < 0)
>> return ret;
>>
>> - return iio_format_value(buf, ret, val, val2);
>> + return iio_format_value(buf, ret, val_len, vals);
>> }
>>
>> /**
>> diff --git a/drivers/iio/industrialio-event.c
>> b/drivers/iio/industrialio-event.c
>> index ea6e06b..1b4f31b 100644
>> --- a/drivers/iio/industrialio-event.c
>> +++ b/drivers/iio/industrialio-event.c
>> @@ -270,7 +270,7 @@ static ssize_t iio_ev_value_show(struct device *dev,
>> {
>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> - int val, val2;
>> + int val, val2, val_arr[2];
>> int ret;
>>
>> ret = indio_dev->info->read_event_value(indio_dev,
>> @@ -279,7 +279,9 @@ static ssize_t iio_ev_value_show(struct device *dev,
>> &val, &val2);
>> if (ret < 0)
>> return ret;
>> - return iio_format_value(buf, ret, val, val2);
>> + val_arr[0] = val;
>> + val_arr[1] = val2;
>> + return iio_format_value(buf, ret, 2, val_arr);
>> }
>>
>> static ssize_t iio_ev_value_store(struct device *dev,
>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>> index 0cf5f8e..75e5386 100644
>> --- a/drivers/iio/inkern.c
>> +++ b/drivers/iio/inkern.c
>> @@ -417,12 +417,24 @@ static int iio_channel_read(struct iio_channel
>> *chan, int *val, int *val2,
>> enum iio_chan_info_enum info)
>> {
>> int unused;
>> + int vals[INDIO_MAX_RAW_ELEMENTS];
>> + int ret;
>> + int val_len = 2;
>>
>> if (val2 == NULL)
>> val2 = &unused;
>>
>> - return chan->indio_dev->info->read_raw(chan->indio_dev,
>> chan->channel,
>> - val, val2, info);
>> + if (chan->indio_dev->info->read_raw_multi) {
>> + ret = chan->indio_dev->info->read_raw_multi(chan->indio_dev,
>> + chan->channel, INDIO_MAX_RAW_ELEMENTS,
>> + vals, &val_len, info);
>> + *val = vals[0];
>> + *val2 = vals[1];
>> + } else
>> + ret = chan->indio_dev->info->read_raw(chan->indio_dev,
>> + chan->channel, val, val2, info);
>> +
>> + return ret;
>> }
>>
>> int iio_read_channel_raw(struct iio_channel *chan, int *val)
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 5f2d00e..5629c92 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -288,6 +288,8 @@ static inline s64 iio_get_time_ns(void)
>> #define INDIO_ALL_BUFFER_MODES \
>> (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE)
>>
>> +#define INDIO_MAX_RAW_ELEMENTS 4
>> +
>> struct iio_trigger; /* forward declaration */
>> struct iio_dev;
>>
>> @@ -302,6 +304,14 @@ struct iio_dev;
>> * the channel in question. Return value will specify the
>> * type of value returned by the device. val and val2 will
>> * contain the elements making up the returned value.
>> + * @read_raw_multi: function to return values from the device.
>> + * mask specifies which value. Note 0 means a reading of
> This note 0 bit wants to go now it is much more explicit in the way
> the code
> works, but leave it here for now and we can tidy up both this and the
> read_raw
> callback at the same time.
>> + * the channel in question. Return value will specify the
>> + * type of value returned by the device. vals pointer
>> + * contain the elements making up the returned value.
>> + * max_len specifies maximum number of elements
>> + * vals pointer can contain. val_len is used to return
>> + * length of valid elements in vals.
>> * @write_raw: function to write a value to the device.
>> * Parameters are the same as for read_raw.
>> * @write_raw_get_fmt: callback function to query the expected
>> @@ -328,6 +338,13 @@ struct iio_info {
>> int *val2,
>> long mask);
>>
>> + int (*read_raw_multi)(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int max_len,
>> + int *vals,
>> + int *val_len,
>> + long mask);
>> +
>> int (*write_raw)(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int val,
>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>> index 084d882..a13c224 100644
>> --- a/include/linux/iio/types.h
>> +++ b/include/linux/iio/types.h
>> @@ -79,6 +79,7 @@ enum iio_event_direction {
>> #define IIO_VAL_INT_PLUS_MICRO 2
>> #define IIO_VAL_INT_PLUS_NANO 3
>> #define IIO_VAL_INT_PLUS_MICRO_DB 4
>> +#define IIO_VAL_INT_MULTIPLE 5
>> #define IIO_VAL_FRACTIONAL 10
>> #define IIO_VAL_FRACTIONAL_LOG2 11
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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