[PATCH 0/2] staging: r8188eu: more error code cleanups

[PATCH 0/2] staging: r8188eu: more error code cleanups

[Phillip Potter]

This small series addresses a cleanup suggestion discussed by Greg
Kroah-Hartman and Dan Carpenter, and also includes another function
conversion. It would be a larger series, but my time being what it is,
I intend to chip away at these slowly and steadily.

Phillip Potter (2):
  staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup
  staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

 drivers/staging/r8188eu/core/rtw_ioctl_set.c    | 8 ++++----
 drivers/staging/r8188eu/core/rtw_pwrctrl.c      | 2 +-
 drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c    | 5 ++---
 4 files changed, 8 insertions(+), 9 deletions(-)

-- 
2.36.1

[PATCH 1/2] staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup

[Phillip Potter]

Remove the success initializer from the ret variable in rtw_pwr_wakeup,
as we set it later anyway in the success path, and also set on failure.
This makes the function appear cleaner and more consistent.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/core/rtw_pwrctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
index 8b1c50668dfe..75e655bae16a 100644
--- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
+++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
@@ -381,7 +381,7 @@ 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 = 0;
+	int ret;
 
 	while (pwrpriv->ps_processing && time_before(jiffies, timeout))
 		msleep(10);
-- 
2.36.1

[PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Phillip Potter]

Convert the rtw_set_802_11_add_wep function to use 0 on success and an
appropriate error code on error. Also convert return type to int to allow
negative return values. For the first failure block where keyid >= 4,
use -EOPNOTSUPP as this is most appropriate, and for the second failure
block where rtw_set_key is called, use -ENOMEM, as this is the cause of
failure in that function anyway - in due course, rtw_set_key can be
converted as well.

Finally, convert both call-sites of rtw_set_802_11_add_wep to use the
new semantics, passing through the error code where we can for one of
them.

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_ioctl_set.c    | 8 ++++----
 drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c    | 5 ++---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
index 17f6bcbeebf4..a5b5d7f6a864 100644
--- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c
+++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
@@ -390,16 +390,16 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11
 	return ret;
 }
 
