[PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Abraham Samuel Adekunle]

The sequence number is constrained to a range of [0, 4095], which
is a total of 4096 values. The bitmask operation using `0xfff` is
used to perform this wrap-around. While this is functionally correct,
it obscures the intended semantic of a 4096-based wrap.

Using a modulo operation with `4096u` makes the wrap-around logic
explicit and easier to understand. It clearly signals that the sequence
number cycles though a range of 4096 values.
It also makes the code robust against potential changes of the 4096
upper limit, especially when it becomes a non power of 2 value while
the AND(&) works solely for power of 2 values.

The use of `4096u` also guarantees that the modulo operation is performed
with unsigned arithmetic, preventing potential issues with signed types.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
Changes in v3:
	- Added more description to the commit message
	- Removed blank line in tag block.
	-  Added more patch recipients.
Changes in v2:
	- Changed the commit message to a more descriptive message which
	makes it clear why the patch does the change.
	- Changed the subject title to include `4096u` to show that an unsigned
	module is used.
Changes in v1:
	- Added more patch recipients.

 drivers/staging/rtl8723bs/core/rtw_xmit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 297c93d65315..f534bf2448c3 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -943,7 +943,7 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
 
 			if (psta) {
 				psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
-				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
+				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;
 				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
 
 				SetSeqNum(hdr, pattrib->seqnum);
@@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
 					if (SN_LESS(pattrib->seqnum, tx_seq)) {
 						pattrib->ampdu_en = false;/* AGG BK */
 					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
-						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
+						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;
 
 						pattrib->ampdu_en = true;/* AGG EN */
 					} else {
-						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum+1)&0xfff;
+						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum+1)&4096u;
 						pattrib->ampdu_en = true;/* AGG EN */
 					}
 				}
-- 
2.34.1

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Greg KH]

On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> The sequence number is constrained to a range of [0, 4095], which
> is a total of 4096 values. The bitmask operation using `0xfff` is
> used to perform this wrap-around. While this is functionally correct,
> it obscures the intended semantic of a 4096-based wrap.
> 
> Using a modulo operation with `4096u` makes the wrap-around logic

<snip>

> -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;

I do not see a modulo operation here, only another & operation.

>  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
>  
>  				SetSeqNum(hdr, pattrib->seqnum);
> @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
>  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
>  						pattrib->ampdu_en = false;/* AGG BK */
>  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;

This also looks odd, nothing is being "AND" here, it's an address value
being set (and an odd one at that, but that's another issue...)

How was any of this tested?

Slow down, take some time, and think about what you are wanting to do
here.  There's no rush.

thanks,

greg k-h

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Greg KH]

On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:
> On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> > The sequence number is constrained to a range of [0, 4095], which
> > is a total of 4096 values. The bitmask operation using `0xfff` is
> > used to perform this wrap-around. While this is functionally correct,
> > it obscures the intended semantic of a 4096-based wrap.
> > 
> > Using a modulo operation with `4096u` makes the wrap-around logic
> 
> <snip>
> 
> > -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;
> 
> I do not see a modulo operation here, only another & operation.
> 
> >  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> >  
> >  				SetSeqNum(hdr, pattrib->seqnum);
> > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> >  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> >  						pattrib->ampdu_en = false;/* AGG BK */
> >  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;
> 
> This also looks odd, nothing is being "AND" here, it's an address value
> being set (and an odd one at that, but that's another issue...)

Sorry, no, I was wrong, it is being & here, but not %.  My fault,
the lack of spaces here threw me.

thanks,

greg k-h

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Dan Carpenter]

