Remove the goto statement from rtw_init_drv_sw(). In this function goto
can be replace by return statement. As on goto label exit, function
only return it is not performing any cleanup. Avoiding goto will
improve the function readability.
Signed-off-by: Saurav Girepunje <[email protected]>
---
ChangelogV2:
-On V1 combined the if condition check for functions as below
if (!rtw_init_evt_priv(&padapter->evtpriv) || !rtw_init_mlme_priv(padapter))
return _FAIL;
reverting these changes and keeping them as-is. It will make more clear on
individual function call if something need to handle for error case.
ChangelogV1:
-Remove the goto statement from rtw_init_drv_sw(). In this function goto
can be replace by return statement. As on goto label exit, function
only return it is not performing any cleanup. Avoiding goto will
improve the function readability.
drivers/staging/r8188eu/os_dep/os_intfs.c | 34 ++++++++---------------
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 1418c9c4916c..ec96e837ab88 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -480,48 +480,37 @@ u8 rtw_init_drv_sw(struct adapter *padapter)
{
u8 ret8 = _SUCCESS;
- if ((rtw_init_cmd_priv(&padapter->cmdpriv)) == _FAIL) {
- ret8 = _FAIL;
- goto exit;
- }
+ if ((rtw_init_cmd_priv(&padapter->cmdpriv)) == _FAIL)
+ return _FAIL;
padapter->cmdpriv.padapter = padapter;
- if ((rtw_init_evt_priv(&padapter->evtpriv)) == _FAIL) {
- ret8 = _FAIL;
- goto exit;
- }
+ if ((rtw_init_evt_priv(&padapter->evtpriv)) == _FAIL)
+ return _FAIL;
- if (rtw_init_mlme_priv(padapter) == _FAIL) {
- ret8 = _FAIL;
- goto exit;
- }
+ if (rtw_init_mlme_priv(padapter) == _FAIL)
+ return _FAIL;
rtw_init_wifidirect_timers(padapter);
init_wifidirect_info(padapter, P2P_ROLE_DISABLE);
reset_global_wifidirect_info(padapter);
- if (init_mlme_ext_priv(padapter) == _FAIL) {
- ret8 = _FAIL;
- goto exit;
- }
+ if (init_mlme_ext_priv(padapter) == _FAIL)
+ return _FAIL;
if (_rtw_init_xmit_priv(&padapter->xmitpriv, padapter) == _FAIL) {
DBG_88E("Can't _rtw_init_xmit_priv\n");
- ret8 = _FAIL;
- goto exit;
+ return _FAIL;
}
if (_rtw_init_recv_priv(&padapter->recvpriv, padapter) == _FAIL) {
DBG_88E("Can't _rtw_init_recv_priv\n");
- ret8 = _FAIL;
- goto exit;
+ return _FAIL;
}
if (_rtw_init_sta_priv(&padapter->stapriv) == _FAIL) {
DBG_88E("Can't _rtw_init_sta_priv\n");
- ret8 = _FAIL;
- goto exit;
+ return _FAIL;
}
padapter->stapriv.padapter = padapter;
@@ -539,7 +528,6 @@ u8 rtw_init_drv_sw(struct adapter *padapter)
spin_lock_init(&padapter->br_ext_lock);
-exit:
return ret8;
}
--
2.33.0
On Wed, Nov 03, 2021 at 09:18:41PM +0530, Saurav Girepunje wrote:
> Remove the goto statement from rtw_init_drv_sw(). In this function goto
> can be replace by return statement. As on goto label exit, function
> only return it is not performing any cleanup. Avoiding goto will
> improve the function readability.
>
> Signed-off-by: Saurav Girepunje <[email protected]>
> ---
>
> ChangelogV2:
>
> -On V1 combined the if condition check for functions as below
>
> if (!rtw_init_evt_priv(&padapter->evtpriv) || !rtw_init_mlme_priv(padapter))
> return _FAIL;
> reverting these changes and keeping them as-is. It will make more clear on
> individual function call if something need to handle for error case.
>
> ChangelogV1:
>
> -Remove the goto statement from rtw_init_drv_sw(). In this function goto
> can be replace by return statement. As on goto label exit, function
> only return it is not performing any cleanup. Avoiding goto will
> improve the function readability.
>
> drivers/staging/r8188eu/os_dep/os_intfs.c | 34 ++++++++---------------
> 1 file changed, 11 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> index 1418c9c4916c..ec96e837ab88 100644
> --- a/drivers/staging/r8188eu/os_dep/os_intfs.c
> +++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
> @@ -480,48 +480,37 @@ u8 rtw_init_drv_sw(struct adapter *padapter)
> {
> u8 ret8 = _SUCCESS;
>
> - if ((rtw_init_cmd_priv(&padapter->cmdpriv)) == _FAIL) {
> - ret8 = _FAIL;
> - goto exit;
> - }
> + if ((rtw_init_cmd_priv(&padapter->cmdpriv)) == _FAIL)
> + return _FAIL;
>
> padapter->cmdpriv.padapter = padapter;
>
> - if ((rtw_init_evt_priv(&padapter->evtpriv)) == _FAIL) {
> - ret8 = _FAIL;
> - goto exit;
> - }
> + if ((rtw_init_evt_priv(&padapter->evtpriv)) == _FAIL)
> + return _FAIL;
>
> - if (rtw_init_mlme_priv(padapter) == _FAIL) {
> - ret8 = _FAIL;
> - goto exit;
> - }
> + if (rtw_init_mlme_priv(padapter) == _FAIL)
> + return _FAIL;
>
> rtw_init_wifidirect_timers(padapter);
> init_wifidirect_info(padapter, P2P_ROLE_DISABLE);
> reset_global_wifidirect_info(padapter);
>
> - if (init_mlme_ext_priv(padapter) == _FAIL) {
> - ret8 = _FAIL;
> - goto exit;
> - }
> + if (init_mlme_ext_priv(padapter) == _FAIL)
> + return _FAIL;
>
> if (_rtw_init_xmit_priv(&padapter->xmitpriv, padapter) == _FAIL) {
> DBG_88E("Can't _rtw_init_xmit_priv\n");
> - ret8 = _FAIL;
> - goto exit;
> + return _FAIL;
> }
>
> if (_rtw_init_recv_priv(&padapter->recvpriv, padapter) == _FAIL) {
> DBG_88E("Can't _rtw_init_recv_priv\n");
> - ret8 = _FAIL;
> - goto exit;
> + return _FAIL;
> }
>
> if (_rtw_init_sta_priv(&padapter->stapriv) == _FAIL) {
> DBG_88E("Can't _rtw_init_sta_priv\n");
> - ret8 = _FAIL;
> - goto exit;
> + return _FAIL;
> }
>
> padapter->stapriv.padapter = padapter;
Right after this, ret8 is set, but never checked, which looks like a bug
to me. Can you work on fixing that after I take this patch?
thanks,
greg k-h
On 11/5/21 12:53, Greg KH wrote:
> Right after this, ret8 is set, but never checked, which looks like a bug
> to me. Can you work on fixing that after I take this patch?
>
> thanks,
>
> greg k-h
>
ret8 is returned from this function, but as I said [1] it can be just
removed. It will be always set to _SUCCESS.
[1]
https://lore.kernel.org/linux-staging/[email protected]/
With regards,
Pavel Skripkin
On 05/11/21 4:06 pm, Pavel Skripkin wrote:
> On 11/5/21 12:53, Greg KH wrote:
>> Right after this, ret8 is set, but never checked, which looks like a bug
>> to me. Can you work on fixing that after I take this patch?
Yes, I will send a patch removing a local variable for return value and changing
the return type of function rtw_init_default_value . As rtw_init_default_value always return
success and the return value of this function is not checked.
>>
>> thanks,
>>
>> greg k-h
>>
>
> ret8 is returned from this function, but as I said [1] it can be just removed. It will be always set to _SUCCESS.
>
>
>
> [1] https://lore.kernel.org/linux-staging/[email protected]/
>
>
Pavel also mentioned to remove the local variable ret8.
> With regards,
> Pavel Skripkin
Thanks,
Saurav