[PATCH 00/10] of: fix bugs and improve codes

[PATCH 00/10] of: fix bugs and improve codes

[Zijun Hu]

This patch series is to fix bugs and improve codes for drivers/of/*.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (10):
      of: Fix alias name length calculating error in API of_find_node_opts_by_path()
      of: Correct return value for API of_parse_phandle_with_args_map()
      of: Correct child specifier used as input of the 2nd nexus node
      of: Fix refcount leakage for OF node returned by __of_get_dma_parent()
      of: Fix available buffer size calculating error in API of_device_uevent_modalias()
      of/fdt: Dump __be32 array in CPU type order in of_dump_addr()
      of: Correct comments for of_alias_scan()
      of: Swap implementation between of_property_present() and of_property_read_bool()
      of: property: Implement of_fwnode_property_present() by of_property_present()
      of: Simplify API of_find_node_with_property() implementation

 drivers/of/address.c     |  2 +-
 drivers/of/base.c        | 25 +++++++++++--------------
 drivers/of/device.c      |  4 ++--
 drivers/of/fdt_address.c |  2 +-
 drivers/of/property.c    |  2 +-
 include/linux/of.h       | 23 ++++++++++++-----------
 6 files changed, 28 insertions(+), 30 deletions(-)
---
base-commit: 16ef9c9de0c48b836c5996c6e9792cb4f658c8f1
change-id: 20241206-of_core_fix-dc3021a06418

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 01/10] of: Fix alias name length calculating error in API of_find_node_opts_by_path()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Alias name length calculated by of_find_node_opts_by_path() is wrong as
explained below:

Take "alias/serial@llc500:115200n8" as its @patch argument for an example
      ^    ^             ^
      0	   5		 19

The right length of alias 'alias' is 5, but the API results in 19 which is
obvious wrong.

The wrong length will cause finding device node failure for such paths.
Fix by using index of either '/' or ':' as the length who comes earlier.

Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7dc394255a0a14cd1aed02ec79c2f787a222b44c..9a9313183d1f1b61918fe7e6fa80c2726b099a1c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -893,10 +893,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		int len;
-		const char *p = separator;
+		const char *p = strchrnul(path, '/');
 
-		if (!p)
-			p = strchrnul(path, '/');
+		if (separator && separator < p)
+			p = separator;
 		len = p - path;
 
 		/* of_aliases must not be NULL */

-- 
2.34.1

[PATCH 02/10] of: Correct return value for API of_parse_phandle_with_args_map()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

@ret is used by of_parse_phandle_with_args_map() to record return value
and it is preseted with -EINVAL before the outer while loop, but it is
changed to 0 by below successful operation within the inner loop:
of_property_read_u32(new, cells_name, &new_size)

So cause 0(success) is returned for all failures which happen after the
operation, that is obviously wrong.

Fix by restoring @ret with preseted -EINVAL after the operation.

Fixes: bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9a9313183d1f1b61918fe7e6fa80c2726b099a1c..b294924aa31cfed1ec06983f420a445f7fae7394 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1516,6 +1516,8 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 			ret = of_property_read_u32(new, cells_name, &new_size);
 			if (ret)
 				goto put;
+			/* Restore preseted error code */
+			ret = -EINVAL;
 
 			/* Check for malformed properties */
 			if (WARN_ON(new_size > MAX_PHANDLE_ARGS))

-- 
2.34.1

[PATCH 03/10] of: Correct child specifier used as input of the 2nd nexus node

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

API of_parse_phandle_with_args_map() will use wrong input for nexus node
Nexus_2 as shown below:

    Node_1		Nexus_1                              Nexus_2
&Nexus_1,arg_1 -> arg_1,&Nexus_2,arg_2' -> &Nexus_2,arg_2 -> arg_2,...
		  map-pass-thru=<...>

