linux-kernel - Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find

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: <41c6f424ed42655a62a7b66aa81604605676a551.camel@gmail.com>
Date: Tue, 22 Oct 2024 10:24:33 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Alexandru Ardelean <aardelean@...libre.com>, David Lechner
	 <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org,
 robh@...nel.org, 	lars@...afoo.de, michael.hennerich@...log.com,
 gstols@...libre.com, brgl@...ev.pl
Subject: Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with
 find_closest() for oversampling

On Tue, 2024-10-22 at 09:31 +0300, Alexandru Ardelean wrote:
> On Mon, Oct 21, 2024 at 10:31 PM David Lechner <dlechner@...libre.com> wrote:
> > 
> > On 10/21/24 2:03 PM, David Lechner wrote:
> > > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > > > There's a small issue with setting oversampling-ratio that seems to have
> > > > been there since the driver was in staging.
> > > > Trying to set an oversampling value of '2' will set an oversampling
> > > > value
> > > > of '1'. This is because find_closest() does an average + rounding of 1 +
> > > > 2,
> > > > and we get '1'.
> > > > 
> > > > This is the only issue with find_closest(), at least in this setup. The
> > > > other values (above 2) work reasonably well. Setting 3, rounds to 2, so
> > > > a
> > > > quick fix is to round 'val' to 3 (if userspace provides 2).
> > > 
> > > This sounds like a bug in find_closest() instead of in this driver.
> > > 
> 
> Adding Bart (the original author of find_closest()).
> 
> > > If there is an exact match in the list, it seems reasonable to expect
> > > that the exact match is returned by find_closest().
> > > 
> > 
> > Likely also affected by this bug since they have values 1, 2 in the list:
> > 
> > * rtq6056_adc_set_average()
> > * si1133_scale_to_swgain()
> 
> Yeah.
> I forgot to mention this sooner.
> But this patch is more of an RFC patch about how to handle this
> situation with find_closest().
> 
> For monotonic values with an increment of 1, find_closest() is a bit buggy.
> Will try to fix find_closest()
> 
> > 

FWIW, I'm not a fan of using find_closest() in this situation. We have an
available attr wich outputs the supported values. To me, -EINVAL is the way to
go if some user writes an invalid value.

I feel the usage of find_closest() in these case is likely to make the code
easier. Maybe an helper analogous to match_string() would be seen with good eyes
(like match_value()).

But yeah, I guess that changing the behavior now could break some userspace
users.

- Nuno Sá