Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190219182105.19933-1-natechancellor@gmail.com> <20190221080617.2795-1-natechancellor@gmail.com>
In-Reply-To: <20190221080617.2795-1-natechancellor@gmail.com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 21 Feb 2019 16:13:42 -0800
Message-ID: <CAKwvOdmCeHxiGPkXXzjwnTtgzEDDjYRE1QBdu7BHeuzCE4YtRA@mail.gmail.com>
Subject: Re: [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
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>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 21, 2019 at 12:08 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 div_s64, which expects a signed
> dividend.
>
> Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
> Link: https://github.com/ClangBuiltLinux/linux/issues/372
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Fix logic (as the return value of div{,64}_s64 must be used), thanks
>   to Arnd for the review.

oh boy, sorry I missed that in the initial code review, thanks Arnd
for the sharp eye!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Side tangent: we see this kind of difference in APIs a lot (modifying
the parameter vs returning a new value or making a copy then modifying
that) in C++ when a call site isn't passing the explicit address of
some variable or an identifier that's clearly a pointer. Ex.

int foo;
bar(foo);

Doesn't tell you whether bar mutates foo or not without looking at the
definition of bar, as it could be:
void bar(int x);
or
void bar(int& x);

I miss the convention in Ruby of using `!` suffixes on methods to
differentiate between such cases. ex:

"hello".capitalize
vs
"hello".capitalize!

both return the same value, but the one with the ! mutates the
existing object, while the one without creates a new object.  And
that's a very standard convention throughout the standard library.
Whether or not people follow that convention is always another story.

One thing I'm curious about, is "why does do_div exist?" When should I
use do_div vs div_u64 (not div_s64 as is used in this patch)?

>
>  drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> index e9822a3ec373..94132cfd1f56 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> @@ -460,9 +460,7 @@ static int iwl_mvm_ftm_range_resp_valid(struct iwl_mvm *mvm, u8 request_id,
>  static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
>                                      struct cfg80211_pmsr_result *res)
>  {
> -       s64 rtt_avg = res->ftm.rtt_avg * 100;
> -
> -       do_div(rtt_avg, 6666);
> +       s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 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