Re: [PATCH v12 05/17] riscv: Add has_vector/riscv_vsize to save vector features.

[Conor Dooley]

On Fri, Sep 23, 2022 at 09:27:01AM -0700, Chris Stillson wrote:
> On Wed, Sep 21, 2022 at 9:23 PM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
> >
> > On 9/21/22 16:43, Chris Stillson wrote:
> > > From: Greentime Hu <greentime.hu@xxxxxxxxxx>
> > >
> > > This patch is used to detect vector support status of CPU and use
> > > riscv_vsize to save the size of all the vector registers. It assumes
> > > all harts has the same capabilities in SMP system.
> > >
> > > [guoren@xxxxxxxxxxxxxxxxx: add has_vector checking]
> > > Co-developed-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > Co-developed-by: Vincent Chen <vincent.chen@xxxxxxxxxx>
> > > Signed-off-by: Vincent Chen <vincent.chen@xxxxxxxxxx>
> > > Signed-off-by: Greentime Hu <greentime.hu@xxxxxxxxxx>
> > > ---
> > >  arch/riscv/include/asm/vector.h | 14 +++++
> > >  arch/riscv/kernel/cpufeature.c  | 19 +++++++
> > >  arch/riscv/kernel/riscv_ksyms.c |  6 +++
> > >  arch/riscv/kernel/vector.S      | 93 +++++++++++++++++++++++++++++++++
> >
> > This file is not added to the Makefile until patch 8.
> (resending, as I forgot to set it to plain mail)

And please don't top post either :)

> This is the way the original set of patches worked. I tried to change
> them as little as possible for the rebase.

What is your goal with the series? Are you going to work on getting the
whole thing merged, or just looking to tack your patch onto the end of
the on-going series?

There were two warnings from LKP & some comments from reviewers on v10,
I assume that you did not make the changes those reviewers requested as
the build warnings didn't get fixed.
https://lore.kernel.org/linux-riscv/cover.1652257230.git.greentime.hu@xxxxxxxxxx/

I see a couple more caused by another patch in the series too:

../arch/riscv/kvm/vcpu_vector.c:134:6: error: variable 'reg_val' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        if ((rtype == KVM_REG_RISCV_VECTOR) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../arch/riscv/kvm/vcpu_vector.c:139:7: note: uninitialized use occurs here
        if (!reg_val)
             ^~~~~~~
../arch/riscv/kvm/vcpu_vector.c:134:2: note: remove the 'if' if its condition is always true
        if ((rtype == KVM_REG_RISCV_VECTOR) &&
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../arch/riscv/kvm/vcpu_vector.c:134:6: error: variable 'reg_val' is used uninitialized whenever '&&' condition is false [-Werror,-Wsometimes-uninitialized]
        if ((rtype == KVM_REG_RISCV_VECTOR) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../arch/riscv/kvm/vcpu_vector.c:139:7: note: uninitialized use occurs here
        if (!reg_val)
             ^~~~~~~
../arch/riscv/kvm/vcpu_vector.c:134:6: note: remove the '&&' if its condition is always true
        if ((rtype == KVM_REG_RISCV_VECTOR) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../arch/riscv/kvm/vcpu_vector.c:131:15: note: initialize the variable 'reg_val' to silence this warning
        void *reg_val;
                     ^
                      = NULL
2 errors generated.

Do you intend working on getting the series merged, or should I not
bother actually reviewing the individual patches?

If you do intend on getting it merged, can you please run checkpatch
with the --strict option and clear up the stuff it whines about & sort
out the Signed-off-bys in the series? Almost all the patches are missing
your sign-off, which is required as you are now the submitter. You can
also drop Greentime's signoff on any patch he is not the author of.

Give it a few days before resubmitting though, to give people a chance
at looking at the patchset first.

Thanks,
Conor.

> > >  4 files changed, 132 insertions(+)
> > >  create mode 100644 arch/riscv/include/asm/vector.h
> > >  create mode 100644 arch/riscv/kernel/vector.S
> >
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv