staging: rtl8723bs: Use list iterators and helpers

PATCH In review Guenter Roeck 5 participants 11 messages Apr 28, 2021 – Jun 7, 2021 Staging

Share & tools

Guenter Roeck

author

2021-04-28 17:33 UTC #0 lore raw

PATCH 1/1 staging: rtl8723bs: Use list iterators and helpers

The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.

The following coccinelle script was used to convert the code.

@@
identifier v1, v2, v3, v4;
symbol next;
expression e;
iterator name list_for_each;
statement S;
@@

<+...
(
- e = v1->next;
|
- e = get_next(v1);
)
... when != e
- while ( \( v1 != e \| e != v1 \) )
+ list_for_each (e, v1)

  {
    ...
-   v2 = container_of(e, struct v3, v4);
+   v2 = list_entry(e, struct v3, v4);
?-  if (!v2) S
    ...

(
- e = e->next;
|
- e = get_next(e);
)

    ... when != e

}
...+>

@@
identifier v1, v2, v3, v4;
symbol next;
expression e;
iterator name list_for_each;
statement S;
@@

<+...
(
- e = v1->next;
|
- e = get_next(v1);
)
... when != e
- while (1)
+ list_for_each (e, v1)
{
- if ( \( e == v1 \| v1 == e \) )

-   break;
    ...
-   v2 = container_of(e, struct v3, v4);
+   v2 = list_entry(e, struct v3, v4);
?-  if (!v2) S
    ...

(
- e = e->next;
|
- e = get_next(e);
)

    ... when != e

}
...+>

Manually fixed up formatting, and added auto-removed comments back in. Compile tested only.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_ap.c       | 58 +++++----------
 drivers/staging/rtl8723bs/core/rtw_mlme.c     | 72 +++++--------------
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 16 ++---
 drivers/staging/rtl8723bs/core/rtw_sta_mgt.c  | 36 +++-------
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 43 ++++-------
 .../staging/rtl8723bs/hal/rtl8723bs_xmit.c    |  7 +-
 .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 19 ++---
 .../staging/rtl8723bs/os_dep/ioctl_linux.c    | 47 +++---------
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c |  8 +--
 9 files changed, 82 insertions(+), 224 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ap.c +16 ~~-42~~

index 4a9bd4825fab..1b92cf9da524 100644
@@ -188,13 +188,9 @@ void expire_timeout_chk(struct adapter *padapter)
 	spin_lock_bh(&pstapriv->auth_list_lock);
 
 	phead = &pstapriv->auth_list;
-	plist = get_next(phead);
-
 	/* check auth_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, auth_list);
-
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, auth_list);
 
 		if (psta->expire_to > 0) {
 			psta->expire_to--;

@@ -217,12 +213,9 @@ void expire_timeout_chk(struct adapter *padapter)
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 
 	phead = &pstapriv->asoc_list;
-	plist = get_next(phead);
-
 	/* check asoc_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, asoc_list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, asoc_list);
 		if (chk_sta_is_alive(psta) || !psta->expire_to) {
 			psta->expire_to = pstapriv->expire_to;
 			psta->keep_alive_trycnt = 0;

@@ -1210,11 +1203,8 @@ int rtw_acl_add_sta(struct adapter *padapter, u8 *addr)
 	spin_lock_bh(&(pacl_node_q->lock));
 
 	phead = get_list_head(pacl_node_q);
-	plist = get_next(phead);
-
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		paclnode = list_entry(plist, struct rtw_wlan_acl_node, list);
 
 		if (!memcmp(paclnode->addr, addr, ETH_ALEN)) {
 			if (paclnode->valid == true) {

@@ -1266,11 +1256,8 @@ void rtw_acl_remove_sta(struct adapter *padapter, u8 *addr)
 	spin_lock_bh(&(pacl_node_q->lock));
 
 	phead = get_list_head(pacl_node_q);
-	plist = get_next(phead);
-
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		paclnode = list_entry(plist, struct rtw_wlan_acl_node, list);
 
 		if (
 			!memcmp(paclnode->addr, addr, ETH_ALEN) ||

@@ -1716,13 +1703,9 @@ void associated_clients_update(struct adapter *padapter, u8 updated)
 		spin_lock_bh(&pstapriv->asoc_list_lock);
 
 		phead = &pstapriv->asoc_list;
-		plist = get_next(phead);
-
 		/* check asoc_queue */
