staging: rtl8723bs: fix constant on left side of test checkpatch warnings

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.

Quoted 4 lines · 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.

...

Quoted 8 lines

(quoted diff) +1 -2

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

Quoted 1 line

return read_down;

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

...

Review

Quoted 5 lines

(quoted diff) +1 -1

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

Just

	return GetFrameType(pframe) == WIFI_CTRL_TYPE;

Quoted 1 line · Andy Shevchenko

On Mon, Mar 23, 2026 at 09:59:01PM +0530, Prithvi Tambewagh wrote:

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +1 -2

 	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>

}

Is this approach correct?

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

Quoted 2 lines · Prithvi

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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Prithvi

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.

Quoted 6 lines

Signature · 3 lines

-- 
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?

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.

Quoted 1 line

Review

Quoted 3 lines

(quoted diff) +0 -2

-	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.

Quoted 10 lines · Shuah Khan

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.

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?

Quoted 14 lines · 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.

Quoted 6 lines

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?

Yes please.

But remember to do only one logical thing per patch, I see multiple things happening in this one :(

thanks,

greg k-h

Quoted 22 lines · Greg KH

Quoted 14 lines · 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.

Quoted 6 lines

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?

Quoted 8 lines

Yes please.

But remember to do only one logical thing per patch, I see multiple things happening in this one :(

thanks,

greg k-h

Sure...just to confirm I should send RFT patch series, with patch 1 being regarding the change of keeping constant on right side of test (shall I include the minor changes like proper formatting of if statements, or even value == 0 changed to !value, etc. in same patch itself or strictly keep only the constant on right side changes in this patch?) and patch 2 being regarding the API change of using read_poll_timeout_atomic() only, is this correct?

Quoted 32 lines · Prithvi

Quoted 22 lines · Greg KH

Quoted 14 lines · 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.

Quoted 6 lines

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?

Quoted 8 lines

Yes please.

But remember to do only one logical thing per patch, I see multiple things happening in this one :(

thanks,

greg k-h

Quoted 7 lines

Sure...just to confirm I should send RFT patch series, with patch 1 being regarding the change of keeping constant on right side of test (shall I include the minor changes like proper formatting of if statements, or even value == 0 changed to !value, etc. in same patch itself or strictly keep only the constant on right side changes in this patch?) and patch 2 being regarding the API change of using read_poll_timeout_atomic() only, is this correct?

What would you want to see if you had to review a patch series? Which is easier, a change that only does one type of logical thing throughout it, or one that mixes different things on the same lines?

Remember, a human has to read all of these :)

thanks,

greg k-h