| Message ID | 20190221080617.2795-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | [v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp | expand |
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 >
On Thu, 2019-02-21 at 16:13 -0800, Nick Desaulniers wrote: > 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> Thanks, guys, I really didn't pay much attention when I applied the previous versions either. I have applied this in our internal tree and will send it out instead of the previous one as part of our upstreaming process. -- Cheers, Luca.
On Fri, Feb 22, 2019 at 1:14 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > On Thu, Feb 21, 2019 at 12:08 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > 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)? I think do_div() is mostly historic, we've had it since the early days when C compilers were not as good with inline functions. The various other versions are regular functions, and I tend to prefer them for new code, but do_div() is widely known and documented, so I have little hope of it going away any time soon. Arnd
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);
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. drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)