Re: [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()

[Tvrtko Ursulin]

On 18/07/2023 22:44, Nathan Chancellor wrote:
A proposed update to clang's -Wconstant-logical-operand to warn when the
left hand side is a constant shows the following instance in
nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ,
such as CONFIG_HZ=300:

   drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical 
'&&' with constant operand [-Wconstant-logical-operand]
     189 |         if (NSEC_PER_SEC % HZ &&
         |             ~~~~~~~~~~~~~~~~~ ^
   drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise 
operation
     189 |         if (NSEC_PER_SEC % HZ &&
         |                               ^~
         |                               &
   drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to 
silence this warning
   1 warning generated.

Turn this into an explicit comparison against zero to make the
expression a boolean to make it clear this should be a logical check,
not a bitwise one.

So -Wconstant-logical-operand only triggers when it is a constant but 
not zero constant? Why does that make sense is not a kludge to avoid too 
much noise?

Personally, it all feels a bit over the top as a warning,  since code in 
both cases should optimise away. And we may end up papering over it if 
it becomes a default.

Then again this patch IMO does make the code more readable, so I am 
happy to take this one via our tree. Or either give ack to bring it in 
via drm-misc-next:

Acked-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>

Let me know which route works best.

Regards,

Tvrtko

Link: https://reviews.llvm.org/D142609
Signed-off-by: Nathan Chancellor <nat...@kernel.org>
---
  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d4b918fb11ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
  static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
  {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
+       if ((NSEC_PER_SEC % HZ) != 0 &&
            div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;