[PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[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 `% 4096u` makes the wrap-around logic
explicit and easier to understand. It clearly signals that the
sequence number cycles through 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
the signed types.

Found by Coccinelle.

Suggested by Andy Shevchenko <andy@kernel.org>
Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
Coccinelle semantic patch used to find cases:
@@
expression e;

@@
* e & 0xfff

To ensure this change does not affect the functional
behaviour, I compared the generated object files before and
after the change using the `cmp` which compares the two
object files byte by byte as shown below:

$ make drivers/staging/rtl8723bs/core/rtw_xmit.o
$ cmp rtw_xmit_before.o rtw_xmit_after.o

No differences were found in the output, confirming that the
change does not alter the compiled output.

drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 2 +-
drivers/staging/rtl8723bs/core/rtw_recv.c     | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 952ce6dd5af9..f712ea0cabee 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -3511,7 +3511,7 @@ void issue_action_BA(struct adapter *padapter, unsigned char *raddr, unsigned ch
 			/* if ((psta = rtw_get_stainfo(pstapriv, pmlmeinfo->network.mac_address)) != NULL) */
 			psta = rtw_get_stainfo(pstapriv, raddr);
 			if (psta) {
-				start_seq = (psta->sta_xmitpriv.txseq_tid[status & 0x07]&0xfff) + 1;
+				start_seq = (psta->sta_xmitpriv.txseq_tid[status & 0x07] % 4096u) + 1;
 
 				psta->BA_starting_seqctrl[status & 0x07] = start_seq;
 
diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
index a389ba5ecc6f..25411cc5c738 100644
--- a/drivers/staging/rtl8723bs/core/rtw_recv.c
+++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
@@ -1641,7 +1641,7 @@ static int check_indicate_seq(struct recv_reorder_ctrl *preorder_ctrl, u16 seq_n
 	struct dvobj_priv *psdpriv = padapter->dvobj;
 	struct debug_priv *pdbgpriv = &psdpriv->drv_dbg;
 	u8 wsize = preorder_ctrl->wsize_b;
-	u16 wend = (preorder_ctrl->indicate_seq + wsize - 1) & 0xFFF;/*  4096; */
+	u16 wend = (preorder_ctrl->indicate_seq + wsize - 1) % 4096u;
 
 	/*  Rx Reorder initialize condition. */
 	if (preorder_ctrl->indicate_seq == 0xFFFF)
@@ -1657,7 +1657,7 @@ static int check_indicate_seq(struct recv_reorder_ctrl *preorder_ctrl, u16 seq_n
 	/*  2. Incoming SeqNum is larger than the WinEnd => Window shift N */
 	/*  */
 	if (SN_EQUAL(seq_num, preorder_ctrl->indicate_seq)) {
-		preorder_ctrl->indicate_seq = (preorder_ctrl->indicate_seq + 1) & 0xFFF;
+		preorder_ctrl->indicate_seq = (preorder_ctrl->indicate_seq + 1) % 4096u;
 
 	} else if (SN_LESS(wend, seq_num)) {
 		/*  boundary situation, when seq_num cross 0xFFF */
@@ -1772,7 +1772,7 @@ static int recv_indicatepkts_in_order(struct adapter *padapter, struct recv_reor
 			list_del_init(&(prframe->u.hdr.list));
 
 			if (SN_EQUAL(preorder_ctrl->indicate_seq, pattrib->seq_num))
-				preorder_ctrl->indicate_seq = (preorder_ctrl->indicate_seq + 1) & 0xFFF;
+				preorder_ctrl->indicate_seq = (preorder_ctrl->indicate_seq + 1) % 4096u;
 
 			/* Set this as a lock to make sure that only one thread is indicating packet. */
 			/* pTS->RxIndicateState = RXTS_INDICATE_PROCESSING; */
-- 
2.34.1

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Andy Shevchenko]

On Wed, Apr 9, 2025 at 9:48 PM Abraham Samuel Adekunle
<abrahamadekunle50@gmail.com> 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 `% 4096u` makes the wrap-around logic
> explicit and easier to understand. It clearly signals that the
> sequence number cycles through 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
> the signed types.
>
> Found by Coccinelle.

This should be folded to the initial change since it's the same driver
and values most likely are related to each other.


> $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> $ cmp rtw_xmit_before.o rtw_xmit_after.o

cmp is good but not good enough in general. Here it shows the 1:1
binary, but in some cases code can be the same, while binaries are
different. To make sure the code is the same use the bloat-o-meter
tool instead.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Samuel Abraham]

On Thu, Apr 10, 2025 at 6:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Apr 9, 2025 at 9:48 PM Abraham Samuel Adekunle
> <abrahamadekunle50@gmail.com> 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 `% 4096u` makes the wrap-around logic
> > explicit and easier to understand. It clearly signals that the
> > sequence number cycles through 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
> > the signed types.
> >
> > Found by Coccinelle.
>
> This should be folded to the initial change since it's the same driver
> and values most likely are related to each other.

Okay noted.
>
>
> > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > $ cmp rtw_xmit_before.o rtw_xmit_after.o
>
> cmp is good but not good enough in general. Here it shows the 1:1
> binary, but in some cases code can be the same, while binaries are
> different. To make sure the code is the same use the bloat-o-meter
> tool instead.

Okay, thanks for the suggestion. I will check out the bloat-o-meter tool.
Adekunle.

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Dan Carpenter]

On Thu, Apr 10, 2025 at 08:43:53PM +0300, Andy Shevchenko wrote:
> 
> > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> 
> cmp is good but not good enough in general. Here it shows the 1:1
> binary, but in some cases code can be the same, while binaries are
> different. To make sure the code is the same use the bloat-o-meter
> tool instead.
> 

I don't understand what you're saying at all.  cmp shows that the compiler
was automaticall tanslating the "& 0xfff" into "% 04096u" so the resulting
object files are exactly the same.

./scripts/bloat-o-meter just looks at the sizes so it's less useful in
this case.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Andy Shevchenko]

On Fri, Apr 11, 2025 at 1:37 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Thu, Apr 10, 2025 at 08:43:53PM +0300, Andy Shevchenko wrote:
> >
> > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> >
> > cmp is good but not good enough in general. Here it shows the 1:1
> > binary, but in some cases code can be the same, while binaries are
> > different. To make sure the code is the same use the bloat-o-meter
> > tool instead.
>
> I don't understand what you're saying at all.  cmp shows that the compiler
> was automaticall tanslating the "& 0xfff" into "% 04096u" so the resulting
> object files are exactly the same.

There is a possibility that the compiler changes the code to the
equivalent and cmp will break, bloat-o-meter will check at least what
happened to the size of the *code*. object file is not only a code. It
may also contain debug symbols and other stuff which may break cmp,
while the code can be *the same*.

> ./scripts/bloat-o-meter just looks at the sizes so it's less useful in
> this case.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Dan Carpenter]

On Fri, Apr 11, 2025 at 03:47:40PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 11, 2025 at 1:37 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > On Thu, Apr 10, 2025 at 08:43:53PM +0300, Andy Shevchenko wrote:
> > >
> > > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > >
> > > cmp is good but not good enough in general. Here it shows the 1:1
> > > binary, but in some cases code can be the same, while binaries are
> > > different. To make sure the code is the same use the bloat-o-meter
> > > tool instead.
> >
> > I don't understand what you're saying at all.  cmp shows that the compiler
> > was automaticall tanslating the "& 0xfff" into "% 04096u" so the resulting
> > object files are exactly the same.
> 
> There is a possibility that the compiler changes the code to the
> equivalent and cmp will break, bloat-o-meter will check at least what
> happened to the size of the *code*. object file is not only a code. It
> may also contain debug symbols and other stuff which may break cmp,
> while the code can be *the same*.

So far as I can see bloat-o-meter is only looking at the size.  I
deliberately introduced a bug to see if bloat-o-meter said anything but
it said the size was the same.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Andy Shevchenko]

On Sat, Apr 12, 2025 at 5:19 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Fri, Apr 11, 2025 at 03:47:40PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 11, 2025 at 1:37 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > On Thu, Apr 10, 2025 at 08:43:53PM +0300, Andy Shevchenko wrote:
> > > >
> > > > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > > >
> > > > cmp is good but not good enough in general. Here it shows the 1:1
> > > > binary, but in some cases code can be the same, while binaries are
> > > > different. To make sure the code is the same use the bloat-o-meter
> > > > tool instead.
> > >
> > > I don't understand what you're saying at all.  cmp shows that the compiler
> > > was automaticall tanslating the "& 0xfff" into "% 04096u" so the resulting
> > > object files are exactly the same.
> >
> > There is a possibility that the compiler changes the code to the
> > equivalent and cmp will break, bloat-o-meter will check at least what
> > happened to the size of the *code*. object file is not only a code. It
> > may also contain debug symbols and other stuff which may break cmp,
> > while the code can be *the same*.
>
> So far as I can see bloat-o-meter is only looking at the size.  I
> deliberately introduced a bug to see if bloat-o-meter said anything but
> it said the size was the same.

Not "size", "size of the code" is more specific. That makes the
difference to cmp which may lag even on the same code, but slightly
different debuginfo, for example.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Greg KH]

On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> explicit and easier to understand. It clearly signals that the
> sequence number cycles through 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
> the signed types.
> 
> Found by Coccinelle.
> 
> Suggested by Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
> Coccinelle semantic patch used to find cases:
> @@
> expression e;
> 
> @@
> * e & 0xfff
> 
> To ensure this change does not affect the functional
> behaviour, I compared the generated object files before and
> after the change using the `cmp` which compares the two
> object files byte by byte as shown below:
> 
> $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> $ cmp rtw_xmit_before.o rtw_xmit_after.o
> 
> No differences were found in the output, confirming that the
> change does not alter the compiled output.

This is version 11, right?  What happened to the list of previous
versions and what changed down here?

confused,

greg k-h

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Samuel Abraham]