Nexus_1's output arg_2 should be used as input of Nexus_2, but the API
wrongly uses arg_2' instead which != arg_2 due to Nexus_1's map-pass-thru.

Fix by always making @match_array point to @initial_match_array into
which to store nexus output.

Fixes: bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b294924aa31cfed1ec06983f420a445f7fae7394..1c62cda4ebcd9e3dc5f91d10fa68f975226693dd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1542,7 +1542,6 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 		 * specifier into the out_args structure, keeping the
 		 * bits specified in <list>-map-pass-thru.
 		 */
-		match_array = map - new_size;
 		for (i = 0; i < new_size; i++) {
 			__be32 val = *(map - new_size + i);
 
@@ -1551,6 +1550,7 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 				val |= cpu_to_be32(out_args->args[i]) & pass[i];
 			}
 
+			initial_match_array[i] = val;
 			out_args->args[i] = be32_to_cpu(val);
 		}
 		out_args->args_count = list_size = new_size;

-- 
2.34.1

[PATCH 04/10] of: Fix refcount leakage for OF node returned by __of_get_dma_parent()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

__of_get_dma_parent() returns OF device node @args.np, but the node's
refcount is increased twice, by both of_parse_phandle_with_args() and
of_node_get(), so causes refcount leakage for the node.

Fix by directly returning the node got by of_parse_phandle_with_args().

Fixes: f83a6e5dea6c ("of: address: Add support for the parent DMA bus")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/address.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index c5b925ac469f16b8ae4b8275b60210a2d583ff83..cda1cbb82eabdcd39d32446023fbb105b69bc99d 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -619,7 +619,7 @@ struct device_node *__of_get_dma_parent(const struct device_node *np)
 	if (ret < 0)
 		return of_get_parent(np);
 
-	return of_node_get(args.np);
+	return args.np;
 }
 #endif
 

-- 
2.34.1

[PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_device_uevent_modalias() saves MODALIAS value from offset
(@env->buflen - 1), so the available buffer size should be
(sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong
size (sizeof(@env->buf) - @env->buflen).

Fix by using right size (sizeof(@env->buf) - @env->buflen + 1).

Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
 		return -ENOMEM;
 
 	sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
-			 sizeof(env->buf) - env->buflen);
+			 sizeof(env->buf) - env->buflen + 1);
 	if (sl < 0)
 		return sl;
-	if (sl >= (sizeof(env->buf) - env->buflen))
+	if (sl >= (sizeof(env->buf) - env->buflen + 1))
 		return -ENOMEM;
 	env->buflen += sl;
 

-- 
2.34.1

[PATCH 06/10] of/fdt: Dump __be32 array in CPU type order in of_dump_addr()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_dump_addr() dumps __be32 array without conversion to CPU byte order
in advance, that will reduce log readability for little endian CPUs.

Fix by be32_to_cpu() conversion before dump.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/fdt_address.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt_address.c b/drivers/of/fdt_address.c
index 9804d7f067056a9437d37f0b4561d79dedcc3187..1e5311f6f1858b59b99f650bcafa55a8d11225f9 100644
--- a/drivers/of/fdt_address.c
+++ b/drivers/of/fdt_address.c
@@ -28,7 +28,7 @@ static void __init of_dump_addr(const char *s, const __be32 *addr, int na)
 {
 	pr_debug("%s", s);
 	while(na--)
-		pr_cont(" %08x", *(addr++));
+		pr_cont(" %08x", be32_to_cpu(*(addr++)));
 	pr_cont("\n");
 }
 #else

-- 
2.34.1

[PATCH 07/10] of: Correct comments for of_alias_scan()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Correct of_alias_scan() comments by:

