[PATCH v2] of: base: Skip CPU nodes with "fail"/"fail-..." status

[PATCH v2] of: base: Skip CPU nodes with "fail"/"fail-..." status

[Matthias Schiffer]

Allow fully disabling CPU nodes using status = "fail".

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.

Unknown status values (everything that is not "okay"/"ok", "disabled" or
"fail"/"fail-...") will continue to be interpreted like "disabled",
meaning that the CPU can be enabled during boot.

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>
---

v2: Treat unknown status values like "disabled", not like "fail"


 drivers/of/base.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 61de453b885c..5b907600f5b0 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -650,6 +650,28 @@ bool of_device_is_available(const struct device_node *device)
 }
 EXPORT_SYMBOL(of_device_is_available);
 
+/**
+ *  __of_device_is_fail - check if a device has status "fail" or "fail-..."
+ *
+ *  @device: Node to check status for, with locks already held
+ *
+ *  Return: True if the status property is set to "fail" or "fail-..." (for any
+ *  error code suffix), false otherwise
+ */
+static bool __of_device_is_fail(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, "fail") || !strncmp(status, "fail-", 5);
+}
+
 /**
  *  of_device_is_big_endian - check if a device has BE registers
  *
@@ -796,6 +818,9 @@ EXPORT_SYMBOL(of_get_next_available_child);
  * of_get_next_cpu_node - Iterate on cpu nodes
  * @prev:	previous child of the /cpus node, or NULL to get first
  *
+ * Unusable CPUs (those with the status property set to "fail" or "fail-...")
+ * will be skipped.
+ *
  * Return: A cpu node pointer with refcount incremented, use of_node_put()
  * on it when done. Returns NULL when prev is the last child. Decrements
  * the refcount of prev.
@@ -817,6 +842,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_fail(next))
+			continue;
 		if (!(of_node_name_eq(next, "cpu") ||
 		      __of_node_is_type(next, "cpu")))
 			continue;
-- 
2.17.1

Re: [PATCH v2] of: base: Skip CPU nodes with "fail"/"fail-..." status

[Frank Rowand]

On 11/22/21 6:45 AM, Matthias Schiffer wrote:
> Allow fully disabling CPU nodes using status = "fail".
> 
> 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.
> 
> Unknown status values (everything that is not "okay"/"ok", "disabled" or
> "fail"/"fail-...") will continue to be interpreted like "disabled",
> meaning that the CPU can be enabled during boot.
> 

Thank you for including all the references!  I find them helpful.

> References:
> - https://www.lkml.org/lkml/2020/8/26/1237

That reference should be:
  https://lore.kernel.org/all/CAL_Jsq+1LsTBdVaODVfmB0eme2jMpNL4VgKk-OM7rQWyyF0Jbw@mail.gmail.com/

Rob might be willing to fix that himself without a new patch version.

> - https://www.spinics.net/lists/devicetree-spec/msg01007.html

The following reference is the pull request for the devicetree specification
change that is provided in the previous reference.  I wouldn't include this
in the commit, but maybe Rob will.

> - https://github.com/devicetree-org/dt-schema/pull/61
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> v2: Treat unknown status values like "disabled", not like "fail"
> 
> 
>  drivers/of/base.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 61de453b885c..5b907600f5b0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -650,6 +650,28 @@ bool of_device_is_available(const struct device_node *device)
>  }
>  EXPORT_SYMBOL(of_device_is_available);
>  
> +/**
> + *  __of_device_is_fail - check if a device has status "fail" or "fail-..."
> + *
> + *  @device: Node to check status for, with locks already held
> + *
> + *  Return: True if the status property is set to "fail" or "fail-..." (for any
> + *  error code suffix), false otherwise
> + */
> +static bool __of_device_is_fail(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, "fail") || !strncmp(status, "fail-", 5);
> +}
> +
>  /**
>   *  of_device_is_big_endian - check if a device has BE registers
>   *
> @@ -796,6 +818,9 @@ EXPORT_SYMBOL(of_get_next_available_child);
>   * of_get_next_cpu_node - Iterate on cpu nodes
>   * @prev:	previous child of the /cpus node, or NULL to get first
>   *
> + * Unusable CPUs (those with the status property set to "fail" or "fail-...")
> + * will be skipped.
> + *
>   * Return: A cpu node pointer with refcount incremented, use of_node_put()
>   * on it when done. Returns NULL when prev is the last child. Decrements
>   * the refcount of prev.
> @@ -817,6 +842,8 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev)
>  		of_node_put(node);
>  	}

This comment is being really picky.  I would put the check for status value
of fail after the check of node name.  If Rob is willing to accept this
version I am ok with it.

>  	for (; next; next = next->sibling) {
> +		if (__of_device_is_fail(next))
> +			continue;
>  		if (!(of_node_name_eq(next, "cpu") ||
>  		      __of_node_is_type(next, "cpu")))
>  			continue;
> 

Reviewed-by: Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v2] of: base: Skip CPU nodes with "fail"/"fail-..." status

[Sai Prakash Ranjan]

On 11/22/2021 5:15 PM, Matthias Schiffer wrote:
> Allow fully disabling CPU nodes using status = "fail".
>
> 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.
>
> Unknown status values (everything that is not "okay"/"ok", "disabled" or
> "fail"/"fail-...") will continue to be interpreted like "disabled",
> meaning that the CPU can be enabled during boot.
>
> 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>
> ---
>
> v2: Treat unknown status values like "disabled", not like "fail"
>
>
>   drivers/of/base.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 61de453b885c..5b907600f5b0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -650,6 +650,28 @@ bool of_device_is_available(const struct device_node *device)
>   }
>   EXPORT_SYMBOL(of_device_is_available);
>   
> +/**
> + *  __of_device_is_fail - check if a device has status "fail" or "fail-..."
> + *
> + *  @device: Node to check status for, with locks already held
> + *
> + *  Return: True if the status property is set to "fail" or "fail-..." (for any
> + *  error code suffix), false otherwise
> + */
> +static bool __of_device_is_fail(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, "fail") || !strncmp(status, "fail-", 5);
> +}
> +
>   /**
>    *  of_device_is_big_endian - check if a device has BE registers
>    *
> @@ -796,6 +818,9 @@ EXPORT_SYMBOL(of_get_next_available_child);
>    * of_get_next_cpu_node - Iterate on cpu nodes
>    * @prev:	previous child of the /cpus node, or NULL to get first
>    *
> + * Unusable CPUs (those with the status property set to "fail" or "fail-...")
> + * will be skipped.
> + *
>    * Return: A cpu node pointer with refcount incremented, use of_node_put()
>    * on it when done. Returns NULL when prev is the last child. Decrements
>    * the refcount of prev.
> @@ -817,6 +842,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_fail(next))
> +			continue;
>   		if (!(of_node_name_eq(next, "cpu") ||
>   		      __of_node_is_type(next, "cpu")))
>   			continue;

Thanks for the patch, we have a similar usecase coming up and this is 
very useful than fixing up phandles.

Tested-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>

-Sai

Re: [PATCH v2] of: base: Skip CPU nodes with "fail"/"fail-..." status

[Rob Herring]

On Mon, 22 Nov 2021 12:45:36 +0100, Matthias Schiffer wrote:
> Allow fully disabling CPU nodes using status = "fail".
> 
> 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.
> 
> Unknown status values (everything that is not "okay"/"ok", "disabled" or
> "fail"/"fail-...") will continue to be interpreted like "disabled",
> meaning that the CPU can be enabled during boot.
> 
> 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>
> ---
> 
> v2: Treat unknown status values like "disabled", not like "fail"
> 
> 
>  drivers/of/base.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 

Applied, thanks!