| Message ID | 20190219182105.19933-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp | expand |
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 >
On Tue, 2019-02-19 at 11:05 -0800, Nick Desaulniers wrote: > 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> Thanks for the patch and for the review. I've applied this to our internal tree and it will be sent upstreaming following our normal upstreaming process. -- Cheers, Luca.
On Tue, Feb 19, 2019 at 7:22 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > 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); This is wrong: div64_s64 does not modify its argument like do_div(), but it returns the result instead. You also don't want to divide by a 64-bit value when the second argument is a small constant. I think the correct way should be s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666); If you know that the value is positive, using unsigned types and div_u64() would be slightly faster. Arnd
On Wed, Feb 20, 2019 at 11:51:34AM +0100, Arnd Bergmann wrote: > On Tue, Feb 19, 2019 at 7:22 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > > 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); > > This is wrong: div64_s64 does not modify its argument like do_div(), but > it returns the result instead. You also don't want to divide by a 64-bit > value when the second argument is a small constant. > > I think the correct way should be > > s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666); > > If you know that the value is positive, using unsigned types > and div_u64() would be slightly faster. > > Arnd Thanks for the review and explanation, Arnd. Luca, could you drop this version so I can resend it? Nathan
On Wed, 2019-02-20 at 10:56 -0700, Nathan Chancellor wrote: > On Wed, Feb 20, 2019 at 11:51:34AM +0100, Arnd Bergmann wrote: > > On Tue, Feb 19, 2019 at 7:22 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > 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); > > > > This is wrong: div64_s64 does not modify its argument like > > do_div(), but > > it returns the result instead. You also don't want to divide by a > > 64-bit > > value when the second argument is a small constant. > > > > I think the correct way should be > > > > s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666); > > > > If you know that the value is positive, using unsigned types > > and div_u64() would be slightly faster. > > > > Arnd > > Thanks for the review and explanation, Arnd. > > Luca, could you drop this version so I can resend it? Sure, please do! I already applied this internally, but I can just fix it with your new patch and that will be squashed before being sent upstream, so it will look like your second patch. -- Cheers, Luca.
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);
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. 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(-)