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 <[email protected]>
---
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
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
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));
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
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 <[email protected]>
> Signed-off-by: Nicholas Mc Guire<[email protected]>
> ---
> 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;
>