[PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Prithvi Tambewagh]

In all types of comparison/test (using ==, !=, >=, <=, >, <
operators) ensure that the constant lies to the right side of the test,
thus fixing checkpatch warnings : comparisons should place the constant
on the right side of the test, throughout the rtl8723bs driver.

Signed-off-by: Prithvi Tambewagh <activprithvi@gmail.com>
---
v1 link: https://lore.kernel.org/linux-staging/20260323145214.ubhshy2gwp52j5zh@inspiron/T/#mc3b693b37c49fbdde89171b7f1bf61b7ba8eb964

 .../staging/rtl8723bs/hal/HalBtc8723b2Ant.c    |  4 ++--
 drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c |  5 +----
 drivers/staging/rtl8723bs/hal/hal_com.c        |  8 +++-----
 drivers/staging/rtl8723bs/hal/hal_com_phycfg.c |  8 ++++----
 drivers/staging/rtl8723bs/hal/odm.c            |  4 ++--
 drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c   |  3 +--
 .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  | 18 +++++++++---------
 drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c |  2 +-
 drivers/staging/rtl8723bs/include/ieee80211.h  |  4 ++--
 drivers/staging/rtl8723bs/include/wifi.h       |  2 +-
 10 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c b/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
index d32dbf94858f..58f6cf063498 100644
--- a/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
+++ b/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
@@ -2211,7 +2211,7 @@ static void halbtc8723b2ant_RunCoexistMechanism(struct btc_coexist *pBtCoexist)
 	}
 
 	algorithm = halbtc8723b2ant_ActionAlgorithm(pBtCoexist);
