[v3 0/1] Unnecessary code cleanup patches

[v3 0/1] Unnecessary code cleanup patches

[Abraham Samuel Adekunle]

The patchset modifies unnecessary duplicate NULL test on a
value in an expression, when the value has been NULL tested in a
previous expression.

Changes in v2:
- Suggested by Andy Shevchenko.

* Dropped patch files for media drivers from patchset as it is
not meant for outreachy applicants.
* Added full-stop sign to text in commit message.
* Added space around binary operators.
* Made code more breadable by adding a line break.
* Replace bitwise AND (&) with modulo (%) in expression.

Changes in v1:
- Suggested by Andy Shevchenko

* Patch for drivers/staging/media/av7110/sp8870.c and
  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
  is split into two different patches in the patchset for use
  by the different driver maintainers.
* Added subject title for each of the separated patch.
* Patch 1: Removed unnecessary curl braces {} initially inserted.
* Patch 2: Unnecessary {} was also removed for v1.

Abraham Samuel Adekunle (1):
  staging: rtl8723bs: Prevent duplicate NULL tests on a value

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  2 +-
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 58 +++++++++----------
 2 files changed, 30 insertions(+), 30 deletions(-)

-- 
2.34.1

[v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Abraham Samuel Adekunle]

When a value has been tested for NULL in an expression, a
second NULL test on the same value in another expression
is unnecessary when the value has not been assigned NULL.

Remove unnecessary duplicate NULL tests on the same value that
has previously been NULL tested.

Found by Coccinelle.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  2 +-
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 58 +++++++++----------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 90966b7034ab..675226535cd1 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1323,7 +1323,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
 	spin_unlock_bh(&pstapriv->asoc_list_lock);
 
 	/*  now the station is qualified to join our BSS... */
-	if (pstat && (pstat->state & WIFI_FW_ASSOC_SUCCESS) && (status == WLAN_STATUS_SUCCESS)) {
+	if ((pstat->state & WIFI_FW_ASSOC_SUCCESS) && (status == WLAN_STATUS_SUCCESS)) {
 		/* 1 bss_cap_update & sta_info_update */
 		bss_cap_update_on_sta_join(padapter, pstat);
 		sta_info_update(padapter, pstat);
diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 026061b464f7..ae268dda4c4d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -941,35 +941,35 @@ s32 rtw_make_wlanhdr(struct adapter *padapter, u8 *hdr, struct pkt_attrib *pattr
 			if (!(psta->state & _FW_LINKED))
 				return _FAIL;
 
-			if (psta) {
-				psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
-				psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
-				pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
-
-				SetSeqNum(hdr, pattrib->seqnum);
-
-				/* check if enable ampdu */
-				if (pattrib->ht_en && psta->htpriv.ampdu_enable)
-					if (psta->htpriv.agg_enable_bitmap & BIT(pattrib->priority))
-						pattrib->ampdu_en = true;
-
-				/* re-check if enable ampdu by BA_starting_seqctrl */
-				if (pattrib->ampdu_en == true) {
-					u16 tx_seq;
-
-					tx_seq = psta->BA_starting_seqctrl[pattrib->priority & 0x0f];
-
-					/* check BA_starting_seqctrl */
-					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;
-
-						pattrib->ampdu_en = true;/* AGG EN */
-					} else {
-						psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (pattrib->seqnum+1)&0xfff;
-						pattrib->ampdu_en = true;/* AGG EN */
-					}
+			psta->sta_xmitpriv.txseq_tid[pattrib->priority]++;
+			psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
+			pattrib->seqnum = psta->sta_xmitpriv.txseq_tid[pattrib->priority];
+
+			SetSeqNum(hdr, pattrib->seqnum);
+
+			/* check if enable ampdu */
+			if (pattrib->ht_en && psta->htpriv.ampdu_enable)
+				if (psta->htpriv.agg_enable_bitmap & BIT(pattrib->priority))
+					pattrib->ampdu_en = true;
+
+			/* re-check if enable ampdu by BA_starting_seqctrl */
+			if (pattrib->ampdu_en == true) {
+				u16 tx_seq;
+
+				tx_seq = psta->BA_starting_seqctrl[pattrib->priority & 0x0f];
+
+				/* check BA_starting_seqctrl */
+				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;
+
+					pattrib->ampdu_en = true;/* AGG EN */
+				} else {
+					psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
+						(pattrib->seqnum + 1) % 4096;
+					pattrib->ampdu_en = true;/* AGG EN */
 				}
 			}
 		}
-- 
2.34.1

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Andy Shevchenko]

On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
<abrahamadekunle50@gmail.com> wrote:
>
> When a value has been tested for NULL in an expression, a
> second NULL test on the same value in another expression
> is unnecessary when the value has not been assigned NULL.
>
> Remove unnecessary duplicate NULL tests on the same value that
> has previously been NULL tested.
>
> Found by Coccinelle.

...

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

> +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> +                                               (tx_seq + 1) & 0xfff;

> +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> +                                               (pattrib->seqnum + 1) % 4096;

Logically it's obvious that you need to align all cases to have
consistent approach.
Besides that the commit message should mention this change. Something like this
"While at it, convert '& 0xfff' cases to use modulo operator and
decimal number to make the upper limit visible and clear what the
semantic of it is."

-- 
With Best Regards,
Andy Shevchenko

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Samuel Abraham]

