drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 41 +++++++------------ .../staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++ 2 files changed, 18 insertions(+), 26 deletions(-)
Refactor the 'ICS' array in issue_action_BSSCoexistPacket() to improve
readability and maintainability. This addresses technical debt related
to magic numbers and ambiguous array usage.
The original implementation used a multi-purpose 2D array (ICS[8][15])
where magic numbers were prevalent and the first element of each row
was overloaded as a status flag. This patch:
- Introduces descriptive macros: BSS_COEX_MAX_CLASSES,
BSS_COEX_MAX_CHANNELS, and BSS_COEX_MAX_INFO_LEN.
- Splits the overloaded array into two distinct boolean arrays:
'class_active' (for group status) and 'ch_present' (for channel data).
- Converts the logic to use 'bool' types and 0-indexed loops,
conforming to standard C programming practices.
- Adds defensive boundary checks (ch > 0 && ch < MAX) to ensure
robustness against unexpected channel data.
This refactoring maintains the current 2.4GHz reporting behavior while
providing a structured and extensible foundation for future Operating
Class support according to IEEE 802.11 specifications.
Signed-off-by: Michael Huang <tehsiu.huang@gmail.com>
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 41 +++++++------------
.../staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++
2 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 884fcce50d9c..481295224f14 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -3587,8 +3587,9 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
struct mlme_ext_priv *pmlmeext = &(padapter->mlmeextpriv);
struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
struct __queue *queue = &(pmlmepriv->scanned_queue);
- u8 InfoContent[16] = {0};
- u8 ICS[8][15];
+ u8 InfoContent[BSS_COEX_MAX_INFO_LEN] = {0};
+ bool class_active[BSS_COEX_MAX_CLASSES] = {false};
+ bool ch_present[BSS_COEX_MAX_CLASSES][BSS_COEX_MAX_CHANNELS] = {{false}};
if ((pmlmepriv->num_FortyMHzIntolerant == 0) || (pmlmepriv->num_sta_no_ht == 0))
return;
@@ -3642,7 +3643,6 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
/* */
- memset(ICS, 0, sizeof(ICS));
if (pmlmepriv->num_sta_no_ht > 0) {
int i;
@@ -3667,46 +3667,35 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
p = rtw_get_ie(pbss_network->ies + _FIXED_IE_LENGTH_, WLAN_EID_HT_CAPABILITY, &len, pbss_network->ie_length - _FIXED_IE_LENGTH_);
if (!p || len == 0) {/* non-HT */
+ u8 ch = pbss_network->configuration.ds_config;
- if (pbss_network->configuration.ds_config <= 0)
- continue;
-
- ICS[0][pbss_network->configuration.ds_config] = 1;
-
- if (ICS[0][0] == 0)
- ICS[0][0] = 1;
+ if (ch > 0 && ch < BSS_COEX_MAX_CHANNELS) {
+ ch_present[0][ch] = true;
+ class_active[0] = true;
+ }
}
}
spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
-
- for (i = 0; i < 8; i++) {
+ for (i = 0; i < BSS_COEX_MAX_CLASSES; i++) {
int j, k = 0;
- if (ICS[i][0] != 1)
+ if (!class_active[i])
continue;
- InfoContent[k] = i;
+ InfoContent[k++] = i;
/* SET_BSS_INTOLERANT_ELE_REG_CLASS(InfoContent, i); */
- k++;
- for (j = 1; j <= 14; j++) {
- if (ICS[i][j] != 1)
- continue;
-
- if (k < 16) {
- InfoContent[k] = j; /* channel number */
- /* SET_BSS_INTOLERANT_ELE_CHANNEL(InfoContent+k, j); */
- k++;
+ for (j = 0; j < BSS_COEX_MAX_CHANNELS; j++) {
+ if (ch_present[i][j]) {
+ if (k < BSS_COEX_MAX_INFO_LEN)
+ InfoContent[k++] = j; /* channel number */
}
}
-
pframe = rtw_set_ie(pframe, WLAN_EID_BSS_INTOLERANT_CHL_REPORT, k, InfoContent, &(pattrib->pktlen));
-
}
-
}
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index dd5080056e58..3a1067d6f164 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -41,6 +41,9 @@
#define _HW_STATE_STATION_ 0x02
#define _HW_STATE_AP_ 0x03
+#define BSS_COEX_MAX_CLASSES 8
+#define BSS_COEX_MAX_CHANNELS 15
+#define BSS_COEX_MAX_INFO_LEN 16
#define _1M_RATE_ 0
#define _2M_RATE_ 1
--
2.43.0On Wed, Jan 28, 2026 at 12:45:10PM -0800, Michael Huang wrote:
> Refactor the 'ICS' array in issue_action_BSSCoexistPacket() to improve
> readability and maintainability. This addresses technical debt related
> to magic numbers and ambiguous array usage.
>
> The original implementation used a multi-purpose 2D array (ICS[8][15])
> where magic numbers were prevalent and the first element of each row
> was overloaded as a status flag. This patch:
>
> - Introduces descriptive macros: BSS_COEX_MAX_CLASSES,
> BSS_COEX_MAX_CHANNELS, and BSS_COEX_MAX_INFO_LEN.
> - Splits the overloaded array into two distinct boolean arrays:
> 'class_active' (for group status) and 'ch_present' (for channel data).
> - Converts the logic to use 'bool' types and 0-indexed loops,
> conforming to standard C programming practices.
> - Adds defensive boundary checks (ch > 0 && ch < MAX) to ensure
> robustness against unexpected channel data.
>
> This refactoring maintains the current 2.4GHz reporting behavior while
> providing a structured and extensible foundation for future Operating
> Class support according to IEEE 802.11 specifications.
>
> Signed-off-by: Michael Huang <tehsiu.huang@gmail.com>
> ---
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 41 +++++++------------
> .../staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++
> 2 files changed, 18 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 884fcce50d9c..481295224f14 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -3587,8 +3587,9 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
> struct mlme_ext_priv *pmlmeext = &(padapter->mlmeextpriv);
> struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
> struct __queue *queue = &(pmlmepriv->scanned_queue);
> - u8 InfoContent[16] = {0};
> - u8 ICS[8][15];
> + u8 InfoContent[BSS_COEX_MAX_INFO_LEN] = {0};
> + bool class_active[BSS_COEX_MAX_CLASSES] = {false};
We only ever set the first element in the class_active[] array.
#puzzled
> + bool ch_present[BSS_COEX_MAX_CLASSES][BSS_COEX_MAX_CHANNELS] = {{false}};
Btw, use "= {};" to initialize arrays to zero. It's not standard C, but
it's kernel C.
regards,
dan carpenter
>
> We only ever set the first element in the class_active[] array.
> #puzzled
>
> > + bool ch_present[BSS_COEX_MAX_CLASSES][BSS_COEX_MAX_CHANNELS] = {{false}};
>
> Btw, use "= {};" to initialize arrays to zero. It's not standard C, but
> it's kernel C.
>
> regards,
> dan carpenter
>
Hi Dan,
Thanks for your time!! I understand the confusion. I kept the array
structure because it reflects the IEEE 802.11 Intolerant Channel
Report format, which is organized by Regulatory Classes.
Although the driver currently only reports for the 2.4GHz class (index
0), using an array makes the code semantically aligned with the
specification and easier to extend when other operating classes need
to be supported in the future.
I have submitted a V2 which uses the '= {}' initialization you suggested.
Refactor the 'ICS' array in issue_action_BSSCoexistPacket() to improve
readability and maintainability. This addresses technical debt related
to magic numbers and ambiguous array usage.
The original implementation used a multi-purpose 2D array (ICS[8][15])
where magic numbers were prevalent and the first element of each row
was overloaded as a status flag. This patch:
- Introduces descriptive macros: BSS_COEX_MAX_CLASSES,
BSS_COEX_MAX_CHANNELS, and BSS_COEX_MAX_INFO_LEN.
- Splits the overloaded array into two distinct boolean arrays:
'class_active' (for group status) and 'ch_present' (for channel data).
- Converts the logic to use 'bool' types and 0-indexed loops,
conforming to standard C programming practices.
- Adds defensive boundary checks (ch > 0 && ch < MAX) to ensure
robustness against unexpected channel data.
This refactoring maintains the current 2.4GHz reporting behavior while
providing a structured and extensible foundation for future Operating
Class support according to IEEE 802.11 specifications.
Signed-off-by: Michael Huang <tehsiu.huang@gmail.com>
---
v2:
- Use kernel-style = {} for array initialization per Dan Carpenter.
- Separate flags from data using bool arrays per Joe Perches.
- Fix magic numbers with proper defines.
- Retained the 2D structure for 'ch_present' despite only using class 0
for now. This aligns with the IEEE 802.11 Regulatory Class/Channel
List hierarchy, making the code easier to map to the specification
and extensible for future operating class support.
v1:
- Initial refactoring of the ICS array logic.
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 41 +++++++------------
.../staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++
2 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 884fcce50d9c..dc2677275e0f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -3587,8 +3587,9 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
struct mlme_ext_priv *pmlmeext = &(padapter->mlmeextpriv);
struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
struct __queue *queue = &(pmlmepriv->scanned_queue);
- u8 InfoContent[16] = {0};
- u8 ICS[8][15];
+ u8 InfoContent[BSS_COEX_MAX_INFO_LEN] = {};
+ bool class_active[BSS_COEX_MAX_CLASSES] = {};
+ bool ch_present[BSS_COEX_MAX_CLASSES][BSS_COEX_MAX_CHANNELS] = {};
if ((pmlmepriv->num_FortyMHzIntolerant == 0) || (pmlmepriv->num_sta_no_ht == 0))
return;
@@ -3642,7 +3643,6 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
/* */
- memset(ICS, 0, sizeof(ICS));
if (pmlmepriv->num_sta_no_ht > 0) {
int i;
@@ -3667,46 +3667,35 @@ static void issue_action_BSSCoexistPacket(struct adapter *padapter)
p = rtw_get_ie(pbss_network->ies + _FIXED_IE_LENGTH_, WLAN_EID_HT_CAPABILITY, &len, pbss_network->ie_length - _FIXED_IE_LENGTH_);
if (!p || len == 0) {/* non-HT */
+ u8 ch = pbss_network->configuration.ds_config;
- if (pbss_network->configuration.ds_config <= 0)
- continue;
-
- ICS[0][pbss_network->configuration.ds_config] = 1;
-
- if (ICS[0][0] == 0)
- ICS[0][0] = 1;
+ if (ch > 0 && ch < BSS_COEX_MAX_CHANNELS) {
+ ch_present[0][ch] = true;
+ class_active[0] = true;
+ }
}
}
spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
-
- for (i = 0; i < 8; i++) {
+ for (i = 0; i < BSS_COEX_MAX_CLASSES; i++) {
int j, k = 0;
- if (ICS[i][0] != 1)
+ if (!class_active[i])
continue;
- InfoContent[k] = i;
+ InfoContent[k++] = i;
/* SET_BSS_INTOLERANT_ELE_REG_CLASS(InfoContent, i); */
- k++;
- for (j = 1; j <= 14; j++) {
- if (ICS[i][j] != 1)
- continue;
-
- if (k < 16) {
- InfoContent[k] = j; /* channel number */
- /* SET_BSS_INTOLERANT_ELE_CHANNEL(InfoContent+k, j); */
- k++;
+ for (j = 0; j < BSS_COEX_MAX_CHANNELS; j++) {
+ if (ch_present[i][j]) {
+ if (k < BSS_COEX_MAX_INFO_LEN)
+ InfoContent[k++] = j; /* channel number */
}
}
-
pframe = rtw_set_ie(pframe, WLAN_EID_BSS_INTOLERANT_CHL_REPORT, k, InfoContent, &(pattrib->pktlen));
-
}
-
}
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index dd5080056e58..3a1067d6f164 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -41,6 +41,9 @@
#define _HW_STATE_STATION_ 0x02
#define _HW_STATE_AP_ 0x03
+#define BSS_COEX_MAX_CLASSES 8
+#define BSS_COEX_MAX_CHANNELS 15
+#define BSS_COEX_MAX_INFO_LEN 16
#define _1M_RATE_ 0
#define _2M_RATE_ 1
--
2.43.0On Thu, Jan 29, 2026 at 12:01:10AM -0800, Michael Huang wrote: > - Retained the 2D structure for 'ch_present' despite only using class 0 > for now. This aligns with the IEEE 802.11 Regulatory Class/Channel > List hierarchy, making the code easier to map to the specification > and extensible for future operating class support. No one can predit the future and adding unused cruft based on our horoscope is discouraged. Future developers can add it themselves if they want it. regards, dan carpenter
> No one can predit the future and adding unused cruft based on our > horoscope is discouraged. Future developers can add it themselves if > they want it. > > regards, > dan carpenter > Thank Greg and Dan for the feedback. I now have a much better understanding of the development philosophy behind the Linux kernel (My first time). This kind of 'culture shock' is exactly what I was looking for — it's a valuable learning experience that helps me transition toward writing more professional and maintainable kernel code :) . Thanks, Michael
On Thu, Jan 29, 2026 at 12:01:10AM -0800, Michael Huang wrote: > Refactor the 'ICS' array in issue_action_BSSCoexistPacket() to improve > readability and maintainability. This addresses technical debt related > to magic numbers and ambiguous array usage. > > The original implementation used a multi-purpose 2D array (ICS[8][15]) > where magic numbers were prevalent and the first element of each row > was overloaded as a status flag. This patch: > > - Introduces descriptive macros: BSS_COEX_MAX_CLASSES, > BSS_COEX_MAX_CHANNELS, and BSS_COEX_MAX_INFO_LEN. > - Splits the overloaded array into two distinct boolean arrays: > 'class_active' (for group status) and 'ch_present' (for channel data). > - Converts the logic to use 'bool' types and 0-indexed loops, > conforming to standard C programming practices. > - Adds defensive boundary checks (ch > 0 && ch < MAX) to ensure > robustness against unexpected channel data. That's a lot of different things all in one single patch. SHouldn't this be split up into different ones? And did you use AI to generate this patch? Also, how was this tested? What prompted you to want to make this change in the first place? thanks, greg k-h
On Thu, Jan 29, 2026 at 12:54 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> That's a lot of different things all in one single patch. SHouldn't
> this be split up into different ones?
I kept these changes in one patch because they all work together to
replace the old ICS logic and remove the magic numbers. Splitting them
might break the build or make the code inconsistent halfway through.
I was suggested renaming and using a bool array structure in the
earlier patch here:
https://lore.kernel.org/all/e87acd109ea5eb4e3f8fc233788cd0dbd128407d.camel@perches.com/
I kept the 2D structure to maintain parity with the IEEE 802.11 spec
as original implementation. It makes the code semantically correct and
easier to extend for other Operating Classes in the future.
> And did you use AI to generate this patch?
I used AI tools to polish the language of my commit message and to
perform a parity check, ensuring the refactored boolean logic is
functionally identical to the original code.
> Also, how was this tested? What prompted you to want to make this
> change in the first place?
The patch was tested by:
1. Cross-compile the module for the rtl8723bs driver (make
M=drivers/staging/rtl8723bs) to ensure no build regressions.
2. Ran it through scripts/checkpatch.pl to confirm it meets kernel
coding standards.
3. Since I do not have the physical hardware, I performed a manual
check (by AI verification) to ensure the logic remains identical.
Specifically:
- The original code used ICS[0][ch] to store channel data.
- The new code uses ch_present[0][ch] for the same purpose.
- The original ICS[i][0] flag is now explicitly class_active[i].
- Verified that the loop boundaries and index offsets (0-15)
remain exactly the same as the original implementation.
Motivation: This follows up on the cleanup tasks suggested during the
previous discussion in
https://lore.kernel.org/all/e87acd109ea5eb4e3f8fc233788cd0dbd128407d.camel@perches.com.
I want to improve the readability of the driver by removing magic numbers.
>
> thanks,
>
> greg k-h
Thanks,
Michael
On Thu, Jan 29, 2026 at 01:54:37AM -0800, Te-Hsiu Huang wrote: > On Thu, Jan 29, 2026 at 12:54 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > That's a lot of different things all in one single patch. SHouldn't > > this be split up into different ones? > I kept these changes in one patch because they all work together to > replace the old ICS logic and remove the magic numbers. Splitting them > might break the build or make the code inconsistent halfway through. > I was suggested renaming and using a bool array structure in the > earlier patch here: > https://lore.kernel.org/all/e87acd109ea5eb4e3f8fc233788cd0dbd128407d.camel@perches.com/ > I kept the 2D structure to maintain parity with the IEEE 802.11 spec > as original implementation. It makes the code semantically correct and > easier to extend for other Operating Classes in the future. > > > And did you use AI to generate this patch? > I used AI tools to polish the language of my commit message and to > perform a parity check, ensuring the refactored boolean logic is > functionally identical to the original code. Then as per our development rules, you must document this properly. Please read and follow them for when you resubmit this. > > > Also, how was this tested? What prompted you to want to make this > > change in the first place? > The patch was tested by: > 1. Cross-compile the module for the rtl8723bs driver (make > M=drivers/staging/rtl8723bs) to ensure no build regressions. > 2. Ran it through scripts/checkpatch.pl to confirm it meets kernel > coding standards. > 3. Since I do not have the physical hardware, I performed a manual > check (by AI verification) to ensure the logic remains identical. AI can not "verify" anything, you must always check that, sorry. greg k-h
> > > And did you use AI to generate this patch? > > I used AI tools to polish the language of my commit message and to > > perform a parity check, ensuring the refactored boolean logic is > > functionally identical to the original code. > > Then as per our development rules, you must document this properly. > Please read and follow them for when you resubmit this. > > > 3. Since I do not have the physical hardware, I performed a manual > > check (by AI verification) to ensure the logic remains identical. > > AI can not "verify" anything, you must always check that, sorry. > > greg k-h Hi Greg, I apologize for the poor phrasing. I did not mean that I relied solely on AI to verify the code. To ensure the refactor was correct, I performed a manual line-by-line audit to verify that the logic remains identical to the original implementation. I understand that I am responsible for the code's correctness. I will properly document the use of AI tools according to the rules and resubmit this as part of V3, including splitting the changes and providing more test details. Thank you for the guidance!
© 2016 - 2026 Red Hat, Inc.