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

[Jaehee Park <jhpark1013@gmail.com>]

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
> >
> 
>