[PATCH 0/5] staging: r8188eu: use struct ieee80211_mgmt in OnAssocRsp

[PATCH 0/5] staging: r8188eu: use struct ieee80211_mgmt in OnAssocRsp

[Martin Kaiser]

Use a struct ieee80211_mgmt in OnAssocRsp to read elements of the
association response message.

Martin Kaiser (5):
  staging: r8188eu: get da from ieee80211_mgmt
  staging: r8188eu: read status_code from struct ieee80211_mgmt
  staging: r8188eu: read capability info from struct ieee80211_mgmt
  staging: r8188eu: read aid from struct ieee80211_mgmt
  staging: r8188eu: use offsetof for ie start offset

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH 1/5] staging: r8188eu: get da from ieee80211_mgmt

[Martin Kaiser]

Define a struct ieee80211_mgmt in the OnAssocRsp function. Read the
destination address (da) from this struct.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 771910763fec..48ed329cec72 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1268,6 +1268,7 @@ unsigned int OnAssocReq(struct adapter *padapter, struct recv_frame *precv_frame
 
 unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame)
 {
+	struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)precv_frame->rx_data;
 	uint i;
 	int res;
 	unsigned short	status;
@@ -1279,7 +1280,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	uint pkt_len = precv_frame->len;
 
 	/* check A1 matches or not */
-	if (memcmp(myid(&padapter->eeprompriv), get_da(pframe), ETH_ALEN))
+	if (memcmp(myid(&padapter->eeprompriv), mgmt->da, ETH_ALEN))
 		return _SUCCESS;
 
 	if (!(pmlmeinfo->state & (WIFI_FW_AUTH_SUCCESS | WIFI_FW_ASSOC_STATE)))
-- 
2.30.2

Re: [PATCH 1/5] staging: r8188eu: get da from ieee80211_mgmt

[Greg Kroah-Hartman]

On Sun, Jul 24, 2022 at 05:39:13PM +0200, Martin Kaiser wrote:
> Define a struct ieee80211_mgmt in the OnAssocRsp function. Read the
> destination address (da) from this struct.

This explains (sortta) what this patch does, but not why you are doing
this.

To me this looks like a step backwards, why is this change needed at
all?

thanks,

greg k-h

Re: [PATCH 1/5] staging: r8188eu: get da from ieee80211_mgmt

[Martin Kaiser]

Thus wrote Greg Kroah-Hartman (gregkh@linuxfoundation.org):

> On Sun, Jul 24, 2022 at 05:39:13PM +0200, Martin Kaiser wrote:
> > Define a struct ieee80211_mgmt in the OnAssocRsp function. Read the
> > destination address (da) from this struct.

> This explains (sortta) what this patch does, but not why you are doing
> this.

There's a bunch of macros and functions in the r8188eu driver to parse
standard messages. I'm trying to replace these driver-specific versions
with the generic ones that are shared between several drivers.

This patch removs one get_da call. It's not much but hopefully, we can
eventually remove get_da itself.

I know that we should leave the parsing of messages to mac80211 and get
rid of OnAssocRsp etc. Until we're at this point, I hope that doing the
parsing ourselves and using the generic helpers is a useful intermediate
step.

Would it make sense to rewrite the patch description or to summarize the
patches in this series to something like "read some response message
fields from struct ieee80211_mgmt"?

Thanks,
Martin

> To me this looks like a step backwards, why is this change needed at
> all?

> thanks,

> greg k-h

Re: [PATCH 1/5] staging: r8188eu: get da from ieee80211_mgmt

[Greg Kroah-Hartman]

On Thu, Jul 28, 2022 at 07:25:15AM +0200, Martin Kaiser wrote:
> Thus wrote Greg Kroah-Hartman (gregkh@linuxfoundation.org):
> 
> > On Sun, Jul 24, 2022 at 05:39:13PM +0200, Martin Kaiser wrote:
> > > Define a struct ieee80211_mgmt in the OnAssocRsp function. Read the
> > > destination address (da) from this struct.
> 
> > This explains (sortta) what this patch does, but not why you are doing
> > this.
> 
> There's a bunch of macros and functions in the r8188eu driver to parse
> standard messages. I'm trying to replace these driver-specific versions
> with the generic ones that are shared between several drivers.
> 
> This patch removs one get_da call. It's not much but hopefully, we can
> eventually remove get_da itself.
> 
> I know that we should leave the parsing of messages to mac80211 and get
> rid of OnAssocRsp etc. Until we're at this point, I hope that doing the
> parsing ourselves and using the generic helpers is a useful intermediate
> step.
> 
> Would it make sense to rewrite the patch description or to summarize the
> patches in this series to something like "read some response message
> fields from struct ieee80211_mgmt"?

As is, the patch description is not acceptable, so yes, it needs to
change :)

thanks,

greg k-h

[PATCH 2/5] staging: r8188eu: read status_code from struct ieee80211_mgmt

[Martin Kaiser]

Read the status code of the association response message from struct
ieee80211_mgmt instead of parsing the message ourselves.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 48ed329cec72..663048bc8e7d 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1271,7 +1271,6 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)precv_frame->rx_data;
 	uint i;
 	int res;
