[PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Fabian Frederick]

This patch also fixes some comment checkpatch warnings

Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 net/wireless/ibss.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index a6b5bda..a0a1381 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -49,7 +49,7 @@ void __cfg80211_ibss_joined(struct net_device *dev, const u8 *bssid,
 				GFP_KERNEL);
 #ifdef CONFIG_CFG80211_WEXT
 	memset(&wrqu, 0, sizeof(wrqu));
-	memcpy(wrqu.ap_addr.sa_data, bssid, ETH_ALEN);
+	ether_addr_copy(wrqu.ap_addr.sa_data, bssid);
 	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
 #endif
 }
@@ -72,7 +72,7 @@ void cfg80211_ibss_joined(struct net_device *dev, const u8 *bssid,
 		return;
 
 	ev->type = EVENT_IBSS_JOINED;
-	memcpy(ev->ij.bssid, bssid, ETH_ALEN);
+	ether_addr_copy(ev->ij.bssid, bssid);
 	ev->ij.channel = channel;
 
 	spin_lock_irqsave(&wdev->event_lock, flags);
@@ -98,11 +98,10 @@ static int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
 		return -EALREADY;
 
 	if (!params->basic_rates) {
-		/*
-		* If no rates were explicitly configured,
-		* use the mandatory rate set for 11b or
-		* 11a for maximum compatibility.
-		*/
+		/* If no rates were explicitly configured,
+		 * use the mandatory rate set for 11b or
+		 * 11a for maximum compatibility.
+		 */
 		struct ieee80211_supported_band *sband =
 			rdev->wiphy.bands[params->chandef.chan->band];
 		int j;
@@ -190,8 +189,7 @@ static void __cfg80211_clear_ibss(struct net_device *dev, bool nowext)
 
 	rdev_set_qos_map(rdev, dev, NULL);
 
-	/*
-	 * Delete all the keys ... pairwise keys can't really
+	/* Delete all the keys ... pairwise keys can't really
 	 * exist any more anyway, but default keys might.
 	 */
 	if (rdev->ops->del_key)
@@ -524,7 +522,7 @@ int cfg80211_ibss_wext_siwap(struct net_device *dev,
 		return err;
 
 	if (bssid) {
-		memcpy(wdev->wext.bssid, bssid, ETH_ALEN);
+		ether_addr_copy(wdev->wext.bssid, bssid);
 		wdev->wext.ibss.bssid = wdev->wext.bssid;
 	} else
 		wdev->wext.ibss.bssid = NULL;
@@ -550,9 +548,9 @@ int cfg80211_ibss_wext_giwap(struct net_device *dev,
 
 	wdev_lock(wdev);
 	if (wdev->current_bss)
-		memcpy(ap_addr->sa_data, wdev->current_bss->pub.bssid, ETH_ALEN);
+		ether_addr_copy(ap_addr->sa_data, wdev->current_bss->pub.bssid);
 	else if (wdev->wext.ibss.bssid)
-		memcpy(ap_addr->sa_data, wdev->wext.ibss.bssid, ETH_ALEN);
+		ether_addr_copy(ap_addr->sa_data, wdev->wext.ibss.bssid);
 	else
 		memset(ap_addr->sa_data, 0, ETH_ALEN);
 
-- 
1.9.1

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Joe Perches]

On Mon, 2014-05-12 at 19:30 +0200, Fabian Frederick wrote:
> This patch also fixes some comment checkpatch warnings

Hello Fabian.

For all the patches that replace memcpy(foo, bar, ETH_ALEN)
with ether_addr_copy, did you use a tool to verify both
arguments are __aligned(2) or did you do the verification
visually?

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Fabian Frederick]

On Mon, 12 May 2014 10:50:25 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-05-12 at 19:30 +0200, Fabian Frederick wrote:
> > This patch also fixes some comment checkpatch warnings
> 
> Hello Fabian.
> 
> For all the patches that replace memcpy(foo, bar, ETH_ALEN)
> with ether_addr_copy, did you use a tool to verify both
> arguments are __aligned(2) or did you do the verification
> visually?

Hello Joe,

I only replaced ETH_ALEN/memcpy .
AFAICS ETH_ALEN is defined 6 ...

Fabian

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Joe Perches]

On Mon, 2014-05-12 at 20:00 +0200, Fabian Frederick wrote:
> On Mon, 12 May 2014 10:50:25 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Mon, 2014-05-12 at 19:30 +0200, Fabian Frederick wrote:
> > > This patch also fixes some comment checkpatch warnings
> > 
> > Hello Fabian.
> > 
> > For all the patches that replace memcpy(foo, bar, ETH_ALEN)
> > with ether_addr_copy, did you use a tool to verify both
> > arguments are __aligned(2) or did you do the verification
> > visually?
> 
> Hello Joe,
> 
> I only replaced ETH_ALEN/memcpy .
> AFAICS ETH_ALEN is defined 6 ...

The difference here is that both arguments to
ether_addr_copy, like all the is_<foo>_ether_addr
helpers, must be __aligned(2).  memcpy has
no alignment requirement.

Please verify that all these changes are to
__aligned(2) arguments.

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Johannes Berg]

