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: <aD7kcFupREh4lW0s@debian-BULLSEYE-live-builder-AMD64>
Date: Tue, 3 Jun 2025 09:02:56 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Andy Shevchenko <andy@...nel.org>
Cc: 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>, jic23@...nel.org,
	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 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?

> > > > +	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.

Best regards,
Marcelo

> 
> > [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;
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko