Messages in this thread | | | Subject | Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities. | From | Atish Patra <> | Date | Wed, 13 Feb 2019 11:59:55 -0800 |
| |
On 2/13/19 12:44 AM, Johan Hovold wrote: > On Tue, Feb 12, 2019 at 11:58:10AM -0800, Atish Patra wrote: >> 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@wdc.com> >>>> --- >>>> 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 ? > > The problem is that the proposed code might still set elf_hwcap -- it > all depends on the order of the hart nodes in dt (i.e. it will only be > left unset if the first node is malformed). > > For that reason, I'd say it's better to either bail out (hard or at > least with elf_hwcap unset) or to continue processing the other nodes. > > The former might break current systems with malformed dt, though. > > And since the harts are expected to have the same ISA, continuing the > processing while warning and ignoring the malformed node might be > acceptable. >
ok. I will change it to continue unless somebody else has objection.
Thanks for the review.
Regards, Atish > Johan >
| |