Skip to contentSkip to conversation

staging: rtl8188eu: remove unused dump_txrpt_ccx_88e()

PATCH Superseded Michael Straube 2 participants 15 messages Jul 29, 2018 – Jul 30, 2018 Staging

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Michael Straube

author
#0 lore raw

PATCH 1/8 staging: rtl8188eu: remove unused dump_txrpt_ccx_88e()

The function dump_txrpt_ccx_88e() is nerver used, so remove it. Discovered by cppcheck.

Signed-off-by Michael Straube <[email protected]>
---
 drivers/staging/rtl8188eu/hal/rtl8188e_xmit.c | 22 -------------------
 .../staging/rtl8188eu/include/rtl8188e_xmit.h |  1 -
 2 files changed, 23 deletions(-)
drivers/staging/rtl8188eu/hal/rtl8188e_xmit.c +0 -22
index 6883746f3b8b..9b8a284544ac 100644
@@ -10,28 +10,6 @@
 #include <drv_types.h>
 #include <rtl8188e_hal.h>
 
-void dump_txrpt_ccx_88e(void *buf)
-{
-	struct txrpt_ccx_88e *txrpt_ccx = buf;
-
-	DBG_88E("%s:\n"
-		"tag1:%u, pkt_num:%u, txdma_underflow:%u, int_bt:%u, int_tri:%u, int_ccx:%u\n"
-		"mac_id:%u, pkt_ok:%u, bmc:%u\n"
-		"retry_cnt:%u, lifetime_over:%u, retry_over:%u\n"
-		"ccx_qtime:%u\n"
-		"final_data_rate:0x%02x\n"
-		"qsel:%u, sw:0x%03x\n",
-		__func__, txrpt_ccx->tag1, txrpt_ccx->pkt_num,
-		txrpt_ccx->txdma_underflow, txrpt_ccx->int_bt,
-		txrpt_ccx->int_tri, txrpt_ccx->int_ccx,
-		txrpt_ccx->mac_id, txrpt_ccx->pkt_ok, txrpt_ccx->bmc,
-		txrpt_ccx->retry_cnt, txrpt_ccx->lifetime_over,
-		txrpt_ccx->retry_over, txrpt_ccx_qtime_88e(txrpt_ccx),
-		txrpt_ccx->final_data_rate, txrpt_ccx->qsel,
-		txrpt_ccx_sw_88e(txrpt_ccx)
-	);
-}
-
 void handle_txrpt_ccx_88e(struct adapter *adapter, u8 *buf)
 {
 	struct txrpt_ccx_88e *txrpt_ccx = (struct txrpt_ccx_88e *)buf;
drivers/staging/rtl8188eu/include/rtl8188e_xmit.h +0 -1
index 17e7b3016674..20d35480dab8 100644
@@ -152,7 +152,6 @@ void rtl8188eu_xmit_tasklet(void *priv);
 s32 rtl8188eu_xmitframe_complete(struct adapter *padapter,
 				 struct xmit_priv *pxmitpriv);
 
-void dump_txrpt_ccx_88e(void *buf);
 void handle_txrpt_ccx_88e(struct adapter *adapter, u8 *buf);
 
 void _dbg_dump_tx_info(struct adapter *padapter, int frame_tag,
Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 2/8 staging: rtl8188eu: remove unused should_forbid_n_rate()

The function should_forbid_n_rate() is never used, so remove it. Discovered by cppcheck.

Signed-off-by Michael Straube <[email protected]>
---
 .../staging/rtl8188eu/core/rtw_wlan_util.c    | 35 -------------------
 .../staging/rtl8188eu/include/rtw_mlme_ext.h  |  1 -
 2 files changed, 36 deletions(-)
drivers/staging/rtl8188eu/core/rtw_wlan_util.c +0 -35
index 0fe35e141926..b9406583e501 100644
@@ -1094,41 +1094,6 @@ unsigned int is_ap_in_tkip(struct adapter *padapter)
 	}
 }
 
