[PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Mohit Mishra]

In ODM_TxPwrTrackSetPwr_8723B(), the baseband swing index variables
Final_OFDM_Swing_Index and Final_CCK_Swing_Index are declared as u8.
However, their calculation adds Absolute_OFDMSwingIdx, which is a signed
8-bit integer (s8) and can be negative:

    Final_OFDM_Swing_Index = pDM_Odm->DefaultOfdmIndex +
                             pDM_Odm->Absolute_OFDMSwingIdx[RFPath];

If the resulting sum is negative, it underflows under u8 rules (e.g. -5
becomes 251). This causes the lower-limit checks (e.g. <= 0) to fail,
and in MIX_MODE causes the logic to execute the "BBSwing higher than limit"
branch instead of capping to 0.

Additionally, in BBSWING mode, the check for CCK underflow mistakenly
examines the static struct member pDM_Odm->BbSwingIdxCck instead of the
newly calculated Final_CCK_Swing_Index:

    else if (pDM_Odm->BbSwingIdxCck <= 0)

Fix this by changing both swing index variable types to int to enable
signed math and correct branch selection (aligning with the TODO item to
convert remaining unusual variable types). Update the CCK check in
BBSWING mode to examine Final_CCK_Swing_Index.

Note: The fix is scoped to the calculation and branching logic. When
passed downstream to setIqkMatrix_8723B() and setCCKFilterCoefficient(),
the values are already clamped within [0, 42], fitting safely in u8.

Compile-tested only; no hardware available for testing.

Signed-off-by: Mohit Mishra <mishraloopmohit@gmail.com>
---
 drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
index 6c5f56d5a1f4..4e89847700f3 100644
--- a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
+++ b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
@@ -160,8 +160,8 @@ void ODM_TxPwrTrackSetPwr_8723B(
 	u8 PwrTrackingLimit_OFDM = 34; /* 0dB */
 	u8 PwrTrackingLimit_CCK = 28; /* 2dB */
 	u8 TxRate = 0xFF;
-	u8 Final_OFDM_Swing_Index = 0;
-	u8 Final_CCK_Swing_Index = 0;
+	int Final_OFDM_Swing_Index = 0;
+	int Final_CCK_Swing_Index = 0;
 
 	{
 		u16 rate = *(pDM_Odm->pForcedDataRate);
@@ -217,7 +217,7 @@ void ODM_TxPwrTrackSetPwr_8723B(
 
 		if (Final_CCK_Swing_Index >= CCK_TABLE_SIZE)
 			Final_CCK_Swing_Index = CCK_TABLE_SIZE-1;
-		else if (pDM_Odm->BbSwingIdxCck <= 0)
+		else if (Final_CCK_Swing_Index <= 0)
 			Final_CCK_Swing_Index = 0;
 
 		setIqkMatrix_8723B(pDM_Odm, Final_OFDM_Swing_Index, RFPath,
-- 
2.43.0

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Greg Kroah-Hartman]

On Wed, Jul 29, 2026 at 10:57:07PM +0530, Mohit Mishra wrote:
> In ODM_TxPwrTrackSetPwr_8723B(), the baseband swing index variables
> Final_OFDM_Swing_Index and Final_CCK_Swing_Index are declared as u8.
> However, their calculation adds Absolute_OFDMSwingIdx, which is a signed
> 8-bit integer (s8) and can be negative:
> 
>     Final_OFDM_Swing_Index = pDM_Odm->DefaultOfdmIndex +
>                              pDM_Odm->Absolute_OFDMSwingIdx[RFPath];
> 
> If the resulting sum is negative, it underflows under u8 rules (e.g. -5
> becomes 251). This causes the lower-limit checks (e.g. <= 0) to fail,
> and in MIX_MODE causes the logic to execute the "BBSwing higher than limit"
> branch instead of capping to 0.
> 
> Additionally, in BBSWING mode, the check for CCK underflow mistakenly
> examines the static struct member pDM_Odm->BbSwingIdxCck instead of the
> newly calculated Final_CCK_Swing_Index:
> 
>     else if (pDM_Odm->BbSwingIdxCck <= 0)
> 
> Fix this by changing both swing index variable types to int to enable
> signed math and correct branch selection (aligning with the TODO item to
> convert remaining unusual variable types). Update the CCK check in
> BBSWING mode to examine Final_CCK_Swing_Index.
> 
> Note: The fix is scoped to the calculation and branching logic. When
> passed downstream to setIqkMatrix_8723B() and setCCKFilterCoefficient(),
> the values are already clamped within [0, 42], fitting safely in u8.
> 
> Compile-tested only; no hardware available for testing.

How was this issue found?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Mohit Mishra]

On Thu, Jul 30, 2026 at 1:10 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> How was this issue found?

I was going through drivers/staging/rtl8723bs looking for type-safety
issues after noticing a similar pattern in another staging driver.
I traced the swing index variables and noticed they're declared u8 but
calculated from Absolute_OFDMSwingIdx, which is s8 and can be negative.
I used an AI coding assistant to help organize the search and
cross-check the branch logic, but verified the underflow behavior, the
downstream truncation safety, and the TODO alignment myself by reading
the actual code and running checkpatch/build checks locally.

I don't have the hardware to confirm this in practice, which is why I
noted it as compile-tested only.

Regards,
Mohit Mishra

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Greg Kroah-Hartman]

On Thu, Jul 30, 2026 at 02:20:04PM +0530, Mohit Mishra wrote:
> On Thu, Jul 30, 2026 at 1:10 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > How was this issue found?
> 
> I was going through drivers/staging/rtl8723bs looking for type-safety
> issues after noticing a similar pattern in another staging driver.
> I traced the swing index variables and noticed they're declared u8 but
> calculated from Absolute_OFDMSwingIdx, which is s8 and can be negative.
> I used an AI coding assistant to help organize the search and
> cross-check the branch logic, but verified the underflow behavior, the
> downstream truncation safety, and the TODO alignment myself by reading
> the actual code and running checkpatch/build checks locally.
> 
> I don't have the hardware to confirm this in practice, which is why I
> noted it as compile-tested only.

You have to document when you use a LLM, please do so here in the
correct way.

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Nikolay Kulikov]