-u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
+int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
 {
 	int		keyid, res;
 	struct security_priv *psecuritypriv = &padapter->securitypriv;
-	u8		ret = _SUCCESS;
+	int		ret = 0;
 
 	keyid = wep->KeyIndex & 0x3fffffff;
 
 	if (keyid >= 4) {
-		ret = false;
+		ret = -EOPNOTSUPP;
 		goto exit;
 	}
 
@@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
 	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
 
 	if (res == _FAIL)
-		ret = false;
+		ret = -ENOMEM;
 exit:
 
 	return ret;
diff --git a/drivers/staging/r8188eu/include/rtw_ioctl_set.h b/drivers/staging/r8188eu/include/rtw_ioctl_set.h
index 7365079c704f..761b30bdb8ec 100644
--- a/drivers/staging/r8188eu/include/rtw_ioctl_set.h
+++ b/drivers/staging/r8188eu/include/rtw_ioctl_set.h
@@ -11,7 +11,7 @@ typedef u8 NDIS_802_11_PMKID_VALUE[16];
 u8 rtw_set_802_11_authentication_mode(struct adapter *adapt,
 				      enum ndis_802_11_auth_mode authmode);
 u8 rtw_set_802_11_bssid(struct adapter*adapter, u8 *bssid);
-u8 rtw_set_802_11_add_wep(struct adapter *adapter, struct ndis_802_11_wep *wep);
+int rtw_set_802_11_add_wep(struct adapter *adapter, struct ndis_802_11_wep *wep);
 u8 rtw_set_802_11_disassociate(struct adapter *adapter);
 u8 rtw_set_802_11_bssid_list_scan(struct adapter*adapter,
 				  struct ndis_802_11_ssid *pssid,
diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
index 7f91dac2e41b..4d8dbc2a9ef2 100644
--- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
@@ -422,8 +422,7 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param,
 		pwep->KeyIndex |= 0x80000000;
 		memcpy(pwep->KeyMaterial,  param->u.crypt.key, pwep->KeyLength);
 		if (param->u.crypt.set_tx) {
-			if (rtw_set_802_11_add_wep(padapter, pwep) == (u8)_FAIL)
-				ret = -EOPNOTSUPP;
+			ret = rtw_set_802_11_add_wep(padapter, pwep);
 		} else {
 			if (wep_key_idx >= WEP_KEYS) {
 				ret = -EOPNOTSUPP;
@@ -1613,7 +1612,7 @@ static int rtw_wx_set_enc(struct net_device *dev,
 
 	memcpy(wep.KeyMaterial, keybuf, wep.KeyLength);
 
-	if (!rtw_set_802_11_add_wep(padapter, &wep)) {
+	if (rtw_set_802_11_add_wep(padapter, &wep)) {
 		if (rf_on == pwrpriv->rf_pwrstate)
 			ret = -EOPNOTSUPP;
 		goto exit;
-- 
2.36.1

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Dan Carpenter]

On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
>  {
>  	int		keyid, res;
>  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> -	u8		ret = _SUCCESS;
> +	int		ret = 0;
>  
>  	keyid = wep->KeyIndex & 0x3fffffff;
>  
>  	if (keyid >= 4) {
> -		ret = false;
> +		ret = -EOPNOTSUPP;
>  		goto exit;
>  	}
>  
> @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
>  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
>  
>  	if (res == _FAIL)
> -		ret = false;
> +		ret = -ENOMEM;
>  exit:
>  
>  	return ret;

No, this isn't right.  This now returns 1 on success and negative
error codes on error.

There are a couple anti-patterns here:

1) Do nothing gotos
2) Mixing error paths and success paths.

If you avoid mixing error paths and success paths then you get a pattern
called: "Solid return zero."  This is where the end of the function has
a very chunky "return 0;" to mark that it is successful.  You want that.
Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
not chunky.

regards,
dan carpenter

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Phillip Potter]

On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote:
> On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> >  {
> >  	int		keyid, res;
> >  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> > -	u8		ret = _SUCCESS;
> > +	int		ret = 0;
> >  
> >  	keyid = wep->KeyIndex & 0x3fffffff;
> >  
> >  	if (keyid >= 4) {
> > -		ret = false;
> > +		ret = -EOPNOTSUPP;
> >  		goto exit;
> >  	}
> >  
> > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> >  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
> >  
> >  	if (res == _FAIL)
> > -		ret = false;
> > +		ret = -ENOMEM;
> >  exit:
> >  
> >  	return ret;
> 
> No, this isn't right.  This now returns 1 on success and negative
> error codes on error.
> 
> There are a couple anti-patterns here:
> 
> 1) Do nothing gotos
> 2) Mixing error paths and success paths.
> 
> If you avoid mixing error paths and success paths then you get a pattern
> called: "Solid return zero."  This is where the end of the function has
> a very chunky "return 0;" to mark that it is successful.  You want that.
> Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
> not chunky.
> 
> regards,
> dan carpenter
> 

Hi Dan,

Thank you for the review firstly, much appreciated.

I'm happy of course to rewrite this to address any concerns, but
I was hoping I could clarify what you've said though? Apologies if I've
missed it, but how is this function now returning 1 on success? It sets
ret to 0 (success) at the start and then sets it to one of two negative
error codes depending on what happens. Am I missing something here?
(Perfectly possible that I am).

In terms of do nothing gotos, do you mean gotos that just set an error
code then jump to the end? If you'd prefer, as the function just returns
right after the exit label, I can just return the codes directly and have
a 'return 0;' like you say above?

Thanks as always for your insight.

Regards,
Phil

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Joe Perches]

On Sat, 2022-07-30 at 19:36 +0100, Phillip Potter wrote:
> On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote:
> > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  {
> > >  	int		keyid, res;
> > >  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> > > -	u8		ret = _SUCCESS;
> > > +	int		ret = 0;
> > >  
> > >  	keyid = wep->KeyIndex & 0x3fffffff;
> > >  
> > >  	if (keyid >= 4) {
> > > -		ret = false;
> > > +		ret = -EOPNOTSUPP;
> > >  		goto exit;
> > >  	}
> > >  
> > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
> > >  
> > >  	if (res == _FAIL)
> > > -		ret = false;
> > > +		ret = -ENOMEM;
> > >  exit:
> > >  
> > >  	return ret;
> > 
> > No, this isn't right.  This now returns 1 on success and negative
> > error codes on error.
> > 
> > There are a couple anti-patterns here:
> > 
> > 1) Do nothing gotos
> > 2) Mixing error paths and success paths.
> > 
> > If you avoid mixing error paths and success paths then you get a pattern
> > called: "Solid return zero."  This is where the end of the function has
> > a very chunky "return 0;" to mark that it is successful.  You want that.
> > Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
> > not chunky.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> Thank you for the review firstly, much appreciated.
> 
> I'm happy of course to rewrite this to address any concerns, but
> I was hoping I could clarify what you've said though? Apologies if I've
> missed it, but how is this function now returning 1 on success? It sets
> ret to 0 (success) at the start and then sets it to one of two negative
> error codes depending on what happens. Am I missing something here?
> (Perfectly possible that I am).
> 
> In terms of do nothing gotos, do you mean gotos that just set an error
> code then jump to the end? If you'd prefer, as the function just returns
> right after the exit label, I can just return the codes directly and have
> a 'return 0;' like you say above?
> 
> Thanks as always for your insight.

Yes, you've got it right.

I think Dan is suggesting something like the below, but
not necessarily in a single patch:
---
 drivers/staging/r8188eu/core/rtw_ioctl_set.c | 38 ++++++++++++----------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
index 17f6bcbeebf42..2736bbce83b5b 100644
--- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c
+++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
@@ -390,44 +390,38 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11
 	return ret;
 }
 