- Replace /* with /** to start comments since it is not a API.
- Delete return value descriptions since it is a void function.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1c62cda4ebcd9e3dc5f91d10fa68f975226693dd..33abb6227468c03fd191201aa2bbe05a41fdd9f4 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1812,14 +1812,13 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 		 ap->alias, ap->stem, ap->id, np);
 }
 
-/**
+/*
  * of_alias_scan - Scan all properties of the 'aliases' node
  * @dt_alloc:	An allocator that provides a virtual address to memory
  *		for storing the resulting tree
  *
  * The function scans all the properties of the 'aliases' node and populates
- * the global lookup table with the properties.  It returns the
- * number of alias properties found, or an error code in case of failure.
+ * the global lookup table with the properties.
  */
 void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 {

-- 
2.34.1

[PATCH 08/10] of: Swap implementation between of_property_present() and of_property_read_bool()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_property_present() is exactly same as of_property_read_bool().

Swap their implementation to make them more logical as shown below:
find property -> property is present or not -> boolean property value.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 include/linux/of.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index f921786cb8ac782286ed5ff4425a35668204d050..7b0da8d352dffd8b872998903282119b6164a5bc 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1243,17 +1243,16 @@ static inline int of_property_read_string_index(const struct device_node *np,
 }
 
 /**
- * of_property_read_bool - Find a property
- * @np:		device node from which the property value is to be read.
+ * of_property_present - Test if a property is present in a node
+ * @np:		device node to search for the property.
  * @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.
+ * Test for a property present in a device node.
  *
  * Return: true if the property exists false otherwise.
  */
-static inline bool of_property_read_bool(const struct device_node *np,
-					 const char *propname)
+static inline bool of_property_present(const struct device_node *np,
+				       const char *propname)
 {
 	const struct property *prop = of_find_property(np, propname, NULL);
 
@@ -1261,17 +1260,19 @@ static inline bool of_property_read_bool(const struct device_node *np,
 }
 
 /**
- * of_property_present - Test if a property is present in a node
- * @np:		device node to search for the property.
+ * 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.
  *
- * Test for a property present in a device node.
+ * 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_present(const struct device_node *np, const char *propname)
+static inline bool of_property_read_bool(const struct device_node *np,
+					 const char *propname)
 {
-	return of_property_read_bool(np, propname);
+	return of_property_present(np, propname);
 }
 
 /**

-- 
2.34.1

[PATCH 09/10] of: property: Implement of_fwnode_property_present() by of_property_present()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_property_read_bool() is deprecated for non-boolean property, but
of_fwnode_property_present() still uses it.

Fix by using of_property_present() instead of of_property_read_bool().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 519bf9229e613906547b57d8c68e7b8558eff327..dca1a3ebccae1093b2b11f51e8e692bca854d0e3 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -966,7 +966,7 @@ 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_read_bool(to_of_node(fwnode), propname);
+	return of_property_present(to_of_node(fwnode), propname);
 }
 
 static int of_fwnode_property_read_int_array(const struct fwnode_handle *fwnode,

-- 
2.34.1

[PATCH 10/10] of: Simplify API of_find_node_with_property() implementation

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Simplify of_find_node_with_property() implementation
by __of_find_property().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 33abb6227468c03fd191201aa2bbe05a41fdd9f4..5fc061b66116d106c398f04a1d9d235f09741de7 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1026,19 +1026,15 @@ struct device_node *of_find_node_with_property(struct device_node *from,
 	const char *prop_name)
 {
 	struct device_node *np;
-	const struct property *pp;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	for_each_of_allnodes_from(from, np) {
-		for (pp = np->properties; pp; pp = pp->next) {
-			if (of_prop_cmp(pp->name, prop_name) == 0) {
-				of_node_get(np);
-				goto out;
-			}
+		if (__of_find_property(np, prop_name, NULL)) {
+			of_node_get(np);
+			break;
 		}
 	}
-out:
 	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;

-- 
2.34.1

Re: [PATCH 07/10] of: Correct comments for of_alias_scan()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> Correct of_alias_scan() comments by:
>
> - Replace /* with /** to start comments since it is not a API.

But it is because it's in of.h. However, you are correct in that it
has no external callers. So please move the declaration to
of_private.h.

> - Delete return value descriptions since it is a void function.
>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 1c62cda4ebcd9e3dc5f91d10fa68f975226693dd..33abb6227468c03fd191201aa2bbe05a41fdd9f4 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1812,14 +1812,13 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>                  ap->alias, ap->stem, ap->id, np);
>  }
>
> -/**
> +/*
>   * of_alias_scan - Scan all properties of the 'aliases' node
>   * @dt_alloc:  An allocator that provides a virtual address to memory
>   *             for storing the resulting tree
>   *
>   * The function scans all the properties of the 'aliases' node and populates
> - * the global lookup table with the properties.  It returns the
> - * number of alias properties found, or an error code in case of failure.
> + * the global lookup table with the properties.
>   */
>  void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>  {
>
> --
> 2.34.1
>
>

Re: [PATCH 01/10] of: Fix alias name length calculating error in API of_find_node_opts_by_path()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> Alias name length calculated by of_find_node_opts_by_path() is wrong as
> explained below:
>
> Take "alias/serial@llc500:115200n8" as its @patch argument for an example
>       ^    ^             ^
>       0    5             19
>
> The right length of alias 'alias' is 5, but the API results in 19 which is
> obvious wrong.
>
> The wrong length will cause finding device node failure for such paths.
> Fix by using index of either '/' or ':' as the length who comes earlier.

Can you add a test case in the unittest for this.

>
> Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7dc394255a0a14cd1aed02ec79c2f787a222b44c..9a9313183d1f1b61918fe7e6fa80c2726b099a1c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -893,10 +893,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>         /* The path could begin with an alias */
>         if (*path != '/') {
>                 int len;
> -               const char *p = separator;
> +               const char *p = strchrnul(path, '/');
>
> -               if (!p)
> -                       p = strchrnul(path, '/');
> +               if (separator && separator < p)
> +                       p = separator;
>                 len = p - path;
>
>                 /* of_aliases must not be NULL */
>
> --
> 2.34.1
>

Re: [PATCH 02/10] of: Correct return value for API of_parse_phandle_with_args_map()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> @ret is used by of_parse_phandle_with_args_map() to record return value
> and it is preseted with -EINVAL before the outer while loop, but it is
> changed to 0 by below successful operation within the inner loop:
> of_property_read_u32(new, cells_name, &new_size)
>
> So cause 0(success) is returned for all failures which happen after the
> operation, that is obviously wrong.
>
> Fix by restoring @ret with preseted -EINVAL after the operation.

Already have a similar fix queued up.

Rob

Re: [PATCH 01/10] of: Fix alias name length calculating error in API of_find_node_opts_by_path()

[Zijun Hu]

On 2024/12/9 21:24, Rob Herring wrote:
> On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Alias name length calculated by of_find_node_opts_by_path() is wrong as
>> explained below:
>>
>> Take "alias/serial@llc500:115200n8" as its @patch argument for an example
>>       ^    ^             ^
>>       0    5             19
>>
>> The right length of alias 'alias' is 5, but the API results in 19 which is
>> obvious wrong.
>>
>> The wrong length will cause finding device node failure for such paths.
>> Fix by using index of either '/' or ':' as the length who comes earlier.
> 
> Can you add a test case in the unittest for this.

sure. let me try to do it.

> 
>>
>> Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/of/base.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 7dc394255a0a14cd1aed02ec79c2f787a222b44c..9a9313183d1f1b61918fe7e6fa80c2726b099a1c 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -893,10 +893,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>>         /* The path could begin with an alias */
>>         if (*path != '/') {
>>                 int len;
>> -               const char *p = separator;
>> +               const char *p = strchrnul(path, '/');
>>
>> -               if (!p)
>> -                       p = strchrnul(path, '/');
>> +               if (separator && separator < p)
>> +                       p = separator;
>>                 len = p - path;
>>
>>                 /* of_aliases must not be NULL */
>>
>> --
>> 2.34.1
>>

Re: [PATCH 07/10] of: Correct comments for of_alias_scan()

[Zijun Hu]

On 2024/12/9 21:20, Rob Herring wrote:
>> - Replace /* with /** to start comments since it is not a API.
> But it is because it's in of.h. However, you are correct in that it
> has no external callers. So please move the declaration to
> of_private.h.
> 

thank you Rob for code review.
let me do it in next revision.

>> - Delete return value descriptions since it is a void function.

Re: [PATCH 09/10] of: property: Implement of_fwnode_property_present() by of_property_present()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:54 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> of_property_read_bool() is deprecated for non-boolean property, but
> of_fwnode_property_present() still uses it.
>
> Fix by using of_property_present() instead of of_property_read_bool().

of_property_present() just calls of_property_read_bool(). For now. I'm
working on making using of_property_read_bool() on non-boolean a
warning. No point in this change until that happens.

Rob

Re: [PATCH 04/10] of: Fix refcount leakage for OF node returned by __of_get_dma_parent()

[Rob Herring]

On Fri, Dec 06, 2024 at 08:52:30AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> __of_get_dma_parent() returns OF device node @args.np, but the node's
> refcount is increased twice, by both of_parse_phandle_with_args() and
> of_node_get(), so causes refcount leakage for the node.
> 
> Fix by directly returning the node got by of_parse_phandle_with_args().
> 
> Fixes: f83a6e5dea6c ("of: address: Add support for the parent DMA bus")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Rob

Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> of_device_uevent_modalias() saves MODALIAS value from offset
> (@env->buflen - 1), so the available buffer size should be
> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong
> size (sizeof(@env->buf) - @env->buflen).
>
> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1).

Just writing what the diff says already is not that useful. The key
part you need to know is why we back up by 1 character to begin with.

>
> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
>                 return -ENOMEM;
>
>         sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],

This could use a comment why we back up by 1. Better to put it in a
variable than add/subtract 1 everywhere:

/* After add_uevent_event(), buflen is at character after the nul char
which needs to be overwritten */
buflen = env->buflen - 1;

And then use 'buflen' throughout.

> -                        sizeof(env->buf) - env->buflen);
> +                        sizeof(env->buf) - env->buflen + 1);
>         if (sl < 0)
>                 return sl;
> -       if (sl >= (sizeof(env->buf) - env->buflen))
> +       if (sl >= (sizeof(env->buf) - env->buflen + 1))
>                 return -ENOMEM;

There's another potential problem. If we return before here, we end up
with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str".
Maybe that doesn't matter? I haven't looked at the caller.

I think a better solution to all this would be making add_uevent_var
work to construct the full value. We could add "%pOFm" format that
calls of_mod_alias(). Then this function becomes just:

return add_uevent_var(env, "MODALIAS=%pOFm");

And of_device_modalias() can be:

sl = snprintf(str, len, "%pOFm\n", dev->of_node);
if (sl >= len)
  return -ENOMEM;
return sl;

Rob

Re: [PATCH 06/10] of/fdt: Dump __be32 array in CPU type order in of_dump_addr()

[Rob Herring]

On Fri, Dec 06, 2024 at 08:52:32AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> of_dump_addr() dumps __be32 array without conversion to CPU byte order
> in advance, that will reduce log readability for little endian CPUs.
> 
> Fix by be32_to_cpu() conversion before dump.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/fdt_address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Rob

Re: [PATCH 10/10] of: Simplify API of_find_node_with_property() implementation

[Rob Herring]

On Fri, Dec 06, 2024 at 08:52:36AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Simplify of_find_node_with_property() implementation
> by __of_find_property().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Applied, thanks.

Rob

Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Zijun Hu]

On 2024/12/10 04:34, Rob Herring wrote:
> On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> of_device_uevent_modalias() saves MODALIAS value from offset
>> (@env->buflen - 1), so the available buffer size should be
>> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong
>> size (sizeof(@env->buf) - @env->buflen).
>>
>> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1).
> 
> Just writing what the diff says already is not that useful. The key
> part you need to know is why we back up by 1 character to begin with.
> 

will correct commit message in v2.

>>
>> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/of/device.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
>>                 return -ENOMEM;
>>
>>         sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
> 
> This could use a comment why we back up by 1. Better to put it in a
> variable than add/subtract 1 everywhere:
> 
> /* After add_uevent_event(), buflen is at character after the nul char
> which needs to be overwritten */
> buflen = env->buflen - 1;
> 
> And then use 'buflen' throughout.
> 

good proposal. may use it for v2 after discussion done.

>> -                        sizeof(env->buf) - env->buflen);
>> +                        sizeof(env->buf) - env->buflen + 1);
>>         if (sl < 0)
>>                 return sl;
>> -       if (sl >= (sizeof(env->buf) - env->buflen))
>> +       if (sl >= (sizeof(env->buf) - env->buflen + 1))
>>                 return -ENOMEM;
> 
> There's another potential problem. If we return before here, we end up
> with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str".
> Maybe that doesn't matter? I haven't looked at the caller.
> 

