| Message ID | YcIpAKV7Cmi0o7PU@debian-BULLSEYE-live-builder-AMD64 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drivers:iio:dac make expression evaluation 64-bit | expand |
On Wed, Dec 22, 2021 at 5:59 PM Muhammad Usama Anjum <usama.anjum@collabora.com> wrote: > > Two 32-bit values are being evaluated using 32-bit arithmetic and then > passed to s64 type. It is wrong. Expression should be evaluated using > 64-bit arithmetic and then passed. ... > dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * 1000000, > 65536); Shouldn't the above be fixed as well? Has anybody tried to compile on 32-bit arch this? > - dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, span, &rem); > + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536L, span, &rem); > tmp = (s64)rem * 1000000; > dac->ch_data[ch].offset_dec = div_s64(tmp, span); > }
On 12/22/21 11:49 PM, Andy Shevchenko wrote: > On Wed, Dec 22, 2021 at 5:59 PM Muhammad Usama Anjum > <usama.anjum@collabora.com> wrote: >> >> Two 32-bit values are being evaluated using 32-bit arithmetic and then >> passed to s64 type. It is wrong. Expression should be evaluated using >> 64-bit arithmetic and then passed. > > ... > >> dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * 1000000, >> 65536); > > Shouldn't the above be fixed as well? Has anybody tried to compile on > 32-bit arch this? No, it correct already. In this case, rem is being typecasted to s64 and then multiplied with a 32-bit number, 1000000. Thus 64-bit arithmetic is being performed here.
On 12/23/21 5:34 PM, Muhammad Usama Anjum wrote: > On 12/22/21 11:49 PM, Andy Shevchenko wrote: >> On Wed, Dec 22, 2021 at 5:59 PM Muhammad Usama Anjum >> <usama.anjum@collabora.com> wrote: >>> Two 32-bit values are being evaluated using 32-bit arithmetic and then >>> passed to s64 type. It is wrong. Expression should be evaluated using >>> 64-bit arithmetic and then passed. >> ... >> >>> dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * 1000000, >>> 65536); >> Shouldn't the above be fixed as well? Has anybody tried to compile on >> 32-bit arch this? > No, it correct already. In this case, rem is being typecasted to s64 and > then multiplied with a 32-bit number, 1000000. Thus 64-bit arithmetic is > being performed here. What Andy means is that this needs to be DIV_S64_ROUND_CLOSEST() to work on 32-bit platforms. But it is clearly unrelated to your change and should be in its own patch.
> >>> Two 32-bit values are being evaluated using 32-bit arithmetic and > >>> then passed to s64 type. It is wrong. Expression should be evaluated > >>> using 64-bit arithmetic and then passed. > >> ... > >> > >>> dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * > 1000000, > >>> 65536); > >> Shouldn't the above be fixed as well? Has anybody tried to compile on > >> 32-bit arch this? > > No, it correct already. In this case, rem is being typecasted to s64 > > and then multiplied with a 32-bit number, 1000000. Thus 64-bit > > arithmetic is being performed here. > > What Andy means is that this needs to be DIV_S64_ROUND_CLOSEST() to > work on 32-bit platforms. But it is clearly unrelated to your change and should > be in its own patch. Indeed, I didn't test it on 32 bits. But both changes make sense to me. Regards, Mihail
On Wed, Dec 22, 2021 at 12:20:32AM +0500, Muhammad Usama Anjum wrote: > Two 32-bit values are being evaluated using 32-bit arithmetic and then > passed to s64 type. It is wrong. Expression should be evaluated using > 64-bit arithmetic and then passed. > > Fixes: 8f2b54824b ("drivers:iio:dac: Add AD3552R driver support") > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > drivers/iio/dac/ad3552r.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c > index 97f13c0b9631..b03d3c7cd4c4 100644 > --- a/drivers/iio/dac/ad3552r.c > +++ b/drivers/iio/dac/ad3552r.c > @@ -770,7 +770,7 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch) > dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * 1000000, > 65536); > > - dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, span, &rem); > + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536L, span, &rem); "v_min" is relatively close to zero on a number line so this can't overflow. There is no way that this change affects anything at runtime (except making the code a tiny tiny bit slower). And it should be 65536LL for 32 bit systems? But I just don't see the point of this change. Presumably it is to make a static analyzer happy? regards, dan carpenter
> From: Dan Carpenter <dan.carpenter@oracle.com> > Sent: Wednesday, January 5, 2022 2:39 PM > To: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Jonathan Cameron > <jic23@kernel.org>; Chindris, Mihail <Mihail.Chindris@analog.com>; > open list:IIO SUBSYSTEM AND DRIVERS <linux-iio@vger.kernel.org>; > open list <linux-kernel@vger.kernel.org>; kernel@collabora.com; > kernel-janitors@vger.kernel.org > Subject: Re: [PATCH] drivers:iio:dac make expression evaluation 64-bit > > [External] > > On Wed, Dec 22, 2021 at 12:20:32AM +0500, Muhammad Usama Anjum > wrote: > > Two 32-bit values are being evaluated using 32-bit arithmetic and > then > > passed to s64 type. It is wrong. Expression should be evaluated using > > 64-bit arithmetic and then passed. > > > > Fixes: 8f2b54824b ("drivers:iio:dac: Add AD3552R driver support") > > Signed-off-by: Muhammad Usama Anjum > <usama.anjum@collabora.com> > > --- > > drivers/iio/dac/ad3552r.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c > > index 97f13c0b9631..b03d3c7cd4c4 100644 > > --- a/drivers/iio/dac/ad3552r.c > > +++ b/drivers/iio/dac/ad3552r.c > > @@ -770,7 +770,7 @@ static void > ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch) > > dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem > * 1000000, > > 65536); > > > > - dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, > span, &rem); > > + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536L, > span, &rem); > > "v_min" is relatively close to zero on a number line so this can't > overflow. There is no way that this change affects anything at runtime > (except making the code a tiny tiny bit slower). > > And it should be 65536LL for 32 bit systems? > If I'm not missing nothing obvious, 65536LL is the right thing to do... I did not really checked, but if v_min * 65536 can never overflow, then yeah, this is not really "fixing" nothing. - Nuno Sá
diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c index 97f13c0b9631..b03d3c7cd4c4 100644 --- a/drivers/iio/dac/ad3552r.c +++ b/drivers/iio/dac/ad3552r.c @@ -770,7 +770,7 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch) dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * 1000000, 65536); - dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, span, &rem); + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536L, span, &rem); tmp = (s64)rem * 1000000; dac->ch_data[ch].offset_dec = div_s64(tmp, span); }
Two 32-bit values are being evaluated using 32-bit arithmetic and then passed to s64 type. It is wrong. Expression should be evaluated using 64-bit arithmetic and then passed. Fixes: 8f2b54824b ("drivers:iio:dac: Add AD3552R driver support") Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- drivers/iio/dac/ad3552r.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)