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 [off-list ref] wrote:

quoted

From: Liam Beguin <redacted>

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 <redacted>
---
 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?

quoted

+				return -EINVAL;
+			break;
+		case IIO_VAL_INT_PLUS_NANO:
+			if (offset_val2 > 1000000)

Similar this is a bit odd.

quoted

+				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.

quoted

+			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?

quoted

+			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