linux-kernel - Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

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: <50107aecc446ba42e312b81e18a6ffe871fa3291.camel@baylibre.com>
Date: Thu, 20 Nov 2025 12:43:09 +0100
From: Francesco Lavra <flavra@...libre.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, Jonathan Cameron
 <jic23@...nel.org>,  David Lechner <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>,  linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on
 a per axis basis

On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra
> > > > > > > wrote:
> 
> ...
> 
> > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > > > (old_enable
> > > > > > > > &
> > > > > > > > ~BIT(axis));
> > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > 
> > > > > > > This is an interesting check. So, old_enable and new_enable
> > > > > > > are
> > > > > > > _not_
> > > > > > > booleans, right?
> > > > > > > So, this means the check test if _any_ of the bit was set and
> > > > > > > kept
> > > > > > > set or
> > > > > > > none were set
> > > > > > > and non is going to be set. Correct? I think a short comment
> > > > > > > would be
> > > > > > > good to have.
> > > > > > 
> > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > interested
> > > > > > in
> > > > > > whether any bit is set, to catch the cases where the bit mask
> > > > > > goes
> > > > > > from
> > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > 
> > > > > If it's a true bitmask (assuming unsigned long type) then all
> > > > > this
> > > > > can be
> > > > > done
> > > > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > > > weights of
> > > > > them
> > > > > (probably that gives even the same size of the object file, but
> > > > > !!
> > > > > instructions
> > > > >  will be changed to hweight() calls (still a single assembly
> > > > > instr on
> > > > > modern
> > > > >  architectures).
> > > > 
> > > > These are u8 variables, so we can't use the bitmap API.
> > > 
> > > OK. But hweight8() can still be used.
> > > 
> > > > And I don't
> > > > understand the reason for using hweight(), given that the !!
> > > > operators
> > > > would still be needed.
> > > 
> > > No, you won't need !! with that.
> > 
> > I still don't understand. Are you proposing to replace `if
> > (!!old_enable ==
> > !!new_enable)` with `if (hweight8(old_enable) ==
> > hweight8(new_enable))`?
> > That won't work, because we only need to check whether the Hamming
> > weight
> > goes from zero to non-zero and vice versa.
> 
>        old_enable = hw->enable_event[event];
>        new_enable = state ? (old_enable | BIT(axis)) :
>                             (old_enable & ~BIT(axis));
>        if (!!old_enable == !!new_enable)
>                return 0;
> 
> If I am not mistaken this will do exactly the same in a simpler way.
> 
>         old_enable = hw->enable_event[event];
>         if (state)
>                 new_enable = old_enable | BIT(axis);
>         else
>                 new_enable = old_enable & ~BIT(axis);
>         if ((new_enable ^ old_enable) != BIT(axis))
>                 return 0;

This doesn't look right to me, if new_enable differs from old_enable by
just one bit (which it does), then the XOR result will always have this bit
(and no others) set, so (new_enable ^ old_enable) will always equal
BIT(axis).
We are not checking if the bit was already set or clear, when this code
runs we already know that the bit is changing, what we are checking is
whether all bits are zero before or after this change.

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)