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]
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