LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
@ 2019-02-19 18:21 Nathan Chancellor
  2019-02-19 19:05 ` Nick Desaulniers
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Nathan Chancellor @ 2019-02-19 18:21 UTC (permalink / raw)
  To: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, Nick Desaulniers,
	Nathan Chancellor

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(-)

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-19 18:21 [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp Nathan Chancellor
@ 2019-02-19 19:05 ` Nick Desaulniers
  2019-02-20  9:39   ` Luca Coelho
  2019-02-20 10:51 ` Arnd Bergmann
  2019-02-21  8:06 ` [PATCH v2] iwlwifi: mvm: Use div_s64 " Nathan Chancellor
  2 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2019-02-19 19:05 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo, linux-wireless, netdev, LKML

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-19 19:05 ` Nick Desaulniers
@ 2019-02-20  9:39   ` Luca Coelho
  0 siblings, 0 replies; 10+ messages in thread
From: Luca Coelho @ 2019-02-20  9:39 UTC (permalink / raw)
  To: Nick Desaulniers, Nathan Chancellor
  Cc: Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless,
	Kalle Valo, linux-wireless, netdev, LKML

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-19 18:21 [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp Nathan Chancellor
  2019-02-19 19:05 ` Nick Desaulniers
@ 2019-02-20 10:51 ` Arnd Bergmann
  2019-02-20 17:56   ` Nathan Chancellor
  2019-02-21  8:06 ` [PATCH v2] iwlwifi: mvm: Use div_s64 " Nathan Chancellor
  2 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2019-02-20 10:51 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo, linux-wireless, Networking,
	Linux Kernel Mailing List, Nick Desaulniers

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-20 10:51 ` Arnd Bergmann
@ 2019-02-20 17:56   ` Nathan Chancellor
  2019-02-21  7:33     ` Luciano Coelho
  0 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2019-02-20 17:56 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo, linux-wireless, Networking,
	Linux Kernel Mailing List, Nick Desaulniers

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-20 17:56   ` Nathan Chancellor
@ 2019-02-21  7:33     ` Luciano Coelho
  0 siblings, 0 replies; 10+ messages in thread
From: Luciano Coelho @ 2019-02-21  7:33 UTC (permalink / raw)
  To: Nathan Chancellor, Arnd Bergmann
  Cc: Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless,
	Kalle Valo, linux-wireless, Networking,
	Linux Kernel Mailing List, Nick Desaulniers

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-19 18:21 [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp Nathan Chancellor
  2019-02-19 19:05 ` Nick Desaulniers
  2019-02-20 10:51 ` Arnd Bergmann
@ 2019-02-21  8:06 ` Nathan Chancellor
  2019-02-22  0:13   ` Nick Desaulniers
  2 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2019-02-21  8:06 UTC (permalink / raw)
  To: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, Nick Desaulniers,
	Nathan Chancellor, Arnd Bergmann

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(-)

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-21  8:06 ` [PATCH v2] iwlwifi: mvm: Use div_s64 " Nathan Chancellor
@ 2019-02-22  0:13   ` Nick Desaulniers
  2019-02-22  7:45     ` Luciano Coelho
  2019-02-22  8:52     ` Arnd Bergmann
  0 siblings, 2 replies; 10+ messages in thread
From: Nick Desaulniers @ 2019-02-22  0:13 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo, linux-wireless, netdev, LKML,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-22  0:13   ` Nick Desaulniers
@ 2019-02-22  7:45     ` Luciano Coelho
  2019-02-22  8:52     ` Arnd Bergmann
  1 sibling, 0 replies; 10+ messages in thread
From: Luciano Coelho @ 2019-02-22  7:45 UTC (permalink / raw)
  To: Nick Desaulniers, Nathan Chancellor
  Cc: Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless,
	Kalle Valo, linux-wireless, netdev, LKML, Arnd Bergmann

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
  2019-02-22  0:13   ` Nick Desaulniers
  2019-02-22  7:45     ` Luciano Coelho
@ 2019-02-22  8:52     ` Arnd Bergmann
  1 sibling, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2019-02-22  8:52 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
	Intel Linux Wireless, Kalle Valo, linux-wireless, Networking,
	LKML

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-02-22  8:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-19 18:21 [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp Nathan Chancellor
2019-02-19 19:05 ` Nick Desaulniers
2019-02-20  9:39   ` Luca Coelho
2019-02-20 10:51 ` Arnd Bergmann
2019-02-20 17:56   ` Nathan Chancellor
2019-02-21  7:33     ` Luciano Coelho
2019-02-21  8:06 ` [PATCH v2] iwlwifi: mvm: Use div_s64 " Nathan Chancellor
2019-02-22  0:13   ` Nick Desaulniers
2019-02-22  7:45     ` Luciano Coelho
2019-02-22  8:52     ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).