On Mon, Apr 14, 2025 at 8:23 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> > explicit and easier to understand. It clearly signals that the
> > sequence number cycles through 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
> > the signed types.
> >
> > Found by Coccinelle.
> >
> > Suggested by Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > ---
> > Coccinelle semantic patch used to find cases:
> > @@
> > expression e;
> >
> > @@
> > * e & 0xfff
> >
> > To ensure this change does not affect the functional
> > behaviour, I compared the generated object files before and
> > after the change using the `cmp` which compares the two
> > object files byte by byte as shown below:
> >
> > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> >
> > No differences were found in the output, confirming that the
> > change does not alter the compiled output.
>
> This is version 11, right?  What happened to the list of previous
> versions and what changed down here?
>
> confused,

Hello Greg,

I collapsed this patch to the previous patchset I had worked on that
made the same changes to the same driver.
So this patch was collapsed into PATCH v10, which is the last version
for this change.

The change log in "[PATCH v10 0/2] staging: rtl8723bs: Improve
readability and clarity of sequence number wrapping" explains this.
This patch was collapsed into patch 2 of this patchset.
Thanks

Adekunle.

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Greg KH]

On Mon, Apr 14, 2025 at 08:38:53AM +0100, Samuel Abraham wrote:
> On Mon, Apr 14, 2025 at 8:23 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> > > explicit and easier to understand. It clearly signals that the
> > > sequence number cycles through 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
> > > the signed types.
> > >
> > > Found by Coccinelle.
> > >
> > > Suggested by Andy Shevchenko <andy@kernel.org>
> > > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > > ---
> > > Coccinelle semantic patch used to find cases:
> > > @@
> > > expression e;
> > >
> > > @@
> > > * e & 0xfff
> > >
> > > To ensure this change does not affect the functional
> > > behaviour, I compared the generated object files before and
> > > after the change using the `cmp` which compares the two
> > > object files byte by byte as shown below:
> > >
> > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > >
> > > No differences were found in the output, confirming that the
> > > change does not alter the compiled output.
> >
> > This is version 11, right?  What happened to the list of previous
> > versions and what changed down here?
> >
> > confused,
> 
> Hello Greg,
> 
> I collapsed this patch to the previous patchset I had worked on that
> made the same changes to the same driver.
> So this patch was collapsed into PATCH v10, which is the last version
> for this change.
> 
> The change log in "[PATCH v10 0/2] staging: rtl8723bs: Improve
> readability and clarity of sequence number wrapping" explains this.
> This patch was collapsed into patch 2 of this patchset.

