[PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Srishti Sharma]

Use list_for_each_entry_safe when the list elements may get deleted
during traversal. Done using the following semantic patch by
coccinelle.

@r@
struct list_head* l;
expression e;
identifier m, list_del_init, f;
type T1;
T1* pos;
iterator name list_for_each_entry_safe;
@@

f(...){

+T1* tmp;
...
-while(...)
+list_for_each_entry_safe(pos,tmp,l,m)
{
...
-pos = container_of(l,T1,m);
...
-l=e;
 <+...
  list_del_init(&pos->m)
 ...+>
}
...
}

Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_ap.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
index 32a4837..a2c599f 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ap.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
@@ -1196,7 +1196,7 @@ int rtw_acl_add_sta(struct adapter *padapter, u8 *addr)
 int rtw_acl_remove_sta(struct adapter *padapter, u8 *addr)
 {
 	struct list_head *plist, *phead;
-	struct rtw_wlan_acl_node *paclnode;
+	struct rtw_wlan_acl_node *paclnode, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	struct wlan_acl_pool *pacl_list = &pstapriv->acl_list;
 	struct __queue *pacl_node_q = &pacl_list->acl_node_q;
@@ -1208,10 +1208,7 @@ int rtw_acl_remove_sta(struct adapter *padapter, u8 *addr)
 	phead = get_list_head(pacl_node_q);
 	plist = phead->next;
 
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = plist->next;
-
+	list_for_each_entry_safe(paclnode, tmp, plist, list) {
 		if (!memcmp(paclnode->addr, addr, ETH_ALEN)) {
 			if (paclnode->valid) {
 				paclnode->valid = false;
@@ -1711,7 +1708,7 @@ u8 ap_free_sta(struct adapter *padapter, struct sta_info *psta,
 int rtw_sta_flush(struct adapter *padapter)
 {
 	struct list_head *phead, *plist;
-	struct sta_info *psta = NULL;
+	struct sta_info *psta = NULL, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info	*pmlmeinfo = &pmlmeext->mlmext_info;
@@ -1727,11 +1724,7 @@ int rtw_sta_flush(struct adapter *padapter)
 	plist = phead->next;
 
 	/* free sta asoc_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, asoc_list);
-
-		plist = plist->next;
-
+	list_for_each_entry_safe(psta, tmp, plist, asoc_list) {
 		list_del_init(&psta->asoc_list);
 		pstapriv->asoc_list_cnt--;
 
@@ -1833,7 +1826,7 @@ void start_ap_mode(struct adapter *padapter)
 void stop_ap_mode(struct adapter *padapter)
 {
 	struct list_head *phead, *plist;
-	struct rtw_wlan_acl_node *paclnode;
+	struct rtw_wlan_acl_node *paclnode, *tmp;
 	struct sta_info *psta = NULL;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
@@ -1855,10 +1848,7 @@ void stop_ap_mode(struct adapter *padapter)
 	spin_lock_bh(&pacl_node_q->lock);
 	phead = get_list_head(pacl_node_q);
 	plist = phead->next;
-	while (phead != plist) {
-		paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
-		plist = plist->next;
-
+	list_for_each_entry_safe(paclnode, tmp, plist, list) {
 		if (paclnode->valid) {
 			paclnode->valid = false;
 
-- 
2.7.4

Re: [PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Dan Carpenter]

On Wed, Oct 04, 2017 at 01:58:32AM +0530, Srishti Sharma wrote:
> Use list_for_each_entry_safe when the list elements may get deleted
> during traversal.

This patch is fine as a cleanup but none of these are actually buggy.

regards,
dan carpenter

Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Julia Lawall]

On Wed, 4 Oct 2017, Dan Carpenter wrote:

> On Wed, Oct 04, 2017 at 01:58:32AM +0530, Srishti Sharma wrote:
> > Use list_for_each_entry_safe when the list elements may get deleted
> > during traversal.
>
> This patch is fine as a cleanup but none of these are actually buggy.

I'm not sure what you are getting at with the comment.  The commit doesn't
say that they were buggy.  Perhaps the commit message could have been more
verbose, like "Use list operators on list_head values.
List_for_each_entry_safe is needed because the list elements get deleted
during the traversal"?

thanks,
julia

>
> regards,
> dan carpenter
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20171004112103.cvu5ipkudfpz2adc%40mwanda.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Dan Carpenter]

On Wed, Oct 04, 2017 at 03:39:30PM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 4 Oct 2017, Dan Carpenter wrote:
> 
> > On Wed, Oct 04, 2017 at 01:58:32AM +0530, Srishti Sharma wrote:
> > > Use list_for_each_entry_safe when the list elements may get deleted
> > > during traversal.
> >
> > This patch is fine as a cleanup but none of these are actually buggy.
> 
> I'm not sure what you are getting at with the comment.  The commit doesn't
> say that they were buggy.  Perhaps the commit message could have been more
> verbose, like "Use list operators on list_head values.
> List_for_each_entry_safe is needed because the list elements get deleted
                           ^^^^^^^^^

It is not *needed*, the original code works fine.  The problem with the
original code, is that it's ugly as sin.

> during the traversal"?

The changelog needs to say *why* we're applying the patch.  At first I
thought it was going to fix a use after free.  What I would prefer in
the changelog is something like:  "This patch is a cleanup and doesn't
change runtime behavior.  It changes an open coded list traversal to
use list_for_each_entry_safe."

regards,
dan carpenter

Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Julia Lawall]

On Wed, 4 Oct 2017, Dan Carpenter wrote:

> On Wed, Oct 04, 2017 at 03:39:30PM +0200, Julia Lawall wrote:
> >
> >
> > On Wed, 4 Oct 2017, Dan Carpenter wrote:
> >
> > > On Wed, Oct 04, 2017 at 01:58:32AM +0530, Srishti Sharma wrote:
> > > > Use list_for_each_entry_safe when the list elements may get deleted
> > > > during traversal.
> > >
> > > This patch is fine as a cleanup but none of these are actually buggy.
> >
> > I'm not sure what you are getting at with the comment.  The commit doesn't
> > say that they were buggy.  Perhaps the commit message could have been more
> > verbose, like "Use list operators on list_head values.
> > List_for_each_entry_safe is needed because the list elements get deleted
>                            ^^^^^^^^^
>
> It is not *needed*, the original code works fine.  The problem with the
> original code, is that it's ugly as sin.
>
> > during the traversal"?
>
> The changelog needs to say *why* we're applying the patch.  At first I
> thought it was going to fix a use after free.  What I would prefer in
> the changelog is something like:  "This patch is a cleanup and doesn't
> change runtime behavior.  It changes an open coded list traversal to
> use list_for_each_entry_safe."

OK, Srishti, you can put that.  But change the subject lines too.

julia

>
> regards,
> dan carpenter
>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20171004144012.jciztelhwjdyzpwg%40mwanda.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Srishti Sharma]

On Wed, Oct 4, 2017 at 8:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Wed, 4 Oct 2017, Dan Carpenter wrote:
>
>> On Wed, Oct 04, 2017 at 03:39:30PM +0200, Julia Lawall wrote:
>> >
>> >
>> > On Wed, 4 Oct 2017, Dan Carpenter wrote:
>> >
>> > > On Wed, Oct 04, 2017 at 01:58:32AM +0530, Srishti Sharma wrote:
>> > > > Use list_for_each_entry_safe when the list elements may get deleted
>> > > > during traversal.
>> > >
>> > > This patch is fine as a cleanup but none of these are actually buggy.
>> >
>> > I'm not sure what you are getting at with the comment.  The commit doesn't
>> > say that they were buggy.  Perhaps the commit message could have been more
>> > verbose, like "Use list operators on list_head values.
>> > List_for_each_entry_safe is needed because the list elements get deleted
>>                            ^^^^^^^^^
>>
>> It is not *needed*, the original code works fine.  The problem with the
>> original code, is that it's ugly as sin.
>>
>> > during the traversal"?
>>
>> The changelog needs to say *why* we're applying the patch.  At first I
>> thought it was going to fix a use after free.  What I would prefer in
>> the changelog is something like:  "This patch is a cleanup and doesn't
>> change runtime behavior.  It changes an open coded list traversal to
>> use list_for_each_entry_safe."
>
> OK, Srishti, you can put that.  But change the subject lines too.
Ok will do ! Thanks again.
Regards,
Srishti
>
> julia
>
>>
>> regards,
>> dan carpenter
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20171004144012.jciztelhwjdyzpwg%40mwanda.
>> For more options, visit https://groups.google.com/d/optout.
>>

[PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Srishti Sharma]

Use list_for_each_entry_safe to make the code more compact. 
Done using the following semantic patch by coccinelle.

@r@
struct list_head* l;
expression e;
identifier m, list_del_init, f;
type T1;
T1* pos;
iterator name list_for_each_entry_safe;
@@

f(...){

+T1* tmp;
<+...
-while(...)
+list_for_each_entry_safe(pos,tmp,l,m)
{
...
-pos = container_of(l,T1,m);
...
-l=e;
 <+...
  list_del_init(&pos->m)
 ...+>
}
...+>
}

Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 52f31c7..4e1d06c 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -5446,6 +5446,7 @@ u8 mlme_evt_hdl(struct adapter *padapter, unsigned char *pbuf)
 
 u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf)
 {
+	struct xmit_frame *tmp;
 	if (send_beacon(padapter) == _FAIL) {
 		DBG_88E("issue_beacon, fail!\n");
 		return H2C_PARAMETERS_ERROR;
@@ -5469,11 +5470,8 @@ u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf)
 			xmitframe_phead = get_list_head(&psta_bmc->sleep_q);
 			xmitframe_plist = xmitframe_phead->next;
 
-			while (xmitframe_phead != xmitframe_plist) {
-				pxmitframe = container_of(xmitframe_plist, struct xmit_frame, list);
-
-				xmitframe_plist = xmitframe_plist->next;
-
+			list_for_each_entry_safe(pxmitframe, tmp,
+						 xmitframe_plist, list) {
 				list_del_init(&pxmitframe->list);
 
 				psta_bmc->sleepq_len--;
-- 
2.7.4

[PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Srishti Sharma]

Use list_for_each_entry_safe to make code more compact. Done 
using the following semantic patch by coccinelle.

@r@
struct list_head* l;
expression e;
identifier m,list_del_init,f;
type T1;
T1* pos;
iterator name list_for_each_entry_safe;
@@

f(...){

+T1* tmp;

<+...

-while(...)
+list_for_each_entry_safe(pos,tmp,l,m)
{
...
-pos = container_of(l,T1,m);
...
-l=e;
 <+...
 list_del_init(&pos->m)
 ...+>
}

...+>

}

Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_xmit.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index e8d9858..6d8820b 100644
--- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
+++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
@@ -1869,7 +1869,7 @@ void wakeup_sta_to_xmit(struct adapter *padapter, struct sta_info *psta)
 	u8 update_mask = 0, wmmps_ac = 0;
 	struct sta_info *psta_bmc;
 	struct list_head *xmitframe_plist, *xmitframe_phead;
-	struct xmit_frame *pxmitframe = NULL;
+	struct xmit_frame *pxmitframe = NULL, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 
 	spin_lock_bh(&psta->sleep_q.lock);
@@ -1877,11 +1877,7 @@ void wakeup_sta_to_xmit(struct adapter *padapter, struct sta_info *psta)
 	xmitframe_phead = get_list_head(&psta->sleep_q);
 	xmitframe_plist = xmitframe_phead->next;
 
-	while (xmitframe_phead != xmitframe_plist) {
-		pxmitframe = container_of(xmitframe_plist, struct xmit_frame, list);
-
-		xmitframe_plist = xmitframe_plist->next;
-
+	list_for_each_entry_safe(pxmitframe, tmp, xmitframe_plist, list) {
 		list_del_init(&pxmitframe->list);
 
 		switch (pxmitframe->attrib.priority) {
@@ -1958,11 +1954,8 @@ void wakeup_sta_to_xmit(struct adapter *padapter, struct sta_info *psta)
 		xmitframe_phead = get_list_head(&psta_bmc->sleep_q);
 		xmitframe_plist = xmitframe_phead->next;
 
-		while (xmitframe_phead != xmitframe_plist) {
-			pxmitframe = container_of(xmitframe_plist, struct xmit_frame, list);
-
-			xmitframe_plist = xmitframe_plist->next;
-
+		list_for_each_entry_safe(pxmitframe, tmp, xmitframe_plist,
+					 list) {
 			list_del_init(&pxmitframe->list);
 
 			psta_bmc->sleepq_len--;
@@ -1997,7 +1990,7 @@ void xmit_delivery_enabled_frames(struct adapter *padapter, struct sta_info *pst
 {
 	u8 wmmps_ac = 0;
 	struct list_head *xmitframe_plist, *xmitframe_phead;
-	struct xmit_frame *pxmitframe = NULL;
+	struct xmit_frame *pxmitframe = NULL, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 
 	spin_lock_bh(&psta->sleep_q.lock);
@@ -2005,11 +1998,7 @@ void xmit_delivery_enabled_frames(struct adapter *padapter, struct sta_info *pst
 	xmitframe_phead = get_list_head(&psta->sleep_q);
 	xmitframe_plist = xmitframe_phead->next;
 
-	while (xmitframe_phead != xmitframe_plist) {
-		pxmitframe = container_of(xmitframe_plist, struct xmit_frame, list);
-
-		xmitframe_plist = xmitframe_plist->next;
-
+	list_for_each_entry_safe(pxmitframe, tmp, xmitframe_plist, list) {
 		switch (pxmitframe->attrib.priority) {
 		case 1:
 		case 2:
-- 
2.7.4

[PATCH] Staging: rtl8188eu: core: Use list_for_each_entry_safe

[Srishti Sharma]

Use list_for_each_entry_safe to make the code more compact. Done 
by the following semantic patch by coccinelle.

@r@
struct list_head* l;
expression e;
identifier m,list_del_init,f;
type T1;
T1* pos;
iterator name list_for_each_entry_safe;
@@

f(...){

+T1* tmp;
<+...
-while(...)
+list_for_each_entry_safe(pos,tmp,l,m)
{
...
-pos = container_of(l,T1,m);
...
-l=e;
 <+...
 list_del_init(&pos->m)
 ...+>
}
...+>

}

Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_ap.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
index a2c599f..551af9e 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ap.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
@@ -280,7 +280,7 @@ void	expire_timeout_chk(struct adapter *padapter)
 {
 	struct list_head *phead, *plist;
 	u8 updated = 0;
-	struct sta_info *psta = NULL;
+	struct sta_info *psta = NULL, *tmp;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 	u8 chk_alive_num = 0;
 	char chk_alive_list[NUM_STA];
@@ -292,10 +292,7 @@ void	expire_timeout_chk(struct adapter *padapter)
 	plist = phead->next;
 
 	/* check auth_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, auth_list);
-		plist = plist->next;
-
+	list_for_each_entry_safe(psta, tmp, plist, auth_list) {
 		if (psta->expire_to > 0) {
 			psta->expire_to--;
 			if (psta->expire_to == 0) {
@@ -326,10 +323,7 @@ void	expire_timeout_chk(struct adapter *padapter)
 	plist = phead->next;
 
 	/* check asoc_queue */
-	while (phead != plist) {
-		psta = container_of(plist, struct sta_info, asoc_list);
-		plist = plist->next;
-
+	list_for_each_entry_safe(psta, tmp, plist, asoc_list) {
 		if (chk_sta_is_alive(psta) || !psta->expire_to) {
 			psta->expire_to = pstapriv->expire_to;
 			psta->keep_alive_trycnt = 0;
-- 
2.7.4