Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities.

[Atish Patra]

On 2/12/19 3:25 AM, Johan Hovold wrote:
> On Tue, Feb 12, 2019 at 03:10:12AM -0800, Atish Patra wrote:
> > Currently, we set hwcap based on first valid hart from DT. This may not
> > be correct always as that hart might not be current booting cpu or may
> > have a different capability.
> >
> > Set hwcap as the capabilities supported by all possible harts with "okay"
> > status.
> >
> > Signed-off-by: Atish Patra <atish.patra@xxxxxxx>
> > ---
> >   arch/riscv/kernel/cpufeature.c | 41 ++++++++++++++++++++++-------------------
> >   1 file changed, 22 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index e7a4701f..a1e4fb34 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/of.h>
> >   #include <asm/processor.h>
> >   #include <asm/hwcap.h>
> > +#include <asm/smp.h>
> >   
> >   unsigned long elf_hwcap __read_mostly;
> >   #ifdef CONFIG_FPU
> > @@ -42,28 +43,30 @@ void riscv_fill_hwcap(void)
> >   
> >   	elf_hwcap = 0;
> >   
> > -	/*
> > -	 * We don't support running Linux on hertergenous ISA systems.  For
> > -	 * now, we just check the ISA of the first "okay" processor.
> > -	 */
> >   	for_each_of_cpu_node(node) {
> > -		if (riscv_of_processor_hartid(node) >= 0)
> > -			break;
> > -	}
> > -	if (!node) {
> > -		pr_warn("Unable to find \"cpu\" devicetree entry\n");
> > -		return;
> > -	}
> > +		unsigned long this_hwcap = 0;
> >   
> > -	if (of_property_read_string(node, "riscv,isa", &isa)) {
> > -		pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> > -		of_node_put(node);
> > -		return;
> > -	}
> > -	of_node_put(node);
> > +		if (riscv_of_processor_hartid(node) < 0)
> > +			continue;

> > -	for (i = 0; i < strlen(isa); ++i)
> > -		elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
> > +		if (of_property_read_string(node, "riscv,isa", &isa)) {
> > +			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> > +			return;
>
> Did you want "continue" here to continue processing the other harts?

Hmm. If a cpu node doesn't have isa in DT, that means DT is wrong. A 
"continue" here will let user space use other harts just with a warning 
message?

Returning here will not set elf_hwcap which forces the user to fix the 
DT. I am not sure what should be the defined behavior in this case.

Any thoughts ?
> Note that you currently leak the device node when returning.
Ahh yes. I will fix it if we continue to return in the error case.

Regards,
Atish
> Johan