that does not matter since current logic follows below 2 rules

1) all strings in @env->buf always terminated with '\0'.
2) both env->buflen and env->envp_idx are not updated once @env->buf
does not enough spaces then failed.

current logic has no difference with normal add_uevent_var() usage.

> I think a better solution to all this would be making add_uevent_var
> work to construct the full value. We could add "%pOFm" format that
> calls of_mod_alias(). Then this function becomes just:
> 
of_modalias() ?

agree with you.
for good practice, users should only use uevent APIs and should not
depend on  uevent's internal implementation.

> return add_uevent_var(env, "MODALIAS=%pOFm");
> 
> And of_device_modalias() can be:
> 
> sl = snprintf(str, len, "%pOFm\n", dev->of_node);
> if (sl >= len)
>   return -ENOMEM;
> return sl;
> 

looks good.

of_request_module() provides another solution.

> Rob

Re: [PATCH 09/10] of: property: Implement of_fwnode_property_present() by of_property_present()

[Zijun Hu]

On 2024/12/10 00:48, Rob Herring wrote:
> On Thu, Dec 5, 2024 at 6:54 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> of_property_read_bool() is deprecated for non-boolean property, but
>> of_fwnode_property_present() still uses it.
>>
>> Fix by using of_property_present() instead of of_property_read_bool().
> 
> of_property_present() just calls of_property_read_bool(). For now. I'm
> working on making using of_property_read_bool() on non-boolean a
> warning. No point in this change until that happens.
> 

