Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

[Fabio M. De Francesco]

On Sunday, September 26, 2021 12:30:14 PM CEST guolongji wrote:
> kfree(NULL) is safe and the check 'if (pstat->passoc_req)' before kfree
> is not possible.
> 
> Signed-off-by: guolongji <guolongji@uniontech.com>
> ---

Why is it "not possible"? Do you get syntax errors? Are the logic and the 
result of the function altered? If not, control is certainly "possible".

Remember that something may be unnecessary and redundant even if 
it is still possible.

Regards,

Fabio

Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

[Greg KH]

On Sun, Sep 26, 2021 at 06:30:14PM +0800, guolongji wrote:
> kfree(NULL) is safe and the check 'if (pstat->passoc_req)' before kfree
> is not possible.
> 
> Signed-off-by: guolongji <guolongji@uniontech.com>

We need a "full" or "real" name here.  Do you really sign documents with
a lower-case "g" like this?  If so, great, just have to check.

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

[Hans de Goede]

Hi,

On 9/27/21 5:30 AM, Longji Guo wrote:
> kfree(NULL) is safe and the check 'if (pstat->passoc_req)' before kfree
> is not necessary.
> 
> Signed-off-by: Longji Guo <guolongji@uniontech.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 375d2a742dd2..a46ab97524aa 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1344,11 +1344,9 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  			issue_asocrsp(padapter, status, pstat, WIFI_REASSOCRSP);
>  
>  		spin_lock_bh(&pstat->lock);
> -		if (pstat->passoc_req) {
> -			kfree(pstat->passoc_req);
> -			pstat->passoc_req = NULL;
> -			pstat->assoc_req_len = 0;
> -		}
> +		kfree(pstat->passoc_req);
> +		pstat->passoc_req = NULL;

This "pstat->passoc_req = NULL;" is not necessary, since directly afterwards
pstat->passoc_req gets overwritten with the return value from rtw_zmalloc()
so please drop this line too.

Regards,

Hans



> +		pstat->assoc_req_len = 0;
>  
>  		pstat->passoc_req =  rtw_zmalloc(pkt_len);
>  		if (pstat->passoc_req) {
> 

Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

[Greg KH]

On Mon, Sep 27, 2021 at 11:30:56AM +0800, Longji Guo wrote:
> kfree(NULL) is safe and the check 'if (pstat->passoc_req)' before kfree
> is not necessary.
> 
> Signed-off-by: Longji Guo <guolongji@uniontech.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 375d2a742dd2..a46ab97524aa 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1344,11 +1344,9 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  			issue_asocrsp(padapter, status, pstat, WIFI_REASSOCRSP);
>  
>  		spin_lock_bh(&pstat->lock);
> -		if (pstat->passoc_req) {
> -			kfree(pstat->passoc_req);
> -			pstat->passoc_req = NULL;
> -			pstat->assoc_req_len = 0;
> -		}
> +		kfree(pstat->passoc_req);
> +		pstat->passoc_req = NULL;
> +		pstat->assoc_req_len = 0;
>  
>  		pstat->passoc_req =  rtw_zmalloc(pkt_len);
>  		if (pstat->passoc_req) {
> -- 
> 2.20.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH] staging: rtl8723bs: remove meaningless pstat->passoc_req check in OnAssocReq()

[Hans de Goede]

Hi,

On 9/27/21 1:49 PM, Longji Guo wrote:
> kfree(NULL) is safe and the check 'if (pstat->passoc_req)' before kfree
> is not necessary.
> 
> Signed-off-by: Longji Guo <guolongji@uniontech.com>
Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Next time please mark a new (v2) version as such please by changing
the [PATCH] in the subject to [PATCH v2].

You can do this, by for example doing:

git format-patch -v2 HEAD~
git send-email v2-0001-.....patch

Regards,

Hans



> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 375d2a742dd2..51390a30fa55 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1344,12 +1344,8 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  			issue_asocrsp(padapter, status, pstat, WIFI_REASSOCRSP);
>  
>  		spin_lock_bh(&pstat->lock);
> -		if (pstat->passoc_req) {
> -			kfree(pstat->passoc_req);
> -			pstat->passoc_req = NULL;
> -			pstat->assoc_req_len = 0;
> -		}
> -
> +		kfree(pstat->passoc_req);
> +		pstat->assoc_req_len = 0;
>  		pstat->passoc_req =  rtw_zmalloc(pkt_len);
>  		if (pstat->passoc_req) {
>  			memcpy(pstat->passoc_req, pframe, pkt_len);
>