-	unsigned short	status;
 	struct ndis_802_11_var_ie *pIE;
 	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
@@ -1291,9 +1290,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 
 	_cancel_timer_ex(&pmlmeext->link_timer);
 
-	/* status */
-	status = le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 2));
-	if (status > 0) {
+	if (le16_to_cpu(mgmt->u.assoc_resp.status_code) > 0) {
 		pmlmeinfo->state = WIFI_FW_NULL_STATE;
 		res = -4;
 		goto report_assoc_result;
-- 
2.30.2

[PATCH 3/5] staging: r8188eu: read capability info from struct ieee80211_mgmt

[Martin Kaiser]

Read the capability info of the association response message from struct
ieee80211_mgmt instead of parsing the message ourselves.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 663048bc8e7d..1a68b131f983 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1296,8 +1296,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 		goto report_assoc_result;
 	}
 
-	/* get capabilities */
-	pmlmeinfo->capability = le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN));
+	pmlmeinfo->capability = le16_to_cpu(mgmt->u.assoc_resp.capab_info);
 
 	/* set slot time */
 	pmlmeinfo->slotTime = (pmlmeinfo->capability & BIT(10)) ? 9 : 20;
-- 
2.30.2

[PATCH 4/5] staging: r8188eu: read aid from struct ieee80211_mgmt

[Martin Kaiser]

Read the aid of the association response message from struct
ieee80211_mgmt instead of parsing the message ourselves.

Remove the cast to int, aid is a u16.
Keep the 0x3fff mask that is currently applied to the aid.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 1a68b131f983..25e47efa5b9c 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1301,8 +1301,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	/* set slot time */
 	pmlmeinfo->slotTime = (pmlmeinfo->capability & BIT(10)) ? 9 : 20;
 
-	/* AID */
-	pmlmeinfo->aid = (int)(le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 4)) & 0x3fff);
+	pmlmeinfo->aid = le16_to_cpu(mgmt->u.assoc_resp.aid) & 0x3fff;
 	res = pmlmeinfo->aid;
 
 	/* following are moved to join event callback function */
-- 
2.30.2

[PATCH 5/5] staging: r8188eu: use offsetof for ie start offset

[Martin Kaiser]

Use offsetof to calculate the start offset of the information elements in
an association response message.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 25e47efa5b9c..37c90d9e66bf 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1307,7 +1307,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	/* following are moved to join event callback function */
 	/* to handle HT, WMM, rate adaptive, update MAC reg */
 	/* for not to handle the synchronous IO in the tasklet */
-	for (i = (6 + WLAN_HDR_A3_LEN); i < pkt_len;) {
+	for (i = offsetof(struct ieee80211_mgmt, u.assoc_resp.variable); i < pkt_len;) {
 		pIE = (struct ndis_802_11_var_ie *)(pframe + i);
 
 		switch (pIE->ElementID) {
-- 
2.30.2

Re: [PATCH 0/5] staging: r8188eu: use struct ieee80211_mgmt in OnAssocRsp

[Philipp Hortmann]

On 7/24/22 17:39, Martin Kaiser wrote:
> Use a struct ieee80211_mgmt in OnAssocRsp to read elements of the
> association response message.
> 
> Martin Kaiser (5):
>    staging: r8188eu: get da from ieee80211_mgmt
>    staging: r8188eu: read status_code from struct ieee80211_mgmt
>    staging: r8188eu: read capability info from struct ieee80211_mgmt
>    staging: r8188eu: read aid from struct ieee80211_mgmt
>    staging: r8188eu: use offsetof for ie start offset
> 
>   drivers/staging/r8188eu/core/rtw_mlme_ext.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

[PATCH v2 0/5] staging: r8188eu: use struct ieee80211_mgmt in OnAssocRsp

[Martin Kaiser]

Use a struct ieee80211_mgmt in OnAssocRsp to read elements of the
association response message. This should be easier than parsing the
message manually.

v2:
 - rewrite the commit messages to say to more clearly why (in my opinion)
   the patches change the code for the better

Martin Kaiser (5):
  staging: r8188eu: replace a get_da call
  staging: r8188eu: read status_code from struct ieee80211_mgmt
  staging: r8188eu: read capability info from struct ieee80211_mgmt
  staging: r8188eu: read aid from struct ieee80211_mgmt
  staging: r8188eu: simplify the calculation of ie start offset

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH v2 1/5] staging: r8188eu: replace a get_da call

[Martin Kaiser]

The r8188eu driver implements an internal get_da function to read the
destination address (da) from an incoming message. Callers of this
function should be updated to use the ieee80211 framework, with the goal
of removing get_da eventually.

This patch replaces a get_da call in the OnAssocRsp function.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - rewrite the summary and the commit message

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 32d0e101d0c2..f5f718488929 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1320,6 +1320,7 @@ unsigned int OnAssocReq(struct adapter *padapter, struct recv_frame *precv_frame
 
 unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame)
 {
+	struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)precv_frame->rx_data;
 	uint i;
 	int res;
 	unsigned short	status;
@@ -1331,7 +1332,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	uint pkt_len = precv_frame->len;
 
 	/* check A1 matches or not */
-	if (memcmp(myid(&padapter->eeprompriv), get_da(pframe), ETH_ALEN))
+	if (memcmp(myid(&padapter->eeprompriv), mgmt->da, ETH_ALEN))
 		return _SUCCESS;
 
 	if (!(pmlmeinfo->state & (WIFI_FW_AUTH_SUCCESS | WIFI_FW_ASSOC_STATE)))
-- 
2.30.2

[PATCH v2 2/5] staging: r8188eu: read status_code from struct ieee80211_mgmt

[Martin Kaiser]

Read the status code of the association response message from struct
ieee80211_mgmt. This should be easier to understand and to review than
calculating the offset manually.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - rewrite the commit message

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index f5f718488929..53444cb1dba9 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1323,7 +1323,6 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)precv_frame->rx_data;
 	uint i;
 	int res;
-	unsigned short	status;
 	struct ndis_802_11_var_ie *pIE;
 	struct mlme_ext_priv	*pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
@@ -1343,9 +1342,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 
 	_cancel_timer_ex(&pmlmeext->link_timer);
 
-	/* status */
-	status = le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 2));
-	if (status > 0) {
+	if (le16_to_cpu(mgmt->u.assoc_resp.status_code) > 0) {
 		pmlmeinfo->state = WIFI_FW_NULL_STATE;
 		res = -4;
 		goto report_assoc_result;
-- 
2.30.2

[PATCH v2 3/5] staging: r8188eu: read capability info from struct ieee80211_mgmt

[Martin Kaiser]

Read the capability info of the association response message from struct
ieee80211_mgmt. This should be easier to understand and to review than
calculating the offset manually.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - rewrite the commit message

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 53444cb1dba9..0a78b8bbb66a 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1348,8 +1348,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 		goto report_assoc_result;
 	}
 
-	/* get capabilities */
-	pmlmeinfo->capability = le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN));
+	pmlmeinfo->capability = le16_to_cpu(mgmt->u.assoc_resp.capab_info);
 
 	/* set slot time */
 	pmlmeinfo->slotTime = (pmlmeinfo->capability & BIT(10)) ? 9 : 20;