what about below idea?

replace all of_property_read_bool() usages with of_property_present()
then remove the former.

> Rob

Re: [PATCH 09/10] of: property: Implement of_fwnode_property_present() by of_property_present()

[Robin Murphy]

On 2024-12-10 12:44 pm, Zijun Hu wrote:
> On 2024/12/10 00:48, Rob Herring wrote:
>> On Thu, Dec 5, 2024 at 6:54 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> of_property_read_bool() is deprecated for non-boolean property, but
>>> of_fwnode_property_present() still uses it.
>>>
>>> Fix by using of_property_present() instead of of_property_read_bool().
>>
>> of_property_present() just calls of_property_read_bool(). For now. I'm
>> working on making using of_property_read_bool() on non-boolean a
>> warning. No point in this change until that happens.
>>
> 
> what about below idea?
> 
> replace all of_property_read_bool() usages with of_property_present()
> then remove the former.

No, the whole reason of_property_present() was added in the first place 
is because reading the effective "value" of a boolean property is a 
semantically different operation from checking whether a property of any 
type exists. Therefore (ab)using a single function for both purposes, 
whichever way round, is not an ideal API design. The fact that they both 
happen to share the same implementation at the moment is, as Rob says, 
not something we want to be tied to forever.

Thanks,
Robin.

Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Rob Herring]

