[PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[Phillip Potter]

Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
error - in all places where we handle this response, we use either -1 or
-EPERM currently anyway, which are equivalent. Also, for other places
along the same call chain where we are using -1, use -EPERM.

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 <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/core/rtw_p2p.c       |  4 +--
 drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 +++---
 drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
 3 files changed, 23 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..1cef3ef4d5f1 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 = -EPERM;
 		goto exit;
 	}
 
 	if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
-		ret = _FAIL;
+		ret = -EPERM;
 		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..e0ae0c3c51f8 100644
--- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
@@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
 
 
 
-	if (_FAIL == rtw_pwr_wakeup(padapter)) {
+	if (rtw_pwr_wakeup(padapter)) {
 		ret = -EPERM;
 		goto exit;
 	}
@@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
 
 
 
-	if (_FAIL == rtw_pwr_wakeup(padapter)) {
-		ret = -1;
+	if (rtw_pwr_wakeup(padapter)) {
+		ret = -EPERM;
 		goto exit;
 	}
 
 	if (!padapter->bup) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
@@ -1049,23 +1049,23 @@ 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;
+	if (rtw_pwr_wakeup(padapter)) {
+		ret = -EPERM;
 		goto exit;
 	}
 
 	if (padapter->bDriverStopped) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
 	if (!padapter->bup) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
 	if (!padapter->hw_init_completed) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
@@ -1164,7 +1164,7 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
 	}
 
 	if (!_status)
-		ret = -1;
+		ret = -EPERM;
 
 exit:
 
@@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
 
 	uint ret = 0, len;
 
-	if (_FAIL == rtw_pwr_wakeup(padapter)) {
-		ret = -1;
+	if (rtw_pwr_wakeup(padapter)) {
+		ret = -EPERM;
 		goto exit;
 	}
 
 	if (!padapter->bup) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
@@ -1268,7 +1268,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
 	}
 
 	if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
-		ret = -1;
+		ret = -EPERM;
 		goto exit;
 	}
 
@@ -1301,7 +1301,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
 				}
 
 				if (!rtw_set_802_11_infrastructure_mode(padapter, pnetwork->network.InfrastructureMode)) {
-					ret = -1;
+					ret = -EPERM;
 					spin_unlock_bh(&queue->lock);
 					goto exit;
 				}
@@ -1312,7 +1312,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
 		spin_unlock_bh(&queue->lock);
 		rtw_set_802_11_authentication_mode(padapter, authmode);
 		if (!rtw_set_802_11_ssid(padapter, &ndis_ssid)) {
-			ret = -1;
+			ret = -EPERM;
 			goto exit;
 		}
 	}
-- 
2.36.1

Re: [PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[Philipp Hortmann]

On 7/24/22 18:30, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
> error - in all places where we handle this response, we use either -1 or
> -EPERM currently anyway, which are equivalent. Also, for other places
> along the same call chain where we are using -1, use -EPERM.
> 
> 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 <phil@philpotter.co.uk>
> ---
>   drivers/staging/r8188eu/core/rtw_p2p.c       |  4 +--
>   drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 +++---
>   drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
>   3 files changed, 23 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..1cef3ef4d5f1 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 = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
> -		ret = _FAIL;
> +		ret = -EPERM;
>   		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..e0ae0c3c51f8 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
>   
>   
>   
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> +	if (rtw_pwr_wakeup(padapter)) {
>   		ret = -EPERM;
>   		goto exit;
>   	}
> @@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
>   
>   
>   
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	if (rtw_pwr_wakeup(padapter)) {
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (!padapter->bup) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
> @@ -1049,23 +1049,23 @@ 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;
> +	if (rtw_pwr_wakeup(padapter)) {
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (padapter->bDriverStopped) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (!padapter->bup) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (!padapter->hw_init_completed) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
> @@ -1164,7 +1164,7 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
>   	}
>   
>   	if (!_status)
> -		ret = -1;
> +		ret = -EPERM;
>   
>   exit:
>   
> @@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
>   
>   	uint ret = 0, len;
>   
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	if (rtw_pwr_wakeup(padapter)) {
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
>   	if (!padapter->bup) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
> @@ -1268,7 +1268,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
>   	}
>   
>   	if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
> -		ret = -1;
> +		ret = -EPERM;
>   		goto exit;
>   	}
>   
> @@ -1301,7 +1301,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
>   				}
>   
>   				if (!rtw_set_802_11_infrastructure_mode(padapter, pnetwork->network.InfrastructureMode)) {
> -					ret = -1;
> +					ret = -EPERM;
>   					spin_unlock_bh(&queue->lock);
>   					goto exit;
>   				}
> @@ -1312,7 +1312,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
>   		spin_unlock_bh(&queue->lock);
>   		rtw_set_802_11_authentication_mode(padapter, authmode);
>   		if (!rtw_set_802_11_ssid(padapter, &ndis_ssid)) {
> -			ret = -1;
> +			ret = -EPERM;
>   			goto exit;
>   		}
>   	}

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

