[PATCH v2 0/6] staging: r8188eu: fix warnings reported by checkpatch

[PATCH v2 0/6] staging: r8188eu: fix warnings reported by checkpatch

[Jaehee Park]

These patches address style issues found by checkpatch in the
core/rtw_mlme.c file.

changes in v2:
- changed the first patch to from removing braces around single-lined 
statements, to a patch that removes the entire if-statement (because it
was redundant). The member within that statement is not being used so 
this patch has been changed to a patch that removes this unused member.
- the log of the third patch has edited.
 
Jaehee Park (6):
  staging: r8188eu: remove unused member free_bss_buf
  staging: r8188eu: remove spaces before tabs
  staging: r8188eu: remove 'added by' author comments
  staging: r8188eu: place constants on the right side of tests
  staging: r8188eu: replace spaces with tabs
  staging: r8188eu: correct typo in comments

 drivers/staging/r8188eu/include/rtw_mlme.h |  1 -
 drivers/staging/r8188eu/core/rtw_mlme.c    | 46 +++++++++-------------
 2 files changed, 18 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Jaehee Park]

The free_bss_buf member of pmlmepriv is unused. Remove all related
lines.

Suggested-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/include/rtw_mlme.h | 1 -
 drivers/staging/r8188eu/core/rtw_mlme.c    | 7 -------
 2 files changed, 8 deletions(-)

diff --git a/drivers/staging/r8188eu/include/rtw_mlme.h b/drivers/staging/r8188eu/include/rtw_mlme.h
index 1dc1fbf049af..0f03ac43079c 100644
--- a/drivers/staging/r8188eu/include/rtw_mlme.h
+++ b/drivers/staging/r8188eu/include/rtw_mlme.h
@@ -319,7 +319,6 @@ struct mlme_priv {
 	struct list_head *pscanned;
 	struct __queue free_bss_pool;
 	struct __queue scanned_queue;
-	u8 *free_bss_buf;
 	u8	key_mask; /* use to restore wep key after hal_init */
 	u32	num_of_scanned;
 
diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index 3e9882f89f76..aed868d1d47b 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -61,7 +61,6 @@ static int _rtw_init_mlme_priv(struct adapter *padapter)
 		res = _FAIL;
 		goto exit;
 	}
-	pmlmepriv->free_bss_buf = pbuf;
 
 	pnetwork = (struct wlan_network *)pbuf;
 
@@ -109,13 +108,7 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv)
 
 void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 {
-
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
-
-	if (pmlmepriv) {
-		vfree(pmlmepriv->free_bss_buf);
-	}
-
 }
 
 struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* _queue *free_queue) */
-- 
2.25.1

[PATCH v2 2/6] staging: r8188eu: remove spaces before tabs

[Jaehee Park]

