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

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <090f78b1-88c9-44e5-959f-07b4f97cd1bc@gmail.com>
Date: Fri, 15 Nov 2024 18:55:41 +0530
From: Suraj Sonawane <surajsonawane0215@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Dan Carpenter <dan.carpenter@...aro.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: gpio-exar: replace division condition with direct
 comparison

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