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

[Oliver Upton]

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, chenqingyun001@xxxxxxxxxx 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 <chenqingyun001@xxxxxxxxxx>
> ---
>  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