| Message ID | 1426593972-17652-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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)); -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)