On Fri, Apr 4, 2025 at 8:53 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> <abrahamadekunle50@gmail.com> wrote:
> >
> > When a value has been tested for NULL in an expression, a
> > second NULL test on the same value in another expression
> > is unnecessary when the value has not been assigned NULL.
> >
> > Remove unnecessary duplicate NULL tests on the same value that
> > has previously been NULL tested.
> >
> > Found by Coccinelle.
>
> ...
>
> > +                       psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
>
> > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > +                                               (tx_seq + 1) & 0xfff;
>
> > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > +                                               (pattrib->seqnum + 1) % 4096;
>
> Logically it's obvious that you need to align all cases to have
> consistent approach.
> Besides that the commit message should mention this change. Something like this
> "While at it, convert '& 0xfff' cases to use modulo operator and
> decimal number to make the upper limit visible and clear what the
> semantic of it is."

Okay, I will do that.
Thank you for being so patient

Adekunle.

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Dan Carpenter]

On Fri, Apr 04, 2025 at 10:53:22AM +0300, Andy Shevchenko wrote:
> On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> <abrahamadekunle50@gmail.com> wrote:
> >
> > When a value has been tested for NULL in an expression, a
> > second NULL test on the same value in another expression
> > is unnecessary when the value has not been assigned NULL.
> >
> > Remove unnecessary duplicate NULL tests on the same value that
> > has previously been NULL tested.
> >
> > Found by Coccinelle.
> 
> ...
> 
> > +                       psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> 
> > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > +                                               (tx_seq + 1) & 0xfff;
> 
> > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > +                                               (pattrib->seqnum + 1) % 4096;
> 
> Logically it's obvious that you need to align all cases to have
> consistent approach.
> Besides that the commit message should mention this change. Something like this
> "While at it, convert '& 0xfff' cases to use modulo operator and
> decimal number to make the upper limit visible and clear what the
> semantic of it is."

No, I'm sorry but that's really against the rules in drivers/staging.
Don't mix unrelated changes into a patch.  It needs to be done as a
separate patch if we're going to do that.

To be honest, I don't even want people fixing line length issues or
adding spaces.  I would have accepted small white space changes but I
prefered the v2 version of this patch.  Once you start changing
"& 0xfff" to "% 4096" that's not white space and it must be done
in a separate patch. I use a script to review white space patches
because I'm always nervous someone will slip something malicious
into 100+ lines of reformated code.  It's really fast to review
patches with my script but once people start mixing things in then
it's a headache for me.

