[PATCH] gpio: gpio-exar: replace division condition with direct comparison

[PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Suraj Sonawane]

Fix an issue detected by the Smatch tool:

drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
replace divide condition 'pin / 8' with 'pin >= 8'
drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
replace divide condition 'pin / 8' with 'pin >= 8'

The division 'pin / 8' was used to check if the pin number is 8 or greater,
which can be confusing and less readable. Replacing it with 'pin >= 8'
makes the code clearer by directly comparing the pin number. This also
removes reliance on integer division, which can be harder to understand
and may introduce subtle bugs in the future.

Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
 drivers/gpio/gpio-exar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 5170fe759..400cc3a0b 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -49,7 +49,7 @@ exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 {
 	unsigned int pin = exar_gpio->first_pin + (offset % 16);
 	unsigned int cascaded = offset / 16;
-	unsigned int addr = pin / 8 ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
+	unsigned int addr = pin >= 8 ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
 
 	return addr + (cascaded ? exar_gpio->cascaded_offset : 0);
 }
@@ -59,7 +59,7 @@ exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 {
 	unsigned int pin = exar_gpio->first_pin + (offset % 16);
 	unsigned int cascaded = offset / 16;
-	unsigned int addr = pin / 8 ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
+	unsigned int addr = pin >= 8 ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
 
 	return addr + (cascaded ? exar_gpio->cascaded_offset : 0);
 }
-- 
2.34.1

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 13 Nov 2024 01:46:59 +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
> replace divide condition 'pin / 8' with 'pin >= 8'
> drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
> replace divide condition 'pin / 8' with 'pin >= 8'
> 
> [...]

Applied, thanks!

[1/1] gpio: gpio-exar: replace division condition with direct comparison
      commit: 8f0aa162bcf818631e845c2e6c090c7b3c64fd9d

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Suraj Sonawane]

On 13/11/24 21:01, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 
> On Wed, 13 Nov 2024 01:46:59 +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
>> drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] gpio: gpio-exar: replace division condition with direct comparison
>        commit: 8f0aa162bcf818631e845c2e6c090c7b3c64fd9d
> 
> Best regards,
Thank you for applying this patch!

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Andy Shevchenko]

+ Dan

I have to comment on this change as it's a bit controversial.

TL;DR: this patch is not more than a (harmless?) noise.

On Wed, Nov 13, 2024 at 01:46:59AM +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
> replace divide condition 'pin / 8' with 'pin >= 8'
> drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
> replace divide condition 'pin / 8' with 'pin >= 8'

This message does not really explain why.

> The division 'pin / 8' was used to check if the pin number is 8 or greater,
> which can be confusing and less readable.

It's inaccurate description. Everyone who is familiar with GPIO HW is
also familiar with line grouping in banks. Here is the clear statement
"get the bank number (where 8 lines per bank), and if it's 0 do this,
else do that". It might be in the future that (new version of) HW will
gain more banks and we would return to "division".

> Replacing it with 'pin >= 8' makes the code clearer by directly
> comparing the pin number.

I don't think this statement is fully true. See above.

> This also removes reliance on integer division,

On top of that "division" here uses power-of-two divisor, which any
optimizing (and this code I think won't ever be built without
optimization turned on) compiler (I think from the very beginning of
the Linux kernel project) knows how to convert to right shifts on
the platforms that support that (and how many do not nowadays? 0?).

Additionally in the cases when we have a / 8; a % 8 type of expressions
coupled together, the compiler actually may issue an integer division
assembly instructions on some ISAs where it gives two values in one
go. Replacing like the above might break that (if the compiler is old
or not clever enough).

> which can be harder to understand

No, "division" by power-of-two numbers is very well understandble.

> and may introduce subtle bugs in the future.

What bugs?

The bottom line is that: I recommend to work with smatch developers
to amend smatch instead.

P.S. I wouldn't like to see similar patches to other GPIO drivers,
especially those that use a / 8; a % 8 type of expressions together. 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Dan Carpenter]

Uh, I had to think back...  I had forgotten that I actually published that
check.  I can unpublish it.

