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
The free_bss_buf member of pmlmepriv is unused. Remove all related
lines.
Suggested-by: Pavel Skripkin <[email protected]>
Signed-off-by: Jaehee Park <[email protected]>
---
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
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 <[email protected]>
Suggested-by: Alison Schofield <[email protected]>
Signed-off-by: Jaehee Park <[email protected]>
---
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
Use tabs instead of spaces. Issue found with checkpatch.
WARNING: suspect code indent for conditional statements
Signed-off-by: Jaehee Park <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Jaehee Park <[email protected]>
> ---
> 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
>
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 <[email protected]>
---
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
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
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
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 <[email protected]>
> > Signed-off-by: Jaehee Park <[email protected]>
> > ---
> > 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
> >
>
>
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Jaehee Park <[email protected]>
With regards,
Pavel Skripkin
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
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
> >
>
>
>
>
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 <[email protected]>
>
> 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 <[email protected]>
> > Signed-off-by: Jaehee Park <[email protected]>
>
>
>
>
> With regards,
> Pavel Skripkin
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
>