diff mbox series

iio: addac: ad74413r: fix integer promotion bug in ad74413_get_input_current_offset()

Message ID 20221118123209.1658420-1-linux@rasmusvillemoes.dk
State New
Headers
Series iio: addac: ad74413r: fix integer promotion bug in ad74413_get_input_current_offset() |

Commit Message

Rasmus Villemoes Nov. 18, 2022, 12:32 p.m. UTC 
  The constant AD74413R_ADC_RESULT_MAX is defined via GENMASK, so its
type is "unsigned long".

Hence in the expression voltage_offset * AD74413R_ADC_RESULT_MAX,
voltage_offset is first promoted to unsigned long, and since it may be
negative, that results in a garbage value. For example, when range is
AD74413R_ADC_RANGE_5V_BI_DIR, voltage_offset is -2500 and
voltage_range is 5000, so the RHS of this assignment is, depending on
sizeof(long), either 826225UL or 3689348814709142UL, which after
truncation to int then results in either 826225 or 1972216214 being
the output from in_currentX_offset.

Casting to int avoids that promotion and results in the correct -32767
output.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sa, Nuno Nov. 18, 2022, 1:17 p.m. UTC | #1 
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Friday, November 18, 2022 1:32 PM
> To: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Jonathan Cameron <jic23@kernel.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] iio: addac: ad74413r: fix integer promotion bug in
> ad74413_get_input_current_offset()
> 
> [External]
> 
> The constant AD74413R_ADC_RESULT_MAX is defined via GENMASK, so its
> type is "unsigned long".
> 
> Hence in the expression voltage_offset * AD74413R_ADC_RESULT_MAX,
> voltage_offset is first promoted to unsigned long, and since it may be
> negative, that results in a garbage value. For example, when range is
> AD74413R_ADC_RANGE_5V_BI_DIR, voltage_offset is -2500 and
> voltage_range is 5000, so the RHS of this assignment is, depending on
> sizeof(long), either 826225UL or 3689348814709142UL, which after
> truncation to int then results in either 826225 or 1972216214 being
> the output from in_currentX_offset.
> 
> Casting to int avoids that promotion and results in the correct -32767
> output.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---

After adding proper Fixes: tag,

Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Rasmus Villemoes Nov. 18, 2022, 1:29 p.m. UTC | #2 
On 18/11/2022 14.17, Sa, Nuno wrote:

>> Casting to int avoids that promotion and results in the correct -32767
>> output.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
> 
> After adding proper Fixes: tag,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>

That would be

Fixes: fea251b6a5db (iio: addac: add AD74413R driver)

Thanks,
Rasmus
Jonathan Cameron Nov. 23, 2022, 8:45 p.m. UTC | #3 
On Fri, 18 Nov 2022 14:29:23 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 18/11/2022 14.17, Sa, Nuno wrote:
> 
> >> Casting to int avoids that promotion and results in the correct -32767
> >> output.
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---  
> > 
> > After adding proper Fixes: tag,
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>  
> 
> That would be
> 
> Fixes: fea251b6a5db (iio: addac: add AD74413R driver)
> 
Applied to the togreg branch of iio.git (as very late in cycle)
and marked for stable.

Thanks,

Jonathan

> Thanks,
> Rasmus
>
diff mbox series

Patch

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 899bcd83f40b..e0e130ba9d3e 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -691,7 +691,7 @@  static int ad74413_get_input_current_offset(struct ad74413r_state *st,
 	if (ret)
 		return ret;
 
-	*val = voltage_offset * AD74413R_ADC_RESULT_MAX / voltage_range;
+	*val = voltage_offset * (int)AD74413R_ADC_RESULT_MAX / voltage_range;
 
 	return IIO_VAL_INT;
 }