Think about it from my side.  I get hundreds of patches each day to
review.  If I see a v10 and then a patch with no version at all, what am
I supposed to do?  I would assume something went really wrong and just
delete this "obviously old" version as it would have been the first
version of the patch, especially as there was no version information
below the --- line.

So, what would you do in my situation?  What would you want to see if
you were in my situation?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Samuel Abraham]

On Mon, Apr 14, 2025 at 10:00 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Apr 14, 2025 at 08:38:53AM +0100, Samuel Abraham wrote:
> > On Mon, Apr 14, 2025 at 8:23 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> > > > explicit and easier to understand. It clearly signals that the
> > > > sequence number cycles through 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
> > > > the signed types.
> > > >
> > > > Found by Coccinelle.
> > > >
> > > > Suggested by Andy Shevchenko <andy@kernel.org>
> > > > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > > > ---
> > > > Coccinelle semantic patch used to find cases:
> > > > @@
> > > > expression e;
> > > >
> > > > @@
> > > > * e & 0xfff
> > > >
> > > > To ensure this change does not affect the functional
> > > > behaviour, I compared the generated object files before and
> > > > after the change using the `cmp` which compares the two
> > > > object files byte by byte as shown below:
> > > >
> > > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > > >
> > > > No differences were found in the output, confirming that the
> > > > change does not alter the compiled output.
> > >
> > > This is version 11, right?  What happened to the list of previous
> > > versions and what changed down here?
> > >
> > > confused,
> >
> > Hello Greg,
> >
> > I collapsed this patch to the previous patchset I had worked on that
> > made the same changes to the same driver.
> > So this patch was collapsed into PATCH v10, which is the last version
> > for this change.
> >
> > The change log in "[PATCH v10 0/2] staging: rtl8723bs: Improve
> > readability and clarity of sequence number wrapping" explains this.
> > This patch was collapsed into patch 2 of this patchset.
>
> Think about it from my side.  I get hundreds of patches each day to
> review.  If I see a v10 and then a patch with no version at all, what am
> I supposed to do?  I would assume something went really wrong and just
> delete this "obviously old" version as it would have been the first
> version of the patch, especially as there was no version information
> below the --- line.
>
> So, what would you do in my situation?  What would you want to see if
> you were in my situation?
>
> thanks,