-u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
+int rtw_set_802_11_add_wep(struct adapter *padapter,
+			   struct ndis_802_11_wep *wep)
 {
-	int		keyid, res;
-	struct security_priv *psecuritypriv = &padapter->securitypriv;
-	u8		ret = _SUCCESS;
+	int keyid;
+	struct security_priv *secpriv = &padapter->securitypriv;
 
 	keyid = wep->KeyIndex & 0x3fffffff;
-
-	if (keyid >= 4) {
-		ret = false;
-		goto exit;
-	}
+	if (keyid >= 4)
+		return -EOPNOTSUPP;
 
 	switch (wep->KeyLength) {
 	case 5:
-		psecuritypriv->dot11PrivacyAlgrthm = _WEP40_;
+		secpriv->dot11PrivacyAlgrthm = _WEP40_;
 		break;
 	case 13:
-		psecuritypriv->dot11PrivacyAlgrthm = _WEP104_;
+		secpriv->dot11PrivacyAlgrthm = _WEP104_;
 		break;
 	default:
-		psecuritypriv->dot11PrivacyAlgrthm = _NO_PRIVACY_;
+		secpriv->dot11PrivacyAlgrthm = _NO_PRIVACY_;
 		break;
 	}
 
-	memcpy(&psecuritypriv->dot11DefKey[keyid].skey[0], &wep->KeyMaterial, wep->KeyLength);
+	memcpy(secpriv->dot11DefKey[keyid].skey, &wep->KeyMaterial,
+	       wep->KeyLength);
 
-	psecuritypriv->dot11DefKeylen[keyid] = wep->KeyLength;
+	secpriv->dot11DefKeylen[keyid] = wep->KeyLength;
+	secpriv->dot11PrivacyKeyIndex = keyid;
 
-	psecuritypriv->dot11PrivacyKeyIndex = keyid;
+	if (rtw_set_key(padapter, secpriv, keyid, 1) == _FAIL)
+		return -ENOMEM;
 
-	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
-
-	if (res == _FAIL)
-		ret = false;
-exit:
-
-	return ret;
+	return 0;
 }
 
 /*

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Dan Carpenter]

On Sun, Jul 31, 2022 at 10:12:56AM -0700, Joe Perches wrote:
> > I'm happy of course to rewrite this to address any concerns, but
> > I was hoping I could clarify what you've said though? Apologies if I've
> > missed it, but how is this function now returning 1 on success? It sets
> > ret to 0 (success) at the start and then sets it to one of two negative
> > error codes depending on what happens. Am I missing something here?
> > (Perfectly possible that I am).
> > 
> > In terms of do nothing gotos, do you mean gotos that just set an error
> > code then jump to the end? If you'd prefer, as the function just returns
> > right after the exit label, I can just return the codes directly and have
> > a 'return 0;' like you say above?
> > 
> > Thanks as always for your insight.
> 
> Yes, you've got it right.
> 
> I think Dan is suggesting something like the below, but
> not necessarily in a single patch:

I always like your style, Joe.

regards,
dan carpenter

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Phillip Potter]

On Sun, Jul 31, 2022 at 10:12:56AM -0700, Joe Perches wrote:
> Yes, you've got it right.
> 
> I think Dan is suggesting something like the below, but
> not necessarily in a single patch:
> ---
>  drivers/staging/r8188eu/core/rtw_ioctl_set.c | 38 ++++++++++++----------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_ioctl_set.c b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
> index 17f6bcbeebf42..2736bbce83b5b 100644
> --- a/drivers/staging/r8188eu/core/rtw_ioctl_set.c
> +++ b/drivers/staging/r8188eu/core/rtw_ioctl_set.c
> @@ -390,44 +390,38 @@ u8 rtw_set_802_11_authentication_mode(struct adapter *padapter, enum ndis_802_11
>  	return ret;
>  }
>  
> -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> +int rtw_set_802_11_add_wep(struct adapter *padapter,
> +			   struct ndis_802_11_wep *wep)
>  {
> -	int		keyid, res;
> -	struct security_priv *psecuritypriv = &padapter->securitypriv;
> -	u8		ret = _SUCCESS;
> +	int keyid;
> +	struct security_priv *secpriv = &padapter->securitypriv;
>  
>  	keyid = wep->KeyIndex & 0x3fffffff;
> -
> -	if (keyid >= 4) {
> -		ret = false;
> -		goto exit;
> -	}
> +	if (keyid >= 4)
> +		return -EOPNOTSUPP;
>  
>  	switch (wep->KeyLength) {
>  	case 5:
> -		psecuritypriv->dot11PrivacyAlgrthm = _WEP40_;
> +		secpriv->dot11PrivacyAlgrthm = _WEP40_;
>  		break;
>  	case 13:
> -		psecuritypriv->dot11PrivacyAlgrthm = _WEP104_;
> +		secpriv->dot11PrivacyAlgrthm = _WEP104_;
>  		break;
>  	default:
> -		psecuritypriv->dot11PrivacyAlgrthm = _NO_PRIVACY_;
> +		secpriv->dot11PrivacyAlgrthm = _NO_PRIVACY_;
>  		break;
>  	}
>  
> -	memcpy(&psecuritypriv->dot11DefKey[keyid].skey[0], &wep->KeyMaterial, wep->KeyLength);
> +	memcpy(secpriv->dot11DefKey[keyid].skey, &wep->KeyMaterial,
> +	       wep->KeyLength);
>  
> -	psecuritypriv->dot11DefKeylen[keyid] = wep->KeyLength;
> +	secpriv->dot11DefKeylen[keyid] = wep->KeyLength;
> +	secpriv->dot11PrivacyKeyIndex = keyid;
>  
> -	psecuritypriv->dot11PrivacyKeyIndex = keyid;
> +	if (rtw_set_key(padapter, secpriv, keyid, 1) == _FAIL)
> +		return -ENOMEM;
>  
> -	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
> -
> -	if (res == _FAIL)
> -		ret = false;
> -exit:
> -
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> 

Hi Joe,

Thanks for the suggestion, this is pretty much what I'd interpreted from
Dan's advice in the meantime. I will prepare a V2 tomorrow.

Regards,
Phil

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Dan Carpenter]

On Sat, Jul 30, 2022 at 07:36:57PM +0100, Phillip Potter wrote:
> On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote:
> > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  {
> > >  	int		keyid, res;
> > >  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> > > -	u8		ret = _SUCCESS;
> > > +	int		ret = 0;
> > >  
> > >  	keyid = wep->KeyIndex & 0x3fffffff;
> > >  
> > >  	if (keyid >= 4) {
> > > -		ret = false;
> > > +		ret = -EOPNOTSUPP;
> > >  		goto exit;
> > >  	}
> > >  
> > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
> > >  
> > >  	if (res == _FAIL)
> > > -		ret = false;
> > > +		ret = -ENOMEM;
> > >  exit:
> > >  
> > >  	return ret;
> > 
> > No, this isn't right.  This now returns 1 on success and negative
> > error codes on error.
> > 
> > There are a couple anti-patterns here:
> > 
> > 1) Do nothing gotos
> > 2) Mixing error paths and success paths.
> > 
> > If you avoid mixing error paths and success paths then you get a pattern
> > called: "Solid return zero."  This is where the end of the function has
> > a very chunky "return 0;" to mark that it is successful.  You want that.
> > Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
> > not chunky.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> Thank you for the review firstly, much appreciated.
> 
> I'm happy of course to rewrite this to address any concerns, but
> I was hoping I could clarify what you've said though? Apologies if I've
> missed it, but how is this function now returning 1 on success? It sets
> ret to 0 (success) at the start and then sets it to one of two negative
> error codes depending on what happens. Am I missing something here?
> (Perfectly possible that I am).

You're right.  I misread "res" as "ret".  It's another anti-pattern to
have two "ret" variables.  The code is fine but so ugly for no reason.

regards,
dan carpenter

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Pavel Skripkin]

Hi Phillip,

Phillip Potter <phil@philpotter.co.uk> says:
> -	if (!rtw_set_802_11_add_wep(padapter, &wep)) {
> +	if (rtw_set_802_11_add_wep(padapter, &wep)) {
>   		if (rf_on == pwrpriv->rf_pwrstate)
>   			ret = -EOPNOTSUPP;
>   		goto exit;

is it intentional to ignore an error in case of rf_on != 
pwrpriv->rf_pwrstate?




With regards,
Pavel Skripkin

Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics

[Phillip Potter]

On Tue, Aug 02, 2022 at 11:10:21PM +0300, Pavel Skripkin wrote:
> Hi Phillip,
> 
> Phillip Potter <phil@philpotter.co.uk> says:
> > -	if (!rtw_set_802_11_add_wep(padapter, &wep)) {
> > +	if (rtw_set_802_11_add_wep(padapter, &wep)) {
> >   		if (rf_on == pwrpriv->rf_pwrstate)
> >   			ret = -EOPNOTSUPP;
> >   		goto exit;
> 
> is it intentional to ignore an error in case of rf_on !=
> pwrpriv->rf_pwrstate?
> 
> 

Hi Pavel,

Somewhat yes, in the sense that this is existing behaviour and changing
it is a semantic change in the driver, thus arguably outside the scope
of a patch/patch set that is intended to just focus on error code
handling (moving from _SUCCESS/_FAIL to 0 and -EWHATEVER).

Not fixed to that by any means though, if you would prefer it be
restructured as well. I need to do a V2 for this anyway.

Regards,
Phil

Re: [PATCH 0/2] staging: r8188eu: more error code cleanups

[Philipp Hortmann]

On 7/29/22 01:11, Phillip Potter wrote:
> This small series addresses a cleanup suggestion discussed by Greg
> Kroah-Hartman and Dan Carpenter, and also includes another function
> conversion. It would be a larger series, but my time being what it is,
> I intend to chip away at these slowly and steadily.
> 
> Phillip Potter (2):
>    staging: r8188eu: remove initializer from ret in rtw_pwr_wakeup
>    staging: r8188eu: convert rtw_set_802_11_add_wep error code semantics
> 
>   drivers/staging/r8188eu/core/rtw_ioctl_set.c    | 8 ++++----
>   drivers/staging/r8188eu/core/rtw_pwrctrl.c      | 2 +-
>   drivers/staging/r8188eu/include/rtw_ioctl_set.h | 2 +-
>   drivers/staging/r8188eu/os_dep/ioctl_linux.c    | 5 ++---
>   4 files changed, 8 insertions(+), 9 deletions(-)
> 

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