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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025041448-destiny-hate-1b70@gregkh>
Date: Mon, 14 Apr 2025 11:00:17 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Samuel Abraham <abrahamadekunle50@...il.com>
Cc: outreachy@...ts.linux.dev, julia.lawall@...ia.fr,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
	david.laight.linux@...il.com, dan.carpenter@...aro.org,
	andy@...nel.org
Subject: Re: [PATCH] staging: rtl8723bs: Replace `& 0xfff` with `% 4096u`

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@...uxfoundation.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@...nel.org>
> > > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@...il.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