| Message ID | 20241111142204.67751-1-hardevsinh.palaniya@siliconsignals.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Refactor conditional logic | expand |
On Mon, Nov 11, 2024 at 07:51:45PM +0530, Hardevsinh Palaniya wrote: > Unnecessarily checks ftr_ovr == tmp in an extra else if, which is not > needed because that condition would already be true by default if the > previous conditions are not satisfied. > @@ -989,7 +989,7 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new) > /* Override was valid */ > ftr_new = tmp; > str = "forced"; > - } else if (ftr_ovr == tmp) { > + } else { > /* Override was the safe value */ > str = "already set"; > } Your changelog wasn't very clear on this but the tests in this if/else tree are if (ftr_ovr != tmp) { } else if (ftr_new != tmp) { } else if (ftr_ovr == tmp) { } so your analysis is accurate, the first and last tests are the inverse of each other so onr must be true. This should be clear from your commit log. Also all of those branches set "str" and we then immediately test if (str) before logging a diagnostic. If we're looking to reduce unneeded tests then either that one is redundant too or there's another bug in the logic (I think from a quick scan just the former).
Hi Mark, Thanks for the input > > @@ -989,7 +989,7 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new) > > /* Override was valid */ > > ftr_new = tmp; > > str = "forced"; > > - } else if (ftr_ovr == tmp) { > > + } else { > > /* Override was the safe value */ > > str = "already set"; > > } > > Your changelog wasn't very clear on this but the tests in this if/else > tree are > > if (ftr_ovr != tmp) { > } else if (ftr_new != tmp) { > } else if (ftr_ovr == tmp) { > } > > so your analysis is accurate, the first and last tests are the inverse > of each other so onr must be true. This should be clear from your > commit log. Also all of those branches set "str" and we then > immediately test > > if (str) > > before logging a diagnostic. If we're looking to reduce unneeded tests > then either that one is redundant too or there's another bug in the > logic (I think from a quick scan just the former). Yes, I agree that also we can remove Best Regards, Hardev
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 718728a85430..d9021b1b9cff 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -989,7 +989,7 @@ static void init_cpu_ftr_reg(u32 sys_reg, u64 new) /* Override was valid */ ftr_new = tmp; str = "forced"; - } else if (ftr_ovr == tmp) { + } else { /* Override was the safe value */ str = "already set"; }
Unnecessarily checks ftr_ovr == tmp in an extra else if, which is not needed because that condition would already be true by default if the previous conditions are not satisfied. Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> --- arch/arm64/kernel/cpufeature.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)