I am so sorry for causing this confusion. It was not intended.
PATCH v9 was the last patchset which had all the recommended changes
implemented.

Later on, I proceeded to check for more cases in the codebase so I
could make the same change.
I then saw more changes and sent the patch independently (Which is
where I made the mistake).
However, it was suggested that since this new patch was making the
same change in the same driver as PATCH v9,
I should incorporate it into the initial change. This was why I
collapsed the change and made PATCH v10.

I'm sorry once again.

Adekunle.

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Samuel Abraham]

On Mon, Apr 14, 2025 at 10:00 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Apr 14, 2025 at 08:38:53AM +0100, Samuel Abraham wrote:
> > On Mon, Apr 14, 2025 at 8:23 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> > > > explicit and easier to understand. It clearly signals that the
> > > > sequence number cycles through 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
> > > > the signed types.
> > > >
> > > > Found by Coccinelle.
> > > >
> > > > Suggested by Andy Shevchenko <andy@kernel.org>
> > > > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > > > ---
> > > > Coccinelle semantic patch used to find cases:
> > > > @@
> > > > expression e;
> > > >
> > > > @@
> > > > * e & 0xfff
> > > >
> > > > To ensure this change does not affect the functional
> > > > behaviour, I compared the generated object files before and
> > > > after the change using the `cmp` which compares the two
> > > > object files byte by byte as shown below:
> > > >
> > > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > > >
> > > > No differences were found in the output, confirming that the
> > > > change does not alter the compiled output.
> > >
> > > This is version 11, right?  What happened to the list of previous
> > > versions and what changed down here?
> > >
> > > confused,
> >
> > Hello Greg,
> >
> > I collapsed this patch to the previous patchset I had worked on that
> > made the same changes to the same driver.
> > So this patch was collapsed into PATCH v10, which is the last version
> > for this change.
> >
> > The change log in "[PATCH v10 0/2] staging: rtl8723bs: Improve
> > readability and clarity of sequence number wrapping" explains this.
> > This patch was collapsed into patch 2 of this patchset.
>
> Think about it from my side.  I get hundreds of patches each day to
> review.  If I see a v10 and then a patch with no version at all, what am
> I supposed to do?  I would assume something went really wrong and just
> delete this "obviously old" version as it would have been the first
> version of the patch, especially as there was no version information
> below the --- line.
>
> So, what would you do in my situation?  What would you want to see if
> you were in my situation?

