[PATCH] iio: imu: adis16550: rework clock range test

[PATCH] iio: imu: adis16550: rework clock range test

[David Lechner]

Rework the clock rate range test to test if sync_mode_data != NULL
instead of testing if the for loop index variable. This makes it easier
for static analyzers to see that we aren't using an uninitialized
sync_mode_data [1].

Link: https://github.com/analogdevicesinc/linux/pull/2831/files#diff-e60c8024905ba921f6aac624cae67d6f9bd54aa5af5a27ae60a8ca21ef120ddaR950 [1]
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
Here is the for loop that comes just before the if statement in the
patch so reviewers don't have to look it up:

	for (i = 0; i < st->info->num_sync; i++) {
		if (st->clk_freq_hz >= st->info->sync_mode[i].min_rate &&
		    st->clk_freq_hz <= st->info->sync_mode[i].max_rate) {
			sync_mode_data = &st->info->sync_mode[i];
			break;
		}
	}

Previously, we were using the index variable `i` to check if we hit
the break or not, but checking if sync_mode_data was assigned is a bit
more straight-forward for machines (and probably some humans too).
---
 drivers/iio/imu/adis16550.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
index 28f0dbd0226cbea67bc6c87d892f7812f21e9304..d733daaa05bcf0b0cfd710330230c671e66feef4 100644
--- a/drivers/iio/imu/adis16550.c
+++ b/drivers/iio/imu/adis16550.c
@@ -909,7 +909,7 @@ static int adis16550_reset(struct adis *adis)
 static int adis16550_config_sync(struct adis16550 *st)
 {
 	struct device *dev = &st->adis.spi->dev;
-	const struct adis16550_sync *sync_mode_data;
+	const struct adis16550_sync *sync_mode_data = NULL;
 	struct clk *clk;
 	int ret, i;
 	u16 mode;
@@ -932,7 +932,7 @@ static int adis16550_config_sync(struct adis16550 *st)
 		}
 	}
 
-	if (i == st->info->num_sync)
+	if (!sync_mode_data)
 		return dev_err_probe(dev, -EINVAL, "Clk rate: %lu not in a valid range",
 				     st->clk_freq_hz);
 

---
base-commit: 6742eff60460e77158d4f1b233f17e0345c9e66a
change-id: 20250702-iio-imu-adis16550-rework-clock-range-test-63816e60586d

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] iio: imu: adis16550: rework clock range test

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
> Rework the clock rate range test to test if sync_mode_data != NULL
> instead of testing if the for loop index variable. This makes it easier
> for static analyzers to see that we aren't using an uninitialized
> sync_mode_data [1].

But at the same time it makes it not to be the usual pattern.,,

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: adis16550: rework clock range test

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
> > Rework the clock rate range test to test if sync_mode_data != NULL
> > instead of testing if the for loop index variable. This makes it easier
> > for static analyzers to see that we aren't using an uninitialized
> > sync_mode_data [1].
> 
> But at the same time it makes it not to be the usual pattern.,,

