2023-07-13 03:28:51

by chenqingyun001

Subject: [PATCH] KVM: arm64:Use #include <linux/cpufeature.h> instead of <asm/cpufeature.h> trailing whitespace braces {} are not necessary for any arm of this statement

include <linux/cpufeature.h> is a generic
Having extra spaces or tabs has no real effect
If there is only one statement in the if branch,
curly braces {} can be omitted

Signed-off-by: Qingyun Chen <[email protected]>
---
arch/arm64/kvm/reset.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index b5dee8e57e77..4e6305dd1909 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -19,7 +19,7 @@

#include <kvm/arm_arch_timer.h>

-#include <asm/cpufeature.h>
+#include <linux/cpufeature.h>
#include <asm/cputype.h>
#include <asm/fpsimd.h>
#include <asm/ptrace.h>
@@ -122,7 +122,7 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu
*vcpu)
kfree(buf);
return ret;
}
-
+
vcpu->arch.sve_state = buf;
vcpu_set_flag(vcpu, VCPU_SVE_FINALIZED);
return 0;
@@ -308,9 +308,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)

switch (vcpu->arch.target) {
default:
- if (vcpu_el1_is_32bit(vcpu)) {
+ if (vcpu_el1_is_32bit(vcpu))
pstate = VCPU_RESET_PSTATE_SVC;
- } else if (vcpu_has_nv(vcpu)) {
+ else if (vcpu_has_nv(vcpu)) {
pstate = VCPU_RESET_PSTATE_EL2;
} else {
pstate = VCPU_RESET_PSTATE_EL1;

2023-07-13 05:38:58

by Oliver Upton

Subject: Re: [PATCH] KVM: arm64:Use #include <linux/cpufeature.h> instead of <asm/cpufeature.h> trailing whitespace braces {} are not necessary for any arm of this statement

Qingyun,

I'm guessing you did not read our developer documentation, nor did you
run scripts/checkpatch.pl...

- The shortlog should be at most 75 characters, but ideally less.

- The body should not be a restatement of the shortlog, and actually
provide meaningful information to the reader not yet captured in the
shortlog.

[*] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

On Thu, Jul 13, 2023 at 11:03:44AM +0800, [email protected] wrote:
> include <linux/cpufeature.h> is a generic
> Having extra spaces or tabs has no real effect
> If there is only one statement in the if branch,
> curly braces {} can be omitted
>
> Signed-off-by: Qingyun Chen <[email protected]>
> ---
> arch/arm64/kvm/reset.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b5dee8e57e77..4e6305dd1909 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -19,7 +19,7 @@
>
> #include <kvm/arm_arch_timer.h>
>
> -#include <asm/cpufeature.h>
> +#include <linux/cpufeature.h>

Changing the include provides absolutely nothing of value. All the
things this compilation unit depends on are described in the asm header.
Furthermore, use of the asm header appears all throughout the arm64
code.

> #include <asm/cputype.h>
> #include <asm/fpsimd.h>
> #include <asm/ptrace.h>
> @@ -122,7 +122,7 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
> kfree(buf);
> return ret;
> }
> -
> +
> vcpu->arch.sve_state = buf;
> vcpu_set_flag(vcpu, VCPU_SVE_FINALIZED);
> return 0;
> @@ -308,9 +308,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>
> switch (vcpu->arch.target) {
> default:
> - if (vcpu_el1_is_32bit(vcpu)) {
> + if (vcpu_el1_is_32bit(vcpu))
> pstate = VCPU_RESET_PSTATE_SVC;
> - } else if (vcpu_has_nv(vcpu)) {
> + else if (vcpu_has_nv(vcpu)) {

No. What we have matches the kernel style guide. If one branch of a
conditional statement requires braces, _all_ branches get braces.

[*] https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces

I strongly suggest you read up on the kernel developer documentation
before your next contribution, as you're unlikely to win any favors with
reviewers with problematic submissions such as this.

--
Thanks,
Oliver