Also if the change accidentally introduces a bug, I want it to be a
one liner change and not something hidden inside a giant reformat.

regards,
dan carpenter

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Samuel Abraham]

On Fri, Apr 4, 2025 at 10:06 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Apr 04, 2025 at 10:53:22AM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> > <abrahamadekunle50@gmail.com> wrote:
> > >
> > > When a value has been tested for NULL in an expression, a
> > > second NULL test on the same value in another expression
> > > is unnecessary when the value has not been assigned NULL.
> > >
> > > Remove unnecessary duplicate NULL tests on the same value that
> > > has previously been NULL tested.
> > >
> > > Found by Coccinelle.
> >
> > ...
> >
> > > +                       psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (tx_seq + 1) & 0xfff;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (pattrib->seqnum + 1) % 4096;
> >
> > Logically it's obvious that you need to align all cases to have
> > consistent approach.
> > Besides that the commit message should mention this change. Something like this
> > "While at it, convert '& 0xfff' cases to use modulo operator and
> > decimal number to make the upper limit visible and clear what the
> > semantic of it is."
>
> No, I'm sorry but that's really against the rules in drivers/staging.
> Don't mix unrelated changes into a patch.  It needs to be done as a
> separate patch if we're going to do that.
>
> To be honest, I don't even want people fixing line length issues or
> adding spaces.  I would have accepted small white space changes but I
> prefered the v2 version of this patch.  Once you start changing
> "& 0xfff" to "% 4096" that's not white space and it must be done
> in a separate patch. I use a script to review white space patches
> because I'm always nervous someone will slip something malicious
> into 100+ lines of reformated code.  It's really fast to review
> patches with my script but once people start mixing things in then
> it's a headache for me.
>
> Also if the change accidentally introduces a bug, I want it to be a
> one liner change and not something hidden inside a giant reformat.
>
> regards,
> dan carpenter

Hello Dan,
Thank you for your review.
Please can you provide some guidance on what the next steps should be for me
as regards the patch?
Thank you.

Adekunle

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Andy Shevchenko]

On Fri, Apr 4, 2025 at 12:06 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Fri, Apr 04, 2025 at 10:53:22AM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> > <abrahamadekunle50@gmail.com> wrote:

...

> > > +                       psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (tx_seq + 1) & 0xfff;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (pattrib->seqnum + 1) % 4096;
> >
> > Logically it's obvious that you need to align all cases to have
> > consistent approach.
> > Besides that the commit message should mention this change. Something like this
> > "While at it, convert '& 0xfff' cases to use modulo operator and
> > decimal number to make the upper limit visible and clear what the
> > semantic of it is."
>
> No, I'm sorry but that's really against the rules in drivers/staging.
> Don't mix unrelated changes into a patch.  It needs to be done as a
> separate patch if we're going to do that.
>
> To be honest, I don't even want people fixing line length issues or
> adding spaces.  I would have accepted small white space changes but I
> prefered the v2 version of this patch.  Once you start changing
> "& 0xfff" to "% 4096" that's not white space and it must be done
> in a separate patch. I use a script to review white space patches
> because I'm always nervous someone will slip something malicious
> into 100+ lines of reformated code.  It's really fast to review
> patches with my script but once people start mixing things in then
> it's a headache for me.

Separate patch is even better, indeed.

> Also if the change accidentally introduces a bug, I want it to be a
> one liner change and not something hidden inside a giant reformat.

The noisy {] have no point to be left. Now I'm curious, what do you
propose here?

-- 
With Best Regards,
Andy Shevchenko

Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Dan Carpenter]

On Fri, Apr 04, 2025 at 01:53:16PM +0300, Andy Shevchenko wrote:
> > Also if the change accidentally introduces a bug, I want it to be a
> > one liner change and not something hidden inside a giant reformat.
> 
> The noisy {] have no point to be left. Now I'm curious, what do you
> propose here?

The patch *must* remove the { and }.  We wouldn't have accepted the v1
patch.

