| Message ID | 20241114142333.18210-1-hardevsinh.palaniya@siliconsignals.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] arm64: Refactor conditional logic | expand |
Hi all, > 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. > > if (ftr_ovr != tmp) { > } else if (ftr_new != tmp) { > } else if (ftr_ovr == tmp) { > > Logic: The first and last conditions are inverses of each other, so > the last condition must be true if the first two conditions are false. > > Additionally, all branches set the variable str, making the subsequent > "if (str)" check redundant > > Reviewed-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> Please consider this patch as v3. Apologies for the inconvenience. Best regards, Hardev
A small nit. The subject line could have been more specific to where this change applies (example below), otherwise it is too broad in the entire arm64 platform context. arm64/cpufeature: Refactor conditional logic in init_cpu_ftr_reg() On 11/14/24 19:52, 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. > > if (ftr_ovr != tmp) { > } else if (ftr_new != tmp) { > } else if (ftr_ovr == tmp) { > > Logic: The first and last conditions are inverses of each other, so > the last condition must be true if the first two conditions are false. > > Additionally, all branches set the variable str, making the subsequent > "if (str)" check redundant > > Reviewed-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> > --- > > Changelog in V2: > > - remove str check > > Change in V3: > > - Add logic in commit msg > - Add review tag > --- > arch/arm64/kernel/cpufeature.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 718728a85430..728709483fb7 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -989,17 +989,16 @@ 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"; > } > > - if (str) > - pr_warn("%s[%d:%d]: %s to %llx\n", > - reg->name, > - ftrp->shift + ftrp->width - 1, > - ftrp->shift, str, > - tmp & (BIT(ftrp->width) - 1)); > + pr_warn("%s[%d:%d]: %s to %llx\n", > + reg->name, > + ftrp->shift + ftrp->width - 1, > + ftrp->shift, str, > + tmp & (BIT(ftrp->width) - 1)); > } else if ((ftr_mask & reg->override->val) == ftr_mask) { > reg->override->val &= ~ftr_mask; > pr_warn("%s[%d:%d]: impossible override, ignored\n", Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 718728a85430..728709483fb7 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -989,17 +989,16 @@ 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"; } - if (str) - pr_warn("%s[%d:%d]: %s to %llx\n", - reg->name, - ftrp->shift + ftrp->width - 1, - ftrp->shift, str, - tmp & (BIT(ftrp->width) - 1)); + pr_warn("%s[%d:%d]: %s to %llx\n", + reg->name, + ftrp->shift + ftrp->width - 1, + ftrp->shift, str, + tmp & (BIT(ftrp->width) - 1)); } else if ((ftr_mask & reg->override->val) == ftr_mask) { reg->override->val &= ~ftr_mask; pr_warn("%s[%d:%d]: impossible override, ignored\n",
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. if (ftr_ovr != tmp) { } else if (ftr_new != tmp) { } else if (ftr_ovr == tmp) { Logic: The first and last conditions are inverses of each other, so the last condition must be true if the first two conditions are false. Additionally, all branches set the variable str, making the subsequent "if (str)" check redundant Reviewed-by: Mark Brown <broonie@kernel.org> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> --- Changelog in V2: - remove str check Change in V3: - Add logic in commit msg - Add review tag --- arch/arm64/kernel/cpufeature.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)