Re: [PATCH] of: skip disabled CPU nodes

[Rob Herring]

On Wed, Sep 9, 2020 at 2:58 AM Matthias Schiffer
<matthias.schiffer@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > 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