LinuxLists.cc - [PATCH] openrisc: use device tree to determine present cpus

2021-01-29 22:19:20

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

On Fri, Jan 29, 2021 at 07:29:31PM +0100, Jan Henrik Weinstock wrote:
> This patch proposes to use the device tree to determine the present cpus
> instead of assuming all CONFIG_NRCPUS are actually present in the system.
>
> Signed-off-by: Jan Henrik Weinstock <[email protected]>
> ---
> arch/openrisc/kernel/smp.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
> index 29c82ef2e..75be7e34f 100644
> --- a/arch/openrisc/kernel/smp.c
> +++ b/arch/openrisc/kernel/smp.c
> @@ -16,6 +16,7 @@
> #include <linux/sched.h>
> #include <linux/sched/mm.h>
> #include <linux/irq.h>
> +#include <linux/of.h>
> #include <asm/cpuinfo.h>
> #include <asm/mmu_context.h>
> #include <asm/tlbflush.h>
> @@ -68,14 +69,25 @@ void __init smp_init_cpus(void)
>
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> - int i;
> + u32 cpu_id;
> + struct device_node *cpu, *cpus;
>
> /*
> * Initialise the present map, which describes the set of CPUs
> * actually populated at the present time.
> */
> - for (i = 0; i < max_cpus; i++)
> - set_cpu_present(i, true);
> + cpus = of_find_node_by_path("/cpus");
> + for_each_child_of_node(cpus, cpu) {
> + if (of_property_read_u32(cpu, "reg", &cpu_id)) {
> + pr_warn("%s missing reg property", cpu->full_name);
> + continue;
> + }
> +
> + if (cpu_id >= max_cpus)
> + continue;
> +
> + set_cpu_present(cpu_id, true);
> + }

Hello, I looked into what other architectures do. Risc-V does something similar
but it does the setup in 2 parts:

- it uses the device tree to set possible CPU's in setup_smp. Using
for_each_of_cpu_node and set_cpu_possible.

- Then in smp_prepare_cpus, it loops over possible cpus with
for_each_possible_cpu.

Note, it seems risc-v does't actually check max_cpus when setting
set_cpu_present which may be a bug.

I think the two part approach is what we want to do:

- we should do set_cpu_possible in smp_init_cpus based on device tree.
Basically the same as your loop above but using for_each_of_cpu_node
and NR_CPUS.
- we can then do set_cpu_present using for_each_possible_cpu in
smp_prepare_cpus.

What do you think?

-Stafford

> }
>
> void __init smp_cpus_done(unsigned int max_cpus)
> --
> 2.17.1
>

2021-01-30 10:43:45

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

Hi Jan,

On Fri, Jan 29, 2021 at 7:34 PM Jan Henrik Weinstock
<[email protected]> wrote:
> This patch proposes to use the device tree to determine the present cpus
> instead of assuming all CONFIG_NRCPUS are actually present in the system.
>
> Signed-off-by: Jan Henrik Weinstock <[email protected]>

Thanks for your patch!

> --- a/arch/openrisc/kernel/smp.c
> +++ b/arch/openrisc/kernel/smp.c

