MIME-Version: 1.0
References: <20211222034646.222189-1-liambeguin@gmail.com> <20211222034646.222189-10-liambeguin@gmail.com>
 <CAHp75Vc0aWrFtNK1ZkHkwP62zNXQJaDcn9pc8Uhfq0kOnWzmJg@mail.gmail.com>
 <YcNwt5RFMNFUimD/@shaak> <CAHp75VdrLTNLWZRgWkLXD23RAF28zh29XybywAPyMtb=GNxXbw@mail.gmail.com>
 <YcODglDWiknz2oeV@shaak>
In-Reply-To: <YcODglDWiknz2oeV@shaak>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 22 Dec 2021 23:32:24 +0200
Message-ID: <CAHp75Vejfr_S7iK7fAvs7ELxE1TJUECvmKv0-G5Zwunyc6nDQA@mail.gmail.com>
Subject: Re: [PATCH v11 09/15] iio: afe: rescale: reduce risk of integer overflow
To:     Liam Beguin <liambeguin@gmail.com>
Cc:     Peter Rosin <peda@axentia.se>, Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote:

...

> > > > > -               tmp = 1 << *val2;
> > > >
> > > > At some point this should be BIT()
> >
> > Forgot to add, If it's 64-bit, then BIT_ULL().
> >
> > > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > > more explicit as we're not working with bitfields. No?
> >
> > You may add a comment. You may use int_pow(), but it will be suboptimal.
> >
> > > > Rule of thumb (in accordance with C standard), always use unsigned
> > > > value as left operand of the _left_ shift.
> > >
> > > Right, that makes sense! In practice though, since we'll most likely
> > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > > enough to simply typecast?
> > >
> > >         tmp = 1 << (unsigned int)*val2;
> >
> > No, it's about the _left_ operand.
> > I haven't checked if tmp is 64-bit, then even that would be still wrong.
>
> Okay so your recommendation is to not use a left shift?

No, I recommend not to use int type as a _leftside_ operand.
BIT() / BIT_ULL() does a left shift anyway.

> I can look into that but given how unlikely it is to fall into those bad
> cases, I'd rather keep things as they are. Would that be okay?

> Also, I don't think using BIT() or BIT_ULL() would address this as they
> both do the same shift, with no extra checks.

They do slightly different versions of it. They use an unsigned int type.

Open coded or not, it's up to you. Just convert to unsigned int.

-- 
With Best Regards,
Andy Shevchenko