linux-kernel - Re: [PATCH v6 1/4] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANZih_QAoSAMzPOq=di9=Wk=WK4+B1Sx80FjfCx41z6dYJQLyA@mail.gmail.com>
Date: Sat, 5 Jul 2025 00:17:28 -0300
From: Andrew Ijano <andrew.ijano@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: andrew.lopes@...mni.usp.br, gustavobastos@....br, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, jstephan@...libre.com, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/4] iio: accel: sca3000: replace usages of internal
 read data helpers by spi helpers

On Sat, Jun 21, 2025 at 2:38 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> Hi. I made two related tweaks.  A few comments inline for further possible cleanup.
>
> Applied to the togreg branch of iio.git and initially pushed out as testing
> for 0-day to take a look at it.
>
Thanks. Since there was a problem with my implementation, I'll send a
new version along with your tweaks too.

> >       if (ret < 0)
> >               goto error_ret;
> >       dev_info(&indio_dev->dev,
> >                "sca3000 revision major=%lu, minor=%lu\n",
> > -              st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
> > -              st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
> > +              ret & SCA3000_REG_REVID_MAJOR_MASK,
> > +              ret & SCA3000_REG_REVID_MINOR_MASK);
> Hmm. doesn't belong in this patch but it is rather odd to not see
> a shift on one of these.
>
> Hmm. Time to miss quote whoever it was who said:
>
> "kernel development is like a murder mystery where you try to solve
>  the crime only to realize you were the murderer."
>
> Below I mention using FIELD_GET() in appropriate places as a possible additional
> cleanup.  Fix this up when you do that (and note it in the patch description for
> that patch).
Ok! I'll do that.

> >       /* mask bottom 2 bits - only ones that are relevant */
> > -     st->rx[0] &= SCA3000_REG_MODE_MODE_MASK;
> > -     switch (st->rx[0]) {
> > +     ret &= SCA3000_REG_MODE_MODE_MASK;
> > +     switch (ret) {
> See discussion below.  This can be simplified as
>         switch (reg & SCA3000_REG_MODE_MASK)
> avoiding the modified 'ret' value in place comment.
>
> This one I'll tweak as it is easy to avoid the ugly pattern.
>
Nice! I'll pay attention to similar cases in the future.

> >
> > -     st->rx[0] &= ~SCA3000_REG_MODE_MODE_MASK;
> > -     st->rx[0] |= (mode & SCA3000_REG_MODE_MODE_MASK);
> > +     ret &= ~SCA3000_REG_MODE_MODE_MASK;
>
> For a further potential cleanup it would be good to use FIELD_GET() and FIELD_PREP()
> in appropriate places in this driver. Do that into a separate local variable
> as using ret for all this is a little confusing as quite a bit of the time
> it's not a variable we are ever going to return.
>
> As a rule of thumb if we are modifying the ret only a little in a single reuse
> (i.e. masking out a bit in a parameter being passed to something else) then
> a local variable is probably overkill. If we are building up a new register
> value it is not really appropriate to do it into ret.
>
> I'm not asking for changes in this patch though as what you have here
> is the simplest and easiest to review change.  If you add those extra
> local variables as part of using FIELD_GET/ FIELD_PREP though that would
> be great.

That's great! I didn't know about FIELD_GET() and FIELD_PREP() before.
This is really something that would be better in a separate patch, I
could try to do that later.

Thanks,
Andrew