[PATCH 0/2] device property: Warn on (of|fwnode|device)_property_read_bool() for non-boolean properties

[PATCH 0/2] device property: Warn on (of|fwnode|device)_property_read_bool() for non-boolean properties

[Rob Herring (Arm)]

This series makes using (of|fwnode|device)_property_read_bool() on 
non-boolean properties give a warning when used with DT. This has been 
deprecated behavior for a while now. Soon we will add yet another 
variant of this function with a Rust binding[1].

Why not put the warning in the fwnode code? The fwnode code doesn't know 
the property's type as that depends on the firmware. For DT, a boolean 
is a property with no value.

Perhaps the ACPI backend should have a warning too? I looked briefly at 
it, but I don't have a clue how or if that can be detected. Doesn't look 
like there's a specific type for booleans. In any case, that would be an 
additional change on top of this series.

[1] https://lore.kernel.org/all/20241025-rust-platform-dev-v1-2-0df8dcf7c20b@kernel.org/

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Rob Herring (Arm) (2):
      device property: Split property reading bool and presence test ops
      of: Warn when of_property_read_bool() is used on non-boolean properties

 drivers/acpi/property.c  |  1 +
 drivers/base/property.c  | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/base/swnode.c    |  1 +
 drivers/of/property.c    | 33 +++++++++++++++++++++++++++++++++
 include/linux/fwnode.h   |  3 +++
 include/linux/of.h       | 29 ++++++++++-------------------
 include/linux/property.h | 15 +++------------
 7 files changed, 89 insertions(+), 31 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20250109-dt-type-warnings-efe52d977bb2

Best regards,
-- 
Rob Herring (Arm) <robh@kernel.org>

[PATCH 1/2] device property: Split property reading bool and presence test ops

[Rob Herring (Arm)]

The fwnode/device property API currently implement
(fwnode|device)_property_read_bool() with (fwnode|device)_property_present().
That does not allow having different behavior depending on the backend.

Specifically, the usage of (fwnode|device)_property_read_bool() on
non-boolean properties is deprecated on DT. In order to add a warning
on this deprecated use, these 2 APIs need separate ops for the backend.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/acpi/property.c  |  1 +
 drivers/base/property.c  | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/base/swnode.c    |  1 +
 drivers/of/property.c    |  7 +++++++
 include/linux/fwnode.h   |  3 +++
 include/linux/of.h       |  4 +++-
 include/linux/property.h | 15 +++------------
 7 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 80a52a4e66dd167e70c80a164a89beb8c834ae13..8c53959ee31e700004de098bdab21dec118cf111 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1656,6 +1656,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
 			acpi_fwnode_device_dma_supported,		\
 		.device_get_dma_attr = acpi_fwnode_device_get_dma_attr,	\
 		.property_present = acpi_fwnode_property_present,	\
+		.property_read_bool = acpi_fwnode_property_present,	\
 		.property_read_int_array =				\
 			acpi_fwnode_property_read_int_array,		\
 		.property_read_string_array =				\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 837d77e3af2bd5bf655e6fa0f64c10a05b361b90..c1392743df9c3f6c554f093f4efae3ea1ad1fdf2 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -70,6 +70,44 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
