| Message ID | 20200826120254.8902-1-matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | of: skip disabled CPU nodes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On 2020-08-26 07:02, Matthias Schiffer wrote: > Allow disabling CPU nodes using status = "disabled". > > This allows a bootloader to change the number of available CPUs (for > example when a common DTS is used for SoC variants with different numbers > of cores) without deleting the nodes altogether (which may require > additional fixups where the CPU nodes are referenced, e.g. a cooling > map). > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/of/base.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index ea44fea99813..d547e9deced1 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) > of_node_put(node); > } > for (; next; next = next->sibling) { > + if (!__of_device_is_available(next)) > + continue; > if (!(of_node_name_eq(next, "cpu") || > __of_node_is_type(next, "cpu"))) > continue; > The original implementation of of_get_next_cpu_node() had that check, but status disabled for cpu nodes has different semantics than other nodes, and the check broke some systems. The check was removed by c961cb3be906 "of: Fix cpu node iterator to not ignore disabled cpu nodes". It would be useful to document that difference in the header comment of of_get_next_cpu_node(). -Frank
On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote: > On 2020-08-26 07:02, Matthias Schiffer wrote: > > Allow disabling CPU nodes using status = "disabled". > > > > This allows a bootloader to change the number of available CPUs > > (for > > example when a common DTS is used for SoC variants with different > > numbers > > of cores) without deleting the nodes altogether (which may require > > additional fixups where the CPU nodes are referenced, e.g. a > > cooling > > map). > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > > > > > --- > > drivers/of/base.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > index ea44fea99813..d547e9deced1 100644 > > --- a/drivers/of/base.c > > +++ b/drivers/of/base.c > > @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct > > device_node *prev) > > of_node_put(node); > > } > > for (; next; next = next->sibling) { > > + if (!__of_device_is_available(next)) > > + continue; > > if (!(of_node_name_eq(next, "cpu") || > > __of_node_is_type(next, "cpu"))) > > continue; > > > > The original implementation of of_get_next_cpu_node() had > that check, but status disabled for cpu nodes has different > semantics than other nodes, and the check broke some systems. > The check was removed by c961cb3be906 "of: Fix cpu node > iterator to not ignore disabled cpu nodes". > > It would be useful to document that difference in the > header comment of of_get_next_cpu_node(). > > -Frank Hmm, I see. This difference in behaviour is quite unfortunate, as I'm currently looking for a way to *really* disable a CPU core. In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the i.MX8M), there are 4 CPU nodes for the full-featured quad-core version. The reduced single- and dual-core versions are currently handled in NXP's U-Boot fork by deleting the additional nodes. Not doing so causes the kernel to hang for a while when trying to online the non-existent cores during boot (at least in linux-imx 5.4 - I have not checked a more recent mainline kernel yet), but the deletion is non-trivial to do without leaving dangling phandle references. Kind regards, Matthias
Hi Rob, On 2020-08-26 08:54, Matthias Schiffer wrote: > On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote: >> On 2020-08-26 07:02, Matthias Schiffer wrote: >>> Allow disabling CPU nodes using status = "disabled". >>> >>> This allows a bootloader to change the number of available CPUs >>> (for >>> example when a common DTS is used for SoC variants with different >>> numbers >>> of cores) without deleting the nodes altogether (which may require >>> additional fixups where the CPU nodes are referenced, e.g. a >>> cooling >>> map). >>> >>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com >>>> >>> --- >>> drivers/of/base.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>> index ea44fea99813..d547e9deced1 100644 >>> --- a/drivers/of/base.c >>> +++ b/drivers/of/base.c >>> @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct >>> device_node *prev) >>> of_node_put(node); >>> } >>> for (; next; next = next->sibling) { >>> + if (!__of_device_is_available(next)) >>> + continue; >>> if (!(of_node_name_eq(next, "cpu") || >>> __of_node_is_type(next, "cpu"))) >>> continue; >>> >> >> The original implementation of of_get_next_cpu_node() had >> that check, but status disabled for cpu nodes has different >> semantics than other nodes, and the check broke some systems. >> The check was removed by c961cb3be906 "of: Fix cpu node >> iterator to not ignore disabled cpu nodes". >> >> It would be useful to document that difference in the >> header comment of of_get_next_cpu_node(). >> >> -Frank > > Hmm, I see. This difference in behaviour is quite unfortunate, as I'm > currently looking for a way to *really* disable a CPU core. > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the > i.MX8M), there are 4 CPU nodes for the full-featured quad-core version. > The reduced single- and dual-core versions are currently handled in > NXP's U-Boot fork by deleting the additional nodes. > > Not doing so causes the kernel to hang for a while when trying to > online the non-existent cores during boot (at least in linux-imx 5.4 - > I have not checked a more recent mainline kernel yet), but the deletion > is non-trivial to do without leaving dangling phandle references. Any thoughts on implementing another universal property that means something like "the hardware described by this node does not exist or is so broken that you better not use it". Matthias, if Rob thinks that is a good idea, then you should start with a new proposal that is also sent to devicetree-spec@vger.kernel.org <devicetree-spec@vger.kernel.org> -Frank > > Kind regards, > Matthias >
On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand <frowand.list@gmail.com> wrote: > > Hi Rob, > > On 2020-08-26 08:54, Matthias Schiffer wrote: > > On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote: > >> On 2020-08-26 07:02, Matthias Schiffer wrote: > >>> Allow disabling CPU nodes using status = "disabled". > >>> > >>> This allows a bootloader to change the number of available CPUs > >>> (for > >>> example when a common DTS is used for SoC variants with different > >>> numbers > >>> of cores) without deleting the nodes altogether (which may require > >>> additional fixups where the CPU nodes are referenced, e.g. a > >>> cooling > >>> map). > >>> > >>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > >>>> > >>> --- > >>> drivers/of/base.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/of/base.c b/drivers/of/base.c > >>> index ea44fea99813..d547e9deced1 100644 > >>> --- a/drivers/of/base.c > >>> +++ b/drivers/of/base.c > >>> @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct > >>> device_node *prev) > >>> of_node_put(node); > >>> } > >>> for (; next; next = next->sibling) { > >>> + if (!__of_device_is_available(next)) > >>> + continue; > >>> if (!(of_node_name_eq(next, "cpu") || > >>> __of_node_is_type(next, "cpu"))) > >>> continue; > >>> > >> > >> The original implementation of of_get_next_cpu_node() had > >> that check, but status disabled for cpu nodes has different > >> semantics than other nodes, and the check broke some systems. > >> The check was removed by c961cb3be906 "of: Fix cpu node > >> iterator to not ignore disabled cpu nodes". > >> > >> It would be useful to document that difference in the > >> header comment of of_get_next_cpu_node(). > >> > >> -Frank > > > > Hmm, I see. This difference in behaviour is quite unfortunate, as I'm > > currently looking for a way to *really* disable a CPU core. > > > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core version. > > The reduced single- and dual-core versions are currently handled in > > NXP's U-Boot fork by deleting the additional nodes. > > > > Not doing so causes the kernel to hang for a while when trying to > > online the non-existent cores during boot (at least in linux-imx 5.4 - > > I have not checked a more recent mainline kernel yet), but the deletion > > is non-trivial to do without leaving dangling phandle references. > > Any thoughts on implementing another universal property that means > something like "the hardware described by this node does not exist > or is so broken that you better not use it". There's a couple of options: The DT spec defines 'fail' value for status. We could use that instead of 'disabled'. The spec behavior with cpu 'disabled' is only on PPC AFAIK. On arm/arm64 (probably riscv now too) we've never followed it where we online 'disabled' CPUs. So we could just make the check conditional on !IS_ENABLED(CONFIG_PPC). This would need some spec update. > Matthias, if Rob thinks that is a good idea, then you should start > with a new proposal that is also sent to > devicetree-spec@vger.kernel.org <devicetree-spec@vger.kernel.org> > > -Frank > > > > > Kind regards, > > Matthias > > >
On Wed, 2020-08-26 at 13:26 -0600, Rob Herring wrote: > On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand <frowand.list@gmail.com> > wrote: > > > > Hi Rob, > > > > On 2020-08-26 08:54, Matthias Schiffer wrote: > > > On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote: > > > > On 2020-08-26 07:02, Matthias Schiffer wrote: > > > > > Allow disabling CPU nodes using status = "disabled". > > > > > > > > > > This allows a bootloader to change the number of available > > > > > CPUs > > > > > (for > > > > > example when a common DTS is used for SoC variants with > > > > > different > > > > > numbers > > > > > of cores) without deleting the nodes altogether (which may > > > > > require > > > > > additional fixups where the CPU nodes are referenced, e.g. a > > > > > cooling > > > > > map). > > > > > > > > > > Signed-off-by: Matthias Schiffer < > > > > > matthias.schiffer@ew.tq-group.com > > > > > > > > > > > > > > > > --- > > > > > drivers/of/base.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > > > > index ea44fea99813..d547e9deced1 100644 > > > > > --- a/drivers/of/base.c > > > > > +++ b/drivers/of/base.c > > > > > @@ -796,6 +796,8 @@ struct device_node > > > > > *of_get_next_cpu_node(struct > > > > > device_node *prev) > > > > > of_node_put(node); > > > > > } > > > > > for (; next; next = next->sibling) { > > > > > + if (!__of_device_is_available(next)) > > > > > + continue; > > > > > if (!(of_node_name_eq(next, "cpu") || > > > > > __of_node_is_type(next, "cpu"))) > > > > > continue; > > > > > > > > > > > > > The original implementation of of_get_next_cpu_node() had > > > > that check, but status disabled for cpu nodes has different > > > > semantics than other nodes, and the check broke some systems. > > > > The check was removed by c961cb3be906 "of: Fix cpu node > > > > iterator to not ignore disabled cpu nodes". > > > > > > > > It would be useful to document that difference in the > > > > header comment of of_get_next_cpu_node(). > > > > > > > > -Frank > > > > > > Hmm, I see. This difference in behaviour is quite unfortunate, as > > > I'm > > > currently looking for a way to *really* disable a CPU core. > > > > > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants > > > of the > > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core > > > version. > > > The reduced single- and dual-core versions are currently handled > > > in > > > NXP's U-Boot fork by deleting the additional nodes. > > > > > > Not doing so causes the kernel to hang for a while when trying to > > > online the non-existent cores during boot (at least in linux-imx > > > 5.4 - > > > I have not checked a more recent mainline kernel yet), but the > > > deletion > > > is non-trivial to do without leaving dangling phandle references. > > > > Any thoughts on implementing another universal property that means > > something like "the hardware described by this node does not exist > > or is so broken that you better not use it". > > There's a couple of options: > > The DT spec defines 'fail' value for status. We could use that > instead > of 'disabled'. > > The spec behavior with cpu 'disabled' is only on PPC AFAIK. On > arm/arm64 (probably riscv now too) we've never followed it where we > online 'disabled' CPUs. So we could just make the check conditional > on > !IS_ENABLED(CONFIG_PPC). This would need some spec update. On ARM(64), the "disabled" status on CPUs doesn't have any effect. I assume this changed with the mentioned commit c961cb3be906 "of: Fix cpu node iterator to not ignore disabled cpu nodes", as reverting it gives me the desired behaviour of considering the disabled CPUs non-existent. So it seems that we already changed the interpretation in a non- compatible way once (back in v4.20), and somehow PPC has yet another different behaviour? How do we get out of this mess? Is going back to the v4.19 logic for non-PPC platforms an acceptable regression fix, or would this be considered another breaking change? > > > Matthias, if Rob thinks that is a good idea, then you should start > > with a new proposal that is also sent to > > devicetree-spec@vger.kernel.org <devicetree-spec@vger.kernel.org> > > > > -Frank > > > > > > > > Kind regards, > > > Matthias > > >
On Thu, 2020-08-27 at 09:10 +0200, Matthias Schiffer wrote: > On Wed, 2020-08-26 at 13:26 -0600, Rob Herring wrote: > > On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand < > > frowand.list@gmail.com> > > wrote: > > > > > > Hi Rob, > > > > > > On 2020-08-26 08:54, Matthias Schiffer wrote: [snip] > > > > > > > > Hmm, I see. This difference in behaviour is quite unfortunate, > > > > as > > > > I'm > > > > currently looking for a way to *really* disable a CPU core. > > > > > > > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other > > > > variants > > > > of the > > > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core > > > > version. > > > > The reduced single- and dual-core versions are currently > > > > handled > > > > in > > > > NXP's U-Boot fork by deleting the additional nodes. > > > > > > > > Not doing so causes the kernel to hang for a while when trying > > > > to > > > > online the non-existent cores during boot (at least in linux- > > > > imx > > > > 5.4 - > > > > I have not checked a more recent mainline kernel yet), but the > > > > deletion > > > > is non-trivial to do without leaving dangling phandle > > > > references. > > > > > > Any thoughts on implementing another universal property that > > > means > > > something like "the hardware described by this node does not > > > exist > > > or is so broken that you better not use it". > > > > There's a couple of options: > > > > The DT spec defines 'fail' value for status. We could use that > > instead > > of 'disabled'. > > > > The spec behavior with cpu 'disabled' is only on PPC AFAIK. On > > arm/arm64 (probably riscv now too) we've never followed it where we > > online 'disabled' CPUs. So we could just make the check conditional > > on > > !IS_ENABLED(CONFIG_PPC). This would need some spec update. > > On ARM(64), the "disabled" status on CPUs doesn't have any effect. I > assume this changed with the mentioned commit c961cb3be906 "of: Fix > cpu > node iterator to not ignore disabled cpu nodes", as reverting it > gives > me the desired behaviour of considering the disabled CPUs non- > existent. > > So it seems that we already changed the interpretation in a non- > compatible way once (back in v4.20), and somehow PPC has yet another > different behaviour? > > How do we get out of this mess? Is going back to the v4.19 logic for > non-PPC platforms an acceptable regression fix, or would this be > considered another breaking change? > Any new insights on this? I'll gladly provide patches or proposals for a spec update if we can decide where we want to go with this. Kind regards, Matthias
On Wed, Sep 9, 2020 at 2:58 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > > On Thu, 2020-08-27 at 09:10 +0200, Matthias Schiffer wrote: > > On Wed, 2020-08-26 at 13:26 -0600, Rob Herring wrote: > > > On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand < > > > frowand.list@gmail.com> > > > wrote: > > > > > > > > Hi Rob, > > > > > > > > On 2020-08-26 08:54, Matthias Schiffer wrote: > [snip] > > > > > > > > > > Hmm, I see. This difference in behaviour is quite unfortunate, > > > > > as > > > > > I'm > > > > > currently looking for a way to *really* disable a CPU core. > > > > > > > > > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other > > > > > variants > > > > > of the > > > > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core > > > > > version. > > > > > The reduced single- and dual-core versions are currently > > > > > handled > > > > > in > > > > > NXP's U-Boot fork by deleting the additional nodes. > > > > > > > > > > Not doing so causes the kernel to hang for a while when trying > > > > > to > > > > > online the non-existent cores during boot (at least in linux- > > > > > imx > > > > > 5.4 - > > > > > I have not checked a more recent mainline kernel yet), but the > > > > > deletion > > > > > is non-trivial to do without leaving dangling phandle > > > > > references. > > > > > > > > Any thoughts on implementing another universal property that > > > > means > > > > something like "the hardware described by this node does not > > > > exist > > > > or is so broken that you better not use it". > > > > > > There's a couple of options: > > > > > > The DT spec defines 'fail' value for status. We could use that > > > instead > > > of 'disabled'. > > > > > > The spec behavior with cpu 'disabled' is only on PPC AFAIK. On > > > arm/arm64 (probably riscv now too) we've never followed it where we > > > online 'disabled' CPUs. So we could just make the check conditional > > > on > > > !IS_ENABLED(CONFIG_PPC). This would need some spec update. > > > > On ARM(64), the "disabled" status on CPUs doesn't have any effect. I > > assume this changed with the mentioned commit c961cb3be906 "of: Fix > > cpu > > node iterator to not ignore disabled cpu nodes", as reverting it > > gives > > me the desired behaviour of considering the disabled CPUs non- > > existent. > > > > So it seems that we already changed the interpretation in a non- > > compatible way once (back in v4.20), and somehow PPC has yet another > > different behaviour? > > > > How do we get out of this mess? Is going back to the v4.19 logic for > > non-PPC platforms an acceptable regression fix, or would this be > > considered another breaking change? Yes, this is my 2nd option above. Need to double check MIPS and Sparc though. > Any new insights on this? I'll gladly provide patches or proposals for > a spec update if we can decide where we want to go with this. I gave 2 options. I'm fine with either one (or both). Using 'fail' is the simplest. Rob
diff --git a/drivers/of/base.c b/drivers/of/base.c index ea44fea99813..d547e9deced1 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) of_node_put(node); } for (; next; next = next->sibling) { + if (!__of_device_is_available(next)) + continue; if (!(of_node_name_eq(next, "cpu") || __of_node_is_type(next, "cpu"))) continue;
Allow disabling CPU nodes using status = "disabled". This allows a bootloader to change the number of available CPUs (for example when a common DTS is used for SoC variants with different numbers of cores) without deleting the nodes altogether (which may require additional fixups where the CPU nodes are referenced, e.g. a cooling map). Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/of/base.c | 2 ++ 1 file changed, 2 insertions(+)