On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> The sequence number is constrained to a range of [0, 4095], which
> is a total of 4096 values. The bitmask operation using `0xfff` is
> used to perform this wrap-around. While this is functionally correct,
> it obscures the intended semantic of a 4096-based wrap.
> 
> Using a modulo operation with `4096u` makes the wrap-around logic
> explicit and easier to understand. It clearly signals that the sequence
> number cycles though a range of 4096 values.
> It also makes the code robust against potential changes of the 4096
> upper limit, especially when it becomes a non power of 2 value while
> the AND(&) works solely for power of 2 values.
> 
> The use of `4096u` also guarantees that the modulo operation is performed
> with unsigned arithmetic, preventing potential issues with signed types.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
> Changes in v3:
> 	- Added more description to the commit message
> 	- Removed blank line in tag block.
> 	-  Added more patch recipients.
> Changes in v2:
> 	- Changed the commit message to a more descriptive message which
> 	makes it clear why the patch does the change.
> 	- Changed the subject title to include `4096u` to show that an unsigned
> 	module is used.
> Changes in v1:
> 	- Added more patch recipients.
> 
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 297c93d65315..f534bf2448c3 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -943,7 +943,7 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
>  
>  			if (psta) {
>  				psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
> -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;

You forgot to change the & to %.

regards,
dan carpenter

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Samuel Abraham]

On Mon, Apr 7, 2025 at 7:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:
> > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> > > The sequence number is constrained to a range of [0, 4095], which
> > > is a total of 4096 values. The bitmask operation using `0xfff` is
> > > used to perform this wrap-around. While this is functionally correct,
> > > it obscures the intended semantic of a 4096-based wrap.
> > >
> > > Using a modulo operation with `4096u` makes the wrap-around logic
> >
> > <snip>
> >
> > > -                           psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > +                           psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;
> >
> > I do not see a modulo operation here, only another & operation.

I'm sorry ...
> >
> > >                             pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > >
> > >                             SetSeqNum(hdr, pattrib->seqnum);
> > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > >                                     if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > >                                             pattrib->ampdu_en = false;/* AGG BK */
> > >                                     } else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > -                                           psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > +                                           psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;
> >
> > This also looks odd, nothing is being "AND" here, it's an address value
> > being set (and an odd one at that, but that's another issue...)
>
> Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> the lack of spaces here threw me.

I want to add spaces for readability. But since the changes occurs on
the same line,
I am a bit confused about the best approach to take
Do I create a patchset, a patch for adding spaces, and a second for
changing from & to %?

Also, should be second patch be based on the file after the first
change I made, or it should be based on the original staging
tree file.

Thanks
Adekunle.

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Samuel Abraham]

