| 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: <MBZEFR.J5QW1P07Y388@crapouillou.net>
Date: Fri, 22 Jul 2022 09:52:34 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Peter Rosin <peda@...ntia.se>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>, list@...ndingux.net,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: afe/rescale: Add support for converting scale
avail table
Hi Peter,
Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@...ntia.se>
a écrit :
> Hi!
>
> 2022-07-21 at 21:15, Paul Cercueil wrote:
>> When the IIO channel has a scale_available attribute, we want the
>> values
>> contained to be properly converted the same way the scale value is.
>>
>> Since rescale_process_scale() may change the encoding type, we must
>> convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.
>>
>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> ---
>> drivers/iio/afe/iio-rescale.c | 85
>> +++++++++++++++++++++++++++++++++
>> include/linux/iio/afe/rescale.h | 2 +
>> 2 files changed, 87 insertions(+)
>>
>> diff --git a/drivers/iio/afe/iio-rescale.c
>> b/drivers/iio/afe/iio-rescale.c
>> index 6949d2151025..5c9970b93384 100644
>> --- a/drivers/iio/afe/iio-rescale.c
>> +++ b/drivers/iio/afe/iio-rescale.c
>> @@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev
>> *indio_dev,
>> *type = IIO_VAL_INT;
>> return iio_read_avail_channel_raw(rescale->source,
>> vals, length);
>> + case IIO_CHAN_INFO_SCALE:
>> + if (rescale->chan_processed) {
>
> I think it is wrong to simply feed the info-scale to the source
> channel if it
> happens to be processed. It still needs the inverse rescale. But see
> below.
Yes, when I started working on that patchset, processed channels
weren't a thing, and I don't think I understood what they are about.
>
>> + return iio_read_avail_channel_attribute(rescale->source,
>> + vals, type,
>> + length,
>> + IIO_CHAN_INFO_SCALE);
>> + } else if (rescale->scale_len) {
>> + *length = rescale->scale_len;
>> + *vals = rescale->scale_data;
>> + return IIO_AVAIL_LIST_WITH_TYPE;
>> + }
>> + fallthrough;
>> default:
>> return -EINVAL;
>> }
>> @@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct
>> iio_dev *indio_dev,
>> buf, len);
>> }
>>
>> +static int rescale_init_scale_avail(struct device *dev, struct
>> rescale *rescale)
>> +{
>> + int ret, type, length, *data;
>> + const int *scale_raw;
>> + unsigned int i;
>> + size_t out_len;
>> +
>> + ret = iio_read_avail_channel_attribute(rescale->source,
>> &scale_raw,
>> + &type, &length,
>> + IIO_CHAN_INFO_SCALE);
>> + if (ret < 0)
>> + return ret;
>> +
>> + switch (ret) {
>> + case IIO_AVAIL_LIST_WITH_TYPE:
>> + out_len = length;
>> + break;
>> + case IIO_AVAIL_LIST:
>> + if (type == IIO_VAL_INT)
>> + out_len = length * 3 / 1;
>> + else
>> + out_len = length * 3 / 2;
>> + break;
>> + default:
>> + /* TODO: Support IIO_AVAIL_RANGE */
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
>> + memcpy(data, scale_raw, sizeof(*scale_raw) * length);
>> + } else if (type == IIO_VAL_INT) {
>> + for (i = 0; i < length; i++) {
>> + data[i * 3 + 0] = scale_raw[i];
>> + data[i * 3 + 2] = IIO_VAL_INT;
>> + }
>> + } else {
>> + for (i = 0; i < length / 2; i++) {
>> + data[i * 3 + 0] = scale_raw[i * 2];
>> + data[i * 3 + 1] = scale_raw[i * 2 + 1];
>> + data[i * 3 + 2] = type;
>> + }
>> + }
>> +
>> + for (i = 0; i < out_len; i += 3) {
>> + ret = rescale_process_scale(rescale, data[i + 2],
>> + &data[i], &data[i + 1]);
>> + if (ret < 0)
>> + return ret;
>> +
>> + data[i + 2] = ret;
>> + }
>> +
>> + rescale->scale_len = out_len;
>> + rescale->scale_data = data;
>> +
>> + return 0;
>> +}
>> +
>> static int rescale_configure_channel(struct device *dev,
>> struct rescale *rescale)
>> {
>> struct iio_chan_spec *chan = &rescale->chan;
>> struct iio_chan_spec const *schan = rescale->source->channel;
>> + int ret;
>>
>> chan->indexed = 1;
>> chan->output = schan->output;
>> @@ -303,6 +378,16 @@ static int rescale_configure_channel(struct
>> device *dev,
>> !rescale->chan_processed)
>> chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
>>
>> + if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
>> + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
>> +
>> + if (!rescale->chan_processed) {
>> + ret = rescale_init_scale_avail(dev, rescale);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>
> Does a (sane) processed channel really have a scale? That seems a bit
> fringe.
> Wouldn't it be better to conditionally publish availability of
> info-scale so
> that it isn't visible for processed channels and dodge that
> rabbit-hole? In
> either case, the above commented implementation of info-scale for
> rescaled
> processed channels is wrong (I think...).
I could set the IIO_CHAN_INFO_SCALE only for non-processed channels,
since this is what I can test with.
Cheers,
-Paul
>> return 0;
>> }
>>
>> diff --git a/include/linux/iio/afe/rescale.h
>> b/include/linux/iio/afe/rescale.h
>> index 6eecb435488f..74de2962f864 100644
>> --- a/include/linux/iio/afe/rescale.h
>> +++ b/include/linux/iio/afe/rescale.h
>> @@ -26,6 +26,8 @@ struct rescale {
>> s32 numerator;
>> s32 denominator;
>> s32 offset;
>> + int scale_len;
>> + int *scale_data;
>> };
>>
>> int rescale_process_scale(struct rescale *rescale, int scale_type,
Powered by blists - more mailing lists