Re: [PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[Dan Carpenter]

On Sun, Jul 24, 2022 at 05:30:55PM +0100, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
> error - in all places where we handle this response, we use either -1 or
> -EPERM currently anyway, which are equivalent. Also, for other places
> along the same call chain where we are using -1, use -EPERM.
> 

I can't get behind a change to -EPERM.  Try to pick an appropriate
error code.  I'm not going to be very strict on it, but we have to at
least *try*.

Probably, leave the return -1; lines alone.  Fixing that seems like an
unrelated change.  We need to do this kind of change but I got bitten by
it before so I want to avoid that next time.  My reviews will hopefully
be more careful now.

> 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 <phil@philpotter.co.uk>
> ---
>  drivers/staging/r8188eu/core/rtw_p2p.c       |  4 +--
>  drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 +++---
>  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
>  3 files changed, 23 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;
>  		}

Fine.  The caller now changes from negative error codes to _SUCCESS/_FAIL.
Later we will transition this to normal error codes so we'll update it
to preserve the error code from rtw_pwr_wakeup() at that point.

> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 930bb4aea435..e0ae0c3c51f8 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
>  
>  
>  
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> +	if (rtw_pwr_wakeup(padapter)) {
>  		ret = -EPERM;
>  		goto exit;
>  	}

This code is returning negative error codes so it should preserve the
code from rtw_pwr_wakeup().

	ret = rtw_pwr_wakeup(padapter);
	if (ret)
		goto exit;

> @@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
>  
>  
>  
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	if (rtw_pwr_wakeup(padapter)) {
> +		ret = -EPERM;

Same.

>  		goto exit;
>  	}
>  
>  	if (!padapter->bup) {
> -		ret = -1;
> +		ret = -EPERM;

Unrelated.

>  		goto exit;
>  	}
>  

[ snip ]

> @@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
>  
>  	uint ret = 0, len;
>  
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	if (rtw_pwr_wakeup(padapter)) {
> +		ret = -EPERM;

Preserve the error code.

>  		goto exit;
>  	}

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[Phillip Potter]

On Mon, Jul 25, 2022 at 01:40:27PM +0300, Dan Carpenter wrote:
> On Sun, Jul 24, 2022 at 05:30:55PM +0100, Phillip Potter wrote:
> > Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
> > error - in all places where we handle this response, we use either -1 or
> > -EPERM currently anyway, which are equivalent. Also, for other places
> > along the same call chain where we are using -1, use -EPERM.
> > 
> 
> I can't get behind a change to -EPERM.  Try to pick an appropriate
> error code.  I'm not going to be very strict on it, but we have to at
> least *try*.
> 
> Probably, leave the return -1; lines alone.  Fixing that seems like an
> unrelated change.  We need to do this kind of change but I got bitten by
> it before so I want to avoid that next time.  My reviews will hopefully
> be more careful now.
> 
> > 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 <phil@philpotter.co.uk>
> > ---
> >  drivers/staging/r8188eu/core/rtw_p2p.c       |  4 +--
> >  drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 +++---
> >  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
> >  3 files changed, 23 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;
> >  		}
> 
> Fine.  The caller now changes from negative error codes to _SUCCESS/_FAIL.
> Later we will transition this to normal error codes so we'll update it
> to preserve the error code from rtw_pwr_wakeup() at that point.
> 
> > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > index 930bb4aea435..e0ae0c3c51f8 100644
> > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > @@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
> >  
> >  
> >  
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > +	if (rtw_pwr_wakeup(padapter)) {
> >  		ret = -EPERM;
> >  		goto exit;
> >  	}
> 
> This code is returning negative error codes so it should preserve the
> code from rtw_pwr_wakeup().
> 
> 	ret = rtw_pwr_wakeup(padapter);
> 	if (ret)
> 		goto exit;
> 
> > @@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
> >  
> >  
> >  
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > -		ret = -1;
> > +	if (rtw_pwr_wakeup(padapter)) {
> > +		ret = -EPERM;
> 
> Same.
> 
> >  		goto exit;
> >  	}
> >  
> >  	if (!padapter->bup) {
> > -		ret = -1;
> > +		ret = -EPERM;
> 
> Unrelated.
> 
> >  		goto exit;
> >  	}
> >  
> 
> [ snip ]
> 
> > @@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
> >  
> >  	uint ret = 0, len;
> >  
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > -		ret = -1;
> > +	if (rtw_pwr_wakeup(padapter)) {
> > +		ret = -EPERM;
> 
> Preserve the error code.
> 
> >  		goto exit;
> >  	}
> 
> regards,
> dan carpenter

