| Message ID | 1549969812-22502-9-git-send-email-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various SMP related fixes | expand |
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? Note that you currently leak the device node when returning. Johan
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 ? > 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 >
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. Johan
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 >
On Wed, 13 Feb 2019 00:44:42 PST (-0800), johan@kernel.org 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. Handling malformed device trees by providing a warning and an empty HWCAP seems like the right way to go to me. > > Johan
On 2/13/19 4:38 PM, Palmer Dabbelt wrote: > On Wed, 13 Feb 2019 00:44:42 PST (-0800), johan@kernel.org 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. > > Handling malformed device trees by providing a warning and an empty HWCAP seems > like the right way to go to me. > If I understand you correctly, you prefer following things to be done in case of malformed DT. 1. Print a warning message 2. Unset the entire HWCAP 3. Return without processing other harts. This will most likely result in panic when user space starts. Is this correct ? Regards, Atish >> >> Johan >
On 2/14/19 3:49 PM, Atish Patra wrote: > On 2/13/19 4:38 PM, Palmer Dabbelt wrote: >> On Wed, 13 Feb 2019 00:44:42 PST (-0800), johan@kernel.org 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. >> >> Handling malformed device trees by providing a warning and an empty HWCAP seems >> like the right way to go to me. >> > > If I understand you correctly, you prefer following things to be done in > case of malformed DT. > > 1. Print a warning message > 2. Unset the entire HWCAP > 3. Return without processing other harts. This will most likely result > in panic when user space starts. > > Is this correct ? > As per our offline discussion, we should let kernel avoid setting any value for the cpu with incorrect DT entry and continue for other harts. A warning is enough. This is fine as long as user space never see that hart. As the hart enumeration depends on riscv_of_processor_hartid, the hart with corrupted isa property will never boot. riscv_of_processor_hartid will return -ENODEV if "riscv,isa" property is not present. Moreover, the discussed conditional statement will not even executed unless there is memory corruption or somebody corrupts the DT on the fly. So we can continue with the patch as it is. I will just resend the series (dropping driver patches) for easy merge. Regards, Atish > Regards, > Atish >>> >>> Johan >> > >
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; + } + + for (i = 0; i < strlen(isa); ++i) + this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; + + /* + * All "okay" hart should have same isa. Set HWCAP based on + * common capabilities of every "okay" hart, in case they don't + * have. + */ + if (elf_hwcap) + elf_hwcap &= this_hwcap; + else + elf_hwcap = this_hwcap; + } /* We don't support systems with F but without D, so mask those out * here. */
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(-)