Re: [PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Peter Rosin]

Hi!

On 2021-06-10 11:06, Jonathan Cameron wrote:
> On Wed, 09 Jun 2021 17:40:47 -0400
> "Liam Beguin" <liambeguin@xxxxxxxxx> wrote:
> 
>> Hi Jonathan,
>>
>> On Wed Jun 9, 2021 at 4:28 PM EDT, Jonathan Cameron wrote:
>>> On Mon, 7 Jun 2021 10:47:13 -0400
>>> Liam Beguin <liambeguin@xxxxxxxxx> wrote:
>>>  
>>>> From: Liam Beguin <lvb@xxxxxxxxxx>
>>>>
>>>> iio_convert_raw_to_processed_unlocked() assumes the offset is an
>>>> integer.
>>>> Make that clear to the consumer by returning an error on unsupported
>>>> offset types without breaking valid implicit truncations.
>>>>
>>>> Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
>>>> ---
>>>>  drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
>>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>> index b69027690ed5..0b5667f22b1d 100644
>>>> --- a/drivers/iio/inkern.c
>>>> +++ b/drivers/iio/inkern.c
>>>> @@ -578,13 +578,37 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
>>>>  static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
>>>>  	int raw, int *processed, unsigned int scale)
>>>>  {
>>>> -	int scale_type, scale_val, scale_val2, offset;
>>>> +	int scale_type, scale_val, scale_val2;
>>>> +	int offset_type, offset_val, offset_val2;
>>>>  	s64 raw64 = raw;
>>>> -	int ret;
>>>>  
>>>> -	ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET);
>>>> -	if (ret >= 0)
>>>> -		raw64 += offset;
>>>> +	offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
>>>> +				       IIO_CHAN_INFO_OFFSET);
>>>> +	if (offset_type >= 0) {
>>>> +		switch (offset_type) {
>>>> +		case IIO_VAL_INT:
>>>> +			break;
>>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>>> +			if (offset_val2 > 1000)  
>>>
>>> What's the logic behind this one? > 1000000
>>> would be an interesting corner case, though I'm not sure we've ever
>>> explicitly disallowed it before.
>>>
>>> Why are we at 1000th of that for the check?
>>>  
>>
>> For these the idea was to go with one milli of precision.
>> I don't know if that's a good criteria but I wanted to start with
>> something. Do you have any suggestions?
>>
>>>> +				return -EINVAL;
>>>> +			break;
>>>> +		case IIO_VAL_INT_PLUS_NANO:
>>>> +			if (offset_val2 > 1000000)  
>>>
>>> Similar this is a bit odd.
>>>  
>>>> +				return -EINVAL;
>>>> +		case IIO_VAL_FRACTIONAL:
>>>> +			if (offset_val2 != 1)
>>>> +				return -EINVAL;  
>>>
>>> We could be more flexible on this, but I don't recall any
>>> channels using this so far.
>>>  
>>>> +			break;
>>>> +		case IIO_VAL_FRACTIONAL_LOG2:
>>>> +			if (offset_val2)
>>>> +				return -EINVAL;  
>>>
>>> Same in this case.
>>>  
>>
>> For these two cases, I went with what Peter suggested in the previous
>> version, to not break on valid implicit truncations.
>>
>> What would be a good precision criteria for all offset types?
> 
> @Peter, what were your thoughts on this.
> 
> I was thinking we'd just not worry about how much truncation was happening
> and just silently eat it.

For the "integer-plus" formats, that was my thinking too. Previously that
was exactly what was happeneing, and v1 randomly broke any user that relied
on 42.424242 being truncated to 42. This is still the case with this v2, as
v2 is allowing only a very slim set of truncations. I meant that this new
code needs to allow all truncations, just as before. Anything short of that
must have a much better description of why it is safe to all of a sudden
disallow truncation. I.e. such a change needs to come with traces of an
audit of how this function is used, and why changing the semantics will not
cause regressions.

For IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2, it seems correct to
error out if the denominator isn't 1, because relying on using an offset of
e.g. 187 for a IIO_VAL_FRACTIONAL of 187/169 is not at all healthy.

Both erroring out and doing a best effort calculation for these fractional
cases with denominator != 1 would be ok by me, because they are plain and
simple severly broken at the moment.

Cheers,
Peter

> J
>>
>>>> +			break;
>>>> +		default:
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		raw64 += offset_val;
>>>> +	}
>>>>  
>>>>  	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
>>>>  					IIO_CHAN_INFO_SCALE);  
>>
>> Thanks for looking at this,
>> Liam
>