Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
error code on error. For the first failure block where ips_leave is
invoked, use -ENOMEM as this is the main cause of failure here anyway.
For the second failure block, use -EBUSY, as it seems the most
appropriate.
Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
rtw_pwr_wakeup as appropriate now that it is converted.
This gets the driver closer to removal of the non-standard _SUCCESS and
_FAIL definitions, which are inverted compared to the standard in-kernel
error code mechanism.
Signed-off-by: Phillip Potter <[email protected]>
---
Changes from V1: Act on feedback from Dan Carpenter:
* Try to use more appropriate error codes than -EPERM.
* Revert the places where existing -1 was converted as they are out of
scope.
* Preserve error codes in places where calling function already uses
proper negative semantics, so that they can be passed through to the
caller.
---
drivers/staging/r8188eu/core/rtw_p2p.c | 4 ++--
drivers/staging/r8188eu/core/rtw_pwrctrl.c | 10 ++++----
drivers/staging/r8188eu/os_dep/ioctl_linux.c | 24 +++++++-------------
3 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index c306aafa183b..bd654d4ff8b4 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -1888,7 +1888,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
if (role == P2P_ROLE_DEVICE || role == P2P_ROLE_CLIENT || role == P2P_ROLE_GO) {
/* leave IPS/Autosuspend */
- if (rtw_pwr_wakeup(padapter) == _FAIL) {
+ if (rtw_pwr_wakeup(padapter)) {
ret = _FAIL;
goto exit;
}
@@ -1902,7 +1902,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
init_wifidirect_info(padapter, role);
} else if (role == P2P_ROLE_DISABLE) {
- if (rtw_pwr_wakeup(padapter) == _FAIL) {
+ if (rtw_pwr_wakeup(padapter)) {
ret = _FAIL;
goto exit;
}
diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
index cf9020a73933..8b1c50668dfe 100644
--- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
+++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
@@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
unsigned long timeout = jiffies + msecs_to_jiffies(3000);
unsigned long deny_time;
- int ret = _SUCCESS;
+ int ret = 0;
while (pwrpriv->ps_processing && time_before(jiffies, timeout))
msleep(10);
/* I think this should be check in IPS, LPS, autosuspend functions... */
if (check_fwstate(pmlmepriv, _FW_LINKED)) {
- ret = _SUCCESS;
+ ret = 0;
goto exit;
}
if (pwrpriv->rf_pwrstate == rf_off && ips_leave(padapter) == _FAIL) {
- ret = _FAIL;
+ ret = -ENOMEM;
goto exit;
}
if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
- ret = _FAIL;
+ ret = -EBUSY;
goto exit;
}
@@ -439,7 +439,7 @@ int rtw_pm_set_ips(struct adapter *padapter, u8 mode)
return 0;
} else if (mode == IPS_NONE) {
rtw_ips_mode_req(pwrctrlpriv, mode);
- if ((padapter->bSurpriseRemoved == 0) && (rtw_pwr_wakeup(padapter) == _FAIL))
+ if ((padapter->bSurpriseRemoved == 0) && rtw_pwr_wakeup(padapter))
return -EFAULT;
} else {
return -EINVAL;
diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
index 930bb4aea435..7f91dac2e41b 100644
--- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
@@ -687,12 +687,9 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
enum ndis_802_11_network_infra networkType;
int ret = 0;
-
-
- if (_FAIL == rtw_pwr_wakeup(padapter)) {
- ret = -EPERM;
+ ret = rtw_pwr_wakeup(padapter);
+ if (ret)
goto exit;
- }
if (!padapter->hw_init_completed) {
ret = -EPERM;
@@ -931,12 +928,9 @@ static int rtw_wx_set_wap(struct net_device *dev,
struct wlan_network *pnetwork = NULL;
enum ndis_802_11_auth_mode authmode;
-
-
- if (_FAIL == rtw_pwr_wakeup(padapter)) {
- ret = -1;
+ ret = rtw_pwr_wakeup(padapter);
+ if (ret)
goto exit;
- }
if (!padapter->bup) {
ret = -1;
@@ -1049,10 +1043,9 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
struct ndis_802_11_ssid ssid[RTW_SSID_SCAN_AMOUNT];
struct wifidirect_info *pwdinfo = &padapter->wdinfo;
- if (_FAIL == rtw_pwr_wakeup(padapter)) {
- ret = -1;
+ ret = rtw_pwr_wakeup(padapter);
+ if (ret)
goto exit;
- }
if (padapter->bDriverStopped) {
ret = -1;
@@ -1252,10 +1245,9 @@ static int rtw_wx_set_essid(struct net_device *dev,
uint ret = 0, len;
- if (_FAIL == rtw_pwr_wakeup(padapter)) {
- ret = -1;
+ ret = rtw_pwr_wakeup(padapter);
+ if (ret)
goto exit;
- }
if (!padapter->bup) {
ret = -1;
--
2.36.1
On 7/26/22 00:07, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> error code on error. For the first failure block where ips_leave is
> invoked, use -ENOMEM as this is the main cause of failure here anyway.
> For the second failure block, use -EBUSY, as it seems the most
> appropriate.
>
> Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> rtw_pwr_wakeup as appropriate now that it is converted.
>
> This gets the driver closer to removal of the non-standard _SUCCESS and
> _FAIL definitions, which are inverted compared to the standard in-kernel
> error code mechanism.
>
> Signed-off-by: Phillip Potter <[email protected]>
> ---
>
> Changes from V1: Act on feedback from Dan Carpenter:
> * Try to use more appropriate error codes than -EPERM.
> * Revert the places where existing -1 was converted as they are out of
> scope.
> * Preserve error codes in places where calling function already uses
> proper negative semantics, so that they can be passed through to the
> caller.
>
> ---
> drivers/staging/r8188eu/core/rtw_p2p.c | 4 ++--
> drivers/staging/r8188eu/core/rtw_pwrctrl.c | 10 ++++----
> drivers/staging/r8188eu/os_dep/ioctl_linux.c | 24 +++++++-------------
> 3 files changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index c306aafa183b..bd654d4ff8b4 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -1888,7 +1888,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
>
> if (role == P2P_ROLE_DEVICE || role == P2P_ROLE_CLIENT || role == P2P_ROLE_GO) {
> /* leave IPS/Autosuspend */
> - if (rtw_pwr_wakeup(padapter) == _FAIL) {
> + if (rtw_pwr_wakeup(padapter)) {
> ret = _FAIL;
> goto exit;
> }
> @@ -1902,7 +1902,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
> init_wifidirect_info(padapter, role);
>
> } else if (role == P2P_ROLE_DISABLE) {
> - if (rtw_pwr_wakeup(padapter) == _FAIL) {
> + if (rtw_pwr_wakeup(padapter)) {
> ret = _FAIL;
> goto exit;
> }
> diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> index cf9020a73933..8b1c50668dfe 100644
> --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> @@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
> struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> unsigned long timeout = jiffies + msecs_to_jiffies(3000);
> unsigned long deny_time;
> - int ret = _SUCCESS;
> + int ret = 0;
>
> while (pwrpriv->ps_processing && time_before(jiffies, timeout))
> msleep(10);
>
> /* I think this should be check in IPS, LPS, autosuspend functions... */
> if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> - ret = _SUCCESS;
> + ret = 0;
> goto exit;
> }
>
> if (pwrpriv->rf_pwrstate == rf_off && ips_leave(padapter) == _FAIL) {
> - ret = _FAIL;
> + ret = -ENOMEM;
> goto exit;
> }
>
> if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
> - ret = _FAIL;
> + ret = -EBUSY;
> goto exit;
> }
>
> @@ -439,7 +439,7 @@ int rtw_pm_set_ips(struct adapter *padapter, u8 mode)
> return 0;
> } else if (mode == IPS_NONE) {
> rtw_ips_mode_req(pwrctrlpriv, mode);
> - if ((padapter->bSurpriseRemoved == 0) && (rtw_pwr_wakeup(padapter) == _FAIL))
> + if ((padapter->bSurpriseRemoved == 0) && rtw_pwr_wakeup(padapter))
> return -EFAULT;
> } else {
> return -EINVAL;
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 930bb4aea435..7f91dac2e41b 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -687,12 +687,9 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
> enum ndis_802_11_network_infra networkType;
> int ret = 0;
>
> -
> -
> - if (_FAIL == rtw_pwr_wakeup(padapter)) {
> - ret = -EPERM;
> + ret = rtw_pwr_wakeup(padapter);
> + if (ret)
> goto exit;
> - }
>
> if (!padapter->hw_init_completed) {
> ret = -EPERM;
> @@ -931,12 +928,9 @@ static int rtw_wx_set_wap(struct net_device *dev,
> struct wlan_network *pnetwork = NULL;
> enum ndis_802_11_auth_mode authmode;
>
> -
> -
> - if (_FAIL == rtw_pwr_wakeup(padapter)) {
> - ret = -1;
> + ret = rtw_pwr_wakeup(padapter);
> + if (ret)
> goto exit;
> - }
>
> if (!padapter->bup) {
> ret = -1;
> @@ -1049,10 +1043,9 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
> struct ndis_802_11_ssid ssid[RTW_SSID_SCAN_AMOUNT];
> struct wifidirect_info *pwdinfo = &padapter->wdinfo;
>
> - if (_FAIL == rtw_pwr_wakeup(padapter)) {
> - ret = -1;
> + ret = rtw_pwr_wakeup(padapter);
> + if (ret)
> goto exit;
> - }
>
> if (padapter->bDriverStopped) {
> ret = -1;
> @@ -1252,10 +1245,9 @@ static int rtw_wx_set_essid(struct net_device *dev,
>
> uint ret = 0, len;
>
> - if (_FAIL == rtw_pwr_wakeup(padapter)) {
> - ret = -1;
> + ret = rtw_pwr_wakeup(padapter);
> + if (ret)
> goto exit;
> - }
>
> if (!padapter->bup) {
> ret = -1;
Tested-by: Philipp Hortmann <[email protected]> # Edimax N150
On Mon, Jul 25, 2022 at 11:07:45PM +0100, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> error code on error. For the first failure block where ips_leave is
> invoked, use -ENOMEM as this is the main cause of failure here anyway.
> For the second failure block, use -EBUSY, as it seems the most
> appropriate.
>
> Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> rtw_pwr_wakeup as appropriate now that it is converted.
>
> This gets the driver closer to removal of the non-standard _SUCCESS and
> _FAIL definitions, which are inverted compared to the standard in-kernel
> error code mechanism.
>
> Signed-off-by: Phillip Potter <[email protected]>
> ---
>
> Changes from V1: Act on feedback from Dan Carpenter:
> * Try to use more appropriate error codes than -EPERM.
> * Revert the places where existing -1 was converted as they are out of
> scope.
> * Preserve error codes in places where calling function already uses
> proper negative semantics, so that they can be passed through to the
> caller.
>
This is a much better patch, right? Everything hangs together better.
There are seven callers which need to be updated and all of them are
updated.
Reviewed-by: Dan Carpenter
On Tue, Jul 26, 2022 at 04:35:59PM +0300, Dan Carpenter wrote:
> On Mon, Jul 25, 2022 at 11:07:45PM +0100, Phillip Potter wrote:
> > Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> > error code on error. For the first failure block where ips_leave is
> > invoked, use -ENOMEM as this is the main cause of failure here anyway.
> > For the second failure block, use -EBUSY, as it seems the most
> > appropriate.
> >
> > Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> > rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> > rtw_pwr_wakeup as appropriate now that it is converted.
> >
> > This gets the driver closer to removal of the non-standard _SUCCESS and
> > _FAIL definitions, which are inverted compared to the standard in-kernel
> > error code mechanism.
> >
> > Signed-off-by: Phillip Potter <[email protected]>
> > ---
> >
> > Changes from V1: Act on feedback from Dan Carpenter:
> > * Try to use more appropriate error codes than -EPERM.
> > * Revert the places where existing -1 was converted as they are out of
> > scope.
> > * Preserve error codes in places where calling function already uses
> > proper negative semantics, so that they can be passed through to the
> > caller.
> >
>
> This is a much better patch, right? Everything hangs together better.
>
> There are seven callers which need to be updated and all of them are
> updated.
>
> Reviewed-by: Dan Carpenter
Oops. I messed up my R-b tag.
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
On Tue, Jul 26, 2022 at 06:46:02PM +0300, Dan Carpenter wrote:
> On Tue, Jul 26, 2022 at 04:35:59PM +0300, Dan Carpenter wrote:
> > On Mon, Jul 25, 2022 at 11:07:45PM +0100, Phillip Potter wrote:
> > > Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> > > error code on error. For the first failure block where ips_leave is
> > > invoked, use -ENOMEM as this is the main cause of failure here anyway.
> > > For the second failure block, use -EBUSY, as it seems the most
> > > appropriate.
> > >
> > > Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> > > rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> > > rtw_pwr_wakeup as appropriate now that it is converted.
> > >
> > > This gets the driver closer to removal of the non-standard _SUCCESS and
> > > _FAIL definitions, which are inverted compared to the standard in-kernel
> > > error code mechanism.
> > >
> > > Signed-off-by: Phillip Potter <[email protected]>
> > > ---
> > >
> > > Changes from V1: Act on feedback from Dan Carpenter:
> > > * Try to use more appropriate error codes than -EPERM.
> > > * Revert the places where existing -1 was converted as they are out of
> > > scope.
> > > * Preserve error codes in places where calling function already uses
> > > proper negative semantics, so that they can be passed through to the
> > > caller.
> > >
> >
> > This is a much better patch, right? Everything hangs together better.
> >
> > There are seven callers which need to be updated and all of them are
> > updated.
> >
> > Reviewed-by: Dan Carpenter
>
> Oops. I messed up my R-b tag.
>
> Reviewed-by: Dan Carpenter <[email protected]>
>
> regards,
> dan carpenter
Agreed, much cleaner this way. Thanks very much for the Reviewed-by tag
:-)
I'll attempt to structure the others in a similar fashion, as far as is
possible anyway.
All the best,
Phil
On Mon, Jul 25, 2022 at 11:07:45PM +0100, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> error code on error. For the first failure block where ips_leave is
> invoked, use -ENOMEM as this is the main cause of failure here anyway.
> For the second failure block, use -EBUSY, as it seems the most
> appropriate.
>
> Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> rtw_pwr_wakeup as appropriate now that it is converted.
>
> This gets the driver closer to removal of the non-standard _SUCCESS and
> _FAIL definitions, which are inverted compared to the standard in-kernel
> error code mechanism.
>
> Signed-off-by: Phillip Potter <[email protected]>
> ---
>
> Changes from V1: Act on feedback from Dan Carpenter:
> * Try to use more appropriate error codes than -EPERM.
> * Revert the places where existing -1 was converted as they are out of
> scope.
> * Preserve error codes in places where calling function already uses
> proper negative semantics, so that they can be passed through to the
> caller.
>
> ---
> drivers/staging/r8188eu/core/rtw_p2p.c | 4 ++--
> drivers/staging/r8188eu/core/rtw_pwrctrl.c | 10 ++++----
> drivers/staging/r8188eu/os_dep/ioctl_linux.c | 24 +++++++-------------
> 3 files changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index c306aafa183b..bd654d4ff8b4 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -1888,7 +1888,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
>
> if (role == P2P_ROLE_DEVICE || role == P2P_ROLE_CLIENT || role == P2P_ROLE_GO) {
> /* leave IPS/Autosuspend */
> - if (rtw_pwr_wakeup(padapter) == _FAIL) {
> + if (rtw_pwr_wakeup(padapter)) {
> ret = _FAIL;
> goto exit;
> }
> @@ -1902,7 +1902,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
> init_wifidirect_info(padapter, role);
>
> } else if (role == P2P_ROLE_DISABLE) {
> - if (rtw_pwr_wakeup(padapter) == _FAIL) {
> + if (rtw_pwr_wakeup(padapter)) {
> ret = _FAIL;
> goto exit;
> }
> diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> index cf9020a73933..8b1c50668dfe 100644
> --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> @@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
> struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> unsigned long timeout = jiffies + msecs_to_jiffies(3000);
> unsigned long deny_time;
> - int ret = _SUCCESS;
> + int ret = 0;
>
> while (pwrpriv->ps_processing && time_before(jiffies, timeout))
> msleep(10);
>
> /* I think this should be check in IPS, LPS, autosuspend functions... */
> if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> - ret = _SUCCESS;
> + ret = 0;
Nit, you don't need to set this again, as you already set it above to 0.
I'll take this as-is, as you are just keeping the original duplicated
logic, but it's something to clean up later.
Nice to see that moving to using the standard error values actually
removed lines of code, that's encouraging.
thanks,
greg k-h
On Wed, Jul 27, 2022 at 03:43:42PM +0300, Dan Carpenter wrote:
> On Wed, Jul 27, 2022 at 08:33:14AM +0200, Greg KH wrote:
> > > diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > > index cf9020a73933..8b1c50668dfe 100644
> > > --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > > +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > > @@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
> > > struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> > > unsigned long timeout = jiffies + msecs_to_jiffies(3000);
> > > unsigned long deny_time;
> > > - int ret = _SUCCESS;
> > > + int ret = 0;
> > >
> > > while (pwrpriv->ps_processing && time_before(jiffies, timeout))
> > > msleep(10);
> > >
> > > /* I think this should be check in IPS, LPS, autosuspend functions... */
> > > if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> > > - ret = _SUCCESS;
> > > + ret = 0;
> >
> > Nit, you don't need to set this again, as you already set it above to 0.
> >
>
> I would sort of prefer to drop the initialization and keep this one.
Fine with me, either works.
greg k-h
On Wed, Jul 27, 2022 at 08:33:14AM +0200, Greg KH wrote:
> > diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > index cf9020a73933..8b1c50668dfe 100644
> > --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> > @@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
> > struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> > unsigned long timeout = jiffies + msecs_to_jiffies(3000);
> > unsigned long deny_time;
> > - int ret = _SUCCESS;
> > + int ret = 0;
> >
> > while (pwrpriv->ps_processing && time_before(jiffies, timeout))
> > msleep(10);
> >
> > /* I think this should be check in IPS, LPS, autosuspend functions... */
> > if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> > - ret = _SUCCESS;
> > + ret = 0;
>
> Nit, you don't need to set this again, as you already set it above to 0.
>
I would sort of prefer to drop the initialization and keep this one.
Otherwise it causes a Smatch warning about missing error codes. It
*looks* buggy too, like it should be an error path. Sometimes people
add a comment explaining why those are success paths and not error paths
which also works.
The Smatch check will no warn if there is a "ret = 0;" within 4(?) lines
of the goto because that's probably intentional.
regards,
dan carpenter