linux-kernel - Re: [PATCH v4 02/11] iio: adc: Add basic support for AD4170

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: <20250607175904.08d7b994@jic23-huawei>
Date: Sat, 7 Jun 2025 17:59:04 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andy@...nel.org>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>, Marcelo Schmitt
 <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org, Ana-Maria Cusco <ana-maria.cusco@...log.com>,
 lars@...afoo.de, Michael.Hennerich@...log.com, dlechner@...libre.com,
 nuno.sa@...log.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl
Subject: Re: [PATCH v4 02/11] iio: adc: Add basic support for AD4170

On Tue, 3 Jun 2025 16:43:42 +0300
Andy Shevchenko <andy@...nel.org> wrote:

> On Tue, Jun 03, 2025 at 09:02:56AM -0300, Marcelo Schmitt wrote:
> > On 06/03, Andy Shevchenko wrote:  
> > > On Mon, Jun 02, 2025 at 01:54:25PM -0300, Marcelo Schmitt wrote:  
> 
> ...
> 
> > > > > > +static bool ad4170_setup_eq(struct ad4170_setup *a, struct ad4170_setup *b)
> > > > > > +{
> > > > > > +	/*
> > > > > > +	 * The use of static_assert() here is to make sure that the comparison
> > > > > > +	 * is adapted whenever struct ad4170_setup is changed.
> > > > > > +	 */  
> > Does the reason given in the comment justify the use of static_assert?  
> 
> Should I repeat myself? It makes a little sense when no memcmp() is involved.

The intent I think is to reduce the chance of a field being added without
this match function being updated.   Not a very strong test but maybe
better than nothing...

Not sure how memcmp() is relevant.

> 
> > > > > > +	static_assert(sizeof(*a) ==
> > > > > > +		      sizeof(struct {
> > > > > > +				     u16 misc;
> > > > > > +				     u16 afe;
> > > > > > +				     u16 filter;
> > > > > > +				     u16 filter_fs;
> > > > > > +				     u32 offset;
> > > > > > +				     u32 gain;
> > > > > > +			     }));  
> > > > > 
> > > > > I think it doesn't make much sense unless one uses memcpy().  
> > > > 
> > > > memcpy() is used to update the setups after reg write succeeds.
> > > > Also, previously, memcmp() was used to compare setups.
> > > > Since struct ad4170_setup has only unsigned integers (no floating point fields
> > > > like ad7124 had [1]), ad4170 works properly when comparing setups with memcmp().
> > > > Though, it was asked to do explicit field matching on previous reviews [2] so
> > > > that's how it had been since then. Well, both ways work for ad4170. We can
> > > > compare setup with memcmp(), or do the comparison field by field. I don't mind
> > > > changing it again if requested. I guess we only need to reach an agreement about
> > > > what to go with.  
> > > 
> > > The question was "why do you need the static_assert() now?"  
> > 
> > To ensure that the comparison function gets updated if struct ad4170_setup is
> > ever modified? This intends to be similar to what was implemented in ad7124
> > driver as the chips have similar channel configuration mechanisms. We also
> > have ad7173 and ad4130 using static_assert for analogous purpose. There was
> > also a comment about static_assert above.  
> 
> Does this won;t work if you changes field types? (Assuming only integers to
> integers) I believe it doesn't affect the field-by-field comparison.
> 
> The other drivers may have different approach, have you studied them? Do they
> use memcmp()
> 
> > > > [1]: https://lore.kernel.org/all/20250303114659.1672695-13-u.kleine-koenig@baylibre.com/
> > > > [2]: https://lore.kernel.org/linux-iio/20250504192117.5e19f44b@jic23-huawei/
> > > >   
> > > > > > +	if (a->misc != b->misc ||
> > > > > > +	    a->afe != b->afe ||
> > > > > > +	    a->filter != b->filter ||
> > > > > > +	    a->filter_fs != b->filter_fs ||
> > > > > > +	    a->offset != b->offset ||
> > > > > > +	    a->gain != b->gain)
> > > > > > +		return false;
> > > > > > +
> > > > > > +	return true;
> > > > > > +}  
>