Re: [PATCH 3/11] mac80211: use ether_addr_equal_64bits

Re: [PATCH 3/11] mac80211: use ether_addr_equal_64bits

[Christian Lamparter]

Subject: p54: use ether_addr_equal_64bits

On Monday, December 30, 2013 07:14:59 PM you wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.
> 
> The structures involved are:
> ieee80211_hdr defined in include/linux/ieee80211.h and
> p54_common defined in drivers/net/wireless/p54/p54.h
> 
> This was done using Coccinelle (http://coccinelle.lip6.fr/).
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Alright, I changed the subject to "p54: ..." (John, can you do that, or do
you want another patch?). Since this has not much to do with mac80211. 

Acked-by: Christian Lamparter <chunkeey@googlemail.com>

Regards,

Christian

Re: [PATCH 11/11] carl9170: use ether_addr_equal_64bits

[Christian Lamparter]

On Monday, December 30, 2013 07:15:07 PM Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.
> 
> The structures involved are:
> ieee80211_hdr defined in include/linux/ieee80211.h,
> ieee80211_bar defined in include/linux/ieee80211.h and
> ath_common defined in drivers/net/wireless/ath/ath.h
> 
> This was done using Coccinelle (http://coccinelle.lip6.fr/).
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Acked-by: Christian Lamparter <chunkeey@googlemail.com>

[PATCH 0/11] use ether_addr_equal_64bits

[Julia Lawall]

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The semantic patch that makes this change is as follows.  To give
moderately good results, this semantic patch requires the spatch options
--recursive-includes and --relax-include-path.  These options make spatch
quite slow.  The results are also very incomplete.  In particular, there is
no result if the structure definition cannot be found or if one of the
arguments is a pointer.  The ocaml code prints some information justifying
the transformation.

// <smpl>
@pre@
expression *e;
position p;
@@

ether_addr_equal@p(...,e,...)

@r@
identifier T;
struct T eth;
identifier fld;
identifier T1;
struct T1 eth1;
identifier fld1;
position p!=pre.p;
@@

ether_addr_equal@p(eth.fld, eth1.fld1)

@ok@
identifier r.T;
type t1, t2;
identifier r.fld, fld2;
position p;
@@

struct T@p { ...
    t1 fld[...];
    t2 fld2;
    ...
  };

@ok1@
identifier r.T1;
type t1, t2;
identifier r.fld1, fld2;
position p1;
@@

struct T1@p1 { ...
    t1 fld1[...];
    t2 fld2;
    ...
  };

@script:ocaml@
rp << r.p;
okp << ok.p;
okp1 << ok1.p1;
t << r.T;
t1 << r.T1;
@@

let rp = List.hd rp in
let okp = List.hd okp in
let okp1 = List.hd okp1 in
Printf.printf "## %s: struct %s defined in %s\n" rp.file t okp.file;
Printf.printf "## %s: struct %s defined in %s\n" rp.file t1 okp1.file;
flush stdout

@depends on ok && ok1@
position r.p;
@@

-ether_addr_equal@p
+ether_addr_equal_64bits
  (...)
// </smpl>

[PATCH 1/11] rt2x00: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_bar defined in include/linux/ieee80211.h and
rt2x00_bar_list_entry defined in drivers/net/wireless/rt2x00/rt2x00.h.

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/rt2x00/rt2x00dev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 00c3fae..2bde672 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -565,10 +565,10 @@ static void rt2x00lib_rxdone_check_ba(struct rt2x00_dev *rt2x00dev,
 
 #undef TID_CHECK
 
-		if (!ether_addr_equal(ba->ra, entry->ta))
+		if (!ether_addr_equal_64bits(ba->ra, entry->ta))
 			continue;
 
-		if (!ether_addr_equal(ba->ta, entry->ra))
+		if (!ether_addr_equal_64bits(ba->ta, entry->ra))
 			continue;
 
 		/* Mark BAR since we received the according BA */

[PATCH 2/11] ath5k: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_mgmt defined in include/linux/ieee80211.h and
ath_common defined in drivers/net/wireless/ath/ath.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/ath/ath5k/base.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 69f58b0..6396ad4 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1245,7 +1245,7 @@ ath5k_check_ibss_tsf(struct ath5k_hw *ah, struct sk_buff *skb,
 
 	if (ieee80211_is_beacon(mgmt->frame_control) &&
 	    le16_to_cpu(mgmt->u.beacon.capab_info) & WLAN_CAPABILITY_IBSS &&
-	    ether_addr_equal(mgmt->bssid, common->curbssid)) {
+	    ether_addr_equal_64bits(mgmt->bssid, common->curbssid)) {
 		/*
 		 * Received an IBSS beacon with the same BSSID. Hardware *must*
 		 * have updated the local TSF. We have to work around various
@@ -1309,7 +1309,7 @@ ath5k_update_beacon_rssi(struct ath5k_hw *ah, struct sk_buff *skb, int rssi)
 
 	/* only beacons from our BSSID */
 	if (!ieee80211_is_beacon(mgmt->frame_control) ||
-	    !ether_addr_equal(mgmt->bssid, common->curbssid))
+	    !ether_addr_equal_64bits(mgmt->bssid, common->curbssid))
 		return;
 
 	ewma_add(&ah->ah_beacon_rssi_avg, rssi);

[PATCH 3/11] mac80211: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h and
p54_common defined in drivers/net/wireless/p54/p54.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/p54/txrx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/p54/txrx.c b/drivers/net/wireless/p54/txrx.c
index f95de0d..b7cc7c2 100644
--- a/drivers/net/wireless/p54/txrx.c
+++ b/drivers/net/wireless/p54/txrx.c
@@ -308,7 +308,7 @@ static void p54_pspoll_workaround(struct p54_common *priv, struct sk_buff *skb)
 		return;
 
 	/* only consider beacons from the associated BSSID */
-	if (!ether_addr_equal(hdr->addr3, priv->bssid))
+	if (!ether_addr_equal_64bits(hdr->addr3, priv->bssid))
 		return;
 
 	tim = p54_find_ie(skb, WLAN_EID_TIM);

[PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
iwl_rxon_cmd defined in drivers/net/wireless/iwlwifi/dvm/commands.h and
ieee80211_hdr defined in include/linux/ieee80211.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/iwlwifi/dvm/rx.c   |    4 ++--
 drivers/net/wireless/iwlwifi/dvm/rxon.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/dvm/rxon.c b/drivers/net/wireless/iwlwifi/dvm/rxon.c
index d7ce2f1..0439dd5 100644
--- a/drivers/net/wireless/iwlwifi/dvm/rxon.c
+++ b/drivers/net/wireless/iwlwifi/dvm/rxon.c
@@ -875,10 +875,10 @@ static int iwl_full_rxon_required(struct iwl_priv *priv,
 
 	/* These items are only settable from the full RXON command */
 	CHK(!iwl_is_associated_ctx(ctx));
-	CHK(!ether_addr_equal(staging->bssid_addr, active->bssid_addr));
-	CHK(!ether_addr_equal(staging->node_addr, active->node_addr));
-	CHK(!ether_addr_equal(staging->wlap_bssid_addr,
-			      active->wlap_bssid_addr));
+	CHK(!ether_addr_equal_64bits(staging->bssid_addr, active->bssid_addr));
+	CHK(!ether_addr_equal_64bits(staging->node_addr, active->node_addr));
+	CHK(!ether_addr_equal_64bits(staging->wlap_bssid_addr,
+				     active->wlap_bssid_addr));
 	CHK_NEQ(staging->dev_type, active->dev_type);
 	CHK_NEQ(staging->channel, active->channel);
 	CHK_NEQ(staging->air_propagation, active->air_propagation);
diff --git a/drivers/net/wireless/iwlwifi/dvm/rx.c b/drivers/net/wireless/iwlwifi/dvm/rx.c
index d71776d..d18570b 100644
--- a/drivers/net/wireless/iwlwifi/dvm/rx.c
+++ b/drivers/net/wireless/iwlwifi/dvm/rx.c
@@ -780,8 +780,8 @@ static void iwlagn_pass_packet_to_mac80211(struct iwl_priv *priv,
 	*/
 	if (unlikely(ieee80211_is_beacon(fc) && priv->passive_no_rx)) {
 		for_each_context(priv, ctx) {
-			if (!ether_addr_equal(hdr->addr3,
-					      ctx->active.bssid_addr))
+			if (!ether_addr_equal_64bits(hdr->addr3,
+						     ctx->active.bssid_addr))
 				continue;
 			iwlagn_lift_passive_no_rx(priv);
 		}

[PATCH 5/11] mwl8k: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h and
mwl8k_priv defined in drivers/net/wireless/mwl8k.c

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/mwl8k.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index b953ad6..13c9ac5 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1258,7 +1258,7 @@ mwl8k_capture_bssid(struct mwl8k_priv *priv, struct ieee80211_hdr *wh)
 {
 	return priv->capture_beacon &&
 		ieee80211_is_beacon(wh->frame_control) &&
-		ether_addr_equal(wh->addr3, priv->capture_bssid);
+		ether_addr_equal_64bits(wh->addr3, priv->capture_bssid);
 }
 
 static inline void mwl8k_save_beacon(struct ieee80211_hw *hw,

[PATCH 6/11] rtlwifi: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h and
rtl_mac defined in drivers/net/wireless/rtlwifi/wifi.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/rtlwifi/base.c |    4 ++--
 drivers/net/wireless/rtlwifi/ps.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rtlwifi/ps.c b/drivers/net/wireless/rtlwifi/ps.c
index 0d81f76..deedae3 100644
--- a/drivers/net/wireless/rtlwifi/ps.c
+++ b/drivers/net/wireless/rtlwifi/ps.c
@@ -478,7 +478,7 @@ void rtl_swlps_beacon(struct ieee80211_hw *hw, void *data, unsigned int len)
 		return;
 
 	/* and only beacons from the associated BSSID, please */
-	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
+	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
 		return;
 
 	rtlpriv->psc.last_beacon = jiffies;
@@ -923,7 +923,7 @@ void rtl_p2p_info(struct ieee80211_hw *hw, void *data, unsigned int len)
 		return;
 
 	/* and only beacons from the associated BSSID, please */
-	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
+	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
 		return;
 
 	/* check if this really is a beacon */
diff --git a/drivers/net/wireless/rtlwifi/base.c b/drivers/net/wireless/rtlwifi/base.c
index fcf9b62..d63a12c 100644
--- a/drivers/net/wireless/rtlwifi/base.c
+++ b/drivers/net/wireless/rtlwifi/base.c
@@ -1293,7 +1293,7 @@ void rtl_beacon_statistic(struct ieee80211_hw *hw, struct sk_buff *skb)
 		return;
 
 	/* and only beacons from the associated BSSID, please */
-	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
+	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
 		return;
 
 	rtlpriv->link_info.bcn_rx_inperiod++;
@@ -1781,7 +1781,7 @@ void rtl_recognize_peer(struct ieee80211_hw *hw, u8 *data, unsigned int len)
 		return;
 
 	/* and only beacons from the associated BSSID, please */
-	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
+	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
 		return;
 
 	if (rtl_find_221_ie(hw, data, len))

[PATCH 7/11] iwlegacy: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h,
il_priv defined in drivers/net/wireless/iwlegacy/common.h and
il_rxon_cmd defined in drivers/net/wireless/iwlegacy/commands.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/iwlegacy/3945.c   |    4 ++--
 drivers/net/wireless/iwlegacy/common.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/iwlegacy/3945.c b/drivers/net/wireless/iwlegacy/3945.c
index f09e257..27e543c 100644
--- a/drivers/net/wireless/iwlegacy/3945.c
+++ b/drivers/net/wireless/iwlegacy/3945.c
@@ -466,10 +466,10 @@ il3945_is_network_packet(struct il_priv *il, struct ieee80211_hdr *header)
 	switch (il->iw_mode) {
 	case NL80211_IFTYPE_ADHOC:	/* Header: Dest. | Source    | BSSID */
 		/* packets to our IBSS update information */
-		return ether_addr_equal(header->addr3, il->bssid);
+		return ether_addr_equal_64bits(header->addr3, il->bssid);
 	case NL80211_IFTYPE_STATION:	/* Header: Dest. | AP{BSSID} | Source */
 		/* packets to our IBSS update information */
-		return ether_addr_equal(header->addr2, il->bssid);
+		return ether_addr_equal_64bits(header->addr2, il->bssid);
 	default:
 		return 1;
 	}
diff --git a/drivers/net/wireless/iwlegacy/common.c b/drivers/net/wireless/iwlegacy/common.c
index a27b14c..da4d24d 100644
--- a/drivers/net/wireless/iwlegacy/common.c
+++ b/drivers/net/wireless/iwlegacy/common.c
@@ -3746,10 +3746,10 @@ il_full_rxon_required(struct il_priv *il)
 
 	/* These items are only settable from the full RXON command */
 	CHK(!il_is_associated(il));
-	CHK(!ether_addr_equal(staging->bssid_addr, active->bssid_addr));
-	CHK(!ether_addr_equal(staging->node_addr, active->node_addr));
-	CHK(!ether_addr_equal(staging->wlap_bssid_addr,
-			      active->wlap_bssid_addr));
+	CHK(!ether_addr_equal_64bits(staging->bssid_addr, active->bssid_addr));
+	CHK(!ether_addr_equal_64bits(staging->node_addr, active->node_addr));
+	CHK(!ether_addr_equal_64bits(staging->wlap_bssid_addr,
+				     active->wlap_bssid_addr));
 	CHK_NEQ(staging->dev_type, active->dev_type);
 	CHK_NEQ(staging->channel, active->channel);
 	CHK_NEQ(staging->air_propagation, active->air_propagation);

[PATCH 8/11] use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h
ath_common defined in drivers/net/wireless/ath/ath.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/ath/ath9k/htc_drv_txrx.c |    2 +-
 drivers/net/wireless/ath/ath9k/recv.c         |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
index c028df7..b41e008 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
@@ -1077,7 +1077,7 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
 
 	if (ieee80211_is_beacon(hdr->frame_control) &&
 	    !is_zero_ether_addr(common->curbssid) &&
-	    ether_addr_equal(hdr->addr3, common->curbssid)) {
+	    ether_addr_equal_64bits(hdr->addr3, common->curbssid)) {
 		s8 rssi = rxbuf->rxstatus.rs_rssi;
 
 		if (likely(last_rssi != ATH_RSSI_DUMMY_MARKER))
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 3692b2a..0152318 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -982,7 +982,7 @@ static bool ath9k_is_mybeacon(struct ath_softc *sc, struct ieee80211_hdr *hdr)
 	if (ieee80211_is_beacon(hdr->frame_control)) {
 		RX_STAT_INC(rx_beacons);
 		if (!is_zero_ether_addr(common->curbssid) &&
-		    ether_addr_equal(hdr->addr3, common->curbssid))
+		    ether_addr_equal_64bits(hdr->addr3, common->curbssid))
 			return true;
 	}
 

[PATCH 9/11] ipw2x00: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structure involved is
libipw_network defined in drivers/net/wireless/ipw2x00/libipw.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/ipw2x00/libipw_rx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ipw2x00/libipw_rx.c b/drivers/net/wireless/ipw2x00/libipw_rx.c
index 9ffe659..ce27859 100644
--- a/drivers/net/wireless/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/ipw2x00/libipw_rx.c
@@ -1468,7 +1468,7 @@ static inline int is_same_network(struct libipw_network *src,
 	 * as one network */
 	return ((src->ssid_len == dst->ssid_len) &&
 		(src->channel == dst->channel) &&
-		ether_addr_equal(src->bssid, dst->bssid) &&
+		ether_addr_equal_64bits(src->bssid, dst->bssid) &&
 		!memcmp(src->ssid, dst->ssid, src->ssid_len));
 }
 

[PATCH 10/11] at76c50x-usb: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
at76_priv defined in drivers/net/wireless/at76c50x-usb.h and
ieee80211_mgmt defined in include/linux/ieee80211.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/at76c50x-usb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 34c8a33..031d4ec 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1721,7 +1721,7 @@ static void at76_mac80211_tx(struct ieee80211_hw *hw,
 	 * following workaround is necessary. If the TX frame is an
 	 * authentication frame extract the bssid and send the CMD_JOIN. */
 	if (mgmt->frame_control & cpu_to_le16(IEEE80211_STYPE_AUTH)) {
-		if (!ether_addr_equal(priv->bssid, mgmt->bssid)) {
+		if (!ether_addr_equal_64bits(priv->bssid, mgmt->bssid)) {
 			memcpy(priv->bssid, mgmt->bssid, ETH_ALEN);
 			ieee80211_queue_work(hw, &priv->work_join_bssid);
 			dev_kfree_skb_any(skb);

[PATCH 11/11] carl9170: use ether_addr_equal_64bits

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
used when each argument is an array within a structure that contains at
least two bytes of data beyond the array.

The structures involved are:
ieee80211_hdr defined in include/linux/ieee80211.h,
ieee80211_bar defined in include/linux/ieee80211.h and
ath_common defined in drivers/net/wireless/ath/ath.h

This was done using Coccinelle (http://coccinelle.lip6.fr/).

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch used is rather long and can be found in message 0 of
this patch series.

 drivers/net/wireless/ath/carl9170/rx.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/rx.c b/drivers/net/wireless/ath/carl9170/rx.c
index e935f61..acd7ddc 100644
--- a/drivers/net/wireless/ath/carl9170/rx.c
+++ b/drivers/net/wireless/ath/carl9170/rx.c
@@ -536,7 +536,7 @@ static void carl9170_ps_beacon(struct ar9170 *ar, void *data, unsigned int len)
 		return;
 
 	/* and only beacons from the associated BSSID, please */
-	if (!ether_addr_equal(hdr->addr3, ar->common.curbssid) ||
+	if (!ether_addr_equal_64bits(hdr->addr3, ar->common.curbssid) ||
 	    !ar->common.curaid)
 		return;
 
@@ -602,8 +602,8 @@ static void carl9170_ba_check(struct ar9170 *ar, void *data, unsigned int len)
 
 		if (bar->start_seq_num == entry_bar->start_seq_num &&
 		    TID_CHECK(bar->control, entry_bar->control) &&
-		    ether_addr_equal(bar->ra, entry_bar->ta) &&
-		    ether_addr_equal(bar->ta, entry_bar->ra)) {
+		    ether_addr_equal_64bits(bar->ra, entry_bar->ta) &&
+		    ether_addr_equal_64bits(bar->ta, entry_bar->ra)) {
 			struct ieee80211_tx_info *tx_info;
 
 			tx_info = IEEE80211_SKB_CB(entry_skb);

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Mon, 2013-12-30 at 19:15 +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.
> 
> The structures involved are:
> iwl_rxon_cmd defined in drivers/net/wireless/iwlwifi/dvm/commands.h and
> ieee80211_hdr defined in include/linux/ieee80211.h
> 
> This was done using Coccinelle (http://coccinelle.lip6.fr/).

Seems to be missing an "iwlwifi:" or so prefix, but I guess we can add
it when we take the patch ...

Is there any way we could catch (sparse, or some other script?) that
struct reorganising won't break the condition needed ("within a
structure that contains at least two more bytes")?

johannes

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

> Seems to be missing an "iwlwifi:" or so prefix, but I guess we can add
> it when we take the patch ...

Sorry.  Not sure why that happened.  I'll look into it.

> Is there any way we could catch (sparse, or some other script?) that
> struct reorganising won't break the condition needed ("within a
> structure that contains at least two more bytes")?

What kind of reorganizing could happen?  Do you mean that the programmer 
might do at some time in the future, or something the compiler might do?

julia

Re: [PATCH 6/11] rtlwifi: use ether_addr_equal_64bits

[Larry Finger]

On 12/30/2013 12:15 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.
>
> The structures involved are:
> ieee80211_hdr defined in include/linux/ieee80211.h and
> rtl_mac defined in drivers/net/wireless/rtlwifi/wifi.h
>
> This was done using Coccinelle (http://coccinelle.lip6.fr/).
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

Larry


>
> ---
> The semantic patch used is rather long and can be found in message 0 of
> this patch series.
>
>   drivers/net/wireless/rtlwifi/base.c |    4 ++--
>   drivers/net/wireless/rtlwifi/ps.c   |    4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/ps.c b/drivers/net/wireless/rtlwifi/ps.c
> index 0d81f76..deedae3 100644
> --- a/drivers/net/wireless/rtlwifi/ps.c
> +++ b/drivers/net/wireless/rtlwifi/ps.c
> @@ -478,7 +478,7 @@ void rtl_swlps_beacon(struct ieee80211_hw *hw, void *data, unsigned int len)
>   		return;
>
>   	/* and only beacons from the associated BSSID, please */
> -	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
> +	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
>   		return;
>
>   	rtlpriv->psc.last_beacon = jiffies;
> @@ -923,7 +923,7 @@ void rtl_p2p_info(struct ieee80211_hw *hw, void *data, unsigned int len)
>   		return;
>
>   	/* and only beacons from the associated BSSID, please */
> -	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
> +	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
>   		return;
>
>   	/* check if this really is a beacon */
> diff --git a/drivers/net/wireless/rtlwifi/base.c b/drivers/net/wireless/rtlwifi/base.c
> index fcf9b62..d63a12c 100644
> --- a/drivers/net/wireless/rtlwifi/base.c
> +++ b/drivers/net/wireless/rtlwifi/base.c
> @@ -1293,7 +1293,7 @@ void rtl_beacon_statistic(struct ieee80211_hw *hw, struct sk_buff *skb)
>   		return;
>
>   	/* and only beacons from the associated BSSID, please */
> -	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
> +	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
>   		return;
>
>   	rtlpriv->link_info.bcn_rx_inperiod++;
> @@ -1781,7 +1781,7 @@ void rtl_recognize_peer(struct ieee80211_hw *hw, u8 *data, unsigned int len)
>   		return;
>
>   	/* and only beacons from the associated BSSID, please */
> -	if (!ether_addr_equal(hdr->addr3, rtlpriv->mac80211.bssid))
> +	if (!ether_addr_equal_64bits(hdr->addr3, rtlpriv->mac80211.bssid))
>   		return;
>
>   	if (rtl_find_221_ie(hw, data, len))
>
>

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:

> > Is there any way we could catch (sparse, or some other script?) that
> > struct reorganising won't break the condition needed ("within a
> > structure that contains at least two more bytes")?
> 
> What kind of reorganizing could happen?  Do you mean that the programmer 
> might do at some time in the future, or something the compiler might do?

I'm just thinking of a programmer, e.g. changing a struct like this:

 struct foo {
   u8 addr[ETH_ALEN];
-  u16 dummy;
 };

for example.

johannes

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Henrique de Moraes Holschuh]

On Mon, 30 Dec 2013, Johannes Berg wrote:
> On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> > > Is there any way we could catch (sparse, or some other script?) that
> > > struct reorganising won't break the condition needed ("within a
> > > structure that contains at least two more bytes")?
> > 
> > What kind of reorganizing could happen?  Do you mean that the programmer 
> > might do at some time in the future, or something the compiler might do?
> 
> I'm just thinking of a programmer, e.g. changing a struct like this:
> 
>  struct foo {
>    u8 addr[ETH_ALEN];
> -  u16 dummy;
>  };
> 
> for example.

That is easily resolved by:

struct foo {
	u8 addr[ETH_ALEN];
	u16 required_padding;	/* do not remove upon pain of death */
};

Preferably with a comment nearby explaining just why that padding is so
important in the first place.

Unfortunately, we do have a lot of code with unexplained padding that one
should never remove, and it is often nowhere nearly as obvious as stray
elements with idiotic non-explanatory names in a structure.

One example is the via-rng hw_random driver, where the buffer used by the
"xstore" via-specific instriction *MUST* be longer than what the "xstore"
instruction is documented to require due to CPU errata (you ask it to write
from 1 to 8 bytes, and it may end up writing 16 bytes).  The xstore buffer
is an array that was made long enough to avoid the errata, but that fact is
documented only in a git commit message.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
> On Mon, 30 Dec 2013, Johannes Berg wrote:
> > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> > > > Is there any way we could catch (sparse, or some other script?) that
> > > > struct reorganising won't break the condition needed ("within a
> > > > structure that contains at least two more bytes")?
> > > 
> > > What kind of reorganizing could happen?  Do you mean that the programmer 
> > > might do at some time in the future, or something the compiler might do?
> > 
> > I'm just thinking of a programmer, e.g. changing a struct like this:
> > 
> >  struct foo {
> >    u8 addr[ETH_ALEN];
> > -  u16 dummy;
> >  };
> > 
> > for example.
> 
> That is easily resolved by:
> 
> struct foo {
> 	u8 addr[ETH_ALEN];
> 	u16 required_padding;	/* do not remove upon pain of death */
> };

That'd be a stupid waste of struct space. If anything, there should be
*only* a comment saying that at least two bytes are needed - I'd still
prefer an automated check.

johannes

--
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 4/11] use ether_addr_equal_64bits

[Joe Perches]

On Tue, 2013-12-31 at 00:13 +0100, Johannes Berg wrote:
> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
> > On Mon, 30 Dec 2013, Johannes Berg wrote:
> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> > > > > Is there any way we could catch (sparse, or some other script?) that
> > > > > struct reorganising won't break the condition needed ("within a
> > > > > structure that contains at least two more bytes")?
> > > > 
> > > > What kind of reorganizing could happen?  Do you mean that the programmer 
> > > > might do at some time in the future, or something the compiler might do?
> > > 
> > > I'm just thinking of a programmer, e.g. changing a struct like this:
> > > 
> > >  struct foo {
> > >    u8 addr[ETH_ALEN];
> > > -  u16 dummy;
> > >  };

I don't know of a way to catch that.
Anyone else?

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Emmanuel Grumbach]

On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
> > On Mon, 30 Dec 2013, Johannes Berg wrote:
> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> > > > > Is there any way we could catch (sparse, or some other script?) that
> > > > > struct reorganising won't break the condition needed ("within a
> > > > > structure that contains at least two more bytes")?
> > > >
> > > > What kind of reorganizing could happen?  Do you mean that the programmer
> > > > might do at some time in the future, or something the compiler might do?
> > >
> > > I'm just thinking of a programmer, e.g. changing a struct like this:
> > >
> > >  struct foo {
> > >    u8 addr[ETH_ALEN];
> > > -  u16 dummy;
> > >  };
> > >
> > > for example.
> >
> > That is easily resolved by:
> >
> > struct foo {
> >       u8 addr[ETH_ALEN];
> >       u16 required_padding;   /* do not remove upon pain of death */
> > };
>
> That'd be a stupid waste of struct space. If anything, there should be
> *only* a comment saying that at least two bytes are needed - I'd still
> prefer an automated check.
>

Frankly I am not sure I like the patch. This flow is not a fast path
at all. While I don't really care for the waste in iwlwifi (because
there isn't), I don't see the real point is make the code more
sensitive to changes to earn basically nothing.

This flow happens only upon association which means a few times an hour maybe...
The only advantage I see here is that people like me who don't always
have a chance to read / write much code outside their little tiny
boring driver get to know about this kind of things. So, from an
educational point of view - this is cool.
But education is one thing, and the code is another.

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

> > > > I'm just thinking of a programmer, e.g. changing a struct like this:
> > > > 
> > > >  struct foo {
> > > >    u8 addr[ETH_ALEN];
> > > > -  u16 dummy;
> > > >  };
> 
> I don't know of a way to catch that.
> Anyone else?

Well, one could have a semantic patch that checks for that.  But the 
problem is that it is very slow, and it only covers the cases that I can 
transform automatically, which currently means no pointers, only explicit 
arrays.

On the other hand, I am finding the structure definition, so I can easily 
update the structure definition with an appropriate comment.

struct foo {
    u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */
    u16 dummy;
};

Unfortunately it is kind of verbose.  Could there be an attribute?  That 
could even easily be checked.

julia

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Ben Greear]

On 12/30/2013 10:32 PM, Julia Lawall wrote:
>>>>> I'm just thinking of a programmer, e.g. changing a struct like this:
>>>>>
>>>>>   struct foo {
>>>>>     u8 addr[ETH_ALEN];
>>>>> -  u16 dummy;
>>>>>   };
>>
>> I don't know of a way to catch that.
>> Anyone else?
>
> Well, one could have a semantic patch that checks for that.  But the
> problem is that it is very slow, and it only covers the cases that I can
> transform automatically, which currently means no pointers, only explicit
> arrays.
>
> On the other hand, I am finding the structure definition, so I can easily
> update the structure definition with an appropriate comment.
>
> struct foo {
>      u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */
>      u16 dummy;
> };
>
> Unfortunately it is kind of verbose.  Could there be an attribute?  That
> could even easily be checked.

Can you not just add a build-time macro to check that sizeof(foo) >= 8
for each of these struct foos?  Or, is it required that the dummy field
be there and be not used by anything else?

Thanks,
Ben

>
> julia
> --
> 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
>


-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

--
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 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Tue, 31 Dec 2013, Ben Greear wrote:

> On 12/30/2013 10:32 PM, Julia Lawall wrote:
> > > > > > I'm just thinking of a programmer, e.g. changing a struct like this:
> > > > > >
> > > > > >   struct foo {
> > > > > >     u8 addr[ETH_ALEN];
> > > > > > -  u16 dummy;
> > > > > >   };
> > >
> > > I don't know of a way to catch that.
> > > Anyone else?
> >
> > Well, one could have a semantic patch that checks for that.  But the
> > problem is that it is very slow, and it only covers the cases that I can
> > transform automatically, which currently means no pointers, only explicit
> > arrays.
> >
> > On the other hand, I am finding the structure definition, so I can easily
> > update the structure definition with an appropriate comment.
> >
> > struct foo {
> >      u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */
> >      u16 dummy;
> > };
> >
> > Unfortunately it is kind of verbose.  Could there be an attribute?  That
> > could even easily be checked.
>
> Can you not just add a build-time macro to check that sizeof(foo) >= 8
> for each of these struct foos?  Or, is it required that the dummy field
> be there and be not used by anything else?

It doesn't matter what the field is used for.  The problem is that is it
necessary to ensure a property of the position of addr within the
structure.  It has to have at least 16 bytes after it.

But maybe something with sizeof(foo) and offset_of would do?

Could the macro be put near the declaration of the structure somehow?

julia

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Ben Greear]

On 12/31/2013 08:09 AM, Julia Lawall wrote:
>
>
> On Tue, 31 Dec 2013, Ben Greear wrote:
>
>> On 12/30/2013 10:32 PM, Julia Lawall wrote:
>>>>>>> I'm just thinking of a programmer, e.g. changing a struct like this:
>>>>>>>
>>>>>>>    struct foo {
>>>>>>>      u8 addr[ETH_ALEN];
>>>>>>> -  u16 dummy;
>>>>>>>    };
>>>>
>>>> I don't know of a way to catch that.
>>>> Anyone else?
>>>
>>> Well, one could have a semantic patch that checks for that.  But the
>>> problem is that it is very slow, and it only covers the cases that I can
>>> transform automatically, which currently means no pointers, only explicit
>>> arrays.
>>>
>>> On the other hand, I am finding the structure definition, so I can easily
>>> update the structure definition with an appropriate comment.
>>>
>>> struct foo {
>>>       u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */
>>>       u16 dummy;
>>> };
>>>
>>> Unfortunately it is kind of verbose.  Could there be an attribute?  That
>>> could even easily be checked.
>>
>> Can you not just add a build-time macro to check that sizeof(foo) >= 8
>> for each of these struct foos?  Or, is it required that the dummy field
>> be there and be not used by anything else?
>
> It doesn't matter what the field is used for.  The problem is that is it
> necessary to ensure a property of the position of addr within the
> structure.  It has to have at least 16 bytes after it.

You mean 16 bits?

>
> But maybe something with sizeof(foo) and offset_of would do?
>
> Could the macro be put near the declaration of the structure somehow?

I think that would work, but do not know all of the details of such
macros, so it's possible there is some catch.

If nothing else, then some run-time code that calculates the offset off
and asserts if it is broken in module initialization or similar might
be good enough.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Tue, 31 Dec 2013, Ben Greear wrote:

> On 12/31/2013 08:09 AM, Julia Lawall wrote:
> >
> >
> > On Tue, 31 Dec 2013, Ben Greear wrote:
> >
> > > On 12/30/2013 10:32 PM, Julia Lawall wrote:
> > > > > > > > I'm just thinking of a programmer, e.g. changing a struct like
> > > > > > > > this:
> > > > > > > >
> > > > > > > >    struct foo {
> > > > > > > >      u8 addr[ETH_ALEN];
> > > > > > > > -  u16 dummy;
> > > > > > > >    };
> > > > >
> > > > > I don't know of a way to catch that.
> > > > > Anyone else?
> > > >
> > > > Well, one could have a semantic patch that checks for that.  But the
> > > > problem is that it is very slow, and it only covers the cases that I can
> > > > transform automatically, which currently means no pointers, only
> > > > explicit
> > > > arrays.
> > > >
> > > > On the other hand, I am finding the structure definition, so I can
> > > > easily
> > > > update the structure definition with an appropriate comment.
> > > >
> > > > struct foo {
> > > >       u8 addr[ETH_ALEN]; /* must be followed by two bytes in the
> > > > structure */
> > > >       u16 dummy;
> > > > };
> > > >
> > > > Unfortunately it is kind of verbose.  Could there be an attribute?  That
> > > > could even easily be checked.
> > >
> > > Can you not just add a build-time macro to check that sizeof(foo) >= 8
> > > for each of these struct foos?  Or, is it required that the dummy field
> > > be there and be not used by anything else?
> >
> > It doesn't matter what the field is used for.  The problem is that is it
> > necessary to ensure a property of the position of addr within the
> > structure.  It has to have at least 16 bytes after it.
>
> You mean 16 bits?

Oops, yes.  16 bits.

> >
> > But maybe something with sizeof(foo) and offset_of would do?
> >
> > Could the macro be put near the declaration of the structure somehow?
>
> I think that would work, but do not know all of the details of such
> macros, so it's possible there is some catch.
>
> If nothing else, then some run-time code that calculates the offset off
> and asserts if it is broken in module initialization or similar might
> be good enough.

Could be OK.  Something right in or after the structure declaration would
be nicest.

julia

>
> Thanks,
> Ben
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/11] rt2x00: use ether_addr_equal_64bits

[Gertjan van Wingerde]

Sent from my iPad

> On 30 dec. 2013, at 19:14, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> 
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.
> 
> The structures involved are:
> ieee80211_bar defined in include/linux/ieee80211.h and
> rt2x00_bar_list_entry defined in drivers/net/wireless/rt2x00/rt2x00.h.
> 
> This was done using Coccinelle (http://coccinelle.lip6.fr/).
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

> 
> ---
> The semantic patch used is rather long and can be found in message 0 of
> this patch series.
> 
> drivers/net/wireless/rt2x00/rt2x00dev.c |    4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index 00c3fae..2bde672 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -565,10 +565,10 @@ static void rt2x00lib_rxdone_check_ba(struct rt2x00_dev *rt2x00dev,
> 
> #undef TID_CHECK
> 
> -        if (!ether_addr_equal(ba->ra, entry->ta))
> +        if (!ether_addr_equal_64bits(ba->ra, entry->ta))
>            continue;
> 
> -        if (!ether_addr_equal(ba->ta, entry->ra))
> +        if (!ether_addr_equal_64bits(ba->ta, entry->ra))
>            continue;
> 
>        /* Mark BAR since we received the according BA */
> 

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

Is there any way to get sizeof evaluated at build time?

julia

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Joe Perches]

On Mon, 2014-01-06 at 09:48 +0100, Julia Lawall wrote:
> Is there any way to get sizeof evaluated at build time?

I'm confused a bit by what you want to accomplish.

Except for variable length arrays, isn't sizeof always
evaluated at build time?

6.5.3.4 The sizeof operator
Constraints
[]
2 The sizeof operator yields the size (in bytes) of its operand, which may be an
expression or the parenthesized name of a type. The size is determined from the type of
the operand. The result is an integer. If the type of the operand is a variable length array
type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an
integer constant.

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Mon, 6 Jan 2014, Joe Perches wrote:

> On Mon, 2014-01-06 at 09:48 +0100, Julia Lawall wrote:
> > Is there any way to get sizeof evaluated at build time?
>
> I'm confused a bit by what you want to accomplish.

To check that a field that is an argument od ether_addr_equal_64bits is
a sufficient distance from the end of the structure.

> Except for variable length arrays, isn't sizeof always
> evaluated at build time?

OK, the question was expressed badly.  Is there any way to use the value
to trigger an action at build time?  The only way I kow to trigger an
action is with #error, but #error is processed by cpp, which doesn't know
about the size of types.

julia

> 6.5.3.4 The sizeof operator
> Constraints
> []
> 2 The sizeof operator yields the size (in bytes) of its operand, which may be an
> expression or the parenthesized name of a type. The size is determined from the type of
> the operand. The result is an integer. If the type of the operand is a variable length array
> type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an
> integer constant.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Tue, 2013-12-31 at 17:40 +0100, Julia Lawall wrote:

> > If nothing else, then some run-time code that calculates the offset off
> > and asserts if it is broken in module initialization or similar might
> > be good enough.
> 
> Could be OK.  Something right in or after the structure declaration would
> be nicest.

I don't think you can put a BUILD_BUG_ON() into the structure
declaration (it's code, not declarations), but I think you could just
put

BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8);

with the user(s?) and that should catch the scenario I was worrying
about?

johannes

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Mon, 2014-01-06 at 10:04 +0100, Julia Lawall wrote:

> OK, the question was expressed badly.  Is there any way to use the value
> to trigger an action at build time?  The only way I kow to trigger an
> action is with #error, but #error is processed by cpp, which doesn't know
> about the size of types.

That's exactly BUILD_BUG_ON(), I believe.

johannes

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Mon, 6 Jan 2014, Johannes Berg wrote:

> On Tue, 2013-12-31 at 17:40 +0100, Julia Lawall wrote:
>
> > > If nothing else, then some run-time code that calculates the offset off
> > > and asserts if it is broken in module initialization or similar might
> > > be good enough.
> >
> > Could be OK.  Something right in or after the structure declaration would
> > be nicest.
>
> I don't think you can put a BUILD_BUG_ON() into the structure
> declaration (it's code, not declarations), but I think you could just
> put
>
> BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8);
>
> with the user(s?) and that should catch the scenario I was worrying
> about?

OK, thanks.  That is what I had in mind.  But I was hoping to be able to
put it with the structure.  Perhaps there is a way to make a macro that
expands to a dummy function that contains the BUILD_BUG_ON?  But I guess
that would waste space?

I think that 8 should be 16?

julia
--
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 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Mon, 6 Jan 2014, Johannes Berg wrote:

> On Mon, 2014-01-06 at 10:04 +0100, Julia Lawall wrote:
>
> > OK, the question was expressed badly.  Is there any way to use the value
> > to trigger an action at build time?  The only way I kow to trigger an
> > action is with #error, but #error is processed by cpp, which doesn't know
> > about the size of types.
>
> That's exactly BUILD_BUG_ON(), I believe.

Maybe BUILD_BUG_ON_ZERO?  It's goal seems to be to return an integer,
which could then be used as an array size in a dummy structure.  The
structure declaration would not generate any code.  The structure would
have to have a name, thoough.

julia

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Geert Uytterhoeven]

On Tue, Dec 31, 2013 at 7:26 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>>
>> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
>> > On Mon, 30 Dec 2013, Johannes Berg wrote:
>> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
>> > > > > Is there any way we could catch (sparse, or some other script?) that
>> > > > > struct reorganising won't break the condition needed ("within a
>> > > > > structure that contains at least two more bytes")?
>> > > >
>> > > > What kind of reorganizing could happen?  Do you mean that the programmer
>> > > > might do at some time in the future, or something the compiler might do?
>> > >
>> > > I'm just thinking of a programmer, e.g. changing a struct like this:
>> > >
>> > >  struct foo {
>> > >    u8 addr[ETH_ALEN];
>> > > -  u16 dummy;
>> > >  };
>> > >
>> > > for example.
>> >
>> > That is easily resolved by:
>> >
>> > struct foo {
>> >       u8 addr[ETH_ALEN];
>> >       u16 required_padding;   /* do not remove upon pain of death */
>> > };

Adding the u16 also changes the alignment of the whole struct. So it may
cost one additional byte _in front of_ the struct.

<asking-stupid-question-see-answer-below>
While you're at it, why not just making a new 64-bit aligned type for
Ethernet addresses, so it'll also work for
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
</asking-stupid-question-see-answer-below>

>> That'd be a stupid waste of struct space. If anything, there should be
>> *only* a comment saying that at least two bytes are needed - I'd still
>> prefer an automated check.
>
> Frankly I am not sure I like the patch. This flow is not a fast path

I also don't like it. To me this sounds like wasting space for nothing.
BTW, would it be that more expensive to always do a 32+16 bit
comparison?

> at all. While I don't really care for the waste in iwlwifi (because
> there isn't), I don't see the real point is make the code more
> sensitive to changes to earn basically nothing.

Thanks to this discussion, my eye fell on:

static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
{
        const u16 *a = (const u16 *) addr1;
        const u16 *b = (const u16 *) addr2;

        BUILD_BUG_ON(ETH_ALEN != 6);
        return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
}

What if addr1 or addr2 are odd, and this is running on an architecture that
doesn't support unaligned accesses at all?? Have we been lucky forever?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Julia Lawall]

On Mon, 6 Jan 2014, Geert Uytterhoeven wrote:

> On Tue, Dec 31, 2013 at 7:26 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> > On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg
> > <johannes@sipsolutions.net> wrote:
> >>
> >> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
> >> > On Mon, 30 Dec 2013, Johannes Berg wrote:
> >> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> >> > > > > Is there any way we could catch (sparse, or some other script?) that
> >> > > > > struct reorganising won't break the condition needed ("within a
> >> > > > > structure that contains at least two more bytes")?
> >> > > >
> >> > > > What kind of reorganizing could happen?  Do you mean that the programmer
> >> > > > might do at some time in the future, or something the compiler might do?
> >> > >
> >> > > I'm just thinking of a programmer, e.g. changing a struct like this:
> >> > >
> >> > >  struct foo {
> >> > >    u8 addr[ETH_ALEN];
> >> > > -  u16 dummy;
> >> > >  };
> >> > >
> >> > > for example.
> >> >
> >> > That is easily resolved by:
> >> >
> >> > struct foo {
> >> >       u8 addr[ETH_ALEN];
> >> >       u16 required_padding;   /* do not remove upon pain of death */
> >> > };
>
> Adding the u16 also changes the alignment of the whole struct. So it may
> cost one additional byte _in front of_ the struct.
>
> <asking-stupid-question-see-answer-below>
> While you're at it, why not just making a new 64-bit aligned type for
> Ethernet addresses, so it'll also work for
> !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
> </asking-stupid-question-see-answer-below>
>
> >> That'd be a stupid waste of struct space. If anything, there should be
> >> *only* a comment saying that at least two bytes are needed - I'd still
> >> prefer an automated check.
> >
> > Frankly I am not sure I like the patch. This flow is not a fast path
>
> I also don't like it. To me this sounds like wasting space for nothing.
> BTW, would it be that more expensive to always do a 32+16 bit
> comparison?

No space is wasted by the patch.  The patch is only generated in the case
where there is currently enough space.

julia

> > at all. While I don't really care for the waste in iwlwifi (because
> > there isn't), I don't see the real point is make the code more
> > sensitive to changes to earn basically nothing.
>
> Thanks to this discussion, my eye fell on:
>
> static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
> {
>         const u16 *a = (const u16 *) addr1;
>         const u16 *b = (const u16 *) addr2;
>
>         BUILD_BUG_ON(ETH_ALEN != 6);
>         return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
> }
>
> What if addr1 or addr2 are odd, and this is running on an architecture that
> doesn't support unaligned accesses at all?? Have we been lucky forever?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Johannes Berg]

On Mon, 2014-01-06 at 10:09 +0100, Julia Lawall wrote:

> > BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8);
> >
> > with the user(s?) and that should catch the scenario I was worrying
> > about?
> 
> OK, thanks.  That is what I had in mind.  But I was hoping to be able to
> put it with the structure.  

Right - you might be able to do that with BUILD_BUG_ON_ZERO() as you
pointed out, I haven't looked at these macros in a while.

> Perhaps there is a way to make a macro that
> expands to a dummy function that contains the BUILD_BUG_ON?  But I guess
> that would waste space?
> 
> I think that 8 should be 16?

No, that should be ETH_ALEN+2 really, I guess - it's not taking into
account the size of the address member itself at all.

johannes

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Dan Carpenter]

On Tue, Dec 31, 2013 at 12:13:08AM +0100, Johannes Berg wrote:
> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:
> > On Mon, 30 Dec 2013, Johannes Berg wrote:
> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:
> > > > > Is there any way we could catch (sparse, or some other script?) that
> > > > > struct reorganising won't break the condition needed ("within a
> > > > > structure that contains at least two more bytes")?
> > > > 
> > > > What kind of reorganizing could happen?  Do you mean that the programmer 
> > > > might do at some time in the future, or something the compiler might do?
> > > 
> > > I'm just thinking of a programmer, e.g. changing a struct like this:
> > > 
> > >  struct foo {
> > >    u8 addr[ETH_ALEN];
> > > -  u16 dummy;
> > >  };
> > > 
> > > for example.
> > 
> > That is easily resolved by:
> > 
> > struct foo {
> > 	u8 addr[ETH_ALEN];
> > 	u16 required_padding;	/* do not remove upon pain of death */
> > };
> 
> That'd be a stupid waste of struct space. If anything, there should be
> *only* a comment saying that at least two bytes are needed - I'd still
> prefer an automated check.
> 

This is the sort of thing where Smatch is probably the right tool.  I'll
let you know how it turns out.

My guess is that it would be rare to run into this bug in real life.
Most structs have 4 or 8 byte alignment and most times the addr is not
at the end of the struct.  But I also agree that this function should
only be used in a fast path.

regards,
dan carpenter

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Eric Dumazet]

On Mon, 2014-01-06 at 10:24 +0100, Geert Uytterhoeven wrote:

> Thanks to this discussion, my eye fell on:
> 
> static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
> {
>         const u16 *a = (const u16 *) addr1;
>         const u16 *b = (const u16 *) addr2;
> 
>         BUILD_BUG_ON(ETH_ALEN != 6);
>         return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
> }
> 
> What if addr1 or addr2 are odd, and this is running on an architecture that
> doesn't support unaligned accesses at all?? Have we been lucky forever?

This function always had the guarantee of u16 alignment.

No protocol ever included an ether address at an odd alignment, and
drivers always make sure a frame is at least 2-byte aligned.

Its kind of obvious for networking people, Stephen did not mention this
property in commit 360ac8e2f1a38c34

Re: [PATCH 4/11] use ether_addr_equal_64bits

[Dan Carpenter]

We're worried about reading beyond the end of the array and it's a heap
allocation and the last char of the eth addr is the last byte of the
page.  This causes an oops.

It's almost impossible to hit that bug.

1) You would have to have the eth addr at the end of the array.
2) It would have to be a packed struct.
3) The struct size would have to be a multiple of 4 because otherwise we
   can't put it at the end of the page.
4) It would need to be allocated on the heap.

You add all those up which is pretty rare so I wasn't able to find
anything like that.  Then you have to get extremely unlucky.

The closest thing I could find were a couple places like like:

static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };

It meets criteria 1 and 2 but not 3 and 4.

regards,
dan carpenter

--
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 0/11] use ether_addr_equal_64bits

[Pavel Machek]

On Mon 2013-12-30 19:14:56, Julia Lawall wrote:
> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> used when each argument is an array within a structure that contains at
> least two bytes of data beyond the array.

I mean, yes, it is probably faster, and yes, most structures probably
contain two more bytes, but... is the uglyness worth the speedup? I'd
say this should not be done except in very time-critical places...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/11] use ether_addr_equal_64bits

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

Am 17.01.2014 22:24, schrieb Pavel Machek:
> On Mon 2013-12-30 19:14:56, Julia Lawall wrote:
>> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
>> used when each argument is an array within a structure that contains at
>> least two bytes of data beyond the array.
> 
> I mean, yes, it is probably faster, and yes, most structures probably
> contain two more bytes, but... is the uglyness worth the speedup? I'd
> say this should not be done except in very time-critical places...
> 									Pavel

This code run on every received beacon, almost on every wifi driver (If
i understand what you mean.)

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH 0/11] use ether_addr_equal_64bits

[Pavel Machek]

On Fri 2014-01-17 23:02:06, Oleksij Rempel wrote:
> Am 17.01.2014 22:24, schrieb Pavel Machek:
> > On Mon 2013-12-30 19:14:56, Julia Lawall wrote:
> >> Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be
> >> used when each argument is an array within a structure that contains at
> >> least two bytes of data beyond the array.
> > 
> > I mean, yes, it is probably faster, and yes, most structures probably
> > contain two more bytes, but... is the uglyness worth the speedup? I'd
> > say this should not be done except in very time-critical places...
> 
> This code run on every received beacon, almost on every wifi driver (If
> i understand what you mean.)

That does not look like "sufficiently often" to me. Can you measure
the improvement at least in some microbenchmark? Is there even
theoretical chance to get one?

You are comparing few bytes, number of cacheline accesses stays same,
there is likely _0_ speedup. And even if you saved 1T, that will be
compeletely lost in the noise.

In some kind of routing code, cache-hot... maybe it would make
sense. But once per interrupt?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html