+/**
+ * device_property_read_bool - Return the value for a boolean property of a device
+ * @dev: Device whose property is being checked
+ * @propname: Name of the property
+ *
+ * Return if property @propname is true or false in the device firmware description.
+ *
+ * Return: true if property @propname is present. Otherwise, returns false.
+ */
+bool device_property_read_bool(const struct device *dev, const char *propname)
+{
+	return fwnode_property_read_bool(dev_fwnode(dev), propname);
+}
+EXPORT_SYMBOL_GPL(device_property_read_bool);
+
+/**
+ * fwnode_property_read_bool - Return the value for a boolean property of a firmware node
+ * @fwnode: Firmware node whose property to check
+ * @propname: Name of the property
+ *
+ * Return if property @propname is true or false in the firmware description.
+ */
+bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
+			     const char *propname)
+{
+	bool ret;
+
+	if (IS_ERR_OR_NULL(fwnode))
+		return false;
+
+	ret = fwnode_call_bool_op(fwnode, property_read_bool, propname);
+	if (ret)
+		return ret;
+
+	return fwnode_call_bool_op(fwnode->secondary, property_read_bool, propname);
+}
+EXPORT_SYMBOL_GPL(fwnode_property_read_bool);
+
 /**
  * device_property_read_u8_array - return a u8 array property of a device
  * @dev: Device to get the property of
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index eb6eb25b343bafaa9c03f881b98a528305fa6093..b1726a3515f6fbe13c2186af1f74479263798e42 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -677,6 +677,7 @@ static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
 	.property_present = software_node_property_present,
+	.property_read_bool = software_node_property_present,
 	.property_read_int_array = software_node_read_int_array,
 	.property_read_string_array = software_node_read_string_array,
 	.get_name = software_node_get_name,
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 519bf9229e613906547b57d8c68e7b8558eff327..1b46be88cc0498fcbe74e7b988f22a86245c366e 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -965,6 +965,12 @@ of_fwnode_device_get_dma_attr(const struct fwnode_handle *fwnode)
 
 static bool of_fwnode_property_present(const struct fwnode_handle *fwnode,
 				       const char *propname)
+{
+	return of_property_present(to_of_node(fwnode), propname);
+}
+
+static bool of_fwnode_property_read_bool(const struct fwnode_handle *fwnode,
+					 const char *propname)
 {
 	return of_property_read_bool(to_of_node(fwnode), propname);
 }
@@ -1562,6 +1568,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.device_dma_supported = of_fwnode_device_dma_supported,
 	.device_get_dma_attr = of_fwnode_device_get_dma_attr,
 	.property_present = of_fwnode_property_present,
+	.property_read_bool = of_fwnode_property_read_bool,
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
 	.get_name = of_fwnode_get_name,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 0d79070c5a70f21e9d73751941f28cefb244950d..0731994b9d7c832cae8a30063f3a64194e4f19aa 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -112,6 +112,7 @@ struct fwnode_reference_args {
  * @device_is_available: Return true if the device is available.
  * @device_get_match_data: Return the device driver match data.
  * @property_present: Return true if a property is present.
+ * @property_read_bool: Return a boolean property value.
  * @property_read_int_array: Read an array of integer properties. Return zero on
  *			     success, a negative error code otherwise.
  * @property_read_string_array: Read an array of string properties. Return zero
@@ -141,6 +142,8 @@ struct fwnode_operations {
 	(*device_get_dma_attr)(const struct fwnode_handle *fwnode);
 	bool (*property_present)(const struct fwnode_handle *fwnode,
 				 const char *propname);
+	bool (*property_read_bool)(const struct fwnode_handle *fwnode,
+				   const char *propname);
 	int (*property_read_int_array)(const struct fwnode_handle *fwnode,
 				       const char *propname,
 				       unsigned int elem_size, void *val,
diff --git a/include/linux/of.h b/include/linux/of.h
index f921786cb8ac782286ed5ff4425a35668204d050..1cb4eb7fc2eded2246c697c3bcaf1b85d43108ab 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1271,7 +1271,9 @@ static inline bool of_property_read_bool(const struct device_node *np,
  */
 static inline bool of_property_present(const struct device_node *np, const char *propname)
 {
-	return of_property_read_bool(np, propname);
+	struct property *prop = of_find_property(np, propname, NULL);
+
+	return prop ? true : false;
 }
 
 /**
diff --git a/include/linux/property.h b/include/linux/property.h
index 61fc20e5f81f8da992aa021996ddf68ef3eff0ed..e214ecd241eb4e6d9e0873d094f2c956bbf66c34 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -37,6 +37,7 @@ struct fwnode_handle *__dev_fwnode(struct device *dev);
 		 struct device *: __dev_fwnode)(dev)
 
 bool device_property_present(const struct device *dev, const char *propname);
+bool device_property_read_bool(const struct device *dev, const char *propname);
 int device_property_read_u8_array(const struct device *dev, const char *propname,
 				  u8 *val, size_t nval);
 int device_property_read_u16_array(const struct device *dev, const char *propname,
@@ -54,6 +55,8 @@ int device_property_match_string(const struct device *dev,
 
 bool fwnode_property_present(const struct fwnode_handle *fwnode,
 			     const char *propname);
+bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
+			     const char *propname);
 int fwnode_property_read_u8_array(const struct fwnode_handle *fwnode,
 				  const char *propname, u8 *val,
 				  size_t nval);
@@ -207,12 +210,6 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
 
 unsigned int device_get_child_node_count(const struct device *dev);
 
-static inline bool device_property_read_bool(const struct device *dev,
-					     const char *propname)
-{
-	return device_property_present(dev, propname);
-}
-
 static inline int device_property_read_u8(const struct device *dev,
 					  const char *propname, u8 *val)
 {
@@ -263,12 +260,6 @@ static inline int device_property_string_array_count(const struct device *dev,
 	return device_property_read_string_array(dev, propname, NULL, 0);
 }
 
-static inline bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
-					     const char *propname)
-{
-	return fwnode_property_present(fwnode, propname);
-}
-
 static inline int fwnode_property_read_u8(const struct fwnode_handle *fwnode,
 					  const char *propname, u8 *val)
 {

-- 
2.45.2

[PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Rob Herring (Arm)]

The use of of_property_read_bool() for non-boolean properties is
deprecated. The primary use of it was to test property presence, but
that has been replaced in favor of of_property_present(). With those
uses now fixed, add a warning to discourage new ones.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/property.c | 26 ++++++++++++++++++++++++++
 include/linux/of.h    | 25 +++++++------------------
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 1b46be88cc0498fcbe74e7b988f22a86245c366e..9a6d84d19ff371def8bedc009bb6ab488458a29b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -31,6 +31,32 @@
 
 #include "of_private.h"
 
+/**
+ * of_property_read_bool - Find a property
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ *
+ * Search for a boolean property in a device node. Usage on non-boolean
+ * property types is deprecated.
+ *
+ * Return: true if the property exists false otherwise.
+ */
+bool of_property_read_bool(const struct device_node *np, const char *propname)
+{
+	struct property *prop = of_find_property(np, propname, NULL);
+
+	/*
+	 * Boolean properties should not have a value. Testing for property
+	 * presence should either use of_property_present() or just read the
+	 * property value and check the returned error code.
+	 */
+	if (prop && prop->length)
+		pr_warn("%pOF: Read of boolean property '%s' with a value.\n", np, propname);
+
+	return prop ? true : false;
+}
+EXPORT_SYMBOL(of_property_read_bool);
+
 /**
  * of_graph_is_present() - check graph's presence
  * @node: pointer to device_node containing graph port
diff --git a/include/linux/of.h b/include/linux/of.h
index 1cb4eb7fc2eded2246c697c3bcaf1b85d43108ab..0cdd58ff0a4190724309ba1eddbac51b188b6136 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -311,6 +311,7 @@ extern struct device_node *of_find_node_with_property(
 extern struct property *of_find_property(const struct device_node *np,
 					 const char *name,
 					 int *lenp);
+extern bool of_property_read_bool(const struct device_node *np, const char *propname);
 extern int of_property_count_elems_of_size(const struct device_node *np,
 				const char *propname, int elem_size);
 extern int of_property_read_u32_index(const struct device_node *np,
@@ -615,6 +616,12 @@ static inline struct device_node *of_find_compatible_node(
 	return NULL;
 }
 
+static inline bool of_property_read_bool(const struct device_node *np,
+					const char *propname)
+{
+	return false;
+}
+
 static inline int of_property_count_elems_of_size(const struct device_node *np,
 			const char *propname, int elem_size)
 {
@@ -1242,24 +1249,6 @@ static inline int of_property_read_string_index(const struct device_node *np,
 	return rc < 0 ? rc : 0;
 }
 
-/**
- * of_property_read_bool - Find a property
- * @np:		device node from which the property value is to be read.
- * @propname:	name of the property to be searched.
- *
- * Search for a boolean property in a device node. Usage on non-boolean
- * property types is deprecated.
- *
- * Return: true if the property exists false otherwise.
- */
-static inline bool of_property_read_bool(const struct device_node *np,
-					 const char *propname)
-{
-	const struct property *prop = of_find_property(np, propname, NULL);
-
-	return prop ? true : false;
-}
-
 /**
  * of_property_present - Test if a property is present in a node
  * @np:		device node to search for the property.

-- 
2.45.2

Re: [PATCH 1/2] device property: Split property reading bool and presence test ops

[Greg Kroah-Hartman]

On Thu, Jan 09, 2025 at 01:42:05PM -0600, Rob Herring (Arm) wrote:
> The fwnode/device property API currently implement
> (fwnode|device)_property_read_bool() with (fwnode|device)_property_present().
> That does not allow having different behavior depending on the backend.
> 
> Specifically, the usage of (fwnode|device)_property_read_bool() on
> non-boolean properties is deprecated on DT. In order to add a warning
> on this deprecated use, these 2 APIs need separate ops for the backend.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Krzysztof Kozlowski]

On Thu, Jan 09, 2025 at 01:42:06PM -0600, Rob Herring (Arm) wrote:
> The use of of_property_read_bool() for non-boolean properties is
> deprecated. The primary use of it was to test property presence, but
> that has been replaced in favor of of_property_present(). With those
> uses now fixed, add a warning to discourage new ones.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/property.c | 26 ++++++++++++++++++++++++++
>  include/linux/of.h    | 25 +++++++------------------
>  2 files changed, 33 insertions(+), 18 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 1/2] device property: Split property reading bool and presence test ops

[Krzysztof Kozlowski]

On Thu, Jan 09, 2025 at 01:42:05PM -0600, Rob Herring (Arm) wrote:
> The fwnode/device property API currently implement
> (fwnode|device)_property_read_bool() with (fwnode|device)_property_present().
> That does not allow having different behavior depending on the backend.
> 
> Specifically, the usage of (fwnode|device)_property_read_bool() on
> non-boolean properties is deprecated on DT. In order to add a warning
> on this deprecated use, these 2 APIs need separate ops for the backend.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/acpi/property.c  |  1 +
>  drivers/base/property.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/base/swnode.c    |  1 +
>  drivers/of/property.c    |  7 +++++++
>  include/linux/fwnode.h   |  3 +++
>  include/linux/of.h       |  4 +++-
>  include/linux/property.h | 15 +++------------
>  7 files changed, 56 insertions(+), 13 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 0/2] device property: Warn on (of|fwnode|device)_property_read_bool() for non-boolean properties

[Andy Shevchenko]

On Thu, Jan 09, 2025 at 01:42:04PM -0600, Rob Herring (Arm) wrote:
> This series makes using (of|fwnode|device)_property_read_bool() on 
> non-boolean properties give a warning when used with DT. This has been 
> deprecated behavior for a while now. Soon we will add yet another 
> variant of this function with a Rust binding[1].
> 
> Why not put the warning in the fwnode code? The fwnode code doesn't know 
> the property's type as that depends on the firmware. For DT, a boolean 
> is a property with no value.
> 
> Perhaps the ACPI backend should have a warning too? I looked briefly at 
> it, but I don't have a clue how or if that can be detected. Doesn't look 
> like there's a specific type for booleans. In any case, that would be an 
> additional change on top of this series.

ACPI doesn't have such a concept, i.e. there is no separate "boolean" type.
All properties must have a value, we just ignore its content in case of
_read_bool() API, so from ACPI perspective this _read_bool() is purely SW
concept.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] device property: Split property reading bool and presence test ops

[Rafael J. Wysocki]

On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> The fwnode/device property API currently implement
> (fwnode|device)_property_read_bool() with (fwnode|device)_property_present().
> That does not allow having different behavior depending on the backend.
>
> Specifically, the usage of (fwnode|device)_property_read_bool() on
> non-boolean properties is deprecated on DT. In order to add a warning
> on this deprecated use, these 2 APIs need separate ops for the backend.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and please route this as needed.

> ---
>  drivers/acpi/property.c  |  1 +
>  drivers/base/property.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/base/swnode.c    |  1 +
>  drivers/of/property.c    |  7 +++++++
>  include/linux/fwnode.h   |  3 +++
>  include/linux/of.h       |  4 +++-
>  include/linux/property.h | 15 +++------------
>  7 files changed, 56 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 80a52a4e66dd167e70c80a164a89beb8c834ae13..8c53959ee31e700004de098bdab21dec118cf111 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1656,6 +1656,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
>                         acpi_fwnode_device_dma_supported,               \
>                 .device_get_dma_attr = acpi_fwnode_device_get_dma_attr, \
>                 .property_present = acpi_fwnode_property_present,       \
> +               .property_read_bool = acpi_fwnode_property_present,     \
>                 .property_read_int_array =                              \
>                         acpi_fwnode_property_read_int_array,            \
>                 .property_read_string_array =                           \
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 837d77e3af2bd5bf655e6fa0f64c10a05b361b90..c1392743df9c3f6c554f093f4efae3ea1ad1fdf2 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -70,6 +70,44 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_present);
>
> +/**
> + * device_property_read_bool - Return the value for a boolean property of a device
> + * @dev: Device whose property is being checked
> + * @propname: Name of the property
> + *
> + * Return if property @propname is true or false in the device firmware description.
> + *
> + * Return: true if property @propname is present. Otherwise, returns false.
> + */
> +bool device_property_read_bool(const struct device *dev, const char *propname)
> +{
> +       return fwnode_property_read_bool(dev_fwnode(dev), propname);
> +}
> +EXPORT_SYMBOL_GPL(device_property_read_bool);
> +
> +/**
> + * fwnode_property_read_bool - Return the value for a boolean property of a firmware node
> + * @fwnode: Firmware node whose property to check
> + * @propname: Name of the property
> + *
> + * Return if property @propname is true or false in the firmware description.
> + */
> +bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
> +                            const char *propname)
> +{
> +       bool ret;
> +
> +       if (IS_ERR_OR_NULL(fwnode))
> +               return false;
> +
> +       ret = fwnode_call_bool_op(fwnode, property_read_bool, propname);
> +       if (ret)
> +               return ret;
> +
> +       return fwnode_call_bool_op(fwnode->secondary, property_read_bool, propname);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_property_read_bool);
> +
>  /**
>   * device_property_read_u8_array - return a u8 array property of a device
>   * @dev: Device to get the property of
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index eb6eb25b343bafaa9c03f881b98a528305fa6093..b1726a3515f6fbe13c2186af1f74479263798e42 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -677,6 +677,7 @@ static const struct fwnode_operations software_node_ops = {
>         .get = software_node_get,
>         .put = software_node_put,
>         .property_present = software_node_property_present,
> +       .property_read_bool = software_node_property_present,
>         .property_read_int_array = software_node_read_int_array,
>         .property_read_string_array = software_node_read_string_array,
>         .get_name = software_node_get_name,
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 519bf9229e613906547b57d8c68e7b8558eff327..1b46be88cc0498fcbe74e7b988f22a86245c366e 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -965,6 +965,12 @@ of_fwnode_device_get_dma_attr(const struct fwnode_handle *fwnode)
>
>  static bool of_fwnode_property_present(const struct fwnode_handle *fwnode,
>                                        const char *propname)
> +{
> +       return of_property_present(to_of_node(fwnode), propname);
> +}
> +
> +static bool of_fwnode_property_read_bool(const struct fwnode_handle *fwnode,
> +                                        const char *propname)
>  {
>         return of_property_read_bool(to_of_node(fwnode), propname);
>  }
> @@ -1562,6 +1568,7 @@ const struct fwnode_operations of_fwnode_ops = {
>         .device_dma_supported = of_fwnode_device_dma_supported,
>         .device_get_dma_attr = of_fwnode_device_get_dma_attr,
>         .property_present = of_fwnode_property_present,
> +       .property_read_bool = of_fwnode_property_read_bool,
>         .property_read_int_array = of_fwnode_property_read_int_array,
>         .property_read_string_array = of_fwnode_property_read_string_array,
>         .get_name = of_fwnode_get_name,
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0d79070c5a70f21e9d73751941f28cefb244950d..0731994b9d7c832cae8a30063f3a64194e4f19aa 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -112,6 +112,7 @@ struct fwnode_reference_args {
>   * @device_is_available: Return true if the device is available.
>   * @device_get_match_data: Return the device driver match data.
>   * @property_present: Return true if a property is present.
> + * @property_read_bool: Return a boolean property value.
>   * @property_read_int_array: Read an array of integer properties. Return zero on
>   *                          success, a negative error code otherwise.
>   * @property_read_string_array: Read an array of string properties. Return zero
> @@ -141,6 +142,8 @@ struct fwnode_operations {
>         (*device_get_dma_attr)(const struct fwnode_handle *fwnode);
>         bool (*property_present)(const struct fwnode_handle *fwnode,
>                                  const char *propname);
> +       bool (*property_read_bool)(const struct fwnode_handle *fwnode,
> +                                  const char *propname);
>         int (*property_read_int_array)(const struct fwnode_handle *fwnode,
>                                        const char *propname,
>                                        unsigned int elem_size, void *val,
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f921786cb8ac782286ed5ff4425a35668204d050..1cb4eb7fc2eded2246c697c3bcaf1b85d43108ab 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1271,7 +1271,9 @@ static inline bool of_property_read_bool(const struct device_node *np,
>   */
>  static inline bool of_property_present(const struct device_node *np, const char *propname)
>  {
> -       return of_property_read_bool(np, propname);
> +       struct property *prop = of_find_property(np, propname, NULL);
> +
> +       return prop ? true : false;
>  }
>
>  /**
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 61fc20e5f81f8da992aa021996ddf68ef3eff0ed..e214ecd241eb4e6d9e0873d094f2c956bbf66c34 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -37,6 +37,7 @@ struct fwnode_handle *__dev_fwnode(struct device *dev);
>                  struct device *: __dev_fwnode)(dev)
>
>  bool device_property_present(const struct device *dev, const char *propname);
> +bool device_property_read_bool(const struct device *dev, const char *propname);
>  int device_property_read_u8_array(const struct device *dev, const char *propname,
>                                   u8 *val, size_t nval);
>  int device_property_read_u16_array(const struct device *dev, const char *propname,
> @@ -54,6 +55,8 @@ int device_property_match_string(const struct device *dev,
>
>  bool fwnode_property_present(const struct fwnode_handle *fwnode,
>                              const char *propname);
> +bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
> +                            const char *propname);
>  int fwnode_property_read_u8_array(const struct fwnode_handle *fwnode,
>                                   const char *propname, u8 *val,
>                                   size_t nval);
> @@ -207,12 +210,6 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
>
>  unsigned int device_get_child_node_count(const struct device *dev);
>
> -static inline bool device_property_read_bool(const struct device *dev,
> -                                            const char *propname)
> -{
> -       return device_property_present(dev, propname);
> -}
> -
>  static inline int device_property_read_u8(const struct device *dev,
>                                           const char *propname, u8 *val)
>  {
> @@ -263,12 +260,6 @@ static inline int device_property_string_array_count(const struct device *dev,
>         return device_property_read_string_array(dev, propname, NULL, 0);
>  }
>
> -static inline bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
> -                                            const char *propname)
> -{
> -       return fwnode_property_present(fwnode, propname);
> -}
> -
>  static inline int fwnode_property_read_u8(const struct fwnode_handle *fwnode,
>                                           const char *propname, u8 *val)
>  {
>
> --
> 2.45.2
>

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Geert Uytterhoeven]

Hi Rob,

On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> The use of of_property_read_bool() for non-boolean properties is
> deprecated. The primary use of it was to test property presence, but
> that has been replaced in favor of of_property_present(). With those
> uses now fixed, add a warning to discourage new ones.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

Thanks for your patch, which is now commit c141ecc3cecd7647 ("of:
Warn when of_property_read_bool() is used on non-boolean properties")
in dt-rh/for-next.

I have bisected a failure in secondary CPU bring-up on R-Car H1 (quad
Cortex-A9 MPCore) to this commit:

     Detected Renesas R-Car Gen1 r8a7779 ES1.0
     smp: Bringing up secondary CPUs ...
    -CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
    -CPU1: Spectre v2: using BPIALL workaround
    -CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
    -CPU2: Spectre v2: using BPIALL workaround
    -CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
    -CPU3: Spectre v2: using BPIALL workaround
    -smp: Brought up 1 node, 4 CPUs
    -SMP: Total of 4 processors activated (2000.00 BogoMIPS).
    +CPU1: failed to come online
    +CPU2: failed to come online
    +CPU3: failed to come online
    +smp: Brought up 1 node, 1 CPU
    +SMP: Total of 1 processors activated (500.00 BogoMIPS).
     CPU: All CPU(s) started in SVC mode.

Reverting this commit on top of my work tree fixes the issue, too.
However, I do not see how this commit could impact CPU bring-up?

I added debug code to of_property_read_bool(), to print all look-ups.
I only saw a few before CPU bring-up, nothing relevant:

     NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
    +OF: of_property_read_bool(interrupt-controller): 1
     rcu: srcu_init: Setting srcu_struct sizes based on contention.
    +OF: of_property_read_bool(interrupt-controller): 1
     sched_clock: 64 bits at 250MHz, resolution 4ns, wraps every 4398046511102ns
     clocksource: arm_global_timer: mask: 0xffffffffffffffff
max_cycles: 0x7350b89c29, max_idle_ns: 881590431910 ns
     Switching to timer-based delay loop, resolution 4ns
    +OF: of_property_read_bool(interrupt-controller): 1
    +OF: of_property_read_bool(always-on): 0
     Console: colour dummy device 80x30
     printk: legacy console [tty0] enabled

Perhaps something shifted in the code layout?  The obvious suspects
(shmobile_boot_* and shmobile_smp_* asm code, secondary_startup(),
addresses in arch/arm/mach-shmobile/platsmp-scu.c) are still at the
same addresses as before...

Anyone with a clue?
Thanks!

> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -31,6 +31,32 @@
>
>  #include "of_private.h"
>
> +/**
> + * of_property_read_bool - Find a property
> + * @np:                device node from which the property value is to be read.
> + * @propname:  name of the property to be searched.
> + *
> + * Search for a boolean property in a device node. Usage on non-boolean
> + * property types is deprecated.
> + *
> + * Return: true if the property exists false otherwise.
> + */
> +bool of_property_read_bool(const struct device_node *np, const char *propname)
> +{
> +       struct property *prop = of_find_property(np, propname, NULL);
> +
> +       /*
> +        * Boolean properties should not have a value. Testing for property
> +        * presence should either use of_property_present() or just read the
> +        * property value and check the returned error code.
> +        */
> +       if (prop && prop->length)
> +               pr_warn("%pOF: Read of boolean property '%s' with a value.\n", np, propname);
> +
> +       return prop ? true : false;
> +}
> +EXPORT_SYMBOL(of_property_read_bool);
> +
>  /**
>   * of_graph_is_present() - check graph's presence
>   * @node: pointer to device_node containing graph port
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 1cb4eb7fc2eded2246c697c3bcaf1b85d43108ab..0cdd58ff0a4190724309ba1eddbac51b188b6136 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -311,6 +311,7 @@ extern struct device_node *of_find_node_with_property(
>  extern struct property *of_find_property(const struct device_node *np,
>                                          const char *name,
>                                          int *lenp);
> +extern bool of_property_read_bool(const struct device_node *np, const char *propname);
>  extern int of_property_count_elems_of_size(const struct device_node *np,
>                                 const char *propname, int elem_size);
>  extern int of_property_read_u32_index(const struct device_node *np,
> @@ -615,6 +616,12 @@ static inline struct device_node *of_find_compatible_node(
>         return NULL;
>  }
>
> +static inline bool of_property_read_bool(const struct device_node *np,
> +                                       const char *propname)
> +{
> +       return false;
> +}
> +
>  static inline int of_property_count_elems_of_size(const struct device_node *np,
>                         const char *propname, int elem_size)
>  {
> @@ -1242,24 +1249,6 @@ static inline int of_property_read_string_index(const struct device_node *np,
>         return rc < 0 ? rc : 0;
>  }
>
> -/**
> - * of_property_read_bool - Find a property
> - * @np:                device node from which the property value is to be read.
> - * @propname:  name of the property to be searched.
> - *
> - * Search for a boolean property in a device node. Usage on non-boolean
> - * property types is deprecated.
> - *
> - * Return: true if the property exists false otherwise.
> - */
> -static inline bool of_property_read_bool(const struct device_node *np,
> -                                        const char *propname)
> -{
> -       const struct property *prop = of_find_property(np, propname, NULL);
> -
> -       return prop ? true : false;
> -}
> -
>  /**
>   * of_property_present - Test if a property is present in a node
>   * @np:                device node to search for the property.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Andy Shevchenko]

On Tue, Jan 14, 2025 at 07:35:22PM +0100, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > The use of of_property_read_bool() for non-boolean properties is
> > deprecated. The primary use of it was to test property presence, but
> > that has been replaced in favor of of_property_present(). With those
> > uses now fixed, add a warning to discourage new ones.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> 
> Thanks for your patch, which is now commit c141ecc3cecd7647 ("of:
> Warn when of_property_read_bool() is used on non-boolean properties")
> in dt-rh/for-next.
> 
> I have bisected a failure in secondary CPU bring-up on R-Car H1 (quad
> Cortex-A9 MPCore) to this commit:
> 
>      Detected Renesas R-Car Gen1 r8a7779 ES1.0
>      smp: Bringing up secondary CPUs ...
>     -CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
>     -CPU1: Spectre v2: using BPIALL workaround
>     -CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
>     -CPU2: Spectre v2: using BPIALL workaround
>     -CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
>     -CPU3: Spectre v2: using BPIALL workaround
>     -smp: Brought up 1 node, 4 CPUs
>     -SMP: Total of 4 processors activated (2000.00 BogoMIPS).
>     +CPU1: failed to come online
>     +CPU2: failed to come online
>     +CPU3: failed to come online
>     +smp: Brought up 1 node, 1 CPU
>     +SMP: Total of 1 processors activated (500.00 BogoMIPS).
>      CPU: All CPU(s) started in SVC mode.
> 
> Reverting this commit on top of my work tree fixes the issue, too.
> However, I do not see how this commit could impact CPU bring-up?
> 
> I added debug code to of_property_read_bool(), to print all look-ups.
> I only saw a few before CPU bring-up, nothing relevant:
> 
>      NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
>     +OF: of_property_read_bool(interrupt-controller): 1
>      rcu: srcu_init: Setting srcu_struct sizes based on contention.
>     +OF: of_property_read_bool(interrupt-controller): 1
>      sched_clock: 64 bits at 250MHz, resolution 4ns, wraps every 4398046511102ns
>      clocksource: arm_global_timer: mask: 0xffffffffffffffff
> max_cycles: 0x7350b89c29, max_idle_ns: 881590431910 ns
>      Switching to timer-based delay loop, resolution 4ns
>     +OF: of_property_read_bool(interrupt-controller): 1
>     +OF: of_property_read_bool(always-on): 0
>      Console: colour dummy device 80x30
>      printk: legacy console [tty0] enabled
> 
> Perhaps something shifted in the code layout?  The obvious suspects
> (shmobile_boot_* and shmobile_smp_* asm code, secondary_startup(),
> addresses in arch/arm/mach-shmobile/platsmp-scu.c) are still at the
> same addresses as before...
> 
> Anyone with a clue?

Hmm... You meant the patch 2 troubles this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Rob Herring]

On Tue, Jan 14, 2025 at 12:35 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > The use of of_property_read_bool() for non-boolean properties is
> > deprecated. The primary use of it was to test property presence, but
> > that has been replaced in favor of of_property_present(). With those
> > uses now fixed, add a warning to discourage new ones.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>
> Thanks for your patch, which is now commit c141ecc3cecd7647 ("of:
> Warn when of_property_read_bool() is used on non-boolean properties")
> in dt-rh/for-next.
>
> I have bisected a failure in secondary CPU bring-up on R-Car H1 (quad
> Cortex-A9 MPCore) to this commit:
>
>      Detected Renesas R-Car Gen1 r8a7779 ES1.0
>      smp: Bringing up secondary CPUs ...
>     -CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
>     -CPU1: Spectre v2: using BPIALL workaround
>     -CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
>     -CPU2: Spectre v2: using BPIALL workaround
>     -CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
>     -CPU3: Spectre v2: using BPIALL workaround
>     -smp: Brought up 1 node, 4 CPUs
>     -SMP: Total of 4 processors activated (2000.00 BogoMIPS).
>     +CPU1: failed to come online
>     +CPU2: failed to come online
>     +CPU3: failed to come online
>     +smp: Brought up 1 node, 1 CPU
>     +SMP: Total of 1 processors activated (500.00 BogoMIPS).
>      CPU: All CPU(s) started in SVC mode.
>
> Reverting this commit on top of my work tree fixes the issue, too.
> However, I do not see how this commit could impact CPU bring-up?

Strange. Perhaps the of_property_read_bool was inlined into some
special section before?

Rob

Re: [PATCH 1/2] device property: Split property reading bool and presence test ops

[Sakari Ailus]

On Thu, Jan 09, 2025 at 01:42:05PM -0600, Rob Herring (Arm) wrote:
> The fwnode/device property API currently implement
> (fwnode|device)_property_read_bool() with (fwnode|device)_property_present().
> That does not allow having different behavior depending on the backend.
> 
> Specifically, the usage of (fwnode|device)_property_read_bool() on
> non-boolean properties is deprecated on DT. In order to add a warning
> on this deprecated use, these 2 APIs need separate ops for the backend.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

Thanks!

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Geert Uytterhoeven]

On Tue, Jan 14, 2025 at 8:19 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 14, 2025 at 12:35 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > The use of of_property_read_bool() for non-boolean properties is
> > > deprecated. The primary use of it was to test property presence, but
> > > that has been replaced in favor of of_property_present(). With those
> > > uses now fixed, add a warning to discourage new ones.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> >
> > Thanks for your patch, which is now commit c141ecc3cecd7647 ("of:
> > Warn when of_property_read_bool() is used on non-boolean properties")
> > in dt-rh/for-next.
> >
> > I have bisected a failure in secondary CPU bring-up on R-Car H1 (quad
> > Cortex-A9 MPCore) to this commit:
> >
> >      Detected Renesas R-Car Gen1 r8a7779 ES1.0
> >      smp: Bringing up secondary CPUs ...
> >     -CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
> >     -CPU1: Spectre v2: using BPIALL workaround
> >     -CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
> >     -CPU2: Spectre v2: using BPIALL workaround
> >     -CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
> >     -CPU3: Spectre v2: using BPIALL workaround
> >     -smp: Brought up 1 node, 4 CPUs
> >     -SMP: Total of 4 processors activated (2000.00 BogoMIPS).
> >     +CPU1: failed to come online
> >     +CPU2: failed to come online
> >     +CPU3: failed to come online
> >     +smp: Brought up 1 node, 1 CPU
> >     +SMP: Total of 1 processors activated (500.00 BogoMIPS).
> >      CPU: All CPU(s) started in SVC mode.
> >
> > Reverting this commit on top of my work tree fixes the issue, too.
> > However, I do not see how this commit could impact CPU bring-up?
>
> Strange. Perhaps the of_property_read_bool was inlined into some
> special section before?

I re-added the old inline of_property_read_bool(), but with a different
name.  CPU bringup starts working again if I replace at least one call
to of_property_read_bool() in arch/arm/mm/cache-l2x0.c:aurora_of_parse()
by a call to the inline variant, or even if I just add

    pr_info("xf_property_read_bool(np, \"wt-override\") = %d\n",
xf_property_read_bool(np, "wt-override"));

to that function. Note that that function is not called at all on my platform.

This small change causes quite some reordering in arch/arm/mm/cache-l2x0.s,
so it looks like a layout issue. More analysis will follow...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] of: Warn when of_property_read_bool() is used on non-boolean properties

[Geert Uytterhoeven]

On Wed, 15 Jan 2025 at 12:20, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Jan 14, 2025 at 8:19 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jan 14, 2025 at 12:35 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Thu, Jan 9, 2025 at 8:42 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > > The use of of_property_read_bool() for non-boolean properties is
> > > > deprecated. The primary use of it was to test property presence, but
> > > > that has been replaced in favor of of_property_present(). With those
> > > > uses now fixed, add a warning to discourage new ones.
> > > >
> > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > >
> > > Thanks for your patch, which is now commit c141ecc3cecd7647 ("of:
> > > Warn when of_property_read_bool() is used on non-boolean properties")
> > > in dt-rh/for-next.
> > >
> > > I have bisected a failure in secondary CPU bring-up on R-Car H1 (quad
> > > Cortex-A9 MPCore) to this commit:
> > >
> > >      Detected Renesas R-Car Gen1 r8a7779 ES1.0
> > >      smp: Bringing up secondary CPUs ...
> > >     -CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
> > >     -CPU1: Spectre v2: using BPIALL workaround
> > >     -CPU2: thread -1, cpu 2, socket 0, mpidr 80000002
> > >     -CPU2: Spectre v2: using BPIALL workaround
> > >     -CPU3: thread -1, cpu 3, socket 0, mpidr 80000003
> > >     -CPU3: Spectre v2: using BPIALL workaround
> > >     -smp: Brought up 1 node, 4 CPUs
> > >     -SMP: Total of 4 processors activated (2000.00 BogoMIPS).
> > >     +CPU1: failed to come online
> > >     +CPU2: failed to come online
> > >     +CPU3: failed to come online
> > >     +smp: Brought up 1 node, 1 CPU
> > >     +SMP: Total of 1 processors activated (500.00 BogoMIPS).
> > >      CPU: All CPU(s) started in SVC mode.
> > >
> > > Reverting this commit on top of my work tree fixes the issue, too.
> > > However, I do not see how this commit could impact CPU bring-up?
> >
> > Strange. Perhaps the of_property_read_bool was inlined into some
> > special section before?
>
> I re-added the old inline of_property_read_bool(), but with a different
> name.  CPU bringup starts working again if I replace at least one call
> to of_property_read_bool() in arch/arm/mm/cache-l2x0.c:aurora_of_parse()
> by a call to the inline variant, or even if I just add
>
>     pr_info("xf_property_read_bool(np, \"wt-override\") = %d\n",
> xf_property_read_bool(np, "wt-override"));
>
> to that function. Note that that function is not called at all on my platform.
>
> This small change causes quite some reordering in arch/arm/mm/cache-l2x0.s,
> so it looks like a layout issue. More analysis will follow...

The assembler SMP bring-up code for Renesas SoCs lacked an alignment
directive.  I have posted a fix:
https://lore.kernel.org/r/CAMuHMdU=QR-JLgEHKWpsr6SbaZRc-Hz9r91JfpP8c3n2G-OjqA@mail.gmail.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds