[PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[Abraham Samuel Adekunle]

The patchset adds spaces around binary operators, breaks long lines to enhance readability
and provides clarity on sequence number wrapping by using a modulo operation % 4096u, in
place of the bitwise AND(&) operation & 0xfff.
The patches are required to be applied in sequence.

Changes in v7:
* PATCH 1:
	- Added blank line to enhance readability
* PATCH 2:
	- changed `power of 2` to `power-of-2` in commit message.
Changes in v6:
	- Modified cover letter BLURB to reference newly added line breaks.
* PATCH 1:
	- Added line breaks to long lines to improve readability.
	- Changed commit message to include the information about the broken
	  lines added to the patch.
	- Changed subject line title to also include the newly added line breaks.
* PATCH 2:
	- Changed instances of `& 0xfff` to `% 4096u` which were now in the broken lines
	  after the broken lines have been done in PATCH 1.
Changes in v5:
	- Converted the patch with the subject "Use % 4096 instead of & 0xfff"
	  patch to a patchset.
	- Added a patch to add spaces around binary operator.
Changes in v4:
	- Corrected patch to use '%' instead of '&'.
	- 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.
Changes in v3:
	- Added more description to the commit message.
	- Removed blank line in the tag block.
	- Added more patch recipients.
Changes in v2:
	- Changed the commit message t 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.

Abraham Samuel Adekunle (2):
  staging: rtl8723bs: Add spaces and line breaks to improve readability
  staging: rtl8723bs: Use % 4096 instead of & 0xfff

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

-- 
2.34.1

[PATCH v8 1/2] staging: rtl8723bs: Add spaces and line breaks to improve readability

[Abraham Samuel Adekunle]

The code contains no spaces around binary operators with long lines
which reduces readability thereby not adhering to Linux kernel coding
style.

Add white spaces around the binary operators and use line breaks to
increase readability and ensure adherence to Linux kernel coding
styles.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_xmit.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 297c93d65315..5def2467f42b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -963,11 +963,14 @@ 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) & 0xfff;
 
 						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) & 0xfff;
+
 						pattrib->ampdu_en = true;/* AGG EN */
 					}
 				}
-- 
2.34.1

[PATCH v8 2/2] staging: rtl8723bs: Use % 4096 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 `% 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.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 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 5def2467f42b..76f25ab6a779 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);
@@ -964,12 +964,12 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
 						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;
+							(tx_seq + 1) % 4096u;
 
 						pattrib->ampdu_en = true;/* AGG EN */
 					} else {
 						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
-							(pattrib->seqnum + 1) & 0xfff;
+							(pattrib->seqnum + 1) % 4096u;
 
 						pattrib->ampdu_en = true;/* AGG EN */
 					}
-- 
2.34.1

Re: [PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 03:41:55PM +0000, Abraham Samuel Adekunle wrote:
> The patchset adds spaces around binary operators, breaks long lines to enhance readability
> and provides clarity on sequence number wrapping by using a modulo operation % 4096u, in
> place of the bitwise AND(&) operation & 0xfff.
> The patches are required to be applied in sequence.

You missed my tags I gave in a previous review round.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[Samuel Abraham]

On Tue, Apr 8, 2025 at 8:41 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Tue, Apr 08, 2025 at 03:41:55PM +0000, Abraham Samuel Adekunle wrote:
> > The patchset adds spaces around binary operators, breaks long lines to enhance readability
> > and provides clarity on sequence number wrapping by using a modulo operation % 4096u, in
> > place of the bitwise AND(&) operation & 0xfff.
> > The patches are required to be applied in sequence.
>
> You missed my tags I gave in a previous review round.

Oh, I'm sorry I do not know how that works, please.
Am I supposed to add them to the commit message?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]



On Tue, 8 Apr 2025, Samuel Abraham wrote:

> On Tue, Apr 8, 2025 at 8:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Tue, Apr 08, 2025 at 03:41:55PM +0000, Abraham Samuel Adekunle wrote:
> > > The patchset adds spaces around binary operators, breaks long lines to enhance readability
> > > and provides clarity on sequence number wrapping by using a modulo operation % 4096u, in
> > > place of the bitwise AND(&) operation & 0xfff.
> > > The patches are required to be applied in sequence.
> >
> > You missed my tags I gave in a previous review round.
>
> Oh, I'm sorry I do not know how that works, please.
> Am I supposed to add them to the commit message?

Yes.

julia

> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >
>

Re: [PATCH v8 0/2] staging: rtl8723bs: Improve readability and clarity of sequence number wrapping

[Samuel Abraham]

On Wed, Apr 9, 2025 at 3:11 AM Julia Lawall <julia.lawall@inria.fr> wrote:
>
>
>
> On Tue, 8 Apr 2025, Samuel Abraham wrote:
>
> > On Tue, Apr 8, 2025 at 8:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > >
> > > On Tue, Apr 08, 2025 at 03:41:55PM +0000, Abraham Samuel Adekunle wrote:
> > > > The patchset adds spaces around binary operators, breaks long lines to enhance readability
> > > > and provides clarity on sequence number wrapping by using a modulo operation % 4096u, in
> > > > place of the bitwise AND(&) operation & 0xfff.
> > > > The patches are required to be applied in sequence.
> > >
> > > You missed my tags I gave in a previous review round.
> >
> > Oh, I'm sorry I do not know how that works, please.
> > Am I supposed to add them to the commit message?
>
> Yes.

Hello Julia.
Thanks for the clarity

Adekunle