linux-kernel - Re: [PATCH v2 1/2] iio: mpl3115: add threshold events support

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: <20251104140315.0000394d@huawei.com>
Date: Tue, 4 Nov 2025 14:03:15 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
CC: Jonathan Cameron <jic23@...nel.org>, Antoni Pokusinski
	<apokusinski01@...il.com>, <dlechner@...libre.com>, <nuno.sa@...log.com>,
	<andy@...nel.org>, <marcelo.schmitt1@...il.com>, <linux-iio@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: mpl3115: add threshold events support

On Mon, 3 Nov 2025 10:22:12 +0200
Andy Shevchenko <andriy.shevchenko@...el.com> wrote:

> On Sun, Nov 02, 2025 at 10:38:08AM +0000, Jonathan Cameron wrote:
> > On Fri, 31 Oct 2025 21:18:22 +0100
> > Antoni Pokusinski <apokusinski01@...il.com> wrote:  
> 
> ...
> 
> 
> > Generally looks good to me, but some comments on the 24 bit value reading.  
> 
> > > +		i2c_smbus_read_i2c_block_data(data->client,
> > > +					      MPL3115_OUT_PRESS,
> > > +					      3, (u8 *)&val_press);  
> > 
> > This is an oddity.  Why read into a __be32 when it's a 24bit number?
> > I guess it doesn't really matter as you just need a big enough space
> > and throw the value away.  However, I'd read it into a u8 [3]; then size off that
> > as well.
> > 
> > There are two existing cases of this in the driver. One of them should use
> > get_unaligned_be24 on a u8[3] buffer.  The other one is more complex as it's
> > reading directly into the scan buffer that gets pushed to the kfifo and is
> > reading into a u8 buffer ultimately anyway so at least there is no
> > real suggestion of it being 32 bits (just a +4 shift to deal with natural
> > alignment as the storage has to be power of 2 in that case.).
> > 
> > hmm. I think either we should tidy up the easy case (_read_info_raw) +
> > use a u8[3] here or just stick to this being odd.
> > My preference would be to have another patch tidying up the other case
> > + use a u8[3] here.  
> 
> Just a side question... Wondering, if we actually can defined __be24 and __le24
> types (or at least u24) for really explicit cases.

Would be useful for readability. Particularly if we could make it work with the
type checking stuff similar to the endian markings, but restricted to only be
accessed with the unaligned accessors.  Possibly worth doing even without that.

Jonathan

>