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