On Tue, Dec 10, 2024 at 6:39 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/12/10 04:34, Rob Herring wrote:
> > On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> of_device_uevent_modalias() saves MODALIAS value from offset
> >> (@env->buflen - 1), so the available buffer size should be
> >> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong
> >> size (sizeof(@env->buf) - @env->buflen).
> >>
> >> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1).
> >
> > Just writing what the diff says already is not that useful. The key
> > part you need to know is why we back up by 1 character to begin with.
> >
>
> will correct commit message in v2.
>
> >>
> >> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  drivers/of/device.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
> >>                 return -ENOMEM;
> >>
> >>         sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
> >
> > This could use a comment why we back up by 1. Better to put it in a
> > variable than add/subtract 1 everywhere:
> >
> > /* After add_uevent_event(), buflen is at character after the nul char
> > which needs to be overwritten */
> > buflen = env->buflen - 1;
> >
> > And then use 'buflen' throughout.
> >
>
> good proposal. may use it for v2 after discussion done.
>
> >> -                        sizeof(env->buf) - env->buflen);
> >> +                        sizeof(env->buf) - env->buflen + 1);
> >>         if (sl < 0)
> >>                 return sl;
> >> -       if (sl >= (sizeof(env->buf) - env->buflen))
> >> +       if (sl >= (sizeof(env->buf) - env->buflen + 1))
> >>                 return -ENOMEM;
> >
> > There's another potential problem. If we return before here, we end up
> > with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str".
> > Maybe that doesn't matter? I haven't looked at the caller.
> >
>
> that does not matter since current logic follows below 2 rules
>
> 1) all strings in @env->buf always terminated with '\0'.