-unsigned int should_forbid_n_rate(struct adapter *padapter)
-{
-	u32 i;
-	struct ndis_802_11_var_ie *pIE;
-	struct mlme_priv	*pmlmepriv = &padapter->mlmepriv;
-	struct wlan_bssid_ex  *cur_network = &pmlmepriv->cur_network.network;
-
-	if (rtw_get_capability((struct wlan_bssid_ex *)cur_network) & WLAN_CAPABILITY_PRIVACY) {
-		for (i = sizeof(struct ndis_802_11_fixed_ie); i < cur_network->ie_length;) {
-			pIE = (struct ndis_802_11_var_ie *)(cur_network->ies + i);
-
-			switch (pIE->ElementID) {
-			case _VENDOR_SPECIFIC_IE_:
-				if (!memcmp(pIE->data, RTW_WPA_OUI, 4) &&
-				    ((!memcmp((pIE->data + 12), WPA_CIPHER_SUITE_CCMP, 4)) ||
-				    (!memcmp((pIE->data + 16), WPA_CIPHER_SUITE_CCMP, 4))))
-					return false;
-				break;
-			case _RSN_IE_2_:
-				if  ((!memcmp((pIE->data + 8), RSN_CIPHER_SUITE_CCMP, 4))  ||
-				       (!memcmp((pIE->data + 12), RSN_CIPHER_SUITE_CCMP, 4)))
-					return false;
-			default:
-				break;
-			}
-
-			i += (pIE->Length + 2);
-		}
-
-		return true;
-	} else {
-		return false;
-	}
-}
-
 unsigned int is_ap_in_wep(struct adapter *padapter)
 {
 	u32 i;
drivers/staging/rtl8188eu/include/rtw_mlme_ext.h +0 -1
index c072e1e25d61..ade68af15e04 100644
@@ -534,7 +534,6 @@ unsigned char get_highest_rate_idx(u32 mask);
 int support_short_GI(struct adapter *padapter, struct ieee80211_ht_cap *caps);
 unsigned int is_ap_in_tkip(struct adapter *padapter);
 unsigned int is_ap_in_wep(struct adapter *padapter);
-unsigned int should_forbid_n_rate(struct adapter *padapter);
 
 void report_join_res(struct adapter *padapter, int res);
 void report_survey_event(struct adapter *padapter,
Signature · 1 line
2.18.0
author
#2 lore raw

PATCH 3/8 staging: rtl8188eu: remove unused rtw_handle_tkip_mic_err()

In reply to Michael Straube

The function rtw_handle_tkip_mic_err() is never used, so remove it. Discovered by cppcheck.

Signed-off-by Michael Straube <[email protected]>
---
 .../staging/rtl8188eu/include/recv_osdep.h    |  2 --
 drivers/staging/rtl8188eu/os_dep/recv_linux.c | 36 -------------------
 2 files changed, 38 deletions(-)
drivers/staging/rtl8188eu/include/recv_osdep.h +0 -2
index d2341521cc8e..2c05964b757d 100644
@@ -19,8 +19,6 @@ s32  rtw_recv_entry(struct recv_frame *precv_frame);
 int rtw_recv_indicatepkt(struct adapter *adapter,
 			 struct recv_frame *recv_frame);
 
-void rtw_handle_tkip_mic_err(struct adapter *padapter, u8 bgroup);
-
 int rtw_os_recvbuf_resource_alloc(struct adapter *adapt, struct recv_buf *buf);
 
 void rtw_init_recv_timer(struct recv_reorder_ctrl *preorder_ctrl);
drivers/staging/rtl8188eu/os_dep/recv_linux.c +0 -36
index deadf26ea2aa..148ee94bbf3d 100644
@@ -25,42 +25,6 @@ int rtw_os_recvbuf_resource_alloc(struct adapter *padapter,
 	return _SUCCESS;
 }
 
-void rtw_handle_tkip_mic_err(struct adapter *padapter, u8 bgroup)
-{
-	union iwreq_data wrqu;
-	struct iw_michaelmicfailure ev;
-	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
-	struct security_priv *psecuritypriv = &padapter->securitypriv;
-	u32 cur_time = 0;
-
-	if (psecuritypriv->last_mic_err_time == 0) {
-		psecuritypriv->last_mic_err_time = jiffies;
-	} else {
-		cur_time = jiffies;
-
-		if (cur_time - psecuritypriv->last_mic_err_time < 60*HZ) {
-			psecuritypriv->btkip_countermeasure = true;
-			psecuritypriv->last_mic_err_time = 0;
-			psecuritypriv->btkip_countermeasure_time = cur_time;
-		} else {
-			psecuritypriv->last_mic_err_time = jiffies;
-		}
-	}
-
-	memset(&ev, 0x00, sizeof(ev));
-	if (bgroup)
-		ev.flags |= IW_MICFAILURE_GROUP;
-	else
-		ev.flags |= IW_MICFAILURE_PAIRWISE;
-
-	ev.src_addr.sa_family = ARPHRD_ETHER;
-	memcpy(ev.src_addr.sa_data, &pmlmepriv->assoc_bssid[0], ETH_ALEN);
-	memset(&wrqu, 0x00, sizeof(wrqu));
-	wrqu.data.length = sizeof(ev);
-	wireless_send_event(padapter->pnetdev, IWEVMICHAELMICFAILURE,
-			    &wrqu, (char *)&ev);
-}
-
 int rtw_recv_indicatepkt(struct adapter *padapter,
 			 struct recv_frame *precv_frame)
 {
Signature · 1 line
2.18.0
author
#3 lore raw

PATCH 4/8 staging: rtl8188eu: remove redundant includes

In reply to Michael Straube

Both osdep_service.h and drv_types.h are included from hal_intf.h, so remove the redundant includes from hal_intf.c.

Signed-off-by Michael Straube <[email protected]>
---
 drivers/staging/rtl8188eu/hal/hal_intf.c | 2 --
 1 file changed, 2 deletions(-)
drivers/staging/rtl8188eu/hal/hal_intf.c +0 -2
index aaa1718ca603..0baf4e616b6a 100644
@@ -6,8 +6,6 @@
  ******************************************************************************/
 
 #define _HAL_INTF_C_
-#include <osdep_service.h>
-#include <drv_types.h>
 #include <hal_intf.h>
 
 uint	 rtw_hal_init(struct adapter *adapt)
Signature · 1 line
2.18.0
author
#4 lore raw

PATCH 5/8 staging: rtl8188eu: replace tabs with spaces

In reply to Michael Straube

Replace tabs with spaces in function definition and variable declarations.

Signed-off-by Michael Straube <[email protected]>
---
 drivers/staging/rtl8188eu/hal/hal_intf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/rtl8188eu/hal/hal_intf.c +3 -3
index 0baf4e616b6a..c43e7b438058 100644
@@ -8,9 +8,9 @@
 #define _HAL_INTF_C_
 #include <hal_intf.h>
 
-uint	 rtw_hal_init(struct adapter *adapt)
+uint rtw_hal_init(struct adapter *adapt)
 {
-	uint	status = _SUCCESS;
+	uint status = _SUCCESS;
 
 	adapt->hw_init_completed = false;
 

@@ -34,7 +34,7 @@ uint	 rtw_hal_init(struct adapter *adapt)
 
 uint rtw_hal_deinit(struct adapter *adapt)
 {
-	uint	status = _SUCCESS;
+	uint status = _SUCCESS;
 
 	status = rtl8188eu_hal_deinit(adapt);
Signature · 1 line
2.18.0
author
#5 lore raw

PATCH 6/8 staging: rtl8188eu: fix comparsion to true

In reply to Michael Straube

Use if(x) instead of if(x == true).

Signed-off-by Michael Straube <[email protected]>
---
 drivers/staging/rtl8188eu/hal/hal_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8188eu/hal/hal_intf.c +1 -1
index c43e7b438058..b21ba01abc57 100644
@@ -50,7 +50,7 @@ void rtw_hal_update_ra_mask(struct adapter *adapt, u32 mac_id, u8 rssi_level)
 {
 	struct mlme_priv *pmlmepriv = &(adapt->mlmepriv);
 
-	if (check_fwstate(pmlmepriv, WIFI_AP_STATE) == true) {
+	if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
 #ifdef CONFIG_88EU_AP_MODE
 		struct sta_info *psta = NULL;
 		struct sta_priv *pstapriv = &adapt->stapriv;
Signature · 1 line
2.18.0
author
#6 lore raw

PATCH 7/8 staging: rtl8188eu: remove unnecessary parentheses

In reply to Michael Straube

Remove unnecessary parentheses, also clears checkpatch issues about missing spaces around '-'.

Signed-off-by Michael Straube <[email protected]>
---
 drivers/staging/rtl8188eu/hal/hal_intf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/rtl8188eu/hal/hal_intf.c +3 -3
index b21ba01abc57..b8fecc952cfc 100644
@@ -48,15 +48,15 @@ uint rtw_hal_deinit(struct adapter *adapt)
 
 void rtw_hal_update_ra_mask(struct adapter *adapt, u32 mac_id, u8 rssi_level)
 {
-	struct mlme_priv *pmlmepriv = &(adapt->mlmepriv);
+	struct mlme_priv *pmlmepriv = &adapt->mlmepriv;
 
 	if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
 #ifdef CONFIG_88EU_AP_MODE
 		struct sta_info *psta = NULL;
 		struct sta_priv *pstapriv = &adapt->stapriv;
 
-		if ((mac_id-1) > 0)
-			psta = pstapriv->sta_aid[(mac_id-1) - 1];
+		if (mac_id - 1 > 0)
+			psta = pstapriv->sta_aid[mac_id - 2];
 		if (psta)
 			add_RATid(adapt, psta, 0);/* todo: based on rssi_level*/
 #endif
Signature · 1 line
2.18.0
author
#7 lore raw

PATCH 8/8 staging: rtl8188eu: use is_broadcast_ether_addr

In reply to Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

Signed-off-by Michael Straube <[email protected]>
---
 .../staging/rtl8188eu/os_dep/ioctl_linux.c    | 34 ++++++-------------
 1 file changed, 10 insertions(+), 24 deletions(-)
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +10 -24
index 221fae22bab6..4a8124570e6e 100644
@@ -361,9 +361,7 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param,
 		goto exit;
 	}
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
+	if (is_broadcast_ether_addr(param->sta_addr)) {
 		if (param->u.crypt.idx >= WEP_KEYS) {
 			ret = -EINVAL;
 			goto exit;

@@ -2208,9 +2206,7 @@ static int rtw_set_encryption(struct net_device *dev, struct ieee_param *param,
 		ret =  -EINVAL;
 		goto exit;
 	}
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
+	if (is_broadcast_ether_addr(param->sta_addr)) {
 		if (param->u.crypt.idx >= WEP_KEYS) {
 			ret = -EINVAL;
 			goto exit;

@@ -2471,9 +2467,7 @@ static int rtw_add_sta(struct net_device *dev, struct ieee_param *param)
 	if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)))
 		return -EINVAL;
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param->sta_addr))
 		return -EINVAL;
 
 	psta = rtw_get_stainfo(pstapriv, param->sta_addr);

@@ -2528,9 +2522,7 @@ static int rtw_del_sta(struct net_device *dev, struct ieee_param *param)
 	if (check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)) != true)
 		return -EINVAL;
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param->sta_addr))
 		return -EINVAL;
 
 	psta = rtw_get_stainfo(pstapriv, param->sta_addr);

@@ -2566,9 +2558,7 @@ static int rtw_ioctl_get_sta_data(struct net_device *dev, struct ieee_param *par
 	if (check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)) != true)
 		return -EINVAL;
 
-	if (param_ex->sta_addr[0] == 0xff && param_ex->sta_addr[1] == 0xff &&
-	    param_ex->sta_addr[2] == 0xff && param_ex->sta_addr[3] == 0xff &&
-	    param_ex->sta_addr[4] == 0xff && param_ex->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param_ex->sta_addr))
 		return -EINVAL;
 
 	psta = rtw_get_stainfo(pstapriv, param_ex->sta_addr);