Hello Greg, I'm sorry once again,
Please, do I send a PATCH v11 for this change?
It seems my mistake had made you delete the PATCH v10.

Thanks.
Adekunle

Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

[Greg KH]

On Thu, Apr 17, 2025 at 05:33:30PM +0100, Samuel Abraham wrote:
> On Mon, Apr 14, 2025 at 10:00 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Apr 14, 2025 at 08:38:53AM +0100, Samuel Abraham wrote:
> > > On Mon, Apr 14, 2025 at 8:23 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Apr 09, 2025 at 06:48:02PM +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 `% 4096u` makes the wrap-around logic
> > > > > explicit and easier to understand. It clearly signals that the
> > > > > sequence number cycles through 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
> > > > > the signed types.
> > > > >
> > > > > Found by Coccinelle.
> > > > >
> > > > > Suggested by Andy Shevchenko <andy@kernel.org>
> > > > > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > > > > ---
> > > > > Coccinelle semantic patch used to find cases:
> > > > > @@
> > > > > expression e;
> > > > >
> > > > > @@
> > > > > * e & 0xfff
> > > > >
> > > > > To ensure this change does not affect the functional
> > > > > behaviour, I compared the generated object files before and
> > > > > after the change using the `cmp` which compares the two
> > > > > object files byte by byte as shown below:
> > > > >
> > > > > $ make drivers/staging/rtl8723bs/core/rtw_xmit.o
> > > > > $ cmp rtw_xmit_before.o rtw_xmit_after.o
> > > > >
> > > > > No differences were found in the output, confirming that the
> > > > > change does not alter the compiled output.
> > > >
> > > > This is version 11, right?  What happened to the list of previous
> > > > versions and what changed down here?
> > > >
> > > > confused,
> > >
> > > Hello Greg,
> > >
> > > I collapsed this patch to the previous patchset I had worked on that
> > > made the same changes to the same driver.
> > > So this patch was collapsed into PATCH v10, which is the last version
> > > for this change.
> > >
> > > The change log in "[PATCH v10 0/2] staging: rtl8723bs: Improve
> > > readability and clarity of sequence number wrapping" explains this.
> > > This patch was collapsed into patch 2 of this patchset.
> >
> > Think about it from my side.  I get hundreds of patches each day to
> > review.  If I see a v10 and then a patch with no version at all, what am
> > I supposed to do?  I would assume something went really wrong and just
> > delete this "obviously old" version as it would have been the first
> > version of the patch, especially as there was no version information
> > below the --- line.
> >
> > So, what would you do in my situation?  What would you want to see if
> > you were in my situation?
> 
> Hello Greg, I'm sorry once again,
> Please, do I send a PATCH v11 for this change?

Yes please.

> It seems my mistake had made you delete the PATCH v10.

Yes I did.

thanks,

greg k-h