Ah, right. However, we still end up with a truncated value though it
is nul terminated.

> 2) both env->buflen and env->envp_idx are not updated once @env->buf
> does not enough spaces then failed.
>
> current logic has no difference with normal add_uevent_var() usage.

There is one major difference. add_uevent_var() will not output
anything if the whole string doesn't fit. Whereas we might output a
truncated value because the add_uevent_var() call updated env->buflen
and env->envp_idx. We could unwind that I suppose, but that involves
even more mucking with the internals of the env struct.

> > I think a better solution to all this would be making add_uevent_var
> > work to construct the full value. We could add "%pOFm" format that
> > calls of_mod_alias(). Then this function becomes just:
> >
> of_modalias() ?

Yes.

>
> agree with you.
> for good practice, users should only use uevent APIs and should not
> depend on  uevent's internal implementation.
>
> > return add_uevent_var(env, "MODALIAS=%pOFm");
> >
> > And of_device_modalias() can be:
> >
> > sl = snprintf(str, len, "%pOFm\n", dev->of_node);
> > if (sl >= len)
> >   return -ENOMEM;
> > return sl;
> >
>
> looks good.
>
> of_request_module() provides another solution.

Yes, that can be simplified to just a kasnprintf() call.

Rob

Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Zijun Hu]

On 2024/12/10 22:10, Rob Herring wrote:
> Ah, right. However, we still end up with a truncated value though it
> is nul terminated.
> 
>> 2) both env->buflen and env->envp_idx are not updated once @env->buf
>> does not enough spaces then failed.
>>
>> current logic has no difference with normal add_uevent_var() usage.
> There is one major difference. add_uevent_var() will not output
> anything if the whole string doesn't fit. Whereas we might output a
> truncated value because the add_uevent_var() call updated env->buflen
> and env->envp_idx. We could unwind that I suppose, but that involves
> even more mucking with the internals of the env struct.

you are right.
i would like to try to solve involved issue in next revision. (^^)