[PATCH v2 0/3] Unnecessary code cleanup patches

[PATCH v2 0/3] Unnecessary code cleanup patches

[Abraham Samuel Adekunle]

Changes since 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 curly braces {} initially inserted
* Patch 2: Unnecessary {} was also removed from v1

Abraham Samuel Adekunle (3):
  staging: media: Remove duplicate NULL tests on a value in pci
  staging: media: Remove duplicated NULL tests on a value in av7110
  staging: rtl8723bs: Prevent duplicate NULL tests on a value

 .../media/atomisp/pci/atomisp_gmin_platform.c |  2 +-
 drivers/staging/media/av7110/sp8870.c         | 10 ++--
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  2 +-
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 56 +++++++++----------
 4 files changed, 33 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[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/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index e176483df301..0abac820299a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1292,7 +1292,7 @@ static int gmin_get_config_dsm_var(struct device *dev,
 	 * if it founds something different than string, letting it
 	 * to fall back to the old code.
 	 */
-	if (cur && cur->type != ACPI_TYPE_STRING) {
+	if (cur->type != ACPI_TYPE_STRING) {
 		dev_info(dev, "found non-string _DSM entry for '%s'\n", var);
 		ACPI_FREE(obj);
 		return -EINVAL;
-- 
2.34.1

Re: [PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 02:26:41PM +0100, Abraham Samuel Adekunle 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

Missing period. And Subject should be as simple as

"media: atomisp: Remove ..."

...

Please, send this patch separately from the others.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[Samuel Abraham]

On Thu, Apr 3, 2025 at 2:31 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Thu, Apr 03, 2025 at 02:26:41PM +0100, Abraham Samuel Adekunle 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
>
> Missing period. And Subject should be as simple as
>
> "media: atomisp: Remove ..."
>
> ...
>
> Please, send this patch separately from the others.
>
> --
Thank you very much for your review
Unfortunately I have been told I cannot work on the media driver as an
outreachy applicant.
I do appreciate your apparent reviews.
I will Cc your email when I apply your review to my rtl8723bs driver
patch of the patchset.
Thanks

Adekunle

Re: [PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[Greg KH]

On Thu, Apr 03, 2025 at 02:26:41PM +0100, Abraham Samuel Adekunle 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
> 
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-

I can't take any drivers/staging/media/ changes for outreachy
applications, sorry.  That should have been in the instructions
somewhere.

thanks,

greg k-h

Re: [PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[Hans de Goede]

Hi,

On 3-Apr-25 3:54 PM, Greg KH wrote:
> On Thu, Apr 03, 2025 at 02:26:41PM +0100, Abraham Samuel Adekunle 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
>>
>> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
>> ---
>>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
> 
> I can't take any drivers/staging/media/ changes for outreachy
> applications, sorry.  That should have been in the instructions
> somewhere.

I don't know if this helps wrt outreachy but I can pick
up this atomisp (and I plan to do so).

Regards,

Hans

Re: [PATCH v2 1/3] staging: media: Remove duplicate NULL tests on a value in pci

[Hans de Goede]

Hi,

On 3-Apr-25 3:26 PM, Abraham Samuel Adekunle 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
> 
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>

Thank you for your patch.

I have merged this in my media-atomisp branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

And this patch will be included in my next
pull-request to Mauro (to media subsystem maintainer)

Regards,

Hans




> ---
>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index e176483df301..0abac820299a 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -1292,7 +1292,7 @@ static int gmin_get_config_dsm_var(struct device *dev,
>  	 * if it founds something different than string, letting it
>  	 * to fall back to the old code.
>  	 */
> -	if (cur && cur->type != ACPI_TYPE_STRING) {
> +	if (cur->type != ACPI_TYPE_STRING) {
>  		dev_info(dev, "found non-string _DSM entry for '%s'\n", var);
>  		ACPI_FREE(obj);
>  		return -EINVAL;

[PATCH v2 2/3] staging: media: Remove duplicated NULL tests on a value in av7110

[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/media/av7110/sp8870.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/av7110/sp8870.c b/drivers/staging/media/av7110/sp8870.c
index 0c813860f5b2..65842dd559e3 100644
--- a/drivers/staging/media/av7110/sp8870.c
+++ b/drivers/staging/media/av7110/sp8870.c
@@ -508,12 +508,10 @@ static int sp8870_set_frontend(struct dvb_frontend *fe)
 	}
 
 	if (debug) {
-		if (valid) {
-			if (trials > 1) {
-				pr_info("%s(): firmware lockup!!!\n", __func__);
-				pr_info("%s(): recovered after %i trial(s))\n",  __func__, trials - 1);
-				lockups++;
-			}
+		if (trials > 1) {
+			pr_info("%s(): firmware lockup!!!\n", __func__);
+			pr_info("%s(): recovered after %i trial(s))\n",  __func__, trials - 1);
+			lockups++;
 		}
 		switches++;
 		pr_info("%s(): switches = %i lockups = %i\n", __func__, switches, lockups);
-- 
2.34.1

Re: [PATCH v2 2/3] staging: media: Remove duplicated NULL tests on a value in av7110

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 02:26:42PM +0100, Abraham Samuel Adekunle 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

Missing period. And Subject should be like

"media: sp8870: Remove ..."

When in doubt, run

	$ git log --oneline --no-merges -- $YOUR_FILE_OR_FILES

and look at the result. Use common sense and age and frequency of appearance
for possible variants. The more recent and more often wins.

...

Code wise the change looks good now.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/3] 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     | 56 +++++++++----------
 2 files changed, 28 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..f817cab2f831 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -941,35 +941,33 @@ 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)&0xfff;
+					pattrib->ampdu_en = true;/* AGG EN */
 				}
 			}
 		}
-- 
2.34.1

Re: [PATCH v2 3/3] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 02:26:43PM +0100, Abraham Samuel Adekunle 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

Missing period.

...

> +			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;

While at it, make this more readable:

					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;

Ditto.

Also consider to use module operator as it shows the exact amount of the
records we support in the circular buffer.

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

Since it's power-of-two denominator, it will be optimised by the compiler to
the same code as it's now.

> +					pattrib->ampdu_en = true;/* AGG EN */
>  				}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] staging: rtl8723bs: Prevent duplicate NULL tests on a value

[Samuel Abraham]

On Thu, Apr 3, 2025 at 2:40 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Thu, Apr 03, 2025 at 02:26:43PM +0100, Abraham Samuel Adekunle 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
>
> Missing period.

Noted.
...
> > +                                     psta->BA_starting_seqctrl[pattrib->priority & 0x0f] = (tx_seq+1)&0xfff;
>
> While at it, make this more readable:
>
>                                         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;
>
> Ditto.
>
> Also consider to use module operator as it shows the exact amount of the
> records we support in the circular buffer.
>
>                                         psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
>                                                 (pattrib->seqnum + 1) % 4096;
>
> Since it's power-of-two denominator, it will be optimised by the compiler to
> the same code as it's now.
>
> > +                                     pattrib->ampdu_en = true;/* AGG EN */
> >                               }
>
>
Thank you very much for the review
I will send an updated Patchset

Adekunle