linux-kernel - 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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZK+DhMfudVdhXnm2@thinky-boi>
Date:   Wed, 12 Jul 2023 21:54:28 -0700
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     chenqingyun001@...suo.com
Cc:     maz@...nel.org, catalin.marinas@....com, will@...nel.org,
        kvmarm@...ts.linux.dev, linux-kernel@...r.kernel.org,
        james.morse@....com, suzuki.poulose@....com, yuzenghui@...wei.com
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, chenqingyun001@...suo.com 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@...suo.com>
> ---
>  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