@@ -2622,9 +2612,7 @@ static int rtw_get_sta_wpaie(struct net_device *dev, struct ieee_param *param)
 	if (check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)) != true)
 		return -EINVAL;
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param->sta_addr))
 		return -EINVAL;
 
 	psta = rtw_get_stainfo(pstapriv, param->sta_addr);

@@ -2779,10 +2767,9 @@ static int rtw_ioctl_acl_remove_sta(struct net_device *dev, struct ieee_param *p
 	if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true)
 		return -EINVAL;
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param->sta_addr))
 		return -EINVAL;
+
 	return rtw_acl_remove_sta(padapter, param->sta_addr);
 }
 

@@ -2794,10 +2781,9 @@ static int rtw_ioctl_acl_add_sta(struct net_device *dev, struct ieee_param *para
 	if (check_fwstate(pmlmepriv, WIFI_AP_STATE) != true)
 		return -EINVAL;
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+	if (is_broadcast_ether_addr(param->sta_addr))
 		return -EINVAL;
+
 	return rtw_acl_add_sta(padapter, param->sta_addr);
 }
Signature · 1 line
2.18.0
#8 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c

Quoted 1 line
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +0 -0

[]

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param, · line 367

Quoted 11 lines
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +1 -3
@@ -361,9 +361,7 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param,
 		goto exit;
 	}
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
+	if (is_broadcast_ether_addr(param->sta_addr)) {
 		if (param->u.crypt.idx >= WEP_KEYS) {
 			ret = -EINVAL;
 			goto exit;

etc...

author
#9 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
        u32 cmd;
        u8 sta_addr[ETH_ALEN];
        union {
        ...
        ...
        }; u
};
struct ieee_param_ex {
	u32 cmd;
	u8 sta_addr[ETH_ALEN];
	u8 data[0];
};

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

Regards,
Michael

#10 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 27 lines · Michael Straube
Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
         u32 cmd;
         u8 sta_addr[ETH_ALEN];
         union {
         ...
         ...
         }; u
};
struct ieee_param_ex {
	u32 cmd;
	u8 sta_addr[ETH_ALEN];
	u8 data[0];
};
Quoted 7 lines

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

