linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH staging] staging: rtl8723bs: replace magic numbers in beacon init
@ 2025-06-30 17:33 Marcos Garcia
  2025-06-30 20:13 ` Dan Carpenter
  0 siblings, 1 reply; 5+ messages in thread
From: Marcos Garcia @ 2025-06-30 17:33 UTC (permalink / raw)
  To: gregkh
  Cc: philipp.g.hortmann, dan.carpenter, karanja99erick,
	rodrigo.gobbi.7, linux-staging, linux-kernel, Marcos Garcia

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@gmail.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 */
+#define BCNTCFG_AIFS_LARGEST		0x660F  /* Max AIFS for beacon priority */
+
 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. */
+	/* Fixed: Replaced magic numbers with defines */
+	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 */
+		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 */
-	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);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in beacon init
  2025-06-30 17:33 [PATCH staging] staging: rtl8723bs: replace magic numbers in beacon init Marcos Garcia
@ 2025-06-30 20:13 ` Dan Carpenter
  2025-06-30 21:02   ` [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization Marcos Garcia
  0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-06-30 20:13 UTC (permalink / raw)
  To: Marcos Garcia
  Cc: gregkh, philipp.g.hortmann, karanja99erick, rodrigo.gobbi.7,
	linux-staging, linux-kernel

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@gmail.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);


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization
  2025-06-30 20:13 ` Dan Carpenter
@ 2025-06-30 21:02   ` Marcos Garcia
  2025-06-30 23:16     ` Dan Carpenter
  2025-07-01  5:14     ` Greg KH
  0 siblings, 2 replies; 5+ messages in thread
From: Marcos Garcia @ 2025-06-30 21:02 UTC (permalink / raw)
  To: gregkh
  Cc: philipp.g.hortmann, dan.carpenter, karanja99erick,
	rodrigo.gobbi.7, linux-staging, linux-kernel, Marcos Garcia

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.

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 */
+/*
+ * 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
+
+/*
+ * 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization
  2025-06-30 21:02   ` [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization Marcos Garcia
@ 2025-06-30 23:16     ` Dan Carpenter
  2025-07-01  5:14     ` Greg KH
  1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2025-06-30 23:16 UTC (permalink / raw)
  To: Marcos Garcia
  Cc: gregkh, philipp.g.hortmann, karanja99erick, rodrigo.gobbi.7,
	linux-staging, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization
  2025-06-30 21:02   ` [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization Marcos Garcia
  2025-06-30 23:16     ` Dan Carpenter
@ 2025-07-01  5:14     ` Greg KH
  1 sibling, 0 replies; 5+ messages in thread
From: Greg KH @ 2025-07-01  5:14 UTC (permalink / raw)
  To: Marcos Garcia
  Cc: philipp.g.hortmann, dan.carpenter, karanja99erick,
	rodrigo.gobbi.7, linux-staging, linux-kernel

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.
> 
> 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 */
> +/*
> + * 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
> +
> +/*
> + * 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
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-07-01  5:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-30 17:33 [PATCH staging] staging: rtl8723bs: replace magic numbers in beacon init Marcos Garcia
2025-06-30 20:13 ` Dan Carpenter
2025-06-30 21:02   ` [PATCH staging] staging: rtl8723bs: Replace magic numbers in beacon initialization Marcos Garcia
2025-06-30 23:16     ` Dan Carpenter
2025-07-01  5:14     ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).