Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp

From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Chancellor <natechancellor@gmail.com>
Cc: Johannes Berg <johannes.berg@intel.com>,
	Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
	Luca Coelho <luciano.coelho@intel.com>,
	Intel Linux Wireless <linuxwifi@intel.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
Date: Tue, 19 Feb 2019 11:05:49 -0800	[thread overview]
Message-ID: <CAKwvOdmFE0kQoxvRDcarT+kYLXxqKYJfVHo4uTiYm7-wDefrrw@mail.gmail.com> (raw)
In-Reply-To: <20190219182105.19933-1-natechancellor@gmail.com>

On Tue, Feb 19, 2019 at 10:21 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2: warning:
> comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka 'long
> long *') and 'uint64_t *' (aka 'unsigned long long *'))
> [-Wcompare-distinct-pointer-types]
>         do_div(rtt_avg, 6666);
>         ^~~~~~~~~~~~~~~~~~~~~
> include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
>         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
>                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
> 1 warning generated.
>
> do_div expects an unsigned dividend. Use div64_s64, which expects a
> signed dividend.

Eh, IIRC, signed vs unsigned division has implications for rounding
towards zero or not, but I doubt that the round trip time average (RTT
avg) should ever be negative.  General rule of thumb for C is to keep
arithmetic signed (even when working with non zero values), so rather
than make the literal (6666) a unsigned long, I agree with your change
to keep the division signed as well.  Thanks for the fix.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
> Link: https://github.com/ClangBuiltLinux/linux/issues/372
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> index e9822a3ec373..92b22250eb7d 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> @@ -462,7 +462,7 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
>  {
>         s64 rtt_avg = res->ftm.rtt_avg * 100;
>
> -       do_div(rtt_avg, 6666);
> +       div64_s64(rtt_avg, 6666);
>
>         IWL_DEBUG_INFO(mvm, "entry %d\n", index);
>         IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status);
> --
> 2.21.0.rc1
>

-- 
Thanks,
~Nick Desaulniers