Re: [PATCH] staging: rtl8723bs: refactor rtw_aes_decrypt() to reduce nesting

[Dan Carpenter <dan.carpenter@linaro.org>]

What often happens is that I write a review email and then decide not
to send it.  I probably would have merged your patch as-is, but since
Greg has decided not to then we as well fix this as well.

On Wed, Mar 18, 2026 at 12:51:49AM +0800, Lin YuChen wrote:
> Improve code readability by refactoring rtw_aes_decrypt() to use
> guard clauses and early exits. This reduces the maximum indentation
> level and flattens the logic flow for key assignment and decryption.
> 
> Signed-off-by: Lin YuChen <starpt.official@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_security.c | 91 ++++++++++---------
>  1 file changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
> index b489babe7432..9229e0a1c792 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_security.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_security.c
> @@ -1213,69 +1213,70 @@ u32 rtw_aes_decrypt(struct adapter *padapter, u8 *precvframe)
>  
>  	pframe = (unsigned char *)((union recv_frame *)precvframe)->u.hdr.rx_data;
>  	/* 4 start to encrypt each fragment */
> -	if (prxattrib->encrypt == _AES_) {
> -		stainfo = rtw_get_stainfo(&padapter->stapriv, &prxattrib->ta[0]);
> -		if (stainfo) {
> -			if (is_multicast_ether_addr(prxattrib->ra)) {
> -				static unsigned long start;
> -				static u32 no_gkey_bc_cnt;
> -				static u32 no_gkey_mc_cnt;
> +	if (prxattrib->encrypt != _AES_)
> +		goto exit;

Just return directly.  When I'm reading this patch, I have to think
"Oh, what does goto exit do?"  So I scroll down to the bottom, and I
see it returns res.  So then I scroll up here and I think "what is
res?" but it's not included in the email.  Compare that to just doing
"return _SUCCESS;" where anyone can instantly see what it does without
scrolling up and down and changing to a different window.

>  
> -				if (!psecuritypriv->binstallGrpkey) {
> -					res = _FAIL;
> +	stainfo = rtw_get_stainfo(&padapter->stapriv, &prxattrib->ta[0]);
> +	if (!stainfo) {
> +		res = _FAIL;
> +		goto exit;
> +	}

Same.  Just return directly.

> +	if (is_multicast_ether_addr(prxattrib->ra)) {
> +		static unsigned long start;
> +		static u32 no_gkey_bc_cnt;
> +		static u32 no_gkey_mc_cnt;
>  
> -					if (start == 0)
> -						start = jiffies;
> +		if (!psecuritypriv->binstallGrpkey) {
> +			res = _FAIL;
>  
> -					if (is_broadcast_mac_addr(prxattrib->ra))
> -						no_gkey_bc_cnt++;
> -					else
> -						no_gkey_mc_cnt++;
> +			if (start == 0)
> +				start = jiffies;
>  
> -					if (jiffies_to_msecs(jiffies - start) > 1000) {
> -						if (no_gkey_bc_cnt || no_gkey_mc_cnt) {
> -							netdev_dbg(padapter->pnetdev,
> -								   FUNC_ADPT_FMT " no_gkey_bc_cnt:%u, no_gkey_mc_cnt:%u\n",
> -								   FUNC_ADPT_ARG(padapter),
> -								   no_gkey_bc_cnt,
> -								   no_gkey_mc_cnt);
> -						}
> -						start = jiffies;
> -						no_gkey_bc_cnt = 0;
> -						no_gkey_mc_cnt = 0;
> -					}
> -
> -					goto exit;

Leave this goto exit because changing it would be an unrelated change.
The line is that adding a new goto is related because you were either
going to add a return or a goto.  Changing existing code is an unrelated
change and we want to leave those lines as-is except for pulling them
in a tab.

> -				}
> +			if (is_broadcast_mac_addr(prxattrib->ra))
> +				no_gkey_bc_cnt++;
> +			else
> +				no_gkey_mc_cnt++;
>  
> +			if (jiffies_to_msecs(jiffies - start) > 1000) {
>  				if (no_gkey_bc_cnt || no_gkey_mc_cnt) {
>  					netdev_dbg(padapter->pnetdev,
> -						   FUNC_ADPT_FMT " gkey installed. no_gkey_bc_cnt:%u, no_gkey_mc_cnt:%u\n",
> -						   FUNC_ADPT_ARG(padapter),
> -						   no_gkey_bc_cnt,
> +						   FUNC_ADPT_FMT " no_gkey_bc_cnt:%u, no_gkey_mc_cnt:%u\n",
> +						   FUNC_ADPT_ARG(padapter), no_gkey_bc_cnt,
>  						   no_gkey_mc_cnt);
>  				}
> -				start = 0;
> +				start = jiffies;
>  				no_gkey_bc_cnt = 0;
>  				no_gkey_mc_cnt = 0;
> -
> -				prwskey = psecuritypriv->dot118021XGrpKey[prxattrib->key_index].skey;
> -				if (psecuritypriv->dot118021XGrpKeyid != prxattrib->key_index) {
> -					res = _FAIL;
> -					goto exit;
> -				}
> -			} else {
> -				prwskey = &stainfo->dot118021x_UncstKey.skey[0];
>  			}
>  
> -			length = ((union recv_frame *)precvframe)->u.hdr.len - prxattrib->hdrlen - prxattrib->iv_len;
> +			goto exit;
> +		}
>  
> -			res = aes_decipher(prwskey, prxattrib->hdrlen, pframe, length);
> +		if (no_gkey_bc_cnt || no_gkey_mc_cnt) {
> +			netdev_dbg(padapter->pnetdev,
> +				   FUNC_ADPT_FMT " gkey installed. no_gkey_bc_cnt:%u, no_gkey_mc_cnt:%u\n",
> +				   FUNC_ADPT_ARG(padapter), no_gkey_bc_cnt,
> +				   no_gkey_mc_cnt);
> +		}
> +		start = 0;
> +		no_gkey_bc_cnt = 0;
> +		no_gkey_mc_cnt = 0;
>  
> -		} else {
> +		prwskey = psecuritypriv->dot118021XGrpKey[prxattrib->key_index]
> +				  .skey;
> +		if (psecuritypriv->dot118021XGrpKeyid != prxattrib->key_index) {
>  			res = _FAIL;
> +			goto exit;
>  		}
> +	} else {
> +		prwskey = &stainfo->dot118021x_UncstKey.skey[0];
>  	}
> +
> +	length = ((union recv_frame *)precvframe)->u.hdr.len -
> +		 prxattrib->hdrlen - prxattrib->iv_len;

Checkpatch wants you to break this line up, but it wasn't broken up in
the original code so that's an unrelated change.

regards,
dan carpenter

> +
> +	res = aes_decipher(prwskey, prxattrib->hdrlen, pframe, length);
> +
>  exit:
>  	return res;
>  }
> -- 
> 2.34.1