On Wed, Jul 29, 2026 at 10:57:07PM +0530, Mohit Mishra wrote:
> In ODM_TxPwrTrackSetPwr_8723B(), the baseband swing index variables
> Final_OFDM_Swing_Index and Final_CCK_Swing_Index are declared as u8.
> However, their calculation adds Absolute_OFDMSwingIdx, which is a signed
> 8-bit integer (s8) and can be negative:
> 
>     Final_OFDM_Swing_Index = pDM_Odm->DefaultOfdmIndex +
>                              pDM_Odm->Absolute_OFDMSwingIdx[RFPath];
> 
> If the resulting sum is negative, it underflows under u8 rules (e.g. -5
> becomes 251). This causes the lower-limit checks (e.g. <= 0) to fail,
> and in MIX_MODE causes the logic to execute the "BBSwing higher than limit"
> branch instead of capping to 0.
> 
> Additionally, in BBSWING mode, the check for CCK underflow mistakenly
> examines the static struct member pDM_Odm->BbSwingIdxCck instead of the
> newly calculated Final_CCK_Swing_Index:
> 
>     else if (pDM_Odm->BbSwingIdxCck <= 0)
> 
> Fix this by changing both swing index variable types to int to enable
> signed math and correct branch selection (aligning with the TODO item to
> convert remaining unusual variable types). Update the CCK check in
> BBSWING mode to examine Final_CCK_Swing_Index.
> 
> Note: The fix is scoped to the calculation and branching logic. When
> passed downstream to setIqkMatrix_8723B() and setCCKFilterCoefficient(),
> the values are already clamped within [0, 42], fitting safely in u8.

