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

On Tue, Aug 4, 2026 at 5:40 PM Dan Carpenter <[email protected]> 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 <[email protected]>
> > ---
>
> 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