It's enough to just state that the uses are properly aligned as long as you looked and understand that it's required.

author
#11 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 32 lines · Joe Perches
Quoted 31 lines · Michael Straube
Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
          u32 cmd;
          u8 sta_addr[ETH_ALEN];
          union {
          ...
          ...
          }; u
};
struct ieee_param_ex {
	u32 cmd;
	u8 sta_addr[ETH_ALEN];
	u8 data[0];
};

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Quoted 7 lines
Quoted 3 lines

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

It's enough to just state that the uses are properly aligned as long as you looked and understand that it's required.

Ok, thank you.

I looked at it and understand that it's required. But, as mentioned, at second look I'm not sure about the union.

I guess I need to read a little more about the alignment of unions. Any hints are welcomed. :)

Michael

author
#12 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 41 lines · Michael Straube
Quoted 32 lines · Joe Perches
Quoted 31 lines · Michael Straube
Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
          u32 cmd;
          u8 sta_addr[ETH_ALEN];
          union {
          ...
          ...
          }; u
};

struct ieee_param_ex {
    u32 cmd;
    u8 sta_addr[ETH_ALEN];
    u8 data[0];
};

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Quoted 7 lines
Quoted 3 lines

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

It's enough to just state that the uses are properly aligned as long as you looked and understand that it's required.