-		while (phead != plist) {
-			psta = container_of(plist, struct sta_info, asoc_list);
-
-			plist = get_next(plist);
+		list_for_each(plist, phead) {
+			psta = list_entry(plist, struct sta_info, asoc_list);
 
 			VCS_update(padapter, psta);
 		}

@@ -1972,13 +1955,9 @@ void rtw_sta_flush(struct adapter *padapter)
 
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 	phead = &pstapriv->asoc_list;
-	plist = get_next(phead);
-
 	/* free sta asoc_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, asoc_list);
-
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, asoc_list);
 
 		list_del_init(&psta->asoc_list);
 		pstapriv->asoc_list_cnt--;

@@ -2075,13 +2054,10 @@ void rtw_ap_restore_network(struct adapter *padapter)
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 
 	phead = &pstapriv->asoc_list;
-	plist = get_next(phead);
-
-	while (phead != plist) {
+	list_for_each(plist, phead) {
 		int stainfo_offset;
 
-		psta = container_of(plist, struct sta_info, asoc_list);
-		plist = get_next(plist);
+		psta = list_entry(plist, struct sta_info, asoc_list);
 
 		stainfo_offset = rtw_stainfo_offset(pstapriv, psta);
 		if (stainfo_offset_valid(stainfo_offset))

@@ -2184,10 +2160,8 @@ void stop_ap_mode(struct adapter *padapter)
 	/* for ACL */
 	spin_lock_bh(&(pacl_node_q->lock));
 	phead = get_list_head(pacl_node_q);
-	plist = get_next(phead);
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		paclnode = list_entry(plist, struct rtw_wlan_acl_node, list);
 
 		if (paclnode->valid) {
 			paclnode->valid = false;

drivers/staging/rtl8723bs/core/rtw_mlme.c +16 ~~-56~~

135 lines not on this page · raw message

drivers/staging/rtl8723bs/core/rtw_mlme_ext.c +6 ~~-10~~

index 9031cf7657ae..b01ff36cf08d 100644
@@ -5135,10 +5135,9 @@ void _linked_info_dump(struct adapter *padapter)
 
 			spin_lock_bh(&pstapriv->asoc_list_lock);
 			phead = &pstapriv->asoc_list;
-			plist = get_next(phead);
-			while (phead != plist) {
-				psta = container_of(plist, struct sta_info, asoc_list);
-				plist = get_next(plist);
+			list_for_each(plist, phead) {
+				psta = list_entry(plist, struct sta_info,
+						  asoc_list);
 			}
 			spin_unlock_bh(&pstapriv->asoc_list_lock);

@@ -6080,12 +6079,9 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf)
 		spin_lock_bh(&pxmitpriv->lock);
 
 		xmitframe_phead = get_list_head(&psta_bmc->sleep_q);
-		xmitframe_plist = get_next(xmitframe_phead);
-
-		while (xmitframe_phead != xmitframe_plist) {
-			pxmitframe = container_of(xmitframe_plist, struct xmit_frame, list);
-
-			xmitframe_plist = get_next(xmitframe_plist);
+		list_for_each(xmitframe_plist, xmitframe_phead) {
+			pxmitframe = list_entry(xmitframe_plist,
+						struct xmit_frame, list);
 
 			list_del_init(&pxmitframe->list);

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +11 ~~-25~~

index 85663182b388..9cb2c7a112d2 100644
@@ -124,11 +124,8 @@ void kfree_all_stainfo(struct sta_priv *pstapriv)
 	spin_lock_bh(&pstapriv->sta_hash_lock);
 
 	phead = get_list_head(&pstapriv->free_sta_queue);
-	plist = get_next(phead);
-
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, list);
 	}
 
 	spin_unlock_bh(&pstapriv->sta_hash_lock);

@@ -152,13 +149,11 @@ u32 _rtw_free_sta_priv(struct	sta_priv *pstapriv)
 		spin_lock_bh(&pstapriv->sta_hash_lock);
 		for (index = 0; index < NUM_STA; index++) {
 			phead = &(pstapriv->sta_hash[index]);
-			plist = get_next(phead);
-
-			while (phead != plist) {
+			list_for_each(plist, phead) {
 				int i;
 
-				psta = container_of(plist, struct sta_info, hash_list);
-				plist = get_next(plist);
+				psta = list_entry(plist, struct sta_info,
+						  hash_list);
 
 				for (i = 0; i < 16 ; i++) {
 					preorder_ctrl = &psta->recvreorder_ctrl[i];

@@ -442,12 +437,8 @@ void rtw_free_all_stainfo(struct adapter *padapter)
 
 	for (index = 0; index < NUM_STA; index++) {
 		phead = &(pstapriv->sta_hash[index]);
-		plist = get_next(phead);
-
-		while (phead != plist) {
-			psta = container_of(plist, struct sta_info, hash_list);
-
-			plist = get_next(plist);
+		list_for_each(plist, phead) {
+			psta = list_entry(plist, struct sta_info, hash_list);
 
 			if (pbcmc_stainfo != psta)
 				rtw_free_stainfo(padapter, psta);

@@ -479,17 +470,14 @@ struct sta_info *rtw_get_stainfo(struct sta_priv *pstapriv, u8 *hwaddr)
 	spin_lock_bh(&pstapriv->sta_hash_lock);
 
 	phead = &(pstapriv->sta_hash[index]);
-	plist = get_next(phead);
-
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, hash_list);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, hash_list);
 
 		if ((!memcmp(psta->hwaddr, addr, ETH_ALEN)))
 		 /*  if found the matched address */
 			break;
 
 		psta = NULL;
-		plist = get_next(plist);
 	}
 
 	spin_unlock_bh(&pstapriv->sta_hash_lock);

@@ -539,10 +527,8 @@ u8 rtw_access_ctrl(struct adapter *padapter, u8 *mac_addr)
 
 	spin_lock_bh(&(pacl_node_q->lock));
 	phead = get_list_head(pacl_node_q);
-	plist = get_next(phead);
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		paclnode = list_entry(plist, struct rtw_wlan_acl_node, list);
 
 		if (!memcmp(paclnode->addr, mac_addr, ETH_ALEN))
 			if (paclnode->valid == true) {

drivers/staging/rtl8723bs/core/rtw_xmit.c +13 ~~-30~~

73 lines not on this page · raw message

drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c +3 -4

index bd95e62fb053..ec9f275cbc7e 100644
@@ -223,12 +223,11 @@ static s32 xmit_xmitframes(struct adapter *padapter, struct xmit_priv *pxmitpriv
 		spin_lock_bh(&pxmitpriv->lock);
 
 		sta_phead = get_list_head(phwxmit->sta_queue);
-		sta_plist = get_next(sta_phead);
 		/* because stop_sta_xmit may delete sta_plist at any time */
 		/* so we should add lock here, or while loop can not exit */
-		while (sta_phead != sta_plist) {
-			ptxservq = container_of(sta_plist, struct tx_servq, tx_pending);
-			sta_plist = get_next(sta_plist);
+		list_for_each(sta_plist, sta_phead) {
+			ptxservq = list_entry(sta_plist, struct tx_servq,
+					      tx_pending);
 
 			pframe_queue = &ptxservq->sta_pending;

drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c +4 ~~-15~~

index c1dac6eec59f..11eadefc7225 100644
@@ -1262,14 +1262,9 @@ void rtw_cfg80211_surveydone_event_callback(struct adapter *padapter)
 	spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
 
 	phead = get_list_head(queue);
-	plist = get_next(phead);
-
-	while (1)
+	list_for_each(plist, phead)
 	{
-		if (phead == plist)
-			break;
-
-		pnetwork = container_of(plist, struct wlan_network, list);
+		pnetwork = list_entry(plist, struct wlan_network, list);
 
 		/* report network only if the current channel set contains the channel to which this network belongs */
 		if (rtw_ch_set_search_ch(padapter->mlmeextpriv.channel_set, pnetwork->network.Configuration.DSConfig) >= 0

@@ -1281,8 +1276,6 @@ void rtw_cfg80211_surveydone_event_callback(struct adapter *padapter)
 			rtw_cfg80211_inform_bss(padapter, pnetwork);
 		}
 
-		plist = get_next(plist);
-
 	}
 
 	spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));

@@ -2486,13 +2479,9 @@ static int cfg80211_rtw_del_station(struct wiphy *wiphy, struct net_device *ndev
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 
 	phead = &pstapriv->asoc_list;
-	plist = get_next(phead);
-
 	/* check asoc_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, asoc_list);
-
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, asoc_list);
 
 		if (!memcmp((u8 *)mac, psta->hwaddr, ETH_ALEN)) {
 			if (psta->dot8021xalg != 1 || psta->bpairwise_key_installed) {

drivers/staging/rtl8723bs/os_dep/ioctl_linux.c +10 ~~-37~~

88 lines not on this page · raw message

drivers/staging/rtl8723bs/os_dep/xmit_linux.c +3 -5

index 639408eaf4df..29b579dbf1f3 100644
@@ -139,13 +139,11 @@ static int rtw_mlcst2unicst(struct adapter *padapter, struct sk_buff *skb)
 
 	spin_lock_bh(&pstapriv->asoc_list_lock);
 	phead = &pstapriv->asoc_list;
-	plist = get_next(phead);
-
 	/* free sta asoc_queue */
-	while (phead != plist) {
+	list_for_each(plist, phead) {
 		int stainfo_offset;
-		psta = container_of(plist, struct sta_info, asoc_list);
-		plist = get_next(plist);
+
+		psta = list_entry(plist, struct sta_info, asoc_list);
 
 		stainfo_offset = rtw_stainfo_offset(pstapriv, psta);
 		if (stainfo_offset_valid(stainfo_offset)) {

Signature · 1 line

2.17.1

Greg Kroah-Hartman

maintainer

2021-05-03 16:48 UTC #1 lore raw

Quoted 8 lines · Guenter Roeck

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +0 -0

index 85663182b388..9cb2c7a112d2 100644
@@ -124,11 +124,8 @@ void kfree_all_stainfo(struct sta_priv *pstapriv)
 	spin_lock_bh(&pstapriv->sta_hash_lock);
 
 	phead = get_list_head(&pstapriv->free_sta_queue);

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c void kfree_all_stainfo(struct sta_priv *pstapriv) · line 131

Quoted 10 lines

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +2 -5

-	plist = get_next(phead);
-
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, list);
 	}
 
 	spin_unlock_bh(&pstapriv->sta_hash_lock);

This chunk didn't apply to my tree as someone else cleaned something in this function.

But that doesn't even really matter as this function does nothing at all!

So I'll just leave it alone, and apply the patch without this chunk.

crazy code...

greg k-h

Guenter Roeck

author

2021-05-03 17:37 UTC #2 lore raw

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Guenter Roeck

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +2 -5

index 85663182b388..9cb2c7a112d2 100644
@@ -124,11 +124,8 @@ void kfree_all_stainfo(struct sta_priv *pstapriv)
 	spin_lock_bh(&pstapriv->sta_hash_lock);
 
 	phead = get_list_head(&pstapriv->free_sta_queue);
-	plist = get_next(phead);
-
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, list);
-		plist = get_next(plist);
+	list_for_each(plist, phead) {
+		psta = list_entry(plist, struct sta_info, list);
 	}

Quoted 9 lines

drivers/staging/rtl8723bs/core/rtw_sta_mgt.c void kfree_all_stainfo(struct sta_priv *pstapriv) · line 131
Quoted 1 line
drivers/staging/rtl8723bs/core/rtw_sta_mgt.c +0 -0
 	spin_unlock_bh(&pstapriv->sta_hash_lock);
This chunk didn't apply to my tree as someone else cleaned something in this function.
But that doesn't even really matter as this function does nothing at all!

You are correct. Weird.

Guenter

Fabio Aiuto

2021-06-04 17:26 UTC #3 lore raw

In reply to Guenter Roeck

Hello Guenter,

Quoted 2 lines · Guenter Roeck

The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.

I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.

Only a power off is allowed.

I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.

I'm trying to find out what's wrong with this patch, have you any suggestions?

thank you,

fabio

Guenter Roeck

author

2021-06-05 00:54 UTC #4 lore raw

Quoted 10 lines · Fabio Aiuto

Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.

Quoted 7 lines

Only a power off is allowed.
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?

Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.

Guenter

Hans de Goede

2021-06-07 09:26 UTC #5 lore raw

Hi,

Quoted 13 lines · Guenter Roeck

Quoted 12 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.

Quoted 8 lines

Quoted 5 lines
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.

Since 5.13 is already at rc5 now, maybe we should revert this and try again later ?

Regards,

Hans

Dan Carpenter

2021-06-07 11:17 UTC #6 lore raw

In reply to Guenter Roeck

Quoted 13 lines · Guenter Roeck

Quoted 12 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.

Quoted 8 lines

Quoted 5 lines
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.

Ah... Crud. As near as I can tell Martin fixed a lot of these in the driver he is working on, rtl8188eu. But they still aren't fixed in rtl8723bs. I looked at the functions and the following loops need to changed to list_for_each_safe(). (Doing a search for list_del_init() helped during review).

expire_timeout_chk() (both loops)
rtw_acl_remove_sta()
rtw_sta_flush()
stop_ap_mode()

rtw_free_network_queue()
chk_bmc_sleepq_hdl()
rtw_free_all_stainfo()
rtw_free_xmitframe_queue()
dequeue_xmitframes_to_sleeping_queue()
wakeup_sta_to_xmit() (both loops)
xmit_delivery_enabled_frames()

xmit_xmitframes()
cfg80211_rtw_del_station()

regards,
dan carpenter

Guenter Roeck

author

2021-06-07 11:26 UTC #7 lore raw

Quoted 20 lines · Dan Carpenter

Quoted 19 lines · Guenter Roeck
Quoted 17 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?

Quoted 8 lines

Quoted 2 lines
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.
Ah... Crud. As near as I can tell Martin fixed a lot of these in the driver he is working on, rtl8188eu. But they still aren't fixed in rtl8723bs. I looked at the functions and the following loops need to changed to list_for_each_safe(). (Doing a search for list_del_init() helped during review).

Hans wants the patch introducing the problem reverted, so I won't bother sending a fix. Sorry for the trouble. Learned a lesson (I hope).

Guenter

Quoted 20 lines

expire_timeout_chk() (both loops)
rtw_acl_remove_sta()
rtw_sta_flush()
stop_ap_mode()
rtw_free_network_queue()
chk_bmc_sleepq_hdl()
rtw_free_all_stainfo()
rtw_free_xmitframe_queue()
dequeue_xmitframes_to_sleeping_queue()
wakeup_sta_to_xmit() (both loops)
xmit_delivery_enabled_frames()
xmit_xmitframes()
cfg80211_rtw_del_station()
regards,
dan carpenter

Fabio Aiuto

2021-06-07 11:36 UTC #8 lore raw

Quoted 30 lines · Guenter Roeck

Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Guenter Roeck
Quoted 17 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?
Quoted 8 lines
Quoted 2 lines
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.
Ah... Crud. As near as I can tell Martin fixed a lot of these in the driver he is working on, rtl8188eu. But they still aren't fixed in rtl8723bs. I looked at the functions and the following loops need to changed to list_for_each_safe(). (Doing a search for list_del_init() helped during review).

Quoted 25 lines

Hans wants the patch introducing the problem reverted, so I won't bother sending a fix. Sorry for the trouble. Learned a lesson (I hope).
Guenter
Quoted 20 lines
expire_timeout_chk() (both loops)
rtw_acl_remove_sta()
rtw_sta_flush()
stop_ap_mode()
rtw_free_network_queue()
chk_bmc_sleepq_hdl()
rtw_free_all_stainfo()
rtw_free_xmitframe_queue()
dequeue_xmitframes_to_sleeping_queue()
wakeup_sta_to_xmit() (both loops)
xmit_delivery_enabled_frames()
xmit_xmitframes()
cfg80211_rtw_del_station()
regards,
dan carpenter

thanks to Guenter suggestion I made the fix needed, if there's no particular hurry to revert the change I'm submitting it today or tomorrow at most. Will cross check with Dan's hint either.

thank you,

fabio

Dan Carpenter

2021-06-07 12:11 UTC #9 lore raw

Quoted 52 lines · Fabio Aiuto

Quoted 51 lines · Guenter Roeck
Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Guenter Roeck
Quoted 17 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?
Quoted 8 lines
Quoted 2 lines
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.
Ah... Crud. As near as I can tell Martin fixed a lot of these in the driver he is working on, rtl8188eu. But they still aren't fixed in rtl8723bs. I looked at the functions and the following loops need to changed to list_for_each_safe(). (Doing a search for list_del_init() helped during review).
Hans wants the patch introducing the problem reverted, so I won't bother sending a fix. Sorry for the trouble. Learned a lesson (I hope).
Guenter
Quoted 16 lines
expire_timeout_chk() (both loops)
rtw_acl_remove_sta()
rtw_sta_flush()
stop_ap_mode()
rtw_free_network_queue()
chk_bmc_sleepq_hdl()
rtw_free_all_stainfo()
rtw_free_xmitframe_queue()
dequeue_xmitframes_to_sleeping_queue()
wakeup_sta_to_xmit() (both loops)
xmit_delivery_enabled_frames()
xmit_xmitframes()
cfg80211_rtw_del_station()

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
regards,
dan carpenter
thanks to Guenter suggestion I made the fix needed, if there's no particular hurry to revert the change I'm submitting it today or tomorrow at most. Will cross check with Dan's hint either.

Thanks Fabio. I feel like we can fix this. No need to revert. I'm going through the changes to rtl8188eu and I've spotted a couple functions which Martin missed so I'm going to send those as well.

regards,
dan carpenter

Hans de Goede

2021-06-07 15:34 UTC #10 lore raw

Hi,

Quoted 58 lines · Dan Carpenter

Quoted 57 lines · Fabio Aiuto
Quoted 55 lines · Guenter Roeck
Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Guenter Roeck
Quoted 17 lines · Fabio Aiuto
Hello Guenter,
Quoted 2 lines · Guenter Roeck
The rtl8723bs driver manually re-implements list helper functions and macros in various ways. Replace with existing list helpers.
I'm testing rtl8723bs on a baytrail tablet (Lenovo Ideapad MIIX 300-10IBY) and applying the tag staging-5.13-rc4, loading r8723bs makes the whole system freezing while trying to connect to local AP.
Only a power off is allowed.
I found that commit b3cd518c5abd42fbc747ef55a5fdc40bf7bf01c0 (staging: rtl8723bs: Use list iterators and helpers) introduced the bug.
I'm trying to find out what's wrong with this patch, have you any suggestions?
Quoted 8 lines
Quoted 2 lines
Some of the iterators needed the _safe variant which I didn't take into account. I thought that was fixed, but maybe some locations were missed.
Ah... Crud. As near as I can tell Martin fixed a lot of these in the driver he is working on, rtl8188eu. But they still aren't fixed in rtl8723bs. I looked at the functions and the following loops need to changed to list_for_each_safe(). (Doing a search for list_del_init() helped during review).
Hans wants the patch introducing the problem reverted, so I won't bother sending a fix. Sorry for the trouble. Learned a lesson (I hope).
Guenter
Quoted 20 lines
expire_timeout_chk() (both loops)
rtw_acl_remove_sta()
rtw_sta_flush()
stop_ap_mode()
rtw_free_network_queue()
chk_bmc_sleepq_hdl()
rtw_free_all_stainfo()
rtw_free_xmitframe_queue()
dequeue_xmitframes_to_sleeping_queue()
wakeup_sta_to_xmit() (both loops)
xmit_delivery_enabled_frames()
xmit_xmitframes()
cfg80211_rtw_del_station()
regards,
dan carpenter

Quoted 5 lines

Quoted 3 lines
thanks to Guenter suggestion I made the fix needed, if there's no particular hurry to revert the change I'm submitting it today or tomorrow at most. Will cross check with Dan's hint either.
Thanks Fabio. I feel like we can fix this. No need to revert.

Ack. My suggestion for reverting this was because I did not know a fix was in the works / almost ready, fixing it indeed is better,

Regards,

Hans