regards,
dan carpenter

Re: [v3 0/1] Unnecessary code cleanup patches

[Greg KH]

On Fri, Apr 04, 2025 at 01:02:19AM +0100, Abraham Samuel Adekunle wrote:
> The patchset modifies unnecessary duplicate NULL test on a
> value in an expression, when the value has been NULL tested in a
> previous expression.

Nit, the subject line does nto have "staging:" in it, so I don't know
where this goes.

Also, it really reads as "this is an unnecessary patch" :)

And finally, for a single patch, there's usually not a need for a 0/X
email to describe it, everything should be in just the single patch
itself.

thanks,

greg k-h

Re: [v3 0/1] Unnecessary code cleanup patches

[Julia Lawall]

On Sat, 5 Apr 2025, Greg KH wrote:

> On Fri, Apr 04, 2025 at 01:02:19AM +0100, Abraham Samuel Adekunle wrote:
> > The patchset modifies unnecessary duplicate NULL test on a
> > value in an expression, when the value has been NULL tested in a
> > previous expression.
>
> Nit, the subject line does nto have "staging:" in it, so I don't know
> where this goes.
>
> Also, it really reads as "this is an unnecessary patch" :)

Technically, in English, if you have a noun that is being used as an
adjective and that noun is itself modified in some way, you should use a
hyphen:

Unnecessary-code cleanup patches

But something like

Remove unnecessary code

would have had no ambiguity at all.  Also everything you are doing is a
patch, so "patches" in the subject line doesn't seem very necessary.

julia


>
> And finally, for a single patch, there's usually not a need for a 0/X
> email to describe it, everything should be in just the single patch
> itself.
>
> thanks,
>
> greg k-h
>
>

Re: [v3 0/1] Unnecessary code cleanup patches

[Samuel Abraham]

On Sat, Apr 5, 2025 at 9:49 AM Julia Lawall <julia.lawall@inria.fr> wrote:
>
>
>
> On Sat, 5 Apr 2025, Greg KH wrote:
>
> > On Fri, Apr 04, 2025 at 01:02:19AM +0100, Abraham Samuel Adekunle wrote:
> > > The patchset modifies unnecessary duplicate NULL test on a
> > > value in an expression, when the value has been NULL tested in a
> > > previous expression.
> >
> > Nit, the subject line does nto have "staging:" in it, so I don't know
> > where this goes.
> >
> > Also, it really reads as "this is an unnecessary patch" :)
>
> Technically, in English, if you have a noun that is being used as an
> adjective and that noun is itself modified in some way, you should use a
> hyphen:
>
> Unnecessary-code cleanup patches

Yes this was what I intended to do. ;)
Thank you
>
> But something like
>
> Remove unnecessary code

Yes perfect.
>
> would have had no ambiguity at all.  Also everything you are doing is a
> patch, so "patches" in the subject line doesn't seem very necessary.
>

Thank you, Julia.

I will update it as necessary.

Adekunle.

Re: [v3 0/1] Unnecessary code cleanup patches

[Samuel Abraham]

On Sat, Apr 5, 2025 at 9:37 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Apr 04, 2025 at 01:02:19AM +0100, Abraham Samuel Adekunle wrote:
> > The patchset modifies unnecessary duplicate NULL test on a
> > value in an expression, when the value has been NULL tested in a
> > previous expression.
>
> Nit, the subject line does nto have "staging:" in it, so I don't know
> where this goes.
>
> Also, it really reads as "this is an unnecessary patch" :)

Argh, Not what I meant to say at all :)
Thank you.
>
> And finally, for a single patch, there's usually not a need for a 0/X
> email to describe it, everything should be in just the single patch
> itself.

It initially started as a patchset with modification for three different
drivers, one patch for each driver, but two were dropped from the patchset
for being media drivers.
So I thought I should add the cover letter to the only patch
left in the patchset so details about the changes can be made known.

I've made a note of this. Thanks.
Adekunle.