On Mon, Apr 7, 2025 at 8:20 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> > The sequence number is constrained to a range of [0, 4095], which
> > is a total of 4096 values. The bitmask operation using `0xfff` is
> > used to perform this wrap-around. While this is functionally correct,
> > it obscures the intended semantic of a 4096-based wrap.
> >
> > Using a modulo operation with `4096u` makes the wrap-around logic
> > explicit and easier to understand. It clearly signals that the sequence
> > number cycles though a range of 4096 values.
> > It also makes the code robust against potential changes of the 4096
> > upper limit, especially when it becomes a non power of 2 value while
> > the AND(&) works solely for power of 2 values.
> >
> > The use of `4096u` also guarantees that the modulo operation is performed
> > with unsigned arithmetic, preventing potential issues with signed types.
> >
> > Suggested-by: Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > ---
> > Changes in v3:
> >       - Added more description to the commit message
> >       - Removed blank line in tag block.
> >       -  Added more patch recipients.
> > Changes in v2:
> >       - Changed the commit message to a more descriptive message which
> >       makes it clear why the patch does the change.
> >       - Changed the subject title to include `4096u` to show that an unsigned
> >       module is used.
> > Changes in v1:
> >       - Added more patch recipients.
> >
> >  drivers/staging/rtl8723bs/core/rtw_xmit.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> > index 297c93d65315..f534bf2448c3 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> > @@ -943,7 +943,7 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> >
> >                       if (psta) {
> >                               psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
> > -                             psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > +                             psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;
>
> You forgot to change the & to %.

Thank you very much, Dan. I will correct and resend the patch.

Adekunle

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[David Laight]

On Mon, 7 Apr 2025 08:53:30 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:
> > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:  
> > > The sequence number is constrained to a range of [0, 4095], which
> > > is a total of 4096 values. The bitmask operation using `0xfff` is
> > > used to perform this wrap-around. While this is functionally correct,
> > > it obscures the intended semantic of a 4096-based wrap.
> > > 
> > > Using a modulo operation with `4096u` makes the wrap-around logic  
> > 
> > <snip>
> >   
> > > -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;  
> > 
> > I do not see a modulo operation here, only another & operation.
> >   
> > >  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > >  
> > >  				SetSeqNum(hdr, pattrib->seqnum);
> > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > >  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > >  						pattrib->ampdu_en = false;/* AGG BK */
> > >  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;  
> > 
> > This also looks odd, nothing is being "AND" here, it's an address value
> > being set (and an odd one at that, but that's another issue...)  
> 
> Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> the lack of spaces here threw me.

It is still wrong '& 0xfff' => '% 4096u'.
But it is all rather pointless especially if you can't test it.

Plausibly more useful would be to find ALL of the uses of 0xfff/4096 (I suspect
there is an array lurking somewhere) and change them to use the same constant.
But you need to be able to test the changes - or at least discover that
they make absolutely no difference to the generated object code.

	David

> 
> thanks,
> 
> greg k-h

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 01:21:15PM +0100, David Laight wrote:
> On Mon, 7 Apr 2025 08:53:30 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:
> > > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:  

<snip>

> > > > -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > > +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;  
> > > 
> > > I do not see a modulo operation here, only another & operation.
> > >   
> > > >  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > > >  
> > > >  				SetSeqNum(hdr, pattrib->seqnum);
> > > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > > >  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > > >  						pattrib->ampdu_en = false;/* AGG BK */
> > > >  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > > -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > > +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;  
> > > 
> > > This also looks odd, nothing is being "AND" here, it's an address value
> > > being set (and an odd one at that, but that's another issue...)  
> > 
> > Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> > the lack of spaces here threw me.
> 
> It is still wrong '& 0xfff' => '% 4096u'.

Why?

> But it is all rather pointless especially if you can't test it.

> Plausibly more useful would be to find ALL of the uses of 0xfff/4096 (I suspect
> there is an array lurking somewhere) and change them to use the same constant.
> But you need to be able to test the changes - or at least discover that
> they make absolutely no difference to the generated object code.

The problem with &, it's not non-power-of-2 tolerable solution. Also using
hexadecimal there is not so helpful as when we are talking about sequences
(or indices in the circular buffer), the decimal makes more sense.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Samuel Abraham]

On Mon, Apr 7, 2025 at 1:21 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Mon, 7 Apr 2025 08:53:30 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:
> > > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:
> > > > The sequence number is constrained to a range of [0, 4095], which
> > > > is a total of 4096 values. The bitmask operation using `0xfff` is
> > > > used to perform this wrap-around. While this is functionally correct,
> > > > it obscures the intended semantic of a 4096-based wrap.
> > > >
> > > > Using a modulo operation with `4096u` makes the wrap-around logic
> > >
> > > <snip>
> > >
> > > > -                         psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > > +                         psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;
> > >
> > > I do not see a modulo operation here, only another & operation.
> > >
> > > >                           pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > > >
> > > >                           SetSeqNum(hdr, pattrib->seqnum);
> > > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > > >                                   if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > > >                                           pattrib->ampdu_en = false;/* AGG BK */
> > > >                                   } else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > > -                                         psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > > +                                         psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;
> > >
> > > This also looks odd, nothing is being "AND" here, it's an address value
> > > being set (and an odd one at that, but that's another issue...)
> >
> > Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> > the lack of spaces here threw me.
>
> It is still wrong '& 0xfff' => '% 4096u'.
> But it is all rather pointless especially if you can't test it.
>
> Plausibly more useful would be to find ALL of the uses of 0xfff/4096 (I suspect
> there is an array lurking somewhere) and change them to use the same constant.
> But you need to be able to test the changes - or at least discover that
> they make absolutely no difference to the generated object code.

Yes, thank you for this, David.

I will compare the generated object files for both cases and compare
them to make sure there is no difference.

Then, to check for other cases in the codebase, a semantic patch will
be useful, so I will write one to search for
such cases and change them to use the constant.

Thank you very much

Adekunle

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[David Laight]

On Mon, 7 Apr 2025 15:28:34 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Mon, Apr 07, 2025 at 01:21:15PM +0100, David Laight wrote:
> > On Mon, 7 Apr 2025 08:53:30 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:  
> > > On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:  
> > > > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:    
> 
> <snip>
> 
> > > > > -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > > > +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;    
> > > > 
> > > > I do not see a modulo operation here, only another & operation.
> > > >     
> > > > >  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > > > >  
> > > > >  				SetSeqNum(hdr, pattrib->seqnum);
> > > > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > > > >  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > > > >  						pattrib->ampdu_en = false;/* AGG BK */
> > > > >  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > > > -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > > > +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;    
> > > > 
> > > > This also looks odd, nothing is being "AND" here, it's an address value
> > > > being set (and an odd one at that, but that's another issue...)    
> > > 
> > > Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> > > the lack of spaces here threw me.  
> > 
> > It is still wrong '& 0xfff' => '% 4096u'.  
> 
> Why?

Do some math :-)

> > But it is all rather pointless especially if you can't test it.  
> 
> > Plausibly more useful would be to find ALL of the uses of 0xfff/4096 (I suspect
> > there is an array lurking somewhere) and change them to use the same constant.
> > But you need to be able to test the changes - or at least discover that
> > they make absolutely no difference to the generated object code.  
> 
> The problem with &, it's not non-power-of-2 tolerable solution. Also using
> hexadecimal there is not so helpful as when we are talking about sequences
> (or indices in the circular buffer), the decimal makes more sense.
> 

Except that you either want your circular buffer size to be a power of 2
or reduce with a conditional (eg: if (++x == SIZE) x = 0;) not a divide.

	David

Re: [PATCH v4] staging: rtl8723bs: Use % 4096u instead of & 0xfff

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 07:06:45PM +0100, David Laight wrote:
> On Mon, 7 Apr 2025 15:28:34 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Mon, Apr 07, 2025 at 01:21:15PM +0100, David Laight wrote:
> > > On Mon, 7 Apr 2025 08:53:30 +0200
> > > Greg KH <gregkh@linuxfoundation.org> wrote:  
> > > > On Mon, Apr 07, 2025 at 08:36:35AM +0200, Greg KH wrote:  
> > > > > On Mon, Apr 07, 2025 at 06:30:50AM +0000, Abraham Samuel Adekunle wrote:    

<snip>

> > > > > > -				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> > > > > > +				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 4096u;    
> > > > > 
> > > > > I do not see a modulo operation here, only another & operation.
> > > > >     
> > > > > >  				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
> > > > > >  
> > > > > >  				SetSeqNum(hdr, pattrib->seqnum);
> > > > > > @@ -963,11 +963,11 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
> > > > > >  					if (SN_LESS(pattrib->seqnum, tx_seq)) {
> > > > > >  						pattrib->ampdu_en = false;/* AGG BK */
> > > > > >  					} else if (SN_EQUAL(pattrib->seqnum, tx_seq)) {
> > > > > > -						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
> > > > > > +						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&4096u;    
> > > > > 
> > > > > This also looks odd, nothing is being "AND" here, it's an address value
> > > > > being set (and an odd one at that, but that's another issue...)    
> > > > 
> > > > Sorry, no, I was wrong, it is being & here, but not %.  My fault,
> > > > the lack of spaces here threw me.  
> > > 
> > > It is still wrong '& 0xfff' => '% 4096u'.  
> > 
> > Why?
> 
> Do some math :-)

Can you be more specific, please?

> > > But it is all rather pointless especially if you can't test it.  
> > 
> > > Plausibly more useful would be to find ALL of the uses of 0xfff/4096 (I suspect
> > > there is an array lurking somewhere) and change them to use the same constant.
> > > But you need to be able to test the changes - or at least discover that
> > > they make absolutely no difference to the generated object code.  
> > 
> > The problem with &, it's not non-power-of-2 tolerable solution. Also using
> > hexadecimal there is not so helpful as when we are talking about sequences
> > (or indices in the circular buffer), the decimal makes more sense.
> 
> Except that you either want your circular buffer size to be a power of 2
> or reduce with a conditional (eg: if (++x == SIZE) x = 0;) not a divide.

Where do you see a divide in the generated code? And if even so, it means that
compiler optimiser thinks that it's not worse than the bit operations.

-- 
With Best Regards,
Andy Shevchenko