Hi Dan,

Thanks very much for the review. I'll get a V2 sent out shortly.

Regards,
Phil

Re: [PATCH] staging: r8188eu: convert rtw_pwr_wakeup to correct error code semantics

[Phillip Potter]

On Sun, Jul 24, 2022 at 08:39:42PM +0200, Philipp Hortmann wrote:
> On 7/24/22 18:30, Phillip Potter wrote:
> > Convert the rtw_pwr_wakeup function to use 0 on success and -EPERM on
> > error - in all places where we handle this response, we use either -1 or
> > -EPERM currently anyway, which are equivalent. Also, for other places
> > along the same call chain where we are using -1, use -EPERM.
> > 
> > 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 <phil@philpotter.co.uk>
> > ---
> >   drivers/staging/r8188eu/core/rtw_p2p.c       |  4 +--
> >   drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 +++---
> >   drivers/staging/r8188eu/os_dep/ioctl_linux.c | 32 ++++++++++----------
> >   3 files changed, 23 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..1cef3ef4d5f1 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 = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
> > -		ret = _FAIL;
> > +		ret = -EPERM;
> >   		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..e0ae0c3c51f8 100644
> > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > @@ -689,7 +689,7 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > +	if (rtw_pwr_wakeup(padapter)) {
> >   		ret = -EPERM;
> >   		goto exit;
> >   	}
> > @@ -933,13 +933,13 @@ static int rtw_wx_set_wap(struct net_device *dev,
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > -		ret = -1;
> > +	if (rtw_pwr_wakeup(padapter)) {
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (!padapter->bup) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> > @@ -1049,23 +1049,23 @@ 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;
> > +	if (rtw_pwr_wakeup(padapter)) {
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (padapter->bDriverStopped) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (!padapter->bup) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (!padapter->hw_init_completed) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> > @@ -1164,7 +1164,7 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
> >   	}
> >   	if (!_status)
> > -		ret = -1;
> > +		ret = -EPERM;
> >   exit:
> > @@ -1252,13 +1252,13 @@ static int rtw_wx_set_essid(struct net_device *dev,
> >   	uint ret = 0, len;
> > -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> > -		ret = -1;
> > +	if (rtw_pwr_wakeup(padapter)) {
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> >   	if (!padapter->bup) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> > @@ -1268,7 +1268,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
> >   	}
> >   	if (check_fwstate(pmlmepriv, WIFI_AP_STATE)) {
> > -		ret = -1;
> > +		ret = -EPERM;
> >   		goto exit;
> >   	}
> > @@ -1301,7 +1301,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
> >   				}
> >   				if (!rtw_set_802_11_infrastructure_mode(padapter, pnetwork->network.InfrastructureMode)) {
> > -					ret = -1;
> > +					ret = -EPERM;
> >   					spin_unlock_bh(&queue->lock);
> >   					goto exit;
> >   				}
> > @@ -1312,7 +1312,7 @@ static int rtw_wx_set_essid(struct net_device *dev,
> >   		spin_unlock_bh(&queue->lock);
> >   		rtw_set_802_11_authentication_mode(padapter, authmode);
> >   		if (!rtw_set_802_11_ssid(padapter, &ndis_ssid)) {
> > -			ret = -1;
> > +			ret = -EPERM;
> >   			goto exit;
> >   		}
> >   	}
> 
> Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

Hi Philipp,

Thank you for testing, much appreciated. Need to do a V2 now, but wanted
to thank you anyway for your effort.

Regards,
Phil