I wrote it based on a real issue, and then when I looked at the warnings quite
a few places wrote code like "if (x / 4)" where they had intended to write if
if ((x % 4) == 0).  So it seemed like a good idea.

But in the two years since I published the warning, it has mostly been false
positives.

regards,
dan carpenter

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Bartosz Golaszewski]

On Fri, Nov 15, 2024 at 12:55 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Uh, I had to think back...  I had forgotten that I actually published that
> check.  I can unpublish it.
>
> I wrote it based on a real issue, and then when I looked at the warnings quite
> a few places wrote code like "if (x / 4)" where they had intended to write if
> if ((x % 4) == 0).  So it seemed like a good idea.
>
> But in the two years since I published the warning, it has mostly been false
> positives.
>
> regards,
> dan carpenter
>

Ok, I dropped this patch from my queue. I typically trust smatch so I
picked it up without giving it much thought.

Bart

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Suraj Sonawane]

On 15/11/24 15:06, Andy Shevchenko wrote:
> + Dan
> 
> I have to comment on this change as it's a bit controversial.
> 
> TL;DR: this patch is not more than a (harmless?) noise.
> 
> On Wed, Nov 13, 2024 at 01:46:59AM +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
>> drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
> 
> This message does not really explain why.
> 
>> The division 'pin / 8' was used to check if the pin number is 8 or greater,
>> which can be confusing and less readable.
> 
> It's inaccurate description. Everyone who is familiar with GPIO HW is
> also familiar with line grouping in banks. Here is the clear statement
> "get the bank number (where 8 lines per bank), and if it's 0 do this,
> else do that". It might be in the future that (new version of) HW will
> gain more banks and we would return to "division".
> 
>> Replacing it with 'pin >= 8' makes the code clearer by directly
>> comparing the pin number.
> 
> I don't think this statement is fully true. See above.
> 
>> This also removes reliance on integer division,
> 
> On top of that "division" here uses power-of-two divisor, which any
> optimizing (and this code I think won't ever be built without
> optimization turned on) compiler (I think from the very beginning of
> the Linux kernel project) knows how to convert to right shifts on
> the platforms that support that (and how many do not nowadays? 0?).
> 
> Additionally in the cases when we have a / 8; a % 8 type of expressions
> coupled together, the compiler actually may issue an integer division
> assembly instructions on some ISAs where it gives two values in one
> go. Replacing like the above might break that (if the compiler is old
> or not clever enough).
> 
>> which can be harder to understand
> 
> No, "division" by power-of-two numbers is very well understandble.
> 
>> and may introduce subtle bugs in the future.
> 
> What bugs?
> 
> The bottom line is that: I recommend to work with smatch developers
> to amend smatch instead.
> 
> P.S. I wouldn't like to see similar patches to other GPIO drivers,
> especially those that use a / 8; a % 8 type of expressions together.
> 

I understand your points about the familiarity of line grouping in GPIO 
hardware and the optimization behavior of compilers for power-of-two 
division. I initially thought this could be a good fix as I have seen 
similar changes before. Thank you for the feedback—I will keep this in 
mind before submitting such patches in the future.

Best regards,
Suraj Sonawane

Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Andy Shevchenko]

On Fri, Nov 15, 2024 at 06:55:41PM +0530, Suraj Sonawane wrote:
> On 15/11/24 15:06, Andy Shevchenko wrote:

...

> > P.S. I wouldn't like to see similar patches to other GPIO drivers,
> > especially those that use a / 8; a % 8 type of expressions together.
> 
> I understand your points about the familiarity of line grouping in GPIO
> hardware and the optimization behavior of compilers for power-of-two
> division. I initially thought this could be a good fix as I have seen
> similar changes before.

It really depends on the use case. In some it's good to update like you
proposed, but GPIO drivers (hardware) are special in this sense.

> Thank you for the feedback—I will keep this in mind
> before submitting such patches in the future.

You're welcome! Feel free to Cc me for review, if in doubts.

-- 
With Best Regards,
Andy Shevchenko