| Message ID | 20241112201659.16785-1-surajsonawane0215@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-exar: replace division condition with direct comparison | expand |
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!
+ 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.
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
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); }
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(-)