linux-kernel - Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in beacon init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38411a98-d907-4173-a528-8d50b337de0c@suswa.mountain>
Date: Mon, 30 Jun 2025 23:13:36 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Marcos Garcia <magazo2005@...il.com>
Cc: gregkh@...uxfoundation.org, philipp.g.hortmann@...il.com,
	karanja99erick@...il.com, rodrigo.gobbi.7@...il.com,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in
 beacon init

On Mon, Jun 30, 2025 at 07:33:39PM +0200, Marcos Garcia wrote:
> Replace hardcoded register values in rtl8723b_InitBeaconParameters()
> with properly named defines documenting their purpose:
> - TBTT_PROHIBIT_DEFAULT_MS (0x6404) for 100ms interval + 4ms margin
> - BCNTCFG_AIFS_LARGEST (0x660F) for maximum AIFS value
> 
> This improves maintainability while preserving the original behavior.
> 
> Signed-off-by: Marcos Garcia <magazo2005@...il.com>
> ---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c | 27 +++++++++----------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index 893cab0532ed..cc7886d75a0b 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -1183,31 +1183,28 @@ void rtl8723b_read_chip_version(struct adapter *padapter)
>  	ReadChipVersion8723B(padapter);
>  }
>  
> +/* Beacon Configuration */
> +#define TBTT_PROHIBIT_DEFAULT_MS	0x6404  /* 100ms interval + 4ms margin */

I don't understand the units here.  How is 100 + 4 = 0x6404.
0x6404 is 25604 in decimal...  I don't get it at all.

> +#define BCNTCFG_AIFS_LARGEST		0x660F  /* Max AIFS for beacon priority */

I don't understand this comment.  This feels like ChatGPT...  It's
got that magical feeling where every word follows the other so smoothly
but when you think about the meaning, it's both really vague and
slightly wrong.  What is "beacon priority"?  What other types of
priority are there?  What type of units is this?

> +
>  void rtl8723b_InitBeaconParameters(struct adapter *padapter)
>  {
>  	struct hal_com_data *pHalData = GET_HAL_DATA(padapter);
>  	u16 val16;
>  	u8 val8 = DIS_TSF_UDT;
>  
> -
> -	val16 = val8 | (val8 << 8); /*  port0 and port1 */
> -
> -	/*  Enable prot0 beacon function for PSTDMA */
> -	val16 |= EN_BCN_FUNCTION;
> -
> +	val16 = val8 | (val8 << 8); /* port0 and port1 */
> +	val16 |= EN_BCN_FUNCTION;   /* Enable prot0 beacon function */
>  	rtw_write16(padapter, REG_BCN_CTRL, val16);
>  
> -	/*  TODO: Remove these magic number */
> -	rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);/*  ms */
> -	/*  Firmware will control REG_DRVERLYINT when power saving is enable, */
> -	/*  so don't set this register on STA mode. */

This comment was clear and useful.  Keep it.

> +	/* Fixed: Replaced magic numbers with defines */
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is kind of information about what you're fixing is supposed to
go in the commit message.  Not the comments.


> +	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_DEFAULT_MS);
> +
>  	if (check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE) == false)
> -		rtw_write8(padapter, REG_DRVERLYINT, DRIVER_EARLY_INT_TIME_8723B); /*  5ms */
> -	rtw_write8(padapter, REG_BCNDMATIM, BCN_DMA_ATIME_INT_TIME_8723B); /*  2ms */

These comments that explained that DRIVER_EARLY_INT_TIME_8723B was 5ms
and BCN_DMA_ATIME_INT_TIME_8723B is 2ms are kind of crap.  The define
should have been called something like BCN_DMA_ATIME_2MS or something.
But they were at least better than nothing.

> +		rtw_write8(padapter, REG_DRVERLYINT, DRIVER_EARLY_INT_TIME_8723B);
>  
> -	/*  Suggested by designer timchen. Change beacon AIFS to the largest number */
> -	/*  because test chip does not contension before sending beacon. by tynli. 2009.11.03 */

This comment has a typo s/contension/contention/ and it's not clear
if we're trying to avoid contention or if we don't need to worry about
contention.  So it's not perfect, but it does potentially provide some
useful clues about the code so it's better than the new comments in that
sense.  Let's keep it until we can find a better explanation.

regards,
dan carpenter

> -	rtw_write16(padapter, REG_BCNTCFG, 0x660F);
> +	rtw_write8(padapter, REG_BCNDMATIM, BCN_DMA_ATIME_INT_TIME_8723B);
> +	rtw_write16(padapter, REG_BCNTCFG, BCNTCFG_AIFS_LARGEST);