On Mon, 2014-05-12 at 11:07 -0700, Joe Perches wrote:
> On Mon, 2014-05-12 at 20:00 +0200, Fabian Frederick wrote:
> > On Mon, 12 May 2014 10:50:25 -0700
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > On Mon, 2014-05-12 at 19:30 +0200, Fabian Frederick wrote:
> > > > This patch also fixes some comment checkpatch warnings
> > > 
> > > Hello Fabian.
> > > 
> > > For all the patches that replace memcpy(foo, bar, ETH_ALEN)
> > > with ether_addr_copy, did you use a tool to verify both
> > > arguments are __aligned(2) or did you do the verification
> > > visually?
> > 
> > Hello Joe,
> > 
> > I only replaced ETH_ALEN/memcpy .
> > AFAICS ETH_ALEN is defined 6 ...
> 
> The difference here is that both arguments to
> ether_addr_copy, like all the is_<foo>_ether_addr
> helpers, must be __aligned(2).  memcpy has
> no alignment requirement.
> 
> Please verify that all these changes are to
> __aligned(2) arguments.

Seriously though, who cares. Only two of these patches really touch
paths where performance matters - and one of those is the lib80211 one
which is practically only used for certain ancient Intel devices, which
probably don't run on anything but IA where I'd guess the whole thing
doesn't really matter anyway.

I certainly don't see the benefit in changing all those other files,
particularly since it's not just that we have to verify alignment *now*,
we also have to add alignment attributes so that we don't break
alignment in the future.

Additionally doesn't even really save much typing:

memcpy(x, y, ETH_ALEN);
ether_addr_copy(x, y);

Finally, some of these patches are doing comment reformatting, which
clearly is out of scope for them.

As a consequence, I'm considering the net/wireless/util.c one, but none
of the others.

johannes

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Joe Perches]

On Mon, 2014-05-12 at 20:51 +0200, Johannes Berg wrote:
> On Mon, 2014-05-12 at 11:07 -0700, Joe Perches wrote:
> > On Mon, 2014-05-12 at 20:00 +0200, Fabian Frederick wrote:
> > > On Mon, 12 May 2014 10:50:25 -0700
> > > Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2014-05-12 at 19:30 +0200, Fabian Frederick wrote:
> > > > > This patch also fixes some comment checkpatch warnings
> > > > For all the patches that replace memcpy(foo, bar, ETH_ALEN)
> > > > with ether_addr_copy, did you use a tool to verify both
> > > > arguments are __aligned(2) or did you do the verification
> > > > visually?
> > > I only replaced ETH_ALEN/memcpy .
> > > AFAICS ETH_ALEN is defined 6 ...
> > 
> > The difference here is that both arguments to
> > ether_addr_copy, like all the is_<foo>_ether_addr
> > helpers, must be __aligned(2).  memcpy has
> > no alignment requirement.
> > 
> > Please verify that all these changes are to
> > __aligned(2) arguments.
> 
> Seriously though, who cares. Only two of these patches really touch
> paths where performance matters - and one of those is the lib80211 one
> which is practically only used for certain ancient Intel devices, which
> probably don't run on anything but IA where I'd guess the whole thing
> doesn't really matter anyway.
> 
> I certainly don't see the benefit in changing all those other files,
> particularly since it's not just that we have to verify alignment *now*,
> we also have to add alignment attributes so that we don't break
> alignment in the future.

The same alignment requirements exist in quite a
lot of the code so I don't see that as much of a
continuing problem.

It's more of an initial conversion problem.

> Additionally doesn't even really save much typing:
> 
> memcpy(x, y, ETH_ALEN);
> ether_addr_copy(x, y);

To me the general benefit isn't in the reduced
source code size, but small improvements for
ARM both in code size and execution speed.

I'm not sure it's worth even the initial conversion
verification costs though.  It's quite tedious to
go through the call trees and I don't know of an
automated way to do that.

Re: [PATCH 1/1] net/wireless/ibss.c: replace memcpy by ether_addr_copy

[Johannes Berg]

On Mon, 2014-05-12 at 12:17 -0700, Joe Perches wrote:

> > I certainly don't see the benefit in changing all those other files,
> > particularly since it's not just that we have to verify alignment *now*,
> > we also have to add alignment attributes so that we don't break
> > alignment in the future.
> 
> The same alignment requirements exist in quite a
> lot of the code so I don't see that as much of a
> continuing problem.

I disagree. This may be true for some of the cases we're discussing,
e.g. the change in net/wireless/util.c which has the assumption based on
frame alignment assumptions etc.

However, for things like the mac80211/mlme.c change we're operating
entirely on internal structures that are purely in software and don't
belong to any network frames or similar structs that already have
alignment guarantees.

As a consequence, something like this simple patch:

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5c7169b0ac57..fccbba2f4fe1 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1529,7 +1529,7 @@ struct cfg80211_bss {
 
 	u16 beacon_interval;
 	u16 capability;
-
+	u8 my_new_field;
 	u8 bssid[ETH_ALEN];
 
 	u8 priv[0] __aligned(sizeof(void *));

would already break at least one of the patches. The change looks
innocent enough, and is modifying a purely internal struct that doesn't
get used anywhere but the wireless stack (neither wire frame nor
userspace) so this is a perfectly valid change.

Therefore, the patch should mark the bssid with __aligned(2) or such, to
avoid having simple patches like that introduce problems.



> > Additionally doesn't even really save much typing:
> > 
> > memcpy(x, y, ETH_ALEN);
> > ether_addr_copy(x, y);
> 
> To me the general benefit isn't in the reduced
> source code size, but small improvements for
> ARM both in code size and execution speed.

Well, yes, but if it's in a configuration path, which you invoke maybe a
few times an hour, it doesn't seem worth it at all.

If it's in a frame/data hotpath, like the util.c change that already has
other alignment guarantees, it does seem somewhat worthwhile, but beyond
that I'm not so sure.

johannes