Quoted 6 lines

Ok, thank you.

I looked at it and understand that it's required. But, as mentioned, at second look I'm not sure about the union.

I guess I need to read a little more about the alignment of unions.

For now I will resend the series without this patch.
I don't feel comfortable with sending something I don't fully understand, yet.

Michael

#13 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

In reply to Michael Straube
Quoted 41 lines · Michael Straube
Quoted 32 lines · Joe Perches
Quoted 31 lines · Michael Straube
Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
          u32 cmd;
          u8 sta_addr[ETH_ALEN];
          union {
          ...
          ...
          }; u
};
struct ieee_param_ex {
	u32 cmd;
	u8 sta_addr[ETH_ALEN];
	u8 data[0];
};

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Quoted 7 lines
Quoted 3 lines

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

It's enough to just state that the uses are properly aligned as long as you looked and understand that it's required.

Quoted 7 lines

Ok, thank you.

I looked at it and understand that it's required. But, as mentioned, at second look I'm not sure about the union.

I guess I need to read a little more about the alignment of unions. Any hints are welcomed. :)

The union doesn't matter here.
Only the locations of sta_addr matter.
Both follow a u32, so they are also aligned to a u32.

That is actually not guaranteed by the c standard as the u8 array could have arbitrary padding bytes inserted between the u32, but no compiler used by the kernel does that.

author
#14 lore raw

Re: [PATCH 8/8] staging: rtl8188eu: use is_broadcast_ether_addr

Quoted 50 lines · Joe Perches
Quoted 41 lines · Michael Straube
Quoted 32 lines · Joe Perches
Quoted 31 lines · Michael Straube
Quoted 8 lines · Joe Perches
Quoted 2 lines · Michael Straube

Use is_broadcast_ether_addr instead of checking each byte of the address array for 0xff. Shortens the code and improves readability.

You should show in the commit log that sta_addr is __aligned(2) as required by is_broadcast_ether_addr, otherwise you could be introducing runtime alignment defects.

Ok, sta_addr is used from following structs.

struct ieee_param {
           u32 cmd;
           u8 sta_addr[ETH_ALEN];
           union {
           ...
           ...
           }; u
};
struct ieee_param_ex {
	u32 cmd;
	u8 sta_addr[ETH_ALEN];
	u8 data[0];
};

Well, looking at it now, I'm not sure about the alignment anymore in the struct that contains the union. Is sta_addr in the first struct __aligned(2)?

Quoted 7 lines
Quoted 3 lines

Should I include the snippets in the commit message, or is just writing that sta_addr is __aligned(2) enough? (if it is in the first case...)

It's enough to just state that the uses are properly aligned as long as you looked and understand that it's required.

Quoted 7 lines

Ok, thank you.

I looked at it and understand that it's required. But, as mentioned, at second look I'm not sure about the union.

I guess I need to read a little more about the alignment of unions. Any hints are welcomed. :)

Quoted 9 lines

The union doesn't matter here.
Only the locations of sta_addr matter.
Both follow a u32, so they are also aligned to a u32.

That is actually not guaranteed by the c standard as the u8 array could have arbitrary padding bytes inserted between the u32, but no compiler used by the kernel does that.

Ah ok, thank you.

Michael