There is no need to try to fix the check in BBSWING mode - it never
executes and can be removed.
The ODM_TxPwrTrackSetPwr_8723B() function is always called with 'Method'
set to 'MIX_MODE' (see hal/HalPhyRf.c, lines 252 and 255).


Thanks,
Nikolay

> 
> Compile-tested only; no hardware available for testing.
> 
> Signed-off-by: Mohit Mishra <mishraloopmohit@gmail.com>
> ---
>  drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
> index 6c5f56d5a1f4..4e89847700f3 100644
> --- a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
> +++ b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
> @@ -160,8 +160,8 @@ void ODM_TxPwrTrackSetPwr_8723B(
>  	u8 PwrTrackingLimit_OFDM = 34; /* 0dB */
>  	u8 PwrTrackingLimit_CCK = 28; /* 2dB */
>  	u8 TxRate = 0xFF;
> -	u8 Final_OFDM_Swing_Index = 0;
> -	u8 Final_CCK_Swing_Index = 0;
> +	int Final_OFDM_Swing_Index = 0;
> +	int Final_CCK_Swing_Index = 0;
>  
>  	{
>  		u16 rate = *(pDM_Odm->pForcedDataRate);
> @@ -217,7 +217,7 @@ void ODM_TxPwrTrackSetPwr_8723B(
>  
>  		if (Final_CCK_Swing_Index >= CCK_TABLE_SIZE)
>  			Final_CCK_Swing_Index = CCK_TABLE_SIZE-1;
> -		else if (pDM_Odm->BbSwingIdxCck <= 0)
> +		else if (Final_CCK_Swing_Index <= 0)
>  			Final_CCK_Swing_Index = 0;
>  
>  		setIqkMatrix_8723B(pDM_Odm, Final_OFDM_Swing_Index, RFPath,
> -- 
> 2.43.0
> 
> 

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Dan Carpenter]

On Wed, Jul 29, 2026 at 10:57:07PM +0530, Mohit Mishra wrote:
> In ODM_TxPwrTrackSetPwr_8723B(), the baseband swing index variables
> Final_OFDM_Swing_Index and Final_CCK_Swing_Index are declared as u8.
> However, their calculation adds Absolute_OFDMSwingIdx, which is a signed
> 8-bit integer (s8) and can be negative:
> 
>     Final_OFDM_Swing_Index = pDM_Odm->DefaultOfdmIndex +
>                              pDM_Odm->Absolute_OFDMSwingIdx[RFPath];
> 
> If the resulting sum is negative, it underflows under u8 rules (e.g. -5
> becomes 251). This causes the lower-limit checks (e.g. <= 0) to fail,
> and in MIX_MODE causes the logic to execute the "BBSwing higher than limit"
> branch instead of capping to 0.
> 
> Additionally, in BBSWING mode, the check for CCK underflow mistakenly
> examines the static struct member pDM_Odm->BbSwingIdxCck instead of the
> newly calculated Final_CCK_Swing_Index:
> 
>     else if (pDM_Odm->BbSwingIdxCck <= 0)
> 

This is a separate thing and needs to be in a separate patch with a
Fixes tag.


> Fix this by changing both swing index variable types to int to enable
> signed math and correct branch selection (aligning with the TODO item to
> convert remaining unusual variable types). Update the CCK check in
> BBSWING mode to examine Final_CCK_Swing_Index.
> 
> Note: The fix is scoped to the calculation and branching logic. When
> passed downstream to setIqkMatrix_8723B() and setCCKFilterCoefficient(),
> the values are already clamped within [0, 42], fitting safely in u8.
> 
> Compile-tested only; no hardware available for testing.
> 
> Signed-off-by: Mohit Mishra <mishraloopmohit@gmail.com>
> ---

The type issue is fundamentally a static checker type bugfix that
unsigned values can't be less than zero.  Linus's take on that is
that this code:

	if (x < 0 || x > limit) {

is perfectly fine and readable as a clamp even when x is unsigned.

In this case the commit message has a lot of extra discussion about
how the math could lead to a negative value because we entered negative
data or we had an integer overflow etc.  The result is that we clamped
it to zero instead of to the upper bound.  There is no evidence that
any of this is possible in real life.  And also if it were who cares?
Zero is a valid value.  If you use a complicated integer overflow method
to get zero instead of just doing it the normal way, the result is the
same...

The code is pure garbage, of course.  I also have written that choosing
u8 for this type of variable is dumb:
https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
I don't object to fixing this code as part of a cleanup but the commit
message needs to be more clear that were cleaning it up because the
code is garbage and not because of some kind of complicated safety issue.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723bs: fix underflow logic in swing index calculations

[Mohit Mishra]

On Tue, Aug 4, 2026 at 5:40 PM Dan Carpenter <error27@gmail.com> wrote:
>
> On Wed, Jul 29, 2026 at 10:57:07PM +0530, Mohit Mishra wrote:
> > In ODM_TxPwrTrackSetPwr_8723B(), the baseband swing index variables
> > Final_OFDM_Swing_Index and Final_CCK_Swing_Index are declared as u8.
> > However, their calculation adds Absolute_OFDMSwingIdx, which is a signed
> > 8-bit integer (s8) and can be negative:
> >
> >     Final_OFDM_Swing_Index = pDM_Odm->DefaultOfdmIndex +
> >                              pDM_Odm->Absolute_OFDMSwingIdx[RFPath];
> >
> > If the resulting sum is negative, it underflows under u8 rules (e.g. -5
> > becomes 251). This causes the lower-limit checks (e.g. <= 0) to fail,
> > and in MIX_MODE causes the logic to execute the "BBSwing higher than limit"
> > branch instead of capping to 0.
> >
> > Additionally, in BBSWING mode, the check for CCK underflow mistakenly
> > examines the static struct member pDM_Odm->BbSwingIdxCck instead of the
> > newly calculated Final_CCK_Swing_Index:
> >
> >     else if (pDM_Odm->BbSwingIdxCck <= 0)
> >
>
> This is a separate thing and needs to be in a separate patch with a
> Fixes tag.
>
>
> > Fix this by changing both swing index variable types to int to enable
> > signed math and correct branch selection (aligning with the TODO item to
> > convert remaining unusual variable types). Update the CCK check in
> > BBSWING mode to examine Final_CCK_Swing_Index.
> >
> > Note: The fix is scoped to the calculation and branching logic. When
> > passed downstream to setIqkMatrix_8723B() and setCCKFilterCoefficient(),
> > the values are already clamped within [0, 42], fitting safely in u8.
> >
> > Compile-tested only; no hardware available for testing.
> >
> > Signed-off-by: Mohit Mishra <mishraloopmohit@gmail.com>
> > ---
>
> The type issue is fundamentally a static checker type bugfix that
> unsigned values can't be less than zero.  Linus's take on that is
> that this code:
>
>         if (x < 0 || x > limit) {
>
> is perfectly fine and readable as a clamp even when x is unsigned.
>
> In this case the commit message has a lot of extra discussion about
> how the math could lead to a negative value because we entered negative
> data or we had an integer overflow etc.  The result is that we clamped
> it to zero instead of to the upper bound.  There is no evidence that
> any of this is possible in real life.  And also if it were who cares?
> Zero is a valid value.  If you use a complicated integer overflow method
> to get zero instead of just doing it the normal way, the result is the
> same...
>
> The code is pure garbage, of course.  I also have written that choosing
> u8 for this type of variable is dumb:
> https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
> I don't object to fixing this code as part of a cleanup but the commit
> message needs to be more clear that were cleaning it up because the
> code is garbage and not because of some kind of complicated safety issue.
>
> regards,
> dan carpenter
>
Hi Dan,

Thank you for the detailed feedback and for sharing the article!

That makes total sense. I'll simplify the commit message for v2 to frame this
strictly as a code cleanup,

I've also dropped the BBSWING check change entirely as suggested
keeping v2 as a focused 2-line type cleanup.

I'll submit v2 in reply to this thread

Thanks,
Mohit Mishra