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.1On 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
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
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
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
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
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.
© 2016 - 2025 Red Hat, Inc.