> @@ -68,14 +69,25 @@ void __init smp_init_cpus(void)
>
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> - int i;
> + u32 cpu_id;
> + struct device_node *cpu, *cpus;
>
> /*
> * Initialise the present map, which describes the set of CPUs
> * actually populated at the present time.
> */
> - for (i = 0; i < max_cpus; i++)
> - set_cpu_present(i, true);
> + cpus = of_find_node_by_path("/cpus");
> + for_each_child_of_node(cpus, cpu) {

for_each_of_cpu_node()?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-01-30 11:04:48

by Jan Henrik Weinstock

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

Hi Stafford, Geert,

thanks for your feedback. I have made the following changes to the patch:

1. use for_each_of_cpu_node
2. possible_cpus is now what is in the devicetree, up to NR_CPUS
3. present_cpus is now all possible cpus, up to max_cpus

Signed-off-by: Jan Henrik Weinstock <[email protected]>
---
arch/openrisc/kernel/smp.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index 29c82ef2e..83cbf43d4 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -16,6 +16,7 @@
#include <linux/sched.h>
#include <linux/sched/mm.h>
#include <linux/irq.h>
+#include <linux/of.h>
#include <asm/cpuinfo.h>
#include <asm/mmu_context.h>
#include <asm/tlbflush.h>
@@ -60,22 +61,32 @@ void __init smp_prepare_boot_cpu(void)

void __init smp_init_cpus(void)
{
- int i;
+ struct device_node* cpu;
+ u32 cpu_id;

- for (i = 0; i < NR_CPUS; i++)
- set_cpu_possible(i, true);
+ for_each_of_cpu_node(cpu) {
+ if (of_property_read_u32(cpu, "reg", &cpu_id)) {
+ pr_warn("%s missing reg property", cpu->full_name);
+ continue;
+ }
+
+ if (cpu_id < NR_CPUS)
+ set_cpu_possible(cpu_id, true);
+ }
}

void __init smp_prepare_cpus(unsigned int max_cpus)
{
- int i;
+ unsigned int cpu;

/*
* Initialise the present map, which describes the set of CPUs
* actually populated at the present time.
*/
- for (i = 0; i < max_cpus; i++)
- set_cpu_present(i, true);
+ for_each_possible_cpu(cpu) {
+ if (cpu < max_cpus)
+ set_cpu_present(cpu, true);
+ }
}

void __init smp_cpus_done(unsigned int max_cpus)
--
2.17.1

Attachments:

2021-01-30 23:06:02

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

On Sat, Jan 30, 2021 at 12:00:10PM +0100, Jan Henrik Weinstock wrote:
> Hi Stafford, Geert,
>
> thanks for your feedback. I have made the following changes to the patch:

Hi, Thanks for the updates.

> 1. use for_each_of_cpu_node
> 2. possible_cpus is now what is in the devicetree, up to NR_CPUS
> 3. present_cpus is now all possible cpus, up to max_cpus

This looks good, one small comment below. Can you send the next patch as a v2?

Using 'git format-patch -v2 ...'

> Signed-off-by: Jan Henrik Weinstock <[email protected]>
> ---
You can include the 'Changes since v1' in the space here after '---'.

> arch/openrisc/kernel/smp.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
> index 29c82ef2e..83cbf43d4 100644
> --- a/arch/openrisc/kernel/smp.c
> +++ b/arch/openrisc/kernel/smp.c
> @@ -16,6 +16,7 @@
> #include <linux/sched.h>
> #include <linux/sched/mm.h>
> #include <linux/irq.h>
> +#include <linux/of.h>
> #include <asm/cpuinfo.h>
> #include <asm/mmu_context.h>
> #include <asm/tlbflush.h>
> @@ -60,22 +61,32 @@ void __init smp_prepare_boot_cpu(void)
>
> void __init smp_init_cpus(void)
> {
> - int i;
> + struct device_node* cpu;
> + u32 cpu_id;
>
> - for (i = 0; i < NR_CPUS; i++)
> - set_cpu_possible(i, true);
> + for_each_of_cpu_node(cpu) {
> + if (of_property_read_u32(cpu, "reg", &cpu_id)) {
> + pr_warn("%s missing reg property", cpu->full_name);
> + continue;
> + }
> +
> + if (cpu_id < NR_CPUS)

Should we warn on the else case?

> + set_cpu_possible(cpu_id, true);
> + }
> }

-Stafford

2021-01-30 23:15:18

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

Sorry, Please disrecard the [email protected] address in the mail
distribution list. I must have it a incorrect button.

-Stafford

2021-01-31 08:25:27

by Jan Henrik Weinstock

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

On 31/01/2021 00:03, Stafford Horne wrote:

> This looks good, one small comment below. Can you send the next patch as a v2?
>
> Using 'git format-patch -v2 ...'

Sorry, was not aware of that, will do better next time!

> Should we warn on the else case?

I think it is fine for the kernel to have room for more CPUs than are
actually present (i.e. NR_CPUs > present_cpus is OK). Other Archs do not
show a warning in this case either, therefore I also omitted it.

Gruss
Jan

Attachments:

2021-01-31 21:41:56

[permalink] [raw]

Subject: Re: [PATCH] openrisc: use device tree to determine present cpus

On Sun, Jan 31, 2021 at 09:22:31AM +0100, Jan Henrik Weinstock wrote:
> On 31/01/2021 00:03, Stafford Horne wrote:
>
> > This looks good, one small comment below. Can you send the next patch as a v2?
> >
> > Using 'git format-patch -v2 ...'
>
> Sorry, was not aware of that, will do better next time!
>
> > Should we warn on the else case?
>
> I think it is fine for the kernel to have room for more CPUs than are
> actually present (i.e. NR_CPUs > present_cpus is OK). Other Archs do not
> show a warning in this case either, therefore I also omitted it.

Fair enough.

Reviewed-by: Stafford Horne <[email protected]>

I can queue this for 5.12 when you send v2.

-Stafford

2021-02-01 11:53:01

by Jan Henrik Weinstock

[permalink] [raw]

Subject: [PATCH v2] openrisc: use device tree to determine present cpus

Use the device tree to determine the present cpus instead of assuming
all CONFIG_NRCPUS are actually present in the system.

Signed-off-by: Jan Henrik Weinstock <[email protected]>
---
Changes since v1:
1. use for_each_of_cpu_node
2. possible_cpus is now what is in the devicetree, up to NR_CPUS
3. present_cpus is now all possible cpus, up to max_cpus

arch/openrisc/kernel/smp.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index 75be7e34f..83cbf43d4 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -61,32 +61,31 @@ void __init smp_prepare_boot_cpu(void)

void __init smp_init_cpus(void)
{
- int i;
+ struct device_node* cpu;
+ u32 cpu_id;
+
+ for_each_of_cpu_node(cpu) {
+ if (of_property_read_u32(cpu, "reg", &cpu_id)) {
+ pr_warn("%s missing reg property", cpu->full_name);
+ continue;
+ }

- for (i = 0; i < NR_CPUS; i++)
- set_cpu_possible(i, true);
+ if (cpu_id < NR_CPUS)
+ set_cpu_possible(cpu_id, true);
+ }
}

void __init smp_prepare_cpus(unsigned int max_cpus)
{
- u32 cpu_id;
- struct device_node *cpu, *cpus;
+ unsigned int cpu;

/*
* Initialise the present map, which describes the set of CPUs
* actually populated at the present time.
*/
- cpus = of_find_node_by_path("/cpus");
- for_each_child_of_node(cpus, cpu) {
- if (of_property_read_u32(cpu, "reg", &cpu_id)) {
- pr_warn("%s missing reg property", cpu->full_name);
- continue;
- }
-
- if (cpu_id >= max_cpus)
- continue;
-
- set_cpu_present(cpu_id, true);
+ for_each_possible_cpu(cpu) {
+ if (cpu < max_cpus)
+ set_cpu_present(cpu, true);
}
}

--
2.17.1

Attachments:

2021-02-05 23:15:36

[permalink] [raw]

Subject: Re: [PATCH v2] openrisc: use device tree to determine present cpus

Hi Stafford,

On Fri, Feb 5, 2021 at 3:43 PM Stafford Horne <[email protected]> wrote:
> On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote:
> > Use the device tree to determine the present cpus instead of assuming all
> > CONFIG_NRCPUS are actually present in the system.
> >
> > Signed-off-by: Jan Henrik Weinstock <[email protected]>
>
> Hi Jan,
>
> I cannot apply this patch, it seems you somehow sent it signed as a multipart
> message via Thunderbird.
>
> This causes errors when trying to apply, even after I tried to manually fix the
> patch mail:
>
> Applying: openrisc: use device tree to determine present cpus
> error: sha1 information is lacking or useless (arch/openrisc/kernel/smp.c).
> error: could not build fake ancestor
> Patch failed at 0001 openrisc: use device tree to determine present cpus
>
> Can you send this using 'git send-email?'
>
> If not I can get it applied with some work, otherwise you can point me to a git
> repo which I can pull it from.

"b4 am [email protected]" works
fine for me.

git://git.kernel.org/pub/scm/utils/b4/b4.git

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-02-05 23:42:09

[permalink] [raw]

Subject: Re: [PATCH v2] openrisc: use device tree to determine present cpus

On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote:
> Use the device tree to determine the present cpus instead of assuming all
> CONFIG_NRCPUS are actually present in the system.
>
> Signed-off-by: Jan Henrik Weinstock <[email protected]>

Hi Jan,

I cannot apply this patch, it seems you somehow sent it signed as a multipart
message via Thunderbird.

This causes errors when trying to apply, even after I tried to manually fix the
patch mail:

Applying: openrisc: use device tree to determine present cpus
error: sha1 information is lacking or useless (arch/openrisc/kernel/smp.c).
error: could not build fake ancestor
Patch failed at 0001 openrisc: use device tree to determine present cpus

Can you send this using 'git send-email?'

If not I can get it applied with some work, otherwise you can point me to a git
repo which I can pull it from.

-Stafford

2021-02-06 03:29:03

[permalink] [raw]

Subject: Re: [PATCH v2] openrisc: use device tree to determine present cpus

On Fri, Feb 05, 2021 at 05:07:51PM +0100, Geert Uytterhoeven wrote:
> Hi Stafford,
>
> On Fri, Feb 5, 2021 at 3:43 PM Stafford Horne <[email protected]> wrote:
> > On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote:
> > > Use the device tree to determine the present cpus instead of assuming all
> > > CONFIG_NRCPUS are actually present in the system.
> > >
> > > Signed-off-by: Jan Henrik Weinstock <[email protected]>
> >
> > Hi Jan,
> >
> > I cannot apply this patch, it seems you somehow sent it signed as a multipart
> > message via Thunderbird.
> >
> > This causes errors when trying to apply, even after I tried to manually fix the
> > patch mail:
> >
> > Applying: openrisc: use device tree to determine present cpus
> > error: sha1 information is lacking or useless (arch/openrisc/kernel/smp.c).
> > error: could not build fake ancestor
> > Patch failed at 0001 openrisc: use device tree to determine present cpus
> >
> > Can you send this using 'git send-email?'
> >
> > If not I can get it applied with some work, otherwise you can point me to a git
> > repo which I can pull it from.
>
> "b4 am [email protected]" works
> fine for me.
>
> git://git.kernel.org/pub/scm/utils/b4/b4.git

Did it work? For me I got, base not found.

Looking up
https://lore.kernel.org/r/6dbc27f8-5261-59c5-acba-70f6c6a74ba1%40rwth-aachen.de
Grabbing thread from lore.kernel.org/lkml
Analyzing 9 messages in the thread
Will use the latest revision: v2
You can pick other revisions using the -vN flag
---
Writing
./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx
[PATCH v2] openrisc: use device tree to determine present cpus
---
Total patches: 1
---
Link:
https://lore.kernel.org/r/[email protected]
Base: not found
git am
./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx

-Stafford

2021-02-06 09:37:45

[permalink] [raw]

Subject: Re: [PATCH v2] openrisc: use device tree to determine present cpus

Hi Stafford,

On Fri, Feb 5, 2021 at 11:36 PM Stafford Horne <[email protected]> wrote:
> On Fri, Feb 05, 2021 at 05:07:51PM +0100, Geert Uytterhoeven wrote:
> > On Fri, Feb 5, 2021 at 3:43 PM Stafford Horne <[email protected]> wrote:
> > > On Mon, Feb 01, 2021 at 12:49:31PM +0100, Jan Henrik Weinstock wrote:
> > > > Use the device tree to determine the present cpus instead of assuming all
> > > > CONFIG_NRCPUS are actually present in the system.
> > > >
> > > > Signed-off-by: Jan Henrik Weinstock <[email protected]>
> > >
> > > I cannot apply this patch, it seems you somehow sent it signed as a multipart
> > > message via Thunderbird.
> > >
> > > This causes errors when trying to apply, even after I tried to manually fix the
> > > patch mail:
> > >
> > > Applying: openrisc: use device tree to determine present cpus
> > > error: sha1 information is lacking or useless (arch/openrisc/kernel/smp.c).
> > > error: could not build fake ancestor
> > > Patch failed at 0001 openrisc: use device tree to determine present cpus
> > >
> > > Can you send this using 'git send-email?'
> > >
> > > If not I can get it applied with some work, otherwise you can point me to a git
> > > repo which I can pull it from.
> >
> > "b4 am [email protected]" works
> > fine for me.
> >
> > git://git.kernel.org/pub/scm/utils/b4/b4.git
>
> Did it work? For me I got, base not found.
>
> Looking up
> https://lore.kernel.org/r/6dbc27f8-5261-59c5-acba-70f6c6a74ba1%40rwth-aachen.de
> Grabbing thread from lore.kernel.org/lkml
> Analyzing 9 messages in the thread
> Will use the latest revision: v2
> You can pick other revisions using the -vN flag
> ---
> Writing
> ./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx
> [PATCH v2] openrisc: use device tree to determine present cpus
> ---
> Total patches: 1
> ---
> Link:
> https://lore.kernel.org/r/[email protected]
> Base: not found

That just means the patch contains no information w.r.t. its base, i.e.
against which tree/commit it applies to. To be ignored.

> git am
> ./v2_20210201_jan_weinstock_openrisc_use_device_tree_to_determine_present_cpus.mbx

Just run the above command ;-)

In addition, you can run "formail -s scripts/checkpatch.pl < *mbx" first, to
run the mbox (which could contain multiple patches) through checkpatch.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-02-08 12:36:30