Reading the static analyser output I think the first hunk is only what we need,
but this is still false positive and it's problem of that static
analyser. Have you filed a bug there? (My point is that modifying the code for
the advantage of false positives of some static analyser is wrong road to go
in my opinion.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: adis16550: rework clock range test

[David Lechner]

On 7/2/25 9:59 AM, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:
>> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
>>> Rework the clock rate range test to test if sync_mode_data != NULL
>>> instead of testing if the for loop index variable. This makes it easier
>>> for static analyzers to see that we aren't using an uninitialized
>>> sync_mode_data [1].
>>
>> But at the same time it makes it not to be the usual pattern.,,
> 
> Reading the static analyser output I think the first hunk is only what we need,
> but this is still false positive and it's problem of that static
> analyser. Have you filed a bug there? (My point is that modifying the code for
> the advantage of false positives of some static analyser is wrong road to go
> in my opinion.)
> 

I agree that we shouldn't fix this _only_ to make the static analyzer
happy. But I had to think quite a bit harder to see that the existing
code was correct compared to what I have proposed here.

But if this is a common pattern that I just haven't learned to identify
at a glance yet and everybody else can easily see that the existing code
is correct, then perhaps it isn't worth the change.

Re: [PATCH] iio: imu: adis16550: rework clock range test

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 10:07:17AM -0500, David Lechner wrote:
> On 7/2/25 9:59 AM, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
> >>> Rework the clock rate range test to test if sync_mode_data != NULL
> >>> instead of testing if the for loop index variable. This makes it easier
> >>> for static analyzers to see that we aren't using an uninitialized
> >>> sync_mode_data [1].
> >>
> >> But at the same time it makes it not to be the usual pattern.,,
> > 
> > Reading the static analyser output I think the first hunk is only what we need,
> > but this is still false positive and it's problem of that static
> > analyser. Have you filed a bug there? (My point is that modifying the code for
> > the advantage of false positives of some static analyser is wrong road to go
> > in my opinion.)
> 
> I agree that we shouldn't fix this _only_ to make the static analyzer
> happy. But I had to think quite a bit harder to see that the existing
> code was correct compared to what I have proposed here.
> 
> But if this is a common pattern that I just haven't learned to identify
> at a glance yet and everybody else can easily see that the existing code
> is correct, then perhaps it isn't worth the change.

To me checking against index variable (when it's integer, obviously) is correct
thing to do and regular pattern. OTOH, if the "index" is a pointer and rather
we call it "iterator", the angle of view is different because in some cases
it may lead to stale or invalid value which might be mistakenly dereferenced or
speculated (see more in the discussion about list entry APIs [entry is a
keyword here] and if list_entry_is_head() is a good approach.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: adis16550: rework clock range test

[Jonathan Cameron]

On Wed, 2 Jul 2025 18:17:24 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jul 02, 2025 at 10:07:17AM -0500, David Lechner wrote:
> > On 7/2/25 9:59 AM, Andy Shevchenko wrote:  
> > > On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:  
> > >> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:  
> > >>> Rework the clock rate range test to test if sync_mode_data != NULL
> > >>> instead of testing if the for loop index variable. This makes it easier
> > >>> for static analyzers to see that we aren't using an uninitialized
> > >>> sync_mode_data [1].  
> > >>
> > >> But at the same time it makes it not to be the usual pattern.,,  
> > > 
> > > Reading the static analyser output I think the first hunk is only what we need,
> > > but this is still false positive and it's problem of that static
> > > analyser. Have you filed a bug there? (My point is that modifying the code for
> > > the advantage of false positives of some static analyser is wrong road to go
> > > in my opinion.)  
> > 
> > I agree that we shouldn't fix this _only_ to make the static analyzer
> > happy. But I had to think quite a bit harder to see that the existing
> > code was correct compared to what I have proposed here.
> > 
> > But if this is a common pattern that I just haven't learned to identify
> > at a glance yet and everybody else can easily see that the existing code
> > is correct, then perhaps it isn't worth the change.  
> 
> To me checking against index variable (when it's integer, obviously) is correct
> thing to do and regular pattern. OTOH, if the "index" is a pointer and rather
> we call it "iterator", the angle of view is different because in some cases
> it may lead to stale or invalid value which might be mistakenly dereferenced or
> speculated (see more in the discussion about list entry APIs [entry is a
> keyword here] and if list_entry_is_head() is a good approach.)
> 

Original code looks fine to me and is a very common pattern.  So I'd argue
the static analyzer needs some work.

Jonathan

Re: [PATCH] iio: imu: adis16550: rework clock range test

[David Lechner]

On 7/2/25 10:31 AM, Jonathan Cameron wrote:
> On Wed, 2 Jul 2025 18:17:24 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
>> On Wed, Jul 02, 2025 at 10:07:17AM -0500, David Lechner wrote:
>>> On 7/2/25 9:59 AM, Andy Shevchenko wrote:  
>>>> On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:  
>>>>> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:  
>>>>>> Rework the clock rate range test to test if sync_mode_data != NULL
>>>>>> instead of testing if the for loop index variable. This makes it easier
>>>>>> for static analyzers to see that we aren't using an uninitialized
>>>>>> sync_mode_data [1].  
>>>>>
>>>>> But at the same time it makes it not to be the usual pattern.,,  
>>>>
>>>> Reading the static analyser output I think the first hunk is only what we need,
>>>> but this is still false positive and it's problem of that static
>>>> analyser. Have you filed a bug there? (My point is that modifying the code for
>>>> the advantage of false positives of some static analyser is wrong road to go
>>>> in my opinion.)  
>>>
>>> I agree that we shouldn't fix this _only_ to make the static analyzer
>>> happy. But I had to think quite a bit harder to see that the existing
>>> code was correct compared to what I have proposed here.
>>>
>>> But if this is a common pattern that I just haven't learned to identify
>>> at a glance yet and everybody else can easily see that the existing code
>>> is correct, then perhaps it isn't worth the change.  
>>
>> To me checking against index variable (when it's integer, obviously) is correct
>> thing to do and regular pattern. OTOH, if the "index" is a pointer and rather
>> we call it "iterator", the angle of view is different because in some cases
>> it may lead to stale or invalid value which might be mistakenly dereferenced or
>> speculated (see more in the discussion about list entry APIs [entry is a
>> keyword here] and if list_entry_is_head() is a good approach.)
>>
> 
> Original code looks fine to me and is a very common pattern.  So I'd argue
> the static analyzer needs some work.
> 
> Jonathan

OK, we can drop the patch.