linux-kernel - Re: [PATCH] iio: pac1921: remove unnecessary explicit casts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <172768874424.228843.16360087197143367783@njaxe.localdomain>
Date: Mon, 30 Sep 2024 11:32:24 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: pac1921: remove unnecessary explicit casts

Quoting Jonathan Cameron (2024-09-28 17:28:43)
> On Mon, 16 Sep 2024 14:00:05 +0200
> Matteo Martelli <matteomartelli3@...il.com> wrote:
> 
> > Many explicit casts were introduced to address Wconversion and
> > Wsign-compare warnings. Remove them to improve readability.
> > 
> > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> 
> No fixes tag on this one. Its not a bug, just a readability improvement.
> 
> > Signed-off-by: Matteo Martelli <matteomartelli3@...il.com>
> There are a few cases in here where I think the cast is about ensuring
> we don't overflow in the maths rather than for warning suppression.
> 
> We all love 32 bit architectures after all ;)
> 
> Jonathan
> 
> > ---
> > Link: https://lore.kernel.org/linux-iio/1fa4ab12-0939-477d-bc92-306fd32e4fd9@stanley.mountain/
> > ---
> >  drivers/iio/adc/pac1921.c | 43 +++++++++++++++++++++----------------------
> >  1 file changed, 21 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > index 4c2a1c07bc39..de69a1619a9e 100644
> > --- a/drivers/iio/adc/pac1921.c
> > +++ b/drivers/iio/adc/pac1921.c
> > @@ -240,8 +240,8 @@ static inline void pac1921_calc_scale(int dividend, int divisor, int *val,
> >  {
> >       s64 tmp;
> >  
> > -     tmp = div_s64(dividend * (s64)NANO, divisor);
> > -     *val = (int)div_s64_rem(tmp, NANO, val2);
> > +     tmp = div_s64(dividend * NANO, divisor);
> 
> For this one, NANO is an unsigned long and dividend just an int.
> Either the s64 cast is needed because dividend * NANO might go out of
> unsigned long range which might (I think) be 32 bit on a 32bit machine
> or it doesn't.  If it does, then you need the cast, if not you don't
> need to use div_s64 as it's not that large.

Oops! While removing the casts I was only thinking about the sign change
for this case, which would not be an issue being the dividend always
positive. However you are indeed right, this would overflow in 32-bit
architectures. Thanks for catching it, I am going to reintroduce this
cast and the other similar one in the next patch version.

...

Thanks,
Matteo Martelli