Re: [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization

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

On Mon, Jun 30, 2025 at 11:02:11PM +0200, Marcos Garcia wrote:
> Replace hardcoded register values in rtl8723b_InitBeaconParameters()
> with descriptive defines to improve code readability and maintainability:
> - TBTT_PROHIBIT_DEFAULT (0x6404): Sets TBTT prohibit time to 100ms
>   (bits [15:8]) with a 2ms margin (bits [7:0]).
> - BCNTCFG_DEFAULT (0x660F): Configures maximum AIFS for beacon
>   transmission to ensure high priority, as recommended by the designer.

This commit message references TBTT_PROHIBIT_DEFAULT and BCNTCFG_DEFAULT
which don't exist.  At this point I'm kind of assuming that you're
using ChatGPT to write your patches.

> 
> Preserve original comments where they provide useful context, such as
> firmware control in power-saving mode and designer notes about beacon
> contention. Fix typo "contension" to "contention" in the comment.
> 
> Signed-off-by: Marcos Garcia <magazo2005@gmail.com>
> ---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c | 50 +++++++++++++++----
>  kernel/sched/ext.c                            |  8 ++-
>  2 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index cc7886d75a0b..a3438280e1ab 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -1183,9 +1183,25 @@ void rtl8723b_read_chip_version(struct adapter *padapter)
>  	ReadChipVersion8723B(padapter);
>  }
>  
> -/* Beacon Configuration */
> -#define TBTT_PROHIBIT_DEFAULT_MS	0x6404  /* 100ms interval + 4ms margin */
> -#define BCNTCFG_AIFS_LARGEST		0x660F  /* Max AIFS for beacon priority */

You've written this patch on top of a different patch which we're not
going to apply.

https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/

> +/*
> + * Beacon Configuration
> + *
> + * REG_TBTT_PROHIBIT: 16-bit register where:
> + *   - Bits [15:8] = TBTT prohibit time in units of 1ms
> + *   - Bits [7:0] = TBTT prohibit margin time in units of 0.5ms
> + * Default value of 0x6404 means:
> + *   - 0x64 (100) ms prohibit time
> + *   - 0x04 (4 * 0.5 = 2) ms margin time
> + */
> +#define TBTT_PROHIBIT_100MS_2MS_MARGIN 0x6404

Are you getting this from a spec somewhere?  Where is the link?  So the
original comment was wrong with regards to 4 ms margin vs 2 ms margin?
This kind of information needs to be in the v2 notes.

I think I need to see the spec before I can review further.

regards,
dan carpenter

> +
> +/*
> + * REG_BCNTCFG: Beacon configuration register
> + * 0x660F sets AIFS to maximum value (0xF) with other default parameters
> + * This ensures beacons get highest priority (no contention window),
> + * as suggested by the original designer for test chips.
> + */
> +#define BCNTCFG_MAX_AIFS 0x660F
>  
>  void rtl8723b_InitBeaconParameters(struct adapter *padapter)
>  {
> @@ -1193,19 +1209,31 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
>  	u16 val16;
>  	u8 val8 = DIS_TSF_UDT;
>  
> -	val16 = val8 | (val8 << 8); /* port0 and port1 */
> -	val16 |= EN_BCN_FUNCTION;   /* Enable prot0 beacon function */
> +	/* Configure beacon control for both port 0 and port 1 */
> +	val16 = val8 | (val8 << 8);
> +	val16 |= EN_BCN_FUNCTION; /* Enable beacon function for PSTDMA */
>  	rtw_write16(padapter, REG_BCN_CTRL, val16);
>  
> -	/* Fixed: Replaced magic numbers with defines */
> -	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_DEFAULT_MS);
> +	/* Configure TBTT prohibit timer with 100ms interval + 2ms margin */
> +	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_100MS_2MS_MARGIN);
> +
> +	/*
> +	 * Firmware controls early interrupt timing in power save mode,
> +	 * so only set REG_DRVERLYINT when not in station mode
> +	 */
> +	if (!check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE))
> +		rtw_write8(padapter, REG_DRVERLYINT, DRIVER_EARLY_INT_TIME_8723B); /* 5ms */
>  
> -	if (check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE) == false)
> -		rtw_write8(padapter, REG_DRVERLYINT, DRIVER_EARLY_INT_TIME_8723B);
> +	/* Set beacon DMA interrupt time to 2ms */
> +	rtw_write8(padapter, REG_BCNDMATIM, BCN_DMA_ATIME_INT_TIME_8723B); /* 2ms */
>  
> -	rtw_write8(padapter, REG_BCNDMATIM, BCN_DMA_ATIME_INT_TIME_8723B);
> -	rtw_write16(padapter, REG_BCNTCFG, BCNTCFG_AIFS_LARGEST);
> +	/*
> +	 * Suggested by designer timchen: Set AIFS to maximum to avoid contention
> +	 * before sending beacons on test chips. By tynli, 2009.11.03
> +	 */
> +	rtw_write16(padapter, REG_BCNTCFG, BCNTCFG_MAX_AIFS);
>  
> +	/* Save initial register values for reference */
>  	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
>  	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
>  	pHalData->RegFwHwTxQCtrl = rtw_read8(padapter, REG_FWHW_TXQ_CTRL+2);
> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index b498d867ba21..7cecc0ca700d 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -4258,7 +4258,13 @@ void scx_group_set_weight(struct task_group *tg, unsigned long weight)
>  
>  void scx_group_set_idle(struct task_group *tg, bool idle)
>  {
> -	/* TODO: Implement ops->cgroup_set_idle() */
> +	struct sched_ext_ops *ops;
> +
> +	rcu_read_lock();
> +	ops = rcu_dereference(ext_ops);
> +	if (ops && ops->cgroup_set_idle)
> +		ops->cgroup_set_idle(tg, idle);
> +	rcu_read_unlock();
>  }
>  
>  static void scx_cgroup_lock(void)
> -- 
> 2.50.0
>