[PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Nicholas Mc Guire]

Converting milliseconds to jiffies by "val * HZ / 1000" is technically
OK but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API consolidation only and
should make things more readable.

Patch was compile tested with x86_64_defconfig + CONFIG_BRCMFMAC=m

Patch is agianst 4.0-rc4 (localversion-next is -next-20150317)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index 9b805c9..1996dc2 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct brcmf_cfg80211_vif *vif,
 
 	/* Arm scan timeout timer */
 	mod_timer(&cfg->escan_timeout, jiffies +
-			WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
+		  msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
 
 	return 0;
 
-- 
1.7.10.4

Re: [PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Joe Perches]

On Tue, 2015-03-17 at 08:06 -0400, Nicholas Mc Guire wrote:
> Converting milliseconds to jiffies by "val * HZ / 1000" is technically
> OK but msecs_to_jiffies(val) is the cleaner solution and handles all
> corner cases correctly. This is a minor API consolidation only and
> should make things more readable.

Hi Nicholas

These API consolidation changes now always have a function
call when the compiler may have previously been able to
optimize out the "constant * HZ / 1000" calculation.

Perhaps the [um]secs_to_jiffies calls should be indirected
with yet another static inline with a __builtin_constant_p()
test so that the function calls can again be avoided when
possible.

(and a trivial style note)

> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
[]
> @@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct brcmf_cfg80211_vif *vif,
>  
>  	/* Arm scan timeout timer */
>  	mod_timer(&cfg->escan_timeout, jiffies +
> -			WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
> +		  msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));

It may be nicer to keep the arithmetic on one line

 	mod_timer(&cfg->escan_timeout,
		  jiffies + msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));

Re: [PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Nicholas Mc Guire]

On Tue, 17 Mar 2015, Joe Perches wrote:

> On Tue, 2015-03-17 at 08:06 -0400, Nicholas Mc Guire wrote:
> > Converting milliseconds to jiffies by "val * HZ / 1000" is technically
> > OK but msecs_to_jiffies(val) is the cleaner solution and handles all
> > corner cases correctly. This is a minor API consolidation only and
> > should make things more readable.
> 
> Hi Nicholas
> 
> These API consolidation changes now always have a function
> call when the compiler may have previously been able to
> optimize out the "constant * HZ / 1000" calculation.
>
> Perhaps the [um]secs_to_jiffies calls should be indirected
> with yet another static inline with a __builtin_constant_p()
> test so that the function calls can again be avoided when
> possible.

will give it a try 

> 
> (and a trivial style note)
> 
> > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
> []
> > @@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct brcmf_cfg80211_vif *vif,
> >  
> >  	/* Arm scan timeout timer */
> >  	mod_timer(&cfg->escan_timeout, jiffies +
> > -			WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
> > +		  msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
> 
> It may be nicer to keep the arithmetic on one line

sorry that was plain carelessness afer it went over 80 char.

> 
>  	mod_timer(&cfg->escan_timeout,
> 		  jiffies + msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
>

thx!
hofrat 

Re: [PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Joe Perches]

On Wed, 2015-03-18 at 07:44 +0100, Nicholas Mc Guire wrote:
> On Tue, 17 Mar 2015, Joe Perches wrote:
> > On Tue, 2015-03-17 at 08:06 -0400, Nicholas Mc Guire wrote:
> > > Converting milliseconds to jiffies by "val * HZ / 1000" is technically
> > > OK but msecs_to_jiffies(val) is the cleaner solution and handles all
> > > corner cases correctly. This is a minor API consolidation only and
> > > should make things more readable.

> > These API consolidation changes now always have a function
> > call when the compiler may have previously been able to
> > optimize out the "constant * HZ / 1000" calculation.
> >
> > Perhaps the [um]secs_to_jiffies calls should be indirected
> > with yet another static inline with a __builtin_constant_p()
> > test so that the function calls can again be avoided when
> > possible.
> 
> will give it a try 

OK, thanks.

The tricky bit seems to be the movement of
the Makefile for kernel/time/timeconst.h.

  BC      kernel/time/timeconst.h

as include/linux/time.h would need these
calculated #defines

#define MSEC_TO_HZ_MUL32	U64_C(0x80000000)
#define MSEC_TO_HZ_ADJ32	U64_C(0x0)
#define MSEC_TO_HZ_SHR32	31

and the usec ones too.

That created file would have to be moved from
kernel/time/Makefile to some other location
that could be in the normal include path like
include/linux/timeconst.h so that it can be
used by include/linux/time.h



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Arend van Spriel]

On 03/17/15 13:06, Nicholas Mc Guire wrote:
> Converting milliseconds to jiffies by "val * HZ / 1000" is technically
> OK but msecs_to_jiffies(val) is the cleaner solution and handles all
> corner cases correctly. This is a minor API consolidation only and
> should make things more readable.
>
> Patch was compile tested with x86_64_defconfig + CONFIG_BRCMFMAC=m
>
> Patch is agianst 4.0-rc4 (localversion-next is -next-20150317)

It applies to wireless-drivers-next/master as well so

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Nicholas Mc Guire<hofrat@osadl.org>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
> index 9b805c9..1996dc2 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
> @@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct brcmf_cfg80211_vif *vif,
>
>   	/* Arm scan timeout timer */
>   	mod_timer(&cfg->escan_timeout, jiffies +
> -			WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
> +		  msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
>
>   	return 0;
>