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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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.ursulin@intel.com>

Let me know which route works best.

Regards,

Tvrtko

> Link: https://reviews.llvm.org/D142609
> Signed-off-by: Nathan Chancellor <nathan@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;
>   
>