-- 
2.30.2

[PATCH v2 4/5] staging: r8188eu: read aid from struct ieee80211_mgmt

[Martin Kaiser]

Read the aid of the association response message from struct
ieee80211_mgmt. This should be easier to understand and to review than
calculating the offset manually.

Remove the cast to int, aid is a u16.
Keep the 0x3fff mask that is currently applied to the aid.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - rewrite the commit message

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 0a78b8bbb66a..10074355b82d 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1353,8 +1353,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	/* set slot time */
 	pmlmeinfo->slotTime = (pmlmeinfo->capability & BIT(10)) ? 9 : 20;
 
-	/* AID */
-	pmlmeinfo->aid = (int)(le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 4)) & 0x3fff);
+	pmlmeinfo->aid = le16_to_cpu(mgmt->u.assoc_resp.aid) & 0x3fff;
 	res = pmlmeinfo->aid;
 
 	/* following are moved to join event callback function */
-- 
2.30.2

[PATCH v2 5/5] staging: r8188eu: simplify the calculation of ie start offset

[Martin Kaiser]

Use offsetof to calculate the start offset of the information elements in
an association response message. This should make it easier to understand
how the offset is calculated.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - rewrite the commit message

 drivers/staging/r8188eu/core/rtw_mlme_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 10074355b82d..6d3d5ff9a00e 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -1359,7 +1359,7 @@ unsigned int OnAssocRsp(struct adapter *padapter, struct recv_frame *precv_frame
 	/* following are moved to join event callback function */
 	/* to handle HT, WMM, rate adaptive, update MAC reg */
 	/* for not to handle the synchronous IO in the tasklet */
-	for (i = (6 + WLAN_HDR_A3_LEN); i < pkt_len;) {
+	for (i = offsetof(struct ieee80211_mgmt, u.assoc_resp.variable); i < pkt_len;) {
 		pIE = (struct ndis_802_11_var_ie *)(pframe + i);
 
 		switch (pIE->ElementID) {
-- 
2.30.2

Re: [PATCH v2 0/5] staging: r8188eu: use struct ieee80211_mgmt in OnAssocRsp

[Philipp Hortmann]

On 8/6/22 13:13, Martin Kaiser wrote:
> Use a struct ieee80211_mgmt in OnAssocRsp to read elements of the
> association response message. This should be easier than parsing the
> message manually.
> 
> v2:
>   - rewrite the commit messages to say to more clearly why (in my opinion)
>     the patches change the code for the better
> 
> Martin Kaiser (5):
>    staging: r8188eu: replace a get_da call
>    staging: r8188eu: read status_code from struct ieee80211_mgmt
>    staging: r8188eu: read capability info from struct ieee80211_mgmt
>    staging: r8188eu: read aid from struct ieee80211_mgmt
>    staging: r8188eu: simplify the calculation of ie start offset
> 
>   drivers/staging/r8188eu/core/rtw_mlme_ext.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 
Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150