Message-ID: <19130461cbdd39e92b06ec425db5db2984b41a41.camel@ew.tq-group.com>
Subject: Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled"
 status
From:   Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To:     Frank Rowand <frowand.list@gmail.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>
Date:   Mon, 15 Nov 2021 09:13:42 +0100
In-Reply-To: <7b4afaa7-13df-513a-5986-e1a9f5f5d7ed@gmail.com>
References: <20211108084804.13474-1-matthias.schiffer@ew.tq-group.com>
         <7b4afaa7-13df-513a-5986-e1a9f5f5d7ed@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote:
> On 11/8/21 3:48 AM, Matthias Schiffer wrote:
> > Allow fully disabling CPU nodes using status = "fail". Having no status
> > property at all is still interpreted as "okay" as usual.
> > 
> > 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 could require
> > additional fixups to avoid dangling phandle references.
> > 
> > References:
> > - https://www.lkml.org/lkml/2020/8/26/1237
> > - https://www.spinics.net/lists/devicetree-spec/msg01007.html
> > - https://github.com/devicetree-org/dt-schema/pull/61
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/of/base.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 61de453b885c..4e9973627c8d 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device)
> >  }
> >  EXPORT_SYMBOL(of_device_is_available);
> >  
> > +/**
> > + *  __of_device_is_disabled - check if a device has status "disabled"
> > + *
> > + *  @device: Node to check status for, with locks already held
> > + *
> > + *  Return: True if the status property is set to "disabled",
> > + *  false otherwise
> > + *
> > + *  Most callers should use __of_device_is_available() instead, this function
> > + *  only exists due to the special interpretation of the "disabled" status for
> > + *  CPU nodes.
> > + */
> > +static bool __of_device_is_disabled(const struct device_node *device)
> > +{
> > +	const char *status;
> > +
> > +	if (!device)
> > +		return false;
> > +
> > +	status = __of_get_property(device, "status", NULL);
> > +	if (status == NULL)
> > +		return false;
> > +
> > +	return !strcmp(status, "disabled");
> > +}
> > +
> >  /**
> >   *  of_device_is_big_endian - check if a device has BE registers
> >   *
> > @@ -817,6 +843,9 @@ 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) &&
> > +		    !__of_device_is_disabled(next))
> 
> Shouldn't that just be a check to continue if the device is disabled?
> 
> If adding a check for available, then all of the callers of for_each_of_cpu_node()
> need to be checked.  There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c
> has a comment:
> 
>  * Initialise the CPU possible map early - this describes the CPUs
>  * which may be present or become present in the system.

Previously, there were two option for the (effective) value of the
status of a device_node:

- "okay", "ok" or unset
- anything else (which includes "disabled" and "fail")

__of_device_is_available() checks which of these two is the case.

With the new code, we have 3 cases for the status of CPU nodes:

- "okay", "ok" or unset
- "disabled"
- anything else ("fail", ...)

My patch will only change the behaviour in one case: When a CPU node
has a status that is not "okay", "ok", "disabled" or unset - which is
exactly the point of my patch.

See also the change [1], which removed the !available check a while
ago, and the discussion in [2], which led us to the conclusion that 
of_get_next_cpu_node() must not distinguish "okay" and "disabled" CPU
nodes and we instead need a third status to disable a CPU for real.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/base.c?id=c961cb3be9064d1097ccc019390f8b5739daafc6
[2] https://www.lkml.org/lkml/2020/8/26/1237


> 
> -Frank
> 
> > +			continue;
> >  		if (!(of_node_name_eq(next, "cpu") ||
> >  		      __of_node_is_type(next, "cpu")))
> >  			continue;
> > 
> 
>