From: Liam Beguin <[email protected]>
iio_convert_raw_to_processed_unlocked() assumes the offset is an
integer. Make a best effort to get a valid offset value for fractional
cases without breaking implicit truncations.
Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b69027690ed5..e1712c1099c5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -578,13 +578,39 @@ 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;
+ int tmp;
- 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:
+ fallthrough;
+ case IIO_VAL_INT_PLUS_NANO:
+ /*
+ * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
+ * implicitely truncate the offset to it's integer form.
+ */
+ break;
+ case IIO_VAL_FRACTIONAL:
+ tmp = offset_val / offset_val2;
+ offset_val = tmp;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ tmp = offset_val / (1 << offset_val2);
+ offset_val = tmp;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ raw64 += offset_val;
+ }
scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
IIO_CHAN_INFO_SCALE);
--
2.30.1.489.g328c10930387
On Wed, 30 Jun 2021 21:00:27 -0400
Liam Beguin <[email protected]> wrote:
> From: Liam Beguin <[email protected]>
>
> iio_convert_raw_to_processed_unlocked() assumes the offset is an
> integer. Make a best effort to get a valid offset value for fractional
> cases without breaking implicit truncations.
>
> Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
> Signed-off-by: Liam Beguin <[email protected]>
Looks good, but a few really minor comments / questions inline.
Thanks,
Jonathan
> ---
> drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b69027690ed5..e1712c1099c5 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -578,13 +578,39 @@ 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;
> + int tmp;
>
> - 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:
> + fallthrough;
I'm fairly sure you don't need to mark fallthroughs in the case where
there is nothing in the case statement at all. That case is assumed
to be deliberate by the various static checkers. I am seeing a few
examples as you have it here in kernel, but it certainly isn't particularly common
so I'm assuming those where the result of people falsely thinking it was necessary
or the outcomes of code changes in the surrounding code.
> + case IIO_VAL_INT_PLUS_NANO:
> + /*
> + * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
> + * implicitely truncate the offset to it's integer form.
> + */
> + break;
> + case IIO_VAL_FRACTIONAL:
> + tmp = offset_val / offset_val2;
> + offset_val = tmp;
What benefit do we get from the local variable?
offset_val /= offset_val2; would be alternative.
> + break;
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = offset_val / (1 << offset_val2);
> + offset_val = tmp;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + raw64 += offset_val;
> + }
>
> scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> IIO_CHAN_INFO_SCALE);
On Sun Jul 4, 2021 at 12:26 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:27 -0400
> Liam Beguin <[email protected]> wrote:
>
> > From: Liam Beguin <[email protected]>
> >
> > iio_convert_raw_to_processed_unlocked() assumes the offset is an
> > integer. Make a best effort to get a valid offset value for fractional
> > cases without breaking implicit truncations.
> >
> > Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
> > Signed-off-by: Liam Beguin <[email protected]>
Hi Jonathan,
Thanks for taking the time to review this again.
> Looks good, but a few really minor comments / questions inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++-----
> > 1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index b69027690ed5..e1712c1099c5 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -578,13 +578,39 @@ 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;
> > + int tmp;
> >
> > - 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:
> > + fallthrough;
>
> I'm fairly sure you don't need to mark fallthroughs in the case where
> there is nothing in the case statement at all. That case is assumed
> to be deliberate by the various static checkers. I am seeing a few
> examples as you have it here in kernel, but it certainly isn't
> particularly common
> so I'm assuming those where the result of people falsely thinking it was
> necessary
> or the outcomes of code changes in the surrounding code.
>
I thought it was always required with `-Wimplicit-fallthrough`.
Building without it gives no warnings, and after looking into it a
little, I found a bugzilla thread[1] that confirms what you're saying.
Thanks for pointing that out.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7652
> > + case IIO_VAL_INT_PLUS_NANO:
> > + /*
> > + * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
> > + * implicitely truncate the offset to it's integer form.
> > + */
> > + break;
> > + case IIO_VAL_FRACTIONAL:
> > + tmp = offset_val / offset_val2;
> > + offset_val = tmp;
>
> What benefit do we get from the local variable?
> offset_val /= offset_val2; would be alternative.
>
Apologies for that, will fix!
Thanks,
Liam
> > + break;
> > + case IIO_VAL_FRACTIONAL_LOG2:
> > + tmp = offset_val / (1 << offset_val2);
> > + offset_val = tmp;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + raw64 += offset_val;
> > + }
> >
> > scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> > IIO_CHAN_INFO_SCALE);