| Message ID | 0c710456e4875ff00c1a9fcff9378ed15110dcd3.1425354528.git.joe@perches.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Mon, 2015-03-02 at 19:54 -0800, Joe Perches wrote:
> Use the built-in function instead of memset.
Please don't use <foo> in the title, especially not if the patch only
introduces usage of eth_zero_addr(). It's easier to look for in the
commit log without it.
Other than that, I guess I'll apply this, but I really wish there was a
way to distinguish more easily which of these require alignment and
which don't.
eth_zero_addr() doesn,t but is_zero_ether_addr() does. So does
ether_addr_copy(). Frankly, it's getting a bit confusing, so I can't
really fault anyone for using memset()/memcpy().
johannes
--
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, 2015-03-03 at 09:16 +0100, Johannes Berg wrote: > On Mon, 2015-03-02 at 19:54 -0800, Joe Perches wrote: > > Use the built-in function instead of memset. > > Please don't use <foo> in the title, especially not if the patch only > introduces usage of eth_zero_addr(). It's easier to look for in the > commit log without it. > > Other than that, I guess I'll apply this, but I really wish there was a > way to distinguish more easily which of these require alignment and > which don't. My guess is the eth_zero_addr and eth_broadcast functions are always taking aligned(2) arguments, just like all the is_<foo>_ether_addr functions. > eth_zero_addr() doesn,t but is_zero_ether_addr() does. So does > ether_addr_copy(). Frankly, it's getting a bit confusing, so I can't > really fault anyone for using memset()/memcpy(). I suspect more than anything else all these are historic. -- 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
> > Other than that, I guess I'll apply this, but I really wish there was a > > way to distinguish more easily which of these require alignment and > > which don't. > > My guess is the eth_zero_addr and eth_broadcast functions > are always taking aligned(2) arguments, just like all the > is_<foo>_ether_addr functions. Err, are you serious??? That *clearly* isn't true, and if it was then this patch wouldn't be safe at all. > > eth_zero_addr() doesn,t but is_zero_ether_addr() does. So does > > ether_addr_copy(). Frankly, it's getting a bit confusing, so I can't > > really fault anyone for using memset()/memcpy(). > > I suspect more than anything else all these are historic. I'd expect a mix here, certainly. Not all of them are really old though. johannes -- 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, 2015-03-03 at 09:44 +0100, Johannes Berg wrote: > > > Other than that, I guess I'll apply this, but I really wish there was a > > > way to distinguish more easily which of these require alignment and > > > which don't. > > > > My guess is the eth_zero_addr and eth_broadcast functions > > are always taking aligned(2) arguments, just like all the > > is_<foo>_ether_addr functions. > > Err, are you serious??? Yes. > That *clearly* isn't true, and if it was then > this patch wouldn't be safe at all. And why is that? Until patch 1 of this series, eth_zero_addr and eth_broadcast_addr was just an inline for a memset. Even after patch 1, it's effectively still memset. -- 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, 2015-03-03 at 00:52 -0800, Joe Perches wrote: > > > My guess is the eth_zero_addr and eth_broadcast functions > > > are always taking aligned(2) arguments, just like all the > > > is_<foo>_ether_addr functions. > > > > Err, are you serious??? > > Yes. > > > That *clearly* isn't true, and if it was then > > this patch wouldn't be safe at all. > > And why is that? > > Until patch 1 of this series, eth_zero_addr and > eth_broadcast_addr was just an inline for a memset. > > Even after patch 1, it's effectively still memset. Exactly. It therefore *doesn't* require an aligned(2) argument, unlike what you stated above, hence my question if you're serious (and perhaps looking at some other code that I don't have). My argument/complaint is that it isn't obvious from these which ones do require aligned(2) argument. Therefore, it's not obvious without going back to the definitions where the conversion is safe and where it isn't. Clearly, for example, memcmp() cannot always be replaced with ether_addr_equal(). johannes -- 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, 2015-03-03 at 10:00 +0100, Johannes Berg wrote: > On Tue, 2015-03-03 at 00:52 -0800, Joe Perches wrote: > > > > > My guess is the eth_zero_addr and eth_broadcast functions > > > > are always taking aligned(2) arguments, just like all the > > > > is_<foo>_ether_addr functions. > > > > > > Err, are you serious??? > > > > Yes. > > > > > That *clearly* isn't true, and if it was then > > > this patch wouldn't be safe at all. > > > > And why is that? > > > > Until patch 1 of this series, eth_zero_addr and > > eth_broadcast_addr was just an inline for a memset. > > > > Even after patch 1, it's effectively still memset. > > Exactly. It therefore *doesn't* require an aligned(2) argument, unlike > what you stated above, hence my question if you're serious (and perhaps > looking at some other code that I don't have). Nope, you simply misunderstood what I did write. What I said was that the arguments were likely already aligned(2), not that the alignment was a requirement. -- 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, 2015-03-03 at 02:29 -0800, Joe Perches wrote: > Nope, you simply misunderstood what I did write. > > What I said was that the arguments were likely > already aligned(2), not that the alignment was > a requirement. Fair enough. That's not actually true/guaranteed though as far as this patch is concerned. johannes -- 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
From: Johannes Berg <johannes@sipsolutions.net> Date: Tue, 03 Mar 2015 09:16:57 +0100 > Other than that, I guess I'll apply this, but I really wish there was a > way to distinguish more easily which of these require alignment and > which don't. You can't apply "this" without the dependency patch #1. Therefore this should probably all go through my tree. -- 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, 2015-03-03 at 13:57 -0500, David Miller wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Tue, 03 Mar 2015 09:16:57 +0100 > > > Other than that, I guess I'll apply this, but I really wish there was a > > way to distinguish more easily which of these require alignment and > > which don't. > > You can't apply "this" without the dependency patch #1. > > Therefore this should probably all go through my tree. Hey David. The eth_<foo>_addr functions already exist so 1/14 isn't a dependency. It's just a trivial improvement on existing code with CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and at least the arm 4.6.3 compiler. -- 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, 2015-03-03 at 13:57 -0500, David Miller wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Tue, 03 Mar 2015 09:16:57 +0100 > > > Other than that, I guess I'll apply this, but I really wish there was a > > way to distinguish more easily which of these require alignment and > > which don't. > > You can't apply "this" without the dependency patch #1. Actually, this is the first time I see patch #1, but since it depends on HAVE_EFFICIENT_UNALIGNED_ACCESS it doesn't really matter, the functions already exist. I'm not even sure that the memset in patch #1 really gets more efficient with the u32/u16 write (and if it does, why doesn't the compiler know it) so I guess I'm not even sure I see much point in patch #1, but that doesn't really matter. Anyway, I don't mind if you want to take this directly either, just let me know. johannes -- 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
From: Joe Perches <joe@perches.com> Date: Tue, 03 Mar 2015 11:03:17 -0800 > On Tue, 2015-03-03 at 13:57 -0500, David Miller wrote: >> From: Johannes Berg <johannes@sipsolutions.net> >> Date: Tue, 03 Mar 2015 09:16:57 +0100 >> >> > Other than that, I guess I'll apply this, but I really wish there was a >> > way to distinguish more easily which of these require alignment and >> > which don't. >> >> You can't apply "this" without the dependency patch #1. >> >> Therefore this should probably all go through my tree. > > Hey David. > > The eth_<foo>_addr functions already exist so > 1/14 isn't a dependency. > > It's just a trivial improvement on existing code > with CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and > at least the arm 4.6.3 compiler. You're right. But I think we're skipping this series for now, see my other reply. If GCC can't emit as good an inline memset as we can come up with for 6 byte constant lengths, that's a GCC bug that should be fixed. -- 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/net/wireless/ibss.c b/net/wireless/ibss.c index e24fc58..6309b9c 100644 --- a/net/wireless/ibss.c +++ b/net/wireless/ibss.c @@ -533,7 +533,7 @@ int cfg80211_ibss_wext_giwap(struct net_device *dev, else if (wdev->wext.ibss.bssid) memcpy(ap_addr->sa_data, wdev->wext.ibss.bssid, ETH_ALEN); else - memset(ap_addr->sa_data, 0, ETH_ALEN); + eth_zero_addr(ap_addr->sa_data); wdev_unlock(wdev); diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index d78fd8b..96fe328 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -5683,8 +5683,8 @@ static int nl80211_parse_random_mac(struct nlattr **attrs, int i; if (!attrs[NL80211_ATTR_MAC] && !attrs[NL80211_ATTR_MAC_MASK]) { - memset(mac_addr, 0, ETH_ALEN); - memset(mac_addr_mask, 0, ETH_ALEN); + eth_zero_addr(mac_addr); + eth_zero_addr(mac_addr_mask); mac_addr[0] = 0x2; mac_addr_mask[0] = 0x3; diff --git a/net/wireless/trace.h b/net/wireless/trace.h index b17b369..a00ee88 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -7,6 +7,7 @@ #include <linux/tracepoint.h> #include <linux/rtnetlink.h> +#include <linux/etherdevice.h> #include <net/cfg80211.h> #include "core.h" @@ -15,7 +16,7 @@ if (given_mac) \ memcpy(__entry->entry_mac, given_mac, ETH_ALEN); \ else \ - memset(__entry->entry_mac, 0, ETH_ALEN); \ + eth_zero_addr(__entry->entry_mac); \ } while (0) #define MAC_PR_FMT "%pM" #define MAC_PR_ARG(entry_mac) (__entry->entry_mac) @@ -1077,7 +1078,7 @@ TRACE_EVENT(rdev_auth, if (req->bss) MAC_ASSIGN(bssid, req->bss->bssid); else - memset(__entry->bssid, 0, ETH_ALEN); + eth_zero_addr(__entry->bssid); __entry->auth_type = req->auth_type; ), TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", auth type: %d, bssid: " MAC_PR_FMT, @@ -1103,7 +1104,7 @@ TRACE_EVENT(rdev_assoc, if (req->bss) MAC_ASSIGN(bssid, req->bss->bssid); else - memset(__entry->bssid, 0, ETH_ALEN); + eth_zero_addr(__entry->bssid); MAC_ASSIGN(prev_bssid, req->prev_bssid); __entry->use_mfp = req->use_mfp; __entry->flags = req->flags; @@ -1153,7 +1154,7 @@ TRACE_EVENT(rdev_disassoc, if (req->bss) MAC_ASSIGN(bssid, req->bss->bssid); else - memset(__entry->bssid, 0, ETH_ALEN); + eth_zero_addr(__entry->bssid); __entry->reason_code = req->reason_code; __entry->local_state_change = req->local_state_change; ), diff --git a/net/wireless/wext-sme.c b/net/wireless/wext-sme.c index 368611c..a4e8af3 100644 --- a/net/wireless/wext-sme.c +++ b/net/wireless/wext-sme.c @@ -322,7 +322,7 @@ int cfg80211_mgd_wext_giwap(struct net_device *dev, if (wdev->current_bss) memcpy(ap_addr->sa_data, wdev->current_bss->pub.bssid, ETH_ALEN); else - memset(ap_addr->sa_data, 0, ETH_ALEN); + eth_zero_addr(ap_addr->sa_data); wdev_unlock(wdev); return 0;
Use the built-in function instead of memset. Signed-off-by: Joe Perches <joe@perches.com> --- net/wireless/ibss.c | 2 +- net/wireless/nl80211.c | 4 ++-- net/wireless/trace.h | 9 +++++---- net/wireless/wext-sme.c | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-)