Delete spaces before tabs in the comments. Issue found with checkpatch.
WARNING: please, no space before tabs

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index aed868d1d47b..1620ca181bf7 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -567,8 +567,8 @@ static void rtw_add_network(struct adapter *adapter,
 
 /* select the desired network based on the capability of the (i)bss. */
 /*  check items:	(1) security */
-/* 			(2) network_type */
-/* 			(3) WMM */
+/*			(2) network_type */
+/*			(3) WMM */
 /*			(4) HT */
 /*			(5) others */
 static bool rtw_is_desired_network(struct adapter *adapter, struct wlan_network *pnetwork)
@@ -903,9 +903,9 @@ static struct sta_info *rtw_joinbss_update_stainfo(struct adapter *padapter, str
 			memset((u8 *)&psta->dot11txpn, 0, sizeof(union pn48));
 			memset((u8 *)&psta->dot11rxpn, 0, sizeof(union pn48));
 		}
-		/* 	Commented by Albert 2012/07/21 */
-		/* 	When doing the WPS, the wps_ie_len won't equal to 0 */
-		/* 	And the Wi-Fi driver shouldn't allow the data packet to be tramsmitted. */
+		/*	Commented by Albert 2012/07/21 */
+		/*	When doing the WPS, the wps_ie_len won't equal to 0 */
+		/*	And the Wi-Fi driver shouldn't allow the data packet to be tramsmitted. */
 		if (padapter->securitypriv.wps_ie_len != 0) {
 			psta->ieee8021x_blocked = true;
 			padapter->securitypriv.wps_ie_len = 0;
@@ -1628,8 +1628,8 @@ int rtw_restruct_wmm_ie(struct adapter *adapter, u8 *in_ie, u8 *out_ie, uint in_
 /*  */
 /*  Search by BSSID, */
 /*  Return Value: */
-/* 		-1		:if there is no pre-auth key in the  table */
-/* 		>= 0		:if there is pre-auth key, and   return the entry id */
+/*		-1		:if there is no pre-auth key in the  table */
+/*		>= 0		:if there is pre-auth key, and   return the entry id */
 /*  */
 /*  */
 
-- 
2.25.1

[PATCH v2 3/6] staging: r8188eu: remove 'added by' author comments

[Jaehee Park]

Author comments "Added by Albert" and "Added by Annie" are sprinkled
through the file. These comments are not useful and can be removed.

Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>
Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index 1620ca181bf7..bb9d595a90b9 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -903,7 +903,6 @@ static struct sta_info *rtw_joinbss_update_stainfo(struct adapter *padapter, str
 			memset((u8 *)&psta->dot11txpn, 0, sizeof(union pn48));
 			memset((u8 *)&psta->dot11rxpn, 0, sizeof(union pn48));
 		}
-		/*	Commented by Albert 2012/07/21 */
 		/*	When doing the WPS, the wps_ie_len won't equal to 0 */
 		/*	And the Wi-Fi driver shouldn't allow the data packet to be tramsmitted. */
 		if (padapter->securitypriv.wps_ie_len != 0) {
@@ -1622,9 +1621,6 @@ int rtw_restruct_wmm_ie(struct adapter *adapter, u8 *in_ie, u8 *out_ie, uint in_
 	return ielength;
 }
 
-/*  */
-/*  Ported from 8185: IsInPreAuthKeyList(). (Renamed from SecIsInPreAuthKeyList(), 2006-10-13.) */
-/*  Added by Annie, 2006-05-07. */
 /*  */
 /*  Search by BSSID, */
 /*  Return Value: */
-- 
2.25.1

[PATCH v2 4/6] staging: r8188eu: place constants on the right side of tests

[Jaehee Park]

To comply with the linux coding style, place constants on the right
side of the test in comparisons. Issue found with checkpatch.
WARNING: Comparisons should place the constant on the right side of
the test.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index bb9d595a90b9..d57f2ffa069e 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -715,7 +715,7 @@ void rtw_surveydone_event_callback(struct adapter	*adapter, u8 *pbuf)
 			set_fwstate(pmlmepriv, _FW_UNDER_LINKING);
 			pmlmepriv->to_join = false;
 			s_ret = rtw_select_and_join_from_scanned_queue(pmlmepriv);
-			if (_SUCCESS == s_ret) {
+			if (s_ret == _SUCCESS) {
 			     _set_timer(&pmlmepriv->assoc_timer, MAX_JOIN_TIMEOUT);
 			} else if (s_ret == 2) { /* there is no need to wait for join */
 				_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
@@ -723,7 +723,8 @@ void rtw_surveydone_event_callback(struct adapter	*adapter, u8 *pbuf)
 			} else {
 				if (rtw_to_roaming(adapter) != 0) {
 					if (--pmlmepriv->to_roaming == 0 ||
-					    _SUCCESS != rtw_sitesurvey_cmd(adapter, &pmlmepriv->assoc_ssid, 1, NULL, 0)) {
+					    rtw_sitesurvey_cmd(adapter, &pmlmepriv->assoc_ssid,
+							       1, NULL, 0) != _SUCCESS) {
 						rtw_set_roaming(adapter, 0);
 						rtw_free_assoc_resources(adapter, 1);
 						rtw_indicate_disconnect(adapter);
@@ -1964,7 +1965,7 @@ void rtw_issue_addbareq_cmd(struct adapter *padapter, struct xmit_frame *pxmitfr
 		issued = (phtpriv->agg_enable_bitmap >> priority) & 0x1;
 		issued |= (phtpriv->candidate_tid_bitmap >> priority) & 0x1;
 
-		if (0 == issued) {
+		if (issued == 0) {
 			psta->htpriv.candidate_tid_bitmap |= BIT((u8)priority);
 			rtw_addbareq_cmd(padapter, (u8)priority, pattrib->ra);
 		}
@@ -1991,19 +1992,19 @@ void _rtw_roaming(struct adapter *padapter, struct wlan_network *tgt_network)
 	else
 		pnetwork = &pmlmepriv->cur_network;
 
-	if (0 < rtw_to_roaming(padapter)) {
+	if (rtw_to_roaming(padapter) > 0) {
 		memcpy(&pmlmepriv->assoc_ssid, &pnetwork->network.Ssid, sizeof(struct ndis_802_11_ssid));
 
 		pmlmepriv->assoc_by_bssid = false;
 
 		while (1) {
 			do_join_r = rtw_do_join(padapter);
-			if (_SUCCESS == do_join_r) {
+			if (do_join_r == _SUCCESS) {
 				break;
 			} else {
 				pmlmepriv->to_roaming--;
 
-				if (0 < pmlmepriv->to_roaming) {
+				if (pmlmepriv->to_roaming > 0) {
 					continue;
 				} else {
 					rtw_indicate_disconnect(padapter);
-- 
2.25.1

[PATCH v2 5/6] staging: r8188eu: replace spaces with tabs

[Jaehee Park]

Use tabs instead of spaces. Issue found with checkpatch.
WARNING: suspect code indent for conditional statements

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index d57f2ffa069e..38ae8be675c4 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -716,7 +716,7 @@ void rtw_surveydone_event_callback(struct adapter	*adapter, u8 *pbuf)
 			pmlmepriv->to_join = false;
 			s_ret = rtw_select_and_join_from_scanned_queue(pmlmepriv);
 			if (s_ret == _SUCCESS) {
-			     _set_timer(&pmlmepriv->assoc_timer, MAX_JOIN_TIMEOUT);
+				_set_timer(&pmlmepriv->assoc_timer, MAX_JOIN_TIMEOUT);
 			} else if (s_ret == 2) { /* there is no need to wait for join */
 				_clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
 				rtw_indicate_connect(adapter);
-- 
2.25.1

[PATCH v2 6/6] staging: r8188eu: correct typo in comments

[Jaehee Park]

Correct misspellings in the comments. Issue found with checkpatch.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index 38ae8be675c4..58c7d6cc4a3f 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -186,7 +186,7 @@ void _rtw_free_network_nolock(struct	mlme_priv *pmlmepriv, struct wlan_network *
 /*
 	return the wlan_network with the matching addr
 
-	Shall be calle under atomic context... to avoid possible racing condition...
+	Shall be called under atomic context... to avoid possible racing condition...
 */
 struct wlan_network *_rtw_find_network(struct __queue *scanned_queue, u8 *addr)
 {
@@ -322,7 +322,7 @@ void rtw_free_network_queue(struct adapter *dev, u8 isfreeall)
 /*
 	return the wlan_network with the matching addr
 
-	Shall be calle under atomic context... to avoid possible racing condition...
+	Shall be called under atomic context... to avoid possible racing condition...
 */
 struct	wlan_network *rtw_find_network(struct __queue *scanned_queue, u8 *addr)
 {
@@ -905,7 +905,7 @@ static struct sta_info *rtw_joinbss_update_stainfo(struct adapter *padapter, str
 			memset((u8 *)&psta->dot11rxpn, 0, sizeof(union pn48));
 		}
 		/*	When doing the WPS, the wps_ie_len won't equal to 0 */
-		/*	And the Wi-Fi driver shouldn't allow the data packet to be tramsmitted. */
+		/*	And the Wi-Fi driver shouldn't allow the data packet to be transmitted. */
 		if (padapter->securitypriv.wps_ie_len != 0) {
 			psta->ieee8021x_blocked = true;
 			padapter->securitypriv.wps_ie_len = 0;
@@ -1299,7 +1299,7 @@ void rtw_stadel_event_callback(struct adapter *adapter, u8 *pbuf)
 }
 
 /*
-* _rtw_join_timeout_handler - Timeout/faliure handler for CMD JoinBss
+* _rtw_join_timeout_handler - Timeout/failure handler for CMD JoinBss
 * @adapter: pointer to struct adapter structure
 */
 void _rtw_join_timeout_handler (struct adapter *adapter)
@@ -1334,7 +1334,7 @@ void _rtw_join_timeout_handler (struct adapter *adapter)
 }
 
 /*
-* rtw_scan_timeout_handler - Timeout/Faliure handler for CMD SiteSurvey
+* rtw_scan_timeout_handler - Timeout/Failure handler for CMD SiteSurvey
 * @adapter: pointer to struct adapter structure
 */
 void rtw_scan_timeout_handler (struct adapter *adapter)
-- 
2.25.1

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Fabio M. De Francesco]

On venerdì 15 aprile 2022 04:48:32 CEST Jaehee Park wrote:
> The free_bss_buf member of pmlmepriv is unused. Remove all related
> lines.
> 
> Suggested-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
>  drivers/staging/r8188eu/include/rtw_mlme.h | 1 -
>  drivers/staging/r8188eu/core/rtw_mlme.c    | 7 -------
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/include/rtw_mlme.h b/drivers/
staging/r8188eu/include/rtw_mlme.h
> index 1dc1fbf049af..0f03ac43079c 100644
> --- a/drivers/staging/r8188eu/include/rtw_mlme.h
> +++ b/drivers/staging/r8188eu/include/rtw_mlme.h
> @@ -319,7 +319,6 @@ struct mlme_priv {
>  	struct list_head *pscanned;
>  	struct __queue free_bss_pool;
>  	struct __queue scanned_queue;
> -	u8 *free_bss_buf;
>  	u8	key_mask; /* use to restore wep key after hal_init */
>  	u32	num_of_scanned;
>  
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
r8188eu/core/rtw_mlme.c
> index 3e9882f89f76..aed868d1d47b 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> @@ -61,7 +61,6 @@ static int _rtw_init_mlme_priv(struct adapter 
*padapter)
>  		res = _FAIL;
>  		goto exit;
>  	}
> -	pmlmepriv->free_bss_buf = pbuf;

Hi Jaehee,

"pmlmepriv->free_bss_buf" is assigned with "pbuf". The latter is a pointer 
to virtually contiguous memory which was allocated by vmalloc() or 
vzalloc() (I didn't check, but the vfree() in _rtw_free_mlme_priv() tells 
me that indeed it was).

>  	pnetwork = (struct wlan_network *)pbuf;
>  
> @@ -109,13 +108,7 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv 
*pmlmepriv)
>  
>  void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
>  {
> -
>  	rtw_free_mlme_priv_ie_data(pmlmepriv);
> -
> -	if (pmlmepriv) {
> -		vfree(pmlmepriv->free_bss_buf);
> -	}

Therefore, here you are causing a memory leak, which is something you 
should avoid :) 

Why did you delete that call to vfree()? 

I think that you are misunderstanding what Pavel said. Even if it were true 
that the code makes no use of that region of memory (again, I didn't 
check), nevertheless, that memory was allocated somewhere and its address 
is now in "pmlmepriv->free_bss_buf".

If you can confirm that this memory is allocated for no purpose you should 
also remove the call to vmalloc() / vzalloc().

Thanks,

Fabio M. De Francesco  

> -
>  }
>  
>  struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* 
_queue *free_queue) */
> -- 
> 2.25.1
>

Re: [PATCH v2 4/6] staging: r8188eu: place constants on the right side of tests

[Fabio M. De Francesco]

On venerdì 15 aprile 2022 04:48:35 CEST Jaehee Park wrote:
> To comply with the linux coding style, place constants on the right
> side of the test in comparisons. Issue found with checkpatch.
> WARNING: Comparisons should place the constant on the right side of
> the test.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_mlme.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
r8188eu/core/rtw_mlme.c
> index bb9d595a90b9..d57f2ffa069e 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> @@ -715,7 +715,7 @@ void rtw_surveydone_event_callback(struct adapter	
*adapter, u8 *pbuf)
>  			set_fwstate(pmlmepriv, _FW_UNDER_LINKING);
>  			pmlmepriv->to_join = false;
>  			s_ret = 
rtw_select_and_join_from_scanned_queue(pmlmepriv);
> -			if (_SUCCESS == s_ret) {
> +			if (s_ret == _SUCCESS) {
>  			     _set_timer(&pmlmepriv->assoc_timer, 
MAX_JOIN_TIMEOUT);
>  			} else if (s_ret == 2) { /* there is no need 
to wait for join */
>  				_clr_fwstate_(pmlmepriv, 
_FW_UNDER_LINKING);

As Pavel pointed out, "s_ret" can not ever be equal to '2'; this could have 
been an opportunity to remove a couple of lines of dead code and add one 
more patch to your series.

Thanks,

Fabio

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Jaehee Park]

On Fri, Apr 15, 2022 at 06:29:15AM +0200, Fabio M. De Francesco wrote:
> On venerdì 15 aprile 2022 04:48:32 CEST Jaehee Park wrote:
> > The free_bss_buf member of pmlmepriv is unused. Remove all related
> > lines.
> > 
> > Suggested-by: Pavel Skripkin <paskripkin@gmail.com>
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> >  drivers/staging/r8188eu/include/rtw_mlme.h | 1 -
> >  drivers/staging/r8188eu/core/rtw_mlme.c    | 7 -------
> >  2 files changed, 8 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/include/rtw_mlme.h b/drivers/
> staging/r8188eu/include/rtw_mlme.h
> > index 1dc1fbf049af..0f03ac43079c 100644
> > --- a/drivers/staging/r8188eu/include/rtw_mlme.h
> > +++ b/drivers/staging/r8188eu/include/rtw_mlme.h
> > @@ -319,7 +319,6 @@ struct mlme_priv {
> >  	struct list_head *pscanned;
> >  	struct __queue free_bss_pool;
> >  	struct __queue scanned_queue;
> > -	u8 *free_bss_buf;
> >  	u8	key_mask; /* use to restore wep key after hal_init */
> >  	u32	num_of_scanned;
> >  
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
> r8188eu/core/rtw_mlme.c
> > index 3e9882f89f76..aed868d1d47b 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> > @@ -61,7 +61,6 @@ static int _rtw_init_mlme_priv(struct adapter 
> *padapter)
> >  		res = _FAIL;
> >  		goto exit;
> >  	}
> > -	pmlmepriv->free_bss_buf = pbuf;
> 
> Hi Jaehee,
> 
> "pmlmepriv->free_bss_buf" is assigned with "pbuf". The latter is a pointer 
> to virtually contiguous memory which was allocated by vmalloc() or 
> vzalloc() (I didn't check, but the vfree() in _rtw_free_mlme_priv() tells 
> me that indeed it was).
> 
> >  	pnetwork = (struct wlan_network *)pbuf;
> >  
> > @@ -109,13 +108,7 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv 
> *pmlmepriv)
> >  
> >  void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
> >  {
> > -
> >  	rtw_free_mlme_priv_ie_data(pmlmepriv);
> > -
> > -	if (pmlmepriv) {
> > -		vfree(pmlmepriv->free_bss_buf);
> > -	}
> 
> Therefore, here you are causing a memory leak, which is something you 
> should avoid :) 
> 
> Why did you delete that call to vfree()? 
> 
> I think that you are misunderstanding what Pavel said. Even if it were true 
> that the code makes no use of that region of memory (again, I didn't 
> check), nevertheless, that memory was allocated somewhere and its address 
> is now in "pmlmepriv->free_bss_buf".
> 

My understanding of Pavel's response is the free_bss_buf member of the 
pmlmepriv structure wasn't being used anywhere and that the 
rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv 
structure so the second check is redundant. 

However, as Fabio said, the free_bss_buf member is being used and pbuf 
memory is not being freed.
So I'll revert the patch as it was originally (which was just removing 
the {} around the single if statement). 

> If you can confirm that this memory is allocated for no purpose you should 
> also remove the call to vmalloc() / vzalloc().
> 
> Thanks,
> 
> Fabio M. De Francesco  
> 
> > -
> >  }
> >  
> >  struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* 
> _queue *free_queue) */
> > -- 
> > 2.25.1
> >
> 
> 

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 664 bytes --]

Hi Jaehee,

On 4/17/22 23:14, Jaehee Park wrote:
> My understanding of Pavel's response is the free_bss_buf member of the
> pmlmepriv structure wasn't being used anywhere and that the
> rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv
> structure so the second check is redundant.
> 
> However, as Fabio said, the free_bss_buf member is being used and pbuf
> memory is not being freed.
> So I'll revert the patch as it was originally (which was just removing
> the {} around the single if statement).
> 

Why just `pbuf` allocation can't be removed? This memory is just unused, 
isn't it?




With regards,
Pavel Skripkin

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

Re: [PATCH v2 3/6] staging: r8188eu: remove 'added by' author comments

[Pavel Skripkin]

Hi Jaehee,

On 4/15/22 05:48, Jaehee Park wrote:
> Author comments "Added by Albert" and "Added by Annie" are sprinkled
> through the file. These comments are not useful and can be removed.
> 
> Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>

I don't remember giving that tag, but now I am OK with it, since patch 
looks good

Please, don't put random Reviewed-by tags in your commit message, since 
it's not how Reviewed-by tags work

> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>




With regards,
Pavel Skripkin

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Jaehee Park]

On Sun, Apr 17, 2022 at 11:16:38PM +0300, Pavel Skripkin wrote:
> Hi Jaehee,
> 
> On 4/17/22 23:14, Jaehee Park wrote:
> > My understanding of Pavel's response is the free_bss_buf member of the
> > pmlmepriv structure wasn't being used anywhere and that the
> > rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv
> > structure so the second check is redundant.
> > 
> > However, as Fabio said, the free_bss_buf member is being used and pbuf
> > memory is not being freed.
> > So I'll revert the patch as it was originally (which was just removing
> > the {} around the single if statement).
> > 
> 
> Why just `pbuf` allocation can't be removed? This memory is just unused,
> isn't it?
> 
> 
> 
> 
> With regards,
> Pavel Skripkin


The free_bss_buf member is unused. So it can just be removed right? 
I guess I'm confused by what Pablo is saying about causing a memory 
leak by getting rid of the pointer to the memory allocated by pbuf. 
Sorry if I misunderstood. 

Thanks,
Jaehee

Re: [PATCH v2 3/6] staging: r8188eu: remove 'added by' author comments

[Jaehee Park]

On Sun, Apr 17, 2022 at 11:23:03PM +0300, Pavel Skripkin wrote:
> Hi Jaehee,
> 
> On 4/15/22 05:48, Jaehee Park wrote:
> > Author comments "Added by Albert" and "Added by Annie" are sprinkled
> > through the file. These comments are not useful and can be removed.
> > 
> > Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>
> 
> I don't remember giving that tag, but now I am OK with it, since patch looks
> good
> 
> Please, don't put random Reviewed-by tags in your commit message, since it's
> not how Reviewed-by tags work

Got it. Thanks

> 
> > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> 
> 
> 
> 
> With regards,
> Pavel Skripkin

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Fabio M. De Francesco]

On domenica 17 aprile 2022 22:42:00 CEST Jaehee Park wrote:
> On Sun, Apr 17, 2022 at 11:16:38PM +0300, Pavel Skripkin wrote:
> > Hi Jaehee,
> > 
> > On 4/17/22 23:14, Jaehee Park wrote:
> > > My understanding of Pavel's response is the free_bss_buf member of 
the
> > > pmlmepriv structure wasn't being used anywhere and that the
> > > rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv
> > > structure so the second check is redundant.
> > > 
> > > However, as Fabio said, the free_bss_buf member is being used and 
pbuf
> > > memory is not being freed.
> > > So I'll revert the patch as it was originally (which was just 
removing
> > > the {} around the single if statement).

No, Jaehee. This is not what I said :)

> > > 
> > 
> > Why just `pbuf` allocation can't be removed? This memory is just 
unused,
> > isn't it?

What Pavel said is what I said, but using a different argumentation.

> > 
> > 
> > With regards,
> > Pavel Skripkin
> 
> 
> The free_bss_buf member is unused.

Correct.

> So it can just be removed right?

No.


> I guess I'm confused by what Pablo is saying about causing a memory 
> leak

A memory leak is caused when you allocate some memory and then you lose any 
reference to its address so that it cannot be freed. Right?

> by getting rid of the pointer to the memory allocated by pbuf.

No.
 
> Sorry if I misunderstood. 

No problem. Let's rewind...

"pbuf" is assigned with the address of some memory allocated with a call to 
vzalloc(). Since "pbuf" is a local variable, you see that the above-
mentioned address is stored in free_bss_buf using the line "pmlmepriv-
>free_bss_buf = pbuf". Is it clear?

Well, you decided to delete the line that calls vfree(pmlmepriv-
>free_bss_buf). At this point you have that memory leak.

Pavel noted that pmlmepriv->free_bss_buf is unused, but it contains the 
address of a region of memory that was allocated for no purpose.

Therefore, a correct patch should also remove the allocation that was made 
using kzalloc(). If you merely remove the line with vfree() you cause a 
memory leak.

Please don't revert your patch. Just fix it with a new version that also 
delete the line where "pbuf" is assigned with the value returned by 
kzalloc().

I hope that now I've been clearer.

Thanks,

Fabio

> Thanks,
> Jaehee
> 

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Fabio M. De Francesco]

On domenica 17 aprile 2022 23:13:50 CEST Fabio M. De Francesco wrote:
> On domenica 17 aprile 2022 22:42:00 CEST Jaehee Park wrote:
> > On Sun, Apr 17, 2022 at 11:16:38PM +0300, Pavel Skripkin wrote:
> > > Hi Jaehee,
> > > 
> > > On 4/17/22 23:14, Jaehee Park wrote:
> > > > My understanding of Pavel's response is the free_bss_buf member of 
> the
> > > > pmlmepriv structure wasn't being used anywhere and that the
> > > > rtw_free_mlme_riv_ie_data function frees the memory of the 
pmlmepriv
> > > > structure so the second check is redundant.
> > > > 
> > > > However, as Fabio said, the free_bss_buf member is being used and 
> pbuf
> > > > memory is not being freed.
> > > > So I'll revert the patch as it was originally (which was just 
> removing
> > > > the {} around the single if statement).
> 
> No, Jaehee. This is not what I said :)
> 
> > > > 
> > > 
> > > Why just `pbuf` allocation can't be removed? This memory is just 
> unused,
> > > isn't it?
> 
> What Pavel said is what I said, but using a different argumentation.
> 
> > > 
> > > 
> > > With regards,
> > > Pavel Skripkin
> > 
> > 
> > The free_bss_buf member is unused.
> 
> Correct.
> 
> > So it can just be removed right?
> 
> No.
> 
> 
> > I guess I'm confused by what Pablo is saying about causing a memory 
> > leak
> 
> A memory leak is caused when you allocate some memory and then you lose 
any 
> reference to its address so that it cannot be freed. Right?
> 
> > by getting rid of the pointer to the memory allocated by pbuf.
> 
> No.
>  
> > Sorry if I misunderstood. 
> 
> No problem. Let's rewind...
> 
> "pbuf" is assigned with the address of some memory allocated with a call 
to 
> vzalloc(). Since "pbuf" is a local variable, you see that the above-
> mentioned address is stored in free_bss_buf using the line "pmlmepriv-
> >free_bss_buf = pbuf". Is it clear?
> 
> Well, you decided to delete the line that calls vfree(pmlmepriv-
> >free_bss_buf). At this point you have that memory leak.
> 
> Pavel noted that pmlmepriv->free_bss_buf is unused, but it contains the 
> address of a region of memory that was allocated for no purpose.
> 
> Therefore, a correct patch should also remove the allocation that was 
made 
> using kzalloc(). 

Sorry I made a typo: kzalloc() -> vzalloc().

> If you merely remove the line with vfree() you cause a 
> memory leak.
> 
> Please don't revert your patch. Just fix it with a new version that also 
> delete the line where "pbuf" is assigned with the value returned by 
> kzalloc().

Same here: kzalloc() -> vzalloc().

> 
> I hope that now I've been clearer.

Did you find out where is the line that calls vzalloc() and returns the 
address to the local variable called "ptr"?

As, said before. You should delete it too, otherwise you lose that region 
of memory until the driver is un-linked by "modprobe -r <driver>" or the 
system is shutdown.

Fabio

Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

[Jaehee Park]

On Sun, Apr 17, 2022 at 11:13:50PM +0200, Fabio M. De Francesco wrote:
> On domenica 17 aprile 2022 22:42:00 CEST Jaehee Park wrote:
> > On Sun, Apr 17, 2022 at 11:16:38PM +0300, Pavel Skripkin wrote:
> > > Hi Jaehee,
> > > 
> > > On 4/17/22 23:14, Jaehee Park wrote:
> > > > My understanding of Pavel's response is the free_bss_buf member of 
> the
> > > > pmlmepriv structure wasn't being used anywhere and that the
> > > > rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv
> > > > structure so the second check is redundant.
> > > > 
> > > > However, as Fabio said, the free_bss_buf member is being used and 
> pbuf
> > > > memory is not being freed.
> > > > So I'll revert the patch as it was originally (which was just 
> removing
> > > > the {} around the single if statement).
> 
> No, Jaehee. This is not what I said :)
> 
> > > > 
> > > 
> > > Why just `pbuf` allocation can't be removed? This memory is just 
> unused,
> > > isn't it?
> 
> What Pavel said is what I said, but using a different argumentation.
> 
> > > 
> > > 
> > > With regards,
> > > Pavel Skripkin
> > 
> > 
> > The free_bss_buf member is unused.
> 
> Correct.
> 
> > So it can just be removed right?
> 
> No.
> 
> 
> > I guess I'm confused by what Pablo is saying about causing a memory 
> > leak
> 
> A memory leak is caused when you allocate some memory and then you lose any 
> reference to its address so that it cannot be freed. Right?
> 
> > by getting rid of the pointer to the memory allocated by pbuf.
> 
> No.
>  
> > Sorry if I misunderstood. 
> 
> No problem. Let's rewind...
> 
> "pbuf" is assigned with the address of some memory allocated with a call to 
> vzalloc(). Since "pbuf" is a local variable, you see that the above-
> mentioned address is stored in free_bss_buf using the line "pmlmepriv-
> >free_bss_buf = pbuf". Is it clear?
> 
> Well, you decided to delete the line that calls vfree(pmlmepriv-
> >free_bss_buf). At this point you have that memory leak.
> 
> Pavel noted that pmlmepriv->free_bss_buf is unused, but it contains the 
> address of a region of memory that was allocated for no purpose.
> 
> Therefore, a correct patch should also remove the allocation that was made 
> using kzalloc(). If you merely remove the line with vfree() you cause a 
> memory leak.

Hi Fabio, Thank you so much for explaining this so patiently!
This makes sense. I'll remove the pbuf vzalloc. 
I think I was having trouble because of of how pnetwork was defined
in this function. I'll have to think a little more about how to 
intialize it.
Thanks,
Jaehee

> 
> Please don't revert your patch. Just fix it with a new version that also 
> delete the line where "pbuf" is assigned with the value returned by 
> kzalloc().
> 
> I hope that now I've been clearer.
> 
> Thanks,
> 
> Fabio
> 
> > Thanks,
> > Jaehee
> > 
> 
> 
> 
>