-	if (pCoexSta->bC2hBtInquiryPage && (BT_8723B_2ANT_COEX_ALGO_PANHS != algorithm)) {
+	if (pCoexSta->bC2hBtInquiryPage && (algorithm != BT_8723B_2ANT_COEX_ALGO_PANHS)) {
 		halbtc8723b2ant_ActionBtInquiry(pBtCoexist);
 		return;
 	} else {
@@ -2490,7 +2490,7 @@ void EXhalbtc8723b2ant_BtInfoNotify(
 		return;
 	}
 
-	if (BT_INFO_SRC_8723B_2ANT_WIFI_FW != rspSource) {
+	if (rspSource != BT_INFO_SRC_8723B_2ANT_WIFI_FW) {
 		pCoexSta->btRetryCnt = pCoexSta->btInfoC2h[rspSource][2] & 0xf; /* [3:0] */
 
 		pCoexSta->btRssi = pCoexSta->btInfoC2h[rspSource][3] * 2 + 10;
diff --git a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
index 9df3274c1048..61b35173aa4f 100644
--- a/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
+++ b/drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
@@ -313,7 +313,7 @@ static void GetDeltaSwingTable_8723B(
 	u16 rate = *(pDM_Odm->pForcedDataRate);
 	u8 channel = pHalData->CurrentChannel;
 
-	if (1 <= channel && channel <= 14) {
+	if (channel >= 1 && channel <= 14) {
 		if (IS_CCK_RATE(rate)) {
 			*TemperatureUP_A   = pRFCalibrateInfo->DeltaSwingTableIdx_2GCCKA_P;
 			*TemperatureDOWN_A = pRFCalibrateInfo->DeltaSwingTableIdx_2GCCKA_N;
@@ -1425,9 +1425,6 @@ static void phy_IQCalibrate_8723B(
 		}
 	}
 
-	if (0x00 == PathAOK) {
-	}
-
 /* path B IQK */
 	if (is2T) {
 
diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
index 31b3e880ae6a..00fa5c386d63 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com.c
@@ -107,7 +107,7 @@ u8 hal_com_config_channel_plan(
 	pHalData->bDisableSWChannelPlan = false;
 	chnlPlan = def_channel_plan;
 
-	if (0xFF == hw_channel_plan)
+	if (hw_channel_plan == 0xFF)
 		AutoLoadFail = true;
 
 	if (!AutoLoadFail) {
@@ -122,10 +122,8 @@ u8 hal_com_config_channel_plan(
 		}
 	}
 
-	if (
-		(false == pHalData->bDisableSWChannelPlan) &&
-		rtw_is_channel_plan_valid(sw_channel_plan)
-	)
+	if (!pHalData->bDisableSWChannelPlan &&
+	    rtw_is_channel_plan_valid(sw_channel_plan))
 		chnlPlan = sw_channel_plan;
 
 	return chnlPlan;
diff --git a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
index dc2da49e6738..28ec996cd411 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
@@ -462,18 +462,18 @@ u8 PHY_GetTxPowerIndexBase(
 
 	if (IS_CCK_RATE(Rate))
 		txPower = pHalData->Index24G_CCK_Base[RFPath][chnlIdx];
-	else if (MGN_6M <= Rate)
+	else if (Rate >= MGN_6M)
 		txPower = pHalData->Index24G_BW40_Base[RFPath][chnlIdx];
 
 	/*  OFDM-1T */
-	if ((MGN_6M <= Rate && Rate <= MGN_54M) && !IS_CCK_RATE(Rate))
+	if ((Rate >= MGN_6M && Rate <= MGN_54M) && !IS_CCK_RATE(Rate))
 		txPower += pHalData->OFDM_24G_Diff[RFPath][TX_1S];
 
 	if (BandWidth == CHANNEL_WIDTH_20) { /*  BW20-1S, BW20-2S */
-		if (MGN_MCS0 <= Rate && Rate <= MGN_MCS7)
+		if (Rate >= MGN_MCS0 && Rate <= MGN_MCS7)
 			txPower += pHalData->BW20_24G_Diff[RFPath][TX_1S];
 	} else if (BandWidth == CHANNEL_WIDTH_40) { /*  BW40-1S, BW40-2S */
-		if (MGN_MCS0 <= Rate && Rate <= MGN_MCS7)
+		if (Rate >= MGN_MCS0 && Rate <= MGN_MCS7)
 			txPower += pHalData->BW40_24G_Diff[RFPath][TX_1S];
 	}
 
diff --git a/drivers/staging/rtl8723bs/hal/odm.c b/drivers/staging/rtl8723bs/hal/odm.c
index 639b6da2302b..bbe05211f4ed 100644
--- a/drivers/staging/rtl8723bs/hal/odm.c
+++ b/drivers/staging/rtl8723bs/hal/odm.c
@@ -343,9 +343,9 @@ bool ODM_RAStateCheck(
 	}
 
 	/*  Decide RATRState by RSSI. */
-	if (RSSI > HighRSSIThreshForRA)
+	if (HighRSSIThreshForRA < RSSI)
 		RATRState = DM_RATR_STA_HIGH;
-	else if (RSSI > LowRSSIThreshForRA)
+	else if (LowRSSIThreshForRA < RSSI)
 		RATRState = DM_RATR_STA_MIDDLE;
 	else
 		RATRState = DM_RATR_STA_LOW;
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c b/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
index af6cdda8238d..25bc11ac874c 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
@@ -25,9 +25,8 @@ static u8 _is_fw_read_cmd_down(struct adapter *padapter, u8 msgbox_num)
 
 	do {
 		valid = rtw_read8(padapter, REG_HMETFR) & BIT(msgbox_num);
-		if (0 == valid) {
+		if (!valid)
 			read_down = true;
-		}
 	} while ((!read_down) && (retry_cnts--));
 
 	return read_down;
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 8d259820f103..02c3f4229e74 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -405,11 +405,11 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
 			break;
 	}
 	_FWDownloadEnable(padapter, false);
-	if (_SUCCESS != rtStatus)
+	if (rtStatus != _SUCCESS)
 		goto fwdl_stat;
 
 	rtStatus = _FWFreeToGo(padapter, 10, 200);
-	if (_SUCCESS != rtStatus)
+	if (rtStatus != _SUCCESS)
 		goto fwdl_stat;
 
 fwdl_stat:
@@ -1165,15 +1165,15 @@ s32 rtl8723b_InitLLTTable(struct adapter *padapter)
 
 static void hal_get_chnl_group_8723b(u8 channel, u8 *group)
 {
-	if (1  <= channel && channel <= 2)
+	if (channel >= 1 && channel <= 2)
 		*group = 0;
-	else if (3  <= channel && channel <= 5)
+	else if (channel >= 3 && channel <= 5)
 		*group = 1;
-	else if (6  <= channel && channel <= 8)
+	else if (channel >= 6 && channel <= 8)
 		*group = 2;
-	else if (9  <= channel && channel <= 11)
+	else if (channel >= 9 && channel <= 11)
 		*group = 3;
-	else if (12 <= channel && channel <= 14)
+	else if (channel >= 12 && channel <= 14)
 		*group = 4;
 }
 
@@ -1221,7 +1221,7 @@ static void Hal_ReadPowerValueFromPROM_8723B(
 
 	memset(pwrInfo24G, 0, sizeof(struct TxPowerInfo24G));
 
-	if (0xFF == PROMContent[eeAddr+1])
+	if (PROMContent[eeAddr+1] == 0xFF)
 		AutoLoadFail = true;
 
 	if (AutoLoadFail) {
@@ -2035,7 +2035,7 @@ static void hw_var_set_bcn_func(struct adapter *padapter, u8 variable, u8 *val)
 		val8 &= ~(EN_BCN_FUNCTION | EN_TXBCN_RPT);
 
 		/*  Always enable port0 beacon function for PSTDMA */
-		if (REG_BCN_CTRL == bcn_ctrl_reg)
+		if (bcn_ctrl_reg == REG_BCN_CTRL)
 			val8 |= EN_BCN_FUNCTION;
 
 		rtw_write8(padapter, bcn_ctrl_reg, val8);
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c b/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
index a1f2cbf2cf55..9f6503ac2234 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
@@ -413,7 +413,7 @@ int rtl8723bs_xmit_thread(void *context)
 		if (signal_pending(current)) {
 			flush_signals(current);
 		}
-	} while (_SUCCESS == ret);
+	} while (ret == _SUCCESS);
 
 	complete(&pxmitpriv->SdioXmitTerminate);
 
diff --git a/drivers/staging/rtl8723bs/include/ieee80211.h b/drivers/staging/rtl8723bs/include/ieee80211.h
index 0f28c904a714..0f378830462f 100644
--- a/drivers/staging/rtl8723bs/include/ieee80211.h
+++ b/drivers/staging/rtl8723bs/include/ieee80211.h
@@ -395,8 +395,8 @@ enum {
 };
 
 #define IS_HT_RATE(_rate)				(_rate >= MGN_MCS0 && _rate <= MGN_MCS31)
-#define IS_CCK_RATE(_rate)				(MGN_1M == _rate || _rate == MGN_2M || _rate == MGN_5_5M || _rate == MGN_11M)
-#define IS_OFDM_RATE(_rate)				(MGN_6M <= _rate && _rate <= MGN_54M  && _rate != MGN_11M)
+#define IS_CCK_RATE(_rate)				(_rate == MGN_1M || _rate == MGN_2M || _rate == MGN_5_5M || _rate == MGN_11M)
+#define IS_OFDM_RATE(_rate)				(_rate >= MGN_6M && _rate <= MGN_54M  && _rate != MGN_11M)
 
 
 /* NOTE: This data is for statistical purposes; not all hardware provides this
diff --git a/drivers/staging/rtl8723bs/include/wifi.h b/drivers/staging/rtl8723bs/include/wifi.h
index 230b2c4ffd3b..31eaf40a3425 100644
--- a/drivers/staging/rtl8723bs/include/wifi.h
+++ b/drivers/staging/rtl8723bs/include/wifi.h
@@ -275,7 +275,7 @@ static inline unsigned char *get_hdr_bssid(unsigned char *pframe)
 
 static inline int IsFrameTypeCtrl(unsigned char *pframe)
 {
-	if (WIFI_CTRL_TYPE == GetFrameType(pframe))
+	if (GetFrameType(pframe) == WIFI_CTRL_TYPE)
 		return true;
 	else
 		return false;

base-commit: c369299895a591d96745d6492d4888259b004a9e
-- 
2.34.1

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Andy Shevchenko]

On Mon, Mar 23, 2026 at 09:59:01PM +0530, Prithvi Tambewagh wrote:
> In all types of comparison/test (using ==, !=, >=, <=, >, <
> operators) ensure that the constant lies to the right side of the test,
> thus fixing checkpatch warnings : comparisons should place the constant
> on the right side of the test, throughout the rtl8723bs driver.

...

>  	do {
>  		valid = rtw_read8(padapter, REG_HMETFR) & BIT(msgbox_num);
> -		if (0 == valid) {
> +		if (!valid)
>  			read_down = true;
> -		}
>  	} while ((!read_down) && (retry_cnts--));
>  
>  	return read_down;

This entire piece can be replaced with something from iopoll.h.

...

> -	if (WIFI_CTRL_TYPE == GetFrameType(pframe))
> +	if (GetFrameType(pframe) == WIFI_CTRL_TYPE)
>  		return true;
>  	else
>  		return false;

Just

	return GetFrameType(pframe) == WIFI_CTRL_TYPE;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Prithvi]

On Mon, Mar 23, 2026 at 06:42:49PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2026 at 09:59:01PM +0530, Prithvi Tambewagh wrote:
> > In all types of comparison/test (using ==, !=, >=, <=, >, <
> > operators) ensure that the constant lies to the right side of the test,
> > thus fixing checkpatch warnings : comparisons should place the constant
> > on the right side of the test, throughout the rtl8723bs driver.
> 
> ...
> 
> >  	do {
> >  		valid = rtw_read8(padapter, REG_HMETFR) & BIT(msgbox_num);
> > -		if (0 == valid) {
> > +		if (!valid)
> >  			read_down = true;
> > -		}
> >  	} while ((!read_down) && (retry_cnts--));
> >  
> >  	return read_down;
> 
> This entire piece can be replaced with something from iopoll.h.
> 

Yes...I checked out and found we can use read_poll_timeout_atomic():

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c b/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
index 25bc11ac874c..6fa694ce27ec 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
@@ -9,6 +9,7 @@
 #include <rtl8723b_hal.h>
 #include <linux/etherdevice.h>
 #include "hal_com_h2c.h"
+#include <linux/iopoll.h>
 
 #define MAX_H2C_BOX_NUMS       4
 #define MESSAGE_BOX_SIZE       4
@@ -18,18 +19,17 @@
 
 static u8 _is_fw_read_cmd_down(struct adapter *padapter, u8 msgbox_num)
 {
-       u8 read_down = false;
-       int retry_cnts = 100;
-
        u8 valid;
-
-       do {
-               valid = rtw_read8(padapter, REG_HMETFR) & BIT(msgbox_num);
-               if (!valid)
-                       read_down = true;
-       } while ((!read_down) && (retry_cnts--));
-
-       return read_down;
+       int ret;
+
+       ret = read_poll_timeout_atomic(rtw_read8,
+                         valid,
+                         !(valid & BIT(msgbox_num)),
+                         0,
+                         500,
+                         false,
+                         padapter, REG_HMETFR);
+       return !ret;
 
 }

Is this approach correct?

> ...
> 
> > -	if (WIFI_CTRL_TYPE == GetFrameType(pframe))
> > +	if (GetFrameType(pframe) == WIFI_CTRL_TYPE)
> >  		return true;
> >  	else
> >  		return false;
> 
> Just
> 
> 	return GetFrameType(pframe) == WIFI_CTRL_TYPE;

Sure...I will incorporate this in the next version of patch.

Thanks,
Prithvi

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 06:32:34PM +0530, Prithvi wrote:
> On Mon, Mar 23, 2026 at 06:42:49PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2026 at 09:59:01PM +0530, Prithvi Tambewagh wrote:

...

> > >  	do {
> > >  		valid = rtw_read8(padapter, REG_HMETFR) & BIT(msgbox_num);
> > > -		if (0 == valid) {
> > > +		if (!valid)
> > >  			read_down = true;
> > > -		}
> > >  	} while ((!read_down) && (retry_cnts--));
> > >  
> > >  	return read_down;
> > 
> > This entire piece can be replaced with something from iopoll.h.
> 
> Yes...I checked out and found we can use read_poll_timeout_atomic():

...

>  #include <rtl8723b_hal.h>
>  #include <linux/etherdevice.h>
>  #include "hal_com_h2c.h"
> +#include <linux/iopoll.h>

Just make sure you put it in a better (more ordered) place, like one line above.

  ...
  #include <linux/etherdevice.h>
  #include <linux/iopoll.h>
  ...

...

>  static u8 _is_fw_read_cmd_down(struct adapter *padapter, u8 msgbox_num)
>  {
>         u8 valid;
> +       int ret;
> +
> +       ret = read_poll_timeout_atomic(rtw_read8,
> +                         valid,
> +                         !(valid & BIT(msgbox_num)),
> +                         0,
> +                         500,
> +                         false,
> +                         padapter, REG_HMETFR);
> +       return !ret;

		ret = read_poll_timeout_atomic(rtw_read8,
					       valid, !(valid & BIT(msgbox_num)),
					       0, 500, false,
					       padapter, REG_HMETFR);

		return !ret;

>  }
> 
> Is this approach correct?

Yes, looks correct. Just compress it a bit and fix indentation,
see above how.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Prithvi]

Sure. Also can you please guide if any testing would be required for these 
changes, or I can directly send the v3 patch?

Thanks,
Prithvi

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 07:25:51PM +0530, Prithvi wrote:
> Sure. Also can you please guide if any testing would be required for these 
> changes, or I can directly send the v3 patch?

It's implied that the author of the patch each time before submission (either
a new version of the existing patch or a new patch in general) is tested and
validated it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Prithvi]

On Tue, Mar 24, 2026 at 04:05:51PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 24, 2026 at 07:25:51PM +0530, Prithvi wrote:
> > Sure. Also can you please guide if any testing would be required for these 
> > changes, or I can directly send the v3 patch?
> 
> It's implied that the author of the patch each time before submission (either
> a new version of the existing patch or a new patch in general) is tested and
> validated it.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Understood. I agree that patch must be tested and validated every time.

To be transparent, I do not have access to the physical Realtek hardware, 
so I am unable to perform runtime testing. However, for this v3, I have 
performed compile testing and ensured that the module builds correctly. 
Will this be acceptable for this patch?

Best Regards,
Prithvi

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Shuah Khan]

On 3/24/26 10:02, Prithvi wrote:
> On Tue, Mar 24, 2026 at 04:05:51PM +0200, Andy Shevchenko wrote:
>> On Tue, Mar 24, 2026 at 07:25:51PM +0530, Prithvi wrote:
>>> Sure. Also can you please guide if any testing would be required for these
>>> changes, or I can directly send the v3 patch?
>>
>> It's implied that the author of the patch each time before submission (either
>> a new version of the existing patch or a new patch in general) is tested and
>> validated it.

+1

>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
>>
>>
> 
> Understood. I agree that patch must be tested and validated every time.
> 
> To be transparent, I do not have access to the physical Realtek hardware,
> so I am unable to perform runtime testing. However, for this v3, I have
> performed compile testing and ensured that the module builds correctly.
> Will this be acceptable for this patch?

No -  Hmm. Aren't you part of Fall mentorship program?

We discussed this several times during the program that when someone
sends a patch, it is implied that they tested the patch. Compile
testing isn't sufficient. The changes in this patch not trivial spelling
fixes in comments either.

  
-	if (1 <= channel && channel <= 14) {
+	if (channel >= 1 && channel <= 14) {

I picked just one and there are more like this one.

These aren't just moving the constant lies to the right side of the test,
they change logic and in some cases removing the code and the change log
doesn't say anything about that.

Did you look into if this conditional can just go away?

-	if (0x00 == PathAOK) {
-	}


I am not the maintainer for this, but as your mentor this isn't acceptable.
I wouldn't take this patch unless these changes are tested on the device.

thanks,
-- Shuah

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Dan Carpenter]

There are a couple of different things happening here.  The mentorship
program has its own rules.  The rule that people should test their code
seems like a good rule.

In staging, most of the patches that we apply not tested.  I assumed
this patch wasn't tested.  Patches like this are not a big deal because
we can easily review them.  Sometimes, patches change runtime so unless
it's a security issue, we want those to be tested.

> 
> -	if (0x00 == PathAOK) {
> -	}
> 

We normally wouldn't merge patches which do multiple things at once
but it doesn't really make sense to reverse this condition around since
it's dead code so to me it falls under the "related change" category.

regards,
dan carpenter

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Shuah Khan]

On 3/25/26 01:22, Dan Carpenter wrote:
> There are a couple of different things happening here.  The mentorship
> program has its own rules.  The rule that people should test their code
> seems like a good rule.

Correct. The goal is to avoid introducing regressions.

> 
> In staging, most of the patches that we apply not tested.  I assumed
> this patch wasn't tested.  Patches like this are not a big deal because
> we can easily review them.  Sometimes, patches change runtime so unless
> it's a security issue, we want those to be tested.
> 
>>
>> -	if (0x00 == PathAOK) {
>> -	}
>>
> 
> We normally wouldn't merge patches which do multiple things at once
> but it doesn't really make sense to reverse this condition around since
> it's dead code so to me it falls under the "related change" category.

My primary concern with this change is that it 10 files changed, with
26 insertions(+), 32 deletions(-)

It can be difficult to find any regressions unless the changes are
tested. I understand it is staging repo, but if we relax the rules
for staging irrespective of the scope of change, it becomes lot
harder to find regressions later. We are essentially leaving the
job of testing to users. Also if ans when the driver is pulled into
mainline, it will inherit the regressions that crept in.

thanks,
-- Shuah

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Dan Carpenter]

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

On Wed, Mar 25, 2026 at 11:57:38AM -0600, Shuah Khan wrote:
> My primary concern with this change is that it 10 files changed, with
> 26 insertions(+), 32 deletions(-)
> 
> It can be difficult to find any regressions unless the changes are
> tested. I understand it is staging repo, but if we relax the rules
> for staging irrespective of the scope of change, it becomes lot
> harder to find regressions later. We are essentially leaving the
> job of testing to users. Also if ans when the driver is pulled into
> mainline, it will inherit the regressions that crept in.
> 

To me this is a mechanical patch (basically automatic, flip the
if statement) from a newbie.  These kinds of patches are seldom a
problem.

I took a sample of the patches for five years from 2021 to the end of
2025.  We merged 54406 commits in staging.  We had 401 Fixes tags.  But
then I decided I don't care about Kconfig problems so I cut it down to
380 bugs.  Half of those (189 commits) were from when the driver was
merged.  We tend to not review code very much when it is first merged
to staging.  The other half (191 commits) were from us missing bugs
during review.  191 bugs over five years is 40 bugs per year.

I hand reviewed a bunch of the 191 commits.  It's mostly senior
developers and maintainers who introduce bugs.  They are normally doing
complicated things like adding features or re-working core parts of the
driver.  Quite a few of these patches were tested.

The fallout from newbie patches is pretty rare and often really
minor.  A common thing is "We deleted code but we didn't delete
everything."  The number of serious mess ups is probably just a
couple times per year out of the 40 total bugs per year.

I've attached my script so you can check yourself.  NEW means the
bug was introduced by a new driver.  OLD means, ideally it would
have been caught in review.

regards,
dan carpenter

[-- Attachment #2: old_v_new.sh --]
[-- Type: application/x-sh, Size: 890 bytes --]

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Shuah Khan]

On 3/25/26 15:34, Dan Carpenter wrote:
> On Wed, Mar 25, 2026 at 11:57:38AM -0600, Shuah Khan wrote:
>> My primary concern with this change is that it 10 files changed, with
>> 26 insertions(+), 32 deletions(-)
>>
>> It can be difficult to find any regressions unless the changes are
>> tested. I understand it is staging repo, but if we relax the rules
>> for staging irrespective of the scope of change, it becomes lot
>> harder to find regressions later. We are essentially leaving the
>> job of testing to users. Also if ans when the driver is pulled into
>> mainline, it will inherit the regressions that crept in.
>>
> 
> To me this is a mechanical patch (basically automatic, flip the
> if statement) from a newbie.  These kinds of patches are seldom a
> problem.
> 
> I took a sample of the patches for five years from 2021 to the end of
> 2025.  We merged 54406 commits in staging.  We had 401 Fixes tags.  But
> then I decided I don't care about Kconfig problems so I cut it down to
> 380 bugs.  Half of those (189 commits) were from when the driver was
> merged.  We tend to not review code very much when it is first merged
> to staging.  The other half (191 commits) were from us missing bugs
> during review.  191 bugs over five years is 40 bugs per year.
> 
> I hand reviewed a bunch of the 191 commits.  It's mostly senior
> developers and maintainers who introduce bugs.  They are normally doing
> complicated things like adding features or re-working core parts of the
> driver.  Quite a few of these patches were tested.
> 
> The fallout from newbie patches is pretty rare and often really
> minor.  A common thing is "We deleted code but we didn't delete
> everything."  The number of serious mess ups is probably just a
> couple times per year out of the 40 total bugs per year.
> 
> I've attached my script so you can check yourself.  NEW means the
> bug was introduced by a new driver.  OLD means, ideally it would
> have been caught in review.
> 

I am not the maintainer for this driver, so it is really up to the
maintainer(s) to accept mechanical patches such as this one.

As for the mentorship, I like to use a consistent rule that testing
for regressions is important and that author needs to disclose how
the patch is tested or not tested in the version 1 of the patch and
add RFT tag. That way maintainers can make an informed decision on
whether to accept the patch or not.

thanks,
-- Shuah

Re: [PATCH v2] staging: rtl8723bs: fix constant on left side of test checkpatch warnings

[Prithvi]

Hello all,

Thank you very much for taking time to review this patch.

I understand, that this patch may be considered to be majorly mechanical
changes, but still, the scope of the patch as well as the changes
involved make testing important for this patch.

I agree that, in case of unavailability of hardware for testing, I should
have mentioned it and used the RFT tag for the patch since v1. I
apologize for missing this detail & understand that compile-time testing
can't be sufficient here with all the changes introduced by this patch
and also the concern of the possibility of regressions getting introduced.

Going forward, I will be meticulous about clearly disclosing the testing 
status of the patch and if I am not able to test a patch, I will be sure 
to add RFT tag since v1 of the patch itself.

Lastly, I wanted to kindly ask if it will be alright to send a v3 patch 
with the RFT tag, incorporating the changes discussed in this thread?

Thank you,
Prithvi