Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status

[Frank Rowand]

Hi Mathias,

On 11/16/21 3:32 AM, Matthias Schiffer wrote:
> On Mon, 2021-11-15 at 12:23 -0500, Frank Rowand wrote:
>> Hi Matthias,
>>
>> On 11/15/21 3:13 AM, Matthias Schiffer wrote:
>>> On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote:
>>>> On 11/8/21 3:48 AM, Matthias Schiffer wrote:

It turns out I confused myself a bit...

The first email provided the clue that I needed:

>>>>> Allow fully disabling CPU nodes using status = "fail". Having no status
>>>>> property at all is still interpreted as "okay" as usual.

I managed to forget that sentence before I looked at the code in the patch.

>>>>>
>>>>> 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@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>>  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);
>>>>>  	}

So in the following test:

>>>>>  	for (; next; next = next->sibling) {
>>>>> +		if (!__of_device_is_available(next) &&
>>>>> +		    !__of_device_is_disabled(next))

   (!__of_device_is_available(next) &&
    !__of_device_is_disabled(next))

is meant to detect a status of "fail" or "fail-sss".  Since I forget the sentence
about "fail" from early in the email, I had difficulty in interpreting the intent
of the (!ok && !disabled) form of the test.  The intent of the test would be more
clear if it was checking _for_ "fail" instead of checking for _not_ the other
possible status values.

So you _are_ checking for the status of "fail" check (Rob's choice #1 below)
and I just did not understand that was the intent of the patch.

So I am fine with the patch if you change the above logic to check for
"fail" or "fail-sss".

It would also be good to add a comment to the of_get_next_cpu_node() header
comment that "fail" nodes are excluded.

Sorry for the confusion.

-Frank

>>>>
>>>> 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.
>>
>> Thanks for the links to previous discussion you provided below.  I had
>> forgotten the previous discussion.
>>
>> In [2] Rob ended up saying that there were two options he was fine with.
>> Either (or both), in of_get_next_cpu_node(),
>>
>>   (1) use status of "fail" as the check or
>>
>>   (2) use status of "disabled" as the check, conditional on !IS_ENABLED(CONFIG_PPC)
>>       "this would need some spec update"
>>       "Need to double check MIPS and Sparc though."
>>
>> Neither of these two options are what this patch does.  It seems to me that
>> option 1 is probably the easiest and least intrusive method.
> 
> My intuition is that a device with an unknown status value should not
> be used. For most devices this is already handled by treating any value
> that is not unset, "okay" or "ok" the same. For CPU nodes, this would
> be the case by treating such values like "fail".
> 
> I did a quick grep through the in-tree Device Trees, and I did find a
> few unusual status properties (none of them in CPU nodes though):
> 
> - Typo "failed" (4 cases)
> - Typo "disable" (17 cases)
> - "reserved" (19 cases)
> 
> "fail" appears 2 times, the rest is "okay", "ok" or "disabled".
> 
> I do not have a strong opinion on this though - for our concrete
> usecase, checking for "fail" is fine, and we can treat unknown values
> like "disabled" if you prefer that solution. Should "fail-*" status
> values also be treated like "fail" then?
> 
> 
> 
> 
>>
>> -Frank
>>
>>> 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;
>>>>>
>>>>
>>>>
>>
>>
>