linux-kernel - Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

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: <20251206163950.545da8cc@jic23-huawei>
Date: Sat, 6 Dec 2025 16:39:50 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Jorge Marques <gastmaier@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>, Jorge Marques
 <jorge.marques@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Jonathan Corbet <corbet@....net>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

On Wed, 26 Nov 2025 12:40:00 +0100
Jorge Marques <gastmaier@...il.com> wrote:

> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:  
> > > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > > register (SAR) analog-to-digital converter (ADC) with low-power and
> > > threshold monitoring modes.  
> > 
> > ...
> >   
> Hi Andy,
> > > +#define AD4062_SOFT_RESET	0x81  
> > 
> > The grouping seems a bit strange. Haven't you forgotten a blank line here?
> > Ditto for other similar cases.
> >   
> Ack.

Side note. For efficiency, if you agree with something just delete that
block of the thread and don't say so.  Reviewers should be safe to assume
that anything the author agrees with will just be fixed in the next version.

That lets us focus in very fast on the key discussions.

> >   
> > > +struct ad4062_state {
> > > +	const struct ad4062_chip_info *chip;
> > > +	const struct ad4062_bus_ops *ops;
> > > +	enum ad4062_operation_mode mode;
> > > +	struct completion completion;
> > > +	struct iio_trigger *trigger;
> > > +	struct iio_dev *indio_dev;
> > > +	struct i3c_device *i3cdev;
> > > +	struct regmap *regmap;
> > > +	u16 sampling_frequency;
> > > +	int vref_uv;
> > > +	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> > > +	u8 oversamp_ratio;
> > > +	union {
> > > +		__be32 be32;
> > > +		__be16 be16;
> > > +		u8 bytes[4];
> > > +	} buf __aligned(IIO_DMA_MINALIGN);
> > > +	u8 reg_addr_conv;  
> > 
> > Can't we group u8:s to save a few bytes of memory?
> >   
> Sure
> 
>   struct ad4062_state {
>   	// ...
>   	union {
>   		__be32 be32;
>   		__be16 be16;
>   		u8 bytes[4];
>   	} buf __aligned(IIO_DMA_MINALIGN);
>   	u16 sampling_frequency;
>   	u8 oversamp_ratio;
>   	u8 reg_addr_conv;

Unless my assumption is wrong and those 3 values are passed
as buffers for DMA (or otherwise very carefully protected
against access racing with the DMA buffers) then this is very wrong.
Refresh your memory on why we do the __aligned(IIO_DMA_MINALIGN) and
exactly how that works.

Short answer, nothing must come after it in the structure as it
only forces the start of the buffer, not it's end and hence the
following data ends up in the same cacheline and fun data corruption
is the result.

>   };