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

[PATCH v4 00/14] 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>
---
Changes in v4:
- Remove 2 modalias relevant patches, and add more patches.
- Link to v3: https://lore.kernel.org/r/20241217-of_core_fix-v3-0-3bc49a2e8bda@quicinc.com

Changes in v3:
- Drop 2 applied patches and pick up patch 4/7 again
- Fix build error for patch 6/7.
- Include of_private.h instead of function declaration for patch 2/7
- Correct tile and commit messages.
- Link to v2: https://lore.kernel.org/r/20241216-of_core_fix-v2-0-e69b8f60da63@quicinc.com

Changes in v2:
- Drop applied/conflict/TBD patches.
- Correct based on Rob's comments.
- Link to v1: https://lore.kernel.org/r/20241206-of_core_fix-v1-0-dc28ed56bec3@quicinc.com

---
Zijun Hu (14):
      of: Correct child specifier used as input of the 2nd nexus node
      of: Do not expose of_alias_scan() and correct its comments
      of: Make of_property_present() applicable to all kinds of property
      of: property: Use of_property_present() for of_fwnode_property_present()
      of: Fix available buffer size calculating error in API of_device_uevent_modalias()
      of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()
      of: property: Fix potential fwnode reference's argument count got out of range
      of: Remove a duplicated code block
      of: reserved-memory: Fix using wrong number of cells to get property 'alignment'
      of: reserved-memory: Do not make kmemleak ignore freed address
      of: reserved-memory: Warn for missing static reserved memory regions
      of: reserved-memory: Move an assignment to effective place in __reserved_mem_alloc_size()
      of/fdt: Check fdt_get_mem_rsv() error in early_init_fdt_scan_reserved_mem()
      of: Improve __of_add_property_sysfs() readability

 drivers/of/address.c         | 21 +++------------------
 drivers/of/base.c            |  7 +++----
 drivers/of/device.c          | 14 ++++++++++----
 drivers/of/fdt.c             |  7 ++++++-
 drivers/of/fdt_address.c     | 21 ++++-----------------
 drivers/of/kobj.c            |  3 ++-
 drivers/of/of_private.h      | 20 ++++++++++++++++++++
 drivers/of/of_reserved_mem.c | 15 ++++++++++-----
 drivers/of/pdt.c             |  2 ++
 drivers/of/property.c        |  9 +++++++--
 include/linux/of.h           | 24 ++++++++++++------------
 11 files changed, 79 insertions(+), 64 deletions(-)
---
base-commit: 456f3000f82571697d23c255c451cfcfb5c9ae75
change-id: 20241206-of_core_fix-dc3021a06418

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

[PATCH v4 01/14] 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 bf18d5997770eb81e47e749198dd505a35203d10..969b99838655534915882abe358814d505c6f748 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1536,7 +1536,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);
 
@@ -1545,6 +1544,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

Re: [PATCH v4 01/14] of: Correct child specifier used as input of the 2nd nexus node

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:26:52 +0800, Zijun Hu wrote:
> 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(-)
> 

Applied, thanks!

[PATCH v4 02/14] of: Do not expose of_alias_scan() and correct its comments

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For of_alias_scan():
- Do not expose it since it has no external callers.
- Correct its comments shown below:
  1) Replace /* with /** to start comments since it is not a API.
  2) Delete return value descriptions since it is a void function.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c       | 5 ++---
 drivers/of/of_private.h | 2 ++
 drivers/of/pdt.c        | 2 ++
 include/linux/of.h      | 1 -
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 969b99838655534915882abe358814d505c6f748..5485307e2a3a3d3a216d271c60bdfc346dd38460 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1806,14 +1806,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))
 {
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index ea5a0951ec5e107bab265ab5f6c043e2bfb15ecc..3433ccd330e84fd3a4b54638e0e922069757c8f0 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -119,6 +119,8 @@ extern void *__unflatten_device_tree(const void *blob,
 			      void *(*dt_alloc)(u64 size, u64 align),
 			      bool detached);
 
+void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
+
 /**
  * General utilities for working with live trees.
  *
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 7eda43c66c916198b1c2d8fc5043fcb1edaede7a..cb0cb374b21ff89323e11f34bd767b183e7a401e 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -19,6 +19,8 @@
 #include <linux/of.h>
 #include <linux/of_pdt.h>
 
+#include "of_private.h"
+
 static struct of_pdt_ops *of_pdt_prom_ops __initdata;
 
 #if defined(CONFIG_SPARC)
diff --git a/include/linux/of.h b/include/linux/of.h
index f921786cb8ac782286ed5ff4425a35668204d050..d451c46132b01efe6d4e0b6cf83a3e2084bb3ec6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -397,7 +397,6 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
 				    uint32_t *args,
 				    int size);
 
-extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(const struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
 

-- 
2.34.1

Re: [PATCH v4 02/14] of: Do not expose of_alias_scan() and correct its comments

[Rob Herring]

On Thu, Jan 09, 2025 at 09:26:53PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For of_alias_scan():
> - Do not expose it since it has no external callers.
> - Correct its comments shown below:
>   1) Replace /* with /** to start comments since it is not a API.

You've got that backwards. However, in other places we leave this. I 
prefer if the comment is in kerneldoc format, then it should have '/**' 
to indicate that and so that it is checked by the tools.

>   2) Delete return value descriptions since it is a void function.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c       | 5 ++---
>  drivers/of/of_private.h | 2 ++
>  drivers/of/pdt.c        | 2 ++
>  include/linux/of.h      | 1 -
>  4 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 969b99838655534915882abe358814d505c6f748..5485307e2a3a3d3a216d271c60bdfc346dd38460 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1806,14 +1806,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))
>  {
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index ea5a0951ec5e107bab265ab5f6c043e2bfb15ecc..3433ccd330e84fd3a4b54638e0e922069757c8f0 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -119,6 +119,8 @@ extern void *__unflatten_device_tree(const void *blob,
>  			      void *(*dt_alloc)(u64 size, u64 align),
>  			      bool detached);
>  
> +void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
> +
>  /**
>   * General utilities for working with live trees.
>   *
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 7eda43c66c916198b1c2d8fc5043fcb1edaede7a..cb0cb374b21ff89323e11f34bd767b183e7a401e 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -19,6 +19,8 @@
>  #include <linux/of.h>
>  #include <linux/of_pdt.h>
>  
> +#include "of_private.h"
> +
>  static struct of_pdt_ops *of_pdt_prom_ops __initdata;
>  
>  #if defined(CONFIG_SPARC)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f921786cb8ac782286ed5ff4425a35668204d050..d451c46132b01efe6d4e0b6cf83a3e2084bb3ec6 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -397,7 +397,6 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
>  				    uint32_t *args,
>  				    int size);
>  
> -extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
>  extern int of_alias_get_id(const struct device_node *np, const char *stem);
>  extern int of_alias_get_highest_id(const char *stem);
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 02/14] of: Do not expose of_alias_scan() and correct its comments

[Zijun Hu]

On 2025/1/11 01:43, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:26:53PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For of_alias_scan():
>> - Do not expose it since it has no external callers.
>> - Correct its comments shown below:
>>   1) Replace /* with /** to start comments since it is not a API.
> You've got that backwards. However, in other places we leave this. I 
> prefer if the comment is in kerneldoc format, then it should have '/**' 
> to indicate that and so that it is checked by the tools.
> 

okay, let me cancel this change in v5.

>>   2) Delete return value descriptions since it is a void function.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/of/base.c       | 5 ++---
>>  drivers/of/of_private.h | 2 ++
>>  drivers/of/pdt.c        | 2 ++
>>  include/linux/of.h      | 1 -
>>  4 files changed, 6 insertions(+), 4 deletions(-)

[PATCH v4 03/14] of: Make of_property_present() applicable to all kinds of property

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

API of_property_present() invokes of_property_read_bool() to check if
a property is present or not, and that has 2 shortcomings shown below:

- That narrows down property scope applicable to of_property_present()
  from all kinds of property to only bool type.

- That is less logical since it says a property's presence is decided by
  its bool property value.

Fix by making of_property_read_bool() invoke of_property_present().

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 d451c46132b01efe6d4e0b6cf83a3e2084bb3ec6..fe5d7b74c23b9701743f5debc3d030b765bc914f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1242,17 +1242,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);
 
@@ -1260,17 +1259,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

Re: [PATCH v4 03/14] of: Make of_property_present() applicable to all kinds of property

[Rob Herring]

On Thu, Jan 09, 2025 at 09:26:54PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> API of_property_present() invokes of_property_read_bool() to check if
> a property is present or not, and that has 2 shortcomings shown below:
> 
> - That narrows down property scope applicable to of_property_present()
>   from all kinds of property to only bool type.
> 
> - That is less logical since it says a property's presence is decided by
>   its bool property value.
> 
> Fix by making of_property_read_bool() invoke of_property_present().

I already indicated I wasn't interested in taking this until we have 
reason to split the behavior. I've sent out a series on that now[1].

Rob

[1] https://lore.kernel.org/all/20250109-dt-type-warnings-v1-0-0150e32e716c@kernel.org/

Re: [PATCH v4 03/14] of: Make of_property_present() applicable to all kinds of property

[Zijun Hu]

On 2025/1/11 01:45, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:26:54PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> API of_property_present() invokes of_property_read_bool() to check if
>> a property is present or not, and that has 2 shortcomings shown below:
>>
>> - That narrows down property scope applicable to of_property_present()
>>   from all kinds of property to only bool type.
>>
>> - That is less logical since it says a property's presence is decided by
>>   its bool property value.
>>
>> Fix by making of_property_read_bool() invoke of_property_present().
> 
> I already indicated I wasn't interested in taking this until we have 
> reason to split the behavior. I've sent out a series on that now[1].
> 

the patch you indicate is patch [4/14] as below, not this patch [3/14]
https://lore.kernel.org/all/CAL_JsqJvh5pddoVEgaKQvGth0ncgtC9AAGxMEiK__NiZKrjmxA@mail.gmail.com/

the actual change of this patch is only to swap function name between
of_property_present() and of_property_read_bool() even if it seems this
patch has many changed lines.

okay, let me drop this patch in v5. (^^)

> Rob
> 
> [1] https://lore.kernel.org/all/20250109-dt-type-warnings-v1-0-0150e32e716c@kernel.org/

[PATCH v4 04/14] of: property: Use of_property_present() for of_fwnode_property_present()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Use of_property_present() instead of of_property_read_bool() for
of_fwnode_property_present() since the former is more applicable
obviously.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

---
Hi Rob,

i pick up this change again after some considerations as below:

1) of_property_present() is more suitable than of_property_read_bool()
   here, deprecated API is not main reason.

2) it does not conflict with your job which warns when use
   of_property_read_bool() for non-bool property.
---
 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 v4 05/14] 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 size of space from char '\0' inclusive which ends "MODALIAS=".

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

diff --git a/drivers/of/device.c b/drivers/of/device.c
index edf3be1972658f6dc165f577da53b10c7eebc116..f24c19e7aba8e01656f503ae328a4e08aab5a5f3 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -257,6 +257,7 @@ EXPORT_SYMBOL_GPL(of_device_uevent);
 int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env)
 {
 	int sl;
+	int pos;
 
 	if ((!dev) || (!dev->of_node) || dev->of_node_reused)
 		return -ENODEV;
@@ -265,13 +266,18 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
 
-	sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
-			 sizeof(env->buf) - env->buflen);
+	/*
+	 * @env->buflen is pointing to the char after '\0' now
+	 * override the '\0' to save MODALIAS value.
+	 */
+	pos = env->buflen - 1;
+	sl = of_modalias(dev->of_node, &env->buf[pos],
+			 sizeof(env->buf) - pos);
 	if (sl < 0)
 		return sl;
-	if (sl >= (sizeof(env->buf) - env->buflen))
+	if (sl >= (sizeof(env->buf) - pos))
 		return -ENOMEM;
-	env->buflen += sl;
+	env->buflen = pos + sl + 1;
 
 	return 0;
 }

-- 
2.34.1

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

[Rob Herring]

On Thu, Jan 09, 2025 at 09:26:56PM +0800, Zijun Hu 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 size of space from char '\0' inclusive which ends "MODALIAS=".

I prefer to get the printf specifier change merged rather than reviewing 
if this is correct and doesn't introduce any new bugs. We're under 
utilizing the buffer by 1 byte. I doubt anyone will ever hit that and 
it's not any worse than if they exceed the correct size of the buffer.

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

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

[Zijun Hu]

On 2025/1/11 01:48, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:26:56PM +0800, Zijun Hu 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 size of space from char '\0' inclusive which ends "MODALIAS=".
> 
> I prefer to get the printf specifier change merged rather than reviewing 
> if this is correct and doesn't introduce any new bugs. We're under 
> utilizing the buffer by 1 byte. I doubt anyone will ever hit that and 
> it's not any worse than if they exceed the correct size of the buffer.
> 

got it. you are right.

previous series v3 contains 3 patches related to MODALIAS. and i
keep this patch and drop other 2 patches in this v4 series.

my thoughts about keep this one is shown below:

1) this simple patch may record evolution history of the function.
2) you ever given suggestions about this change.
3) the issue printf specifier solution fix derives from discussion of
this change.

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

[PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

parse_interrupt_map() will use uninitialized variable @imaplen if fails
to get property 'interrupt-map'.

Fix by using the variable after successfully getting the property.

Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
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 dca1a3ebccae1093b2b11f51e8e692bca854d0e3..6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1391,9 +1391,9 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
 	addrcells = of_bus_n_addr_cells(np);
 
 	imap = of_get_property(np, "interrupt-map", &imaplen);
-	imaplen /= sizeof(*imap);
 	if (!imap)
 		return NULL;
+	imaplen /= sizeof(*imap);
 
 	imap_end = imap + imaplen;
 

-- 
2.34.1

Re: [PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Rob Herring]

On Thu, Jan 09, 2025 at 09:26:57PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> parse_interrupt_map() will use uninitialized variable @imaplen if fails
> to get property 'interrupt-map'.
> 
> Fix by using the variable after successfully getting the property.
> 
> Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
> 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 dca1a3ebccae1093b2b11f51e8e692bca854d0e3..6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1391,9 +1391,9 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
>  	addrcells = of_bus_n_addr_cells(np);
>  
>  	imap = of_get_property(np, "interrupt-map", &imaplen);
> -	imaplen /= sizeof(*imap);
>  	if (!imap)
>  		return NULL;
> +	imaplen /= sizeof(*imap);

sizeof() is a compile time constant, there's not an actual dereference 
here.

>  
>  	imap_end = imap + imaplen;
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Zijun Hu]

On 2025/1/11 04:26, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:26:57PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> parse_interrupt_map() will use uninitialized variable @imaplen if fails
>> to get property 'interrupt-map'.
>>
>> Fix by using the variable after successfully getting the property.
>>
>> Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
>> 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 dca1a3ebccae1093b2b11f51e8e692bca854d0e3..6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6 100644
>> --- a/drivers/of/property.c
>> +++ b/drivers/of/property.c
>> @@ -1391,9 +1391,9 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
>>  	addrcells = of_bus_n_addr_cells(np);
>>  
>>  	imap = of_get_property(np, "interrupt-map", &imaplen);
>> -	imaplen /= sizeof(*imap);
>>  	if (!imap)
>>  		return NULL;
>> +	imaplen /= sizeof(*imap);
> 
> sizeof() is a compile time constant, there's not an actual dereference 
> here.
> 

the uninitialized variable is @imaplen, and not sizeof(*imap).

>>  
>>  	imap_end = imap + imaplen;
>>  
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Krzysztof Kozlowski]

On Sat, Jan 11, 2025 at 06:34:21AM +0800, Zijun Hu wrote:
> On 2025/1/11 04:26, Rob Herring wrote:
> > On Thu, Jan 09, 2025 at 09:26:57PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> parse_interrupt_map() will use uninitialized variable @imaplen if fails
> >> to get property 'interrupt-map'.
> >>
> >> Fix by using the variable after successfully getting the property.
> >>
> >> Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
> >> 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 dca1a3ebccae1093b2b11f51e8e692bca854d0e3..6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6 100644
> >> --- a/drivers/of/property.c
> >> +++ b/drivers/of/property.c
> >> @@ -1391,9 +1391,9 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
> >>  	addrcells = of_bus_n_addr_cells(np);
> >>  
> >>  	imap = of_get_property(np, "interrupt-map", &imaplen);
> >> -	imaplen /= sizeof(*imap);
> >>  	if (!imap)
> >>  		return NULL;
> >> +	imaplen /= sizeof(*imap);
> > 
> > sizeof() is a compile time constant, there's not an actual dereference 
> > here.
> > 
> 
> the uninitialized variable is @imaplen, and not sizeof(*imap).

Correct. I think error is harmless, because whatever stack/random value
of imaplen we use in 'imaplen / =sizeof', we immediately return.

Anyway, for code correctness and silencing whatever warnings there are
(if there are you should actually paste them in commit msg):

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

Best regards,
Krzysztof

Re: [PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Zijun Hu]

On 2025/1/11 17:01, Krzysztof Kozlowski wrote:
> On Sat, Jan 11, 2025 at 06:34:21AM +0800, Zijun Hu wrote:
>> On 2025/1/11 04:26, Rob Herring wrote:
>>> On Thu, Jan 09, 2025 at 09:26:57PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> parse_interrupt_map() will use uninitialized variable @imaplen if fails
>>>> to get property 'interrupt-map'.
>>>>
>>>> Fix by using the variable after successfully getting the property.
>>>>
>>>> Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
>>>> 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 dca1a3ebccae1093b2b11f51e8e692bca854d0e3..6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6 100644
>>>> --- a/drivers/of/property.c
>>>> +++ b/drivers/of/property.c
>>>> @@ -1391,9 +1391,9 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
>>>>  	addrcells = of_bus_n_addr_cells(np);
>>>>  
>>>>  	imap = of_get_property(np, "interrupt-map", &imaplen);
>>>> -	imaplen /= sizeof(*imap);
>>>>  	if (!imap)
>>>>  		return NULL;
>>>> +	imaplen /= sizeof(*imap);
>>>
>>> sizeof() is a compile time constant, there's not an actual dereference 
>>> here.
>>>
>>
>> the uninitialized variable is @imaplen, and not sizeof(*imap).
> 
> Correct. I think error is harmless, because whatever stack/random value
> of imaplen we use in 'imaplen / =sizeof', we immediately return.
> 
> Anyway, for code correctness and silencing whatever warnings there are
> (if there are you should actually paste them in commit msg):
> 

thank you Krzysztof Kozlowski for code review.

no warning messages since this issue is found by reading code.

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

Re: [PATCH v4 06/14] of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:26:57 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> parse_interrupt_map() will use uninitialized variable @imaplen if fails
> to get property 'interrupt-map'.
> 
> Fix by using the variable after successfully getting the property.
> 
> Fixes: e7985f43609c ("of: property: Fix fw_devlink handling of interrupt-map")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks!

[PATCH v4 07/14] of: property: Fix potential fwnode reference's argument count got out of range

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Currently, the maximal fwnode reference argument count supported is
8, and the maximal OF node phandle argument count supported is 16, but
of_fwnode_get_reference_args() directly assigns OF node phandle count
@of_args.args_count to fwnode reference count @args->nargs, so may cause
fwnode reference argument count got is out of range, namely, in [9, 16].

Fix by truncating @args->nargs got to 8 and warning if it > 8.

Fixes: b66548e2a9ba ("of: Increase MAX_PHANDLE_ARGS")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/property.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6..5ef9b2ced43ee7c4bfe88ea3cb11f3182da0dab9 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1072,6 +1072,11 @@ of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 	}
 
 	args->nargs = of_args.args_count;
+	if (args->nargs > NR_FWNODE_REFERENCE_ARGS) {
+		pr_warn("%s: Truncate arg count %d for property '%s' phandle index %d\n",
+			__func__, of_args.args_count, prop, index);
+		args->nargs = NR_FWNODE_REFERENCE_ARGS;
+	}
 	args->fwnode = of_fwnode_handle(of_args.np);
 
 	for (i = 0; i < NR_FWNODE_REFERENCE_ARGS; i++)

-- 
2.34.1

Re: [PATCH v4 07/14] of: property: Fix potential fwnode reference's argument count got out of range

[Rob Herring]

On Thu, Jan 09, 2025 at 09:26:58PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Currently, the maximal fwnode reference argument count supported is
> 8, and the maximal OF node phandle argument count supported is 16, but
> of_fwnode_get_reference_args() directly assigns OF node phandle count
> @of_args.args_count to fwnode reference count @args->nargs, so may cause
> fwnode reference argument count got is out of range, namely, in [9, 16].
> 
> Fix by truncating @args->nargs got to 8 and warning if it > 8.
> 
> Fixes: b66548e2a9ba ("of: Increase MAX_PHANDLE_ARGS")

No, it would have been 3e3119d3088f ("device property: Introduce 
fwnode_property_get_reference_args").

Why don't we increase NR_FWNODE_REFERENCE_ARGS or rework things such 
that MAX_PHANDLE_ARGS and NR_FWNODE_REFERENCE_ARGS can't disagree?

> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/property.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 6245cbff3527d762c16e7f4b7b7b3d4f2e9ddbe6..5ef9b2ced43ee7c4bfe88ea3cb11f3182da0dab9 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1072,6 +1072,11 @@ of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
>  	}
>  
>  	args->nargs = of_args.args_count;
> +	if (args->nargs > NR_FWNODE_REFERENCE_ARGS) {
> +		pr_warn("%s: Truncate arg count %d for property '%s' phandle index %d\n",
> +			__func__, of_args.args_count, prop, index);
> +		args->nargs = NR_FWNODE_REFERENCE_ARGS;
> +	}
>  	args->fwnode = of_fwnode_handle(of_args.np);
>  
>  	for (i = 0; i < NR_FWNODE_REFERENCE_ARGS; i++)
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 07/14] of: property: Fix potential fwnode reference's argument count got out of range

[Zijun Hu]

On 2025/1/11 04:35, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:26:58PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Currently, the maximal fwnode reference argument count supported is
>> 8, and the maximal OF node phandle argument count supported is 16, but
>> of_fwnode_get_reference_args() directly assigns OF node phandle count
>> @of_args.args_count to fwnode reference count @args->nargs, so may cause
>> fwnode reference argument count got is out of range, namely, in [9, 16].
>>
>> Fix by truncating @args->nargs got to 8 and warning if it > 8.
>>
>> Fixes: b66548e2a9ba ("of: Increase MAX_PHANDLE_ARGS")
> No, it would have been 3e3119d3088f ("device property: Introduce 
> fwnode_property_get_reference_args").
> 

agree.

> Why don't we increase NR_FWNODE_REFERENCE_ARGS or rework things such 
> that MAX_PHANDLE_ARGS and NR_FWNODE_REFERENCE_ARGS can't disagree?

that may involve ACPI and not sure if there are risk.

the commit you mentioned above have below commit message:

"The semantics is slightly different: the cells property is ignored on
ACPI as the number of arguments can be explicitly obtained from the
firmware interface"

let me do more investigation (^^).

perhaps, various firmware(DT|APCI|SWNODE...) may use
NR_FWNODE_REFERENCE_ARGS instead defining a MACRO such as
MAX_PHANDLE_ARGS separately.

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

[PATCH v4 08/14] of: Remove a duplicated code block

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

address.c has a same code block with fdt_address.c.

Remove a copy by moving the duplicated code block into of_private.h.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/address.c     | 21 +++------------------
 drivers/of/fdt_address.c | 21 ++++-----------------
 drivers/of/of_private.h  | 18 ++++++++++++++++++
 3 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index c5b925ac469f16b8ae4b8275b60210a2d583ff83..6c40f96a19610c19d9594a9605fd9a2fa2c97cd4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -16,25 +16,10 @@
 #include <linux/string.h>
 #include <linux/dma-direct.h> /* for bus_dma_region */
 
-#include "of_private.h"
-
-/* Max address size we deal with */
-#define OF_MAX_ADDR_CELLS	4
-#define OF_CHECK_ADDR_COUNT(na)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
-#define OF_CHECK_COUNTS(na, ns)	(OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
+/* Uncomment me to enable of_dump_addr() debugging output */
+// #define DEBUG
 
-/* Debug utility */
-#ifdef DEBUG
-static void of_dump_addr(const char *s, const __be32 *addr, int na)
-{
-	pr_debug("%s", s);
-	while (na--)
-		pr_cont(" %08x", be32_to_cpu(*(addr++)));
-	pr_cont("\n");
-}
-#else
-static void of_dump_addr(const char *s, const __be32 *addr, int na) { }
-#endif
+#include "of_private.h"
 
 /* Callbacks for bus specific translators */
 struct of_bus {
diff --git a/drivers/of/fdt_address.c b/drivers/of/fdt_address.c
index 1e5311f6f1858b59b99f650bcafa55a8d11225f9..f358d2c807540e79fdd72a7e9a5328bccd88476d 100644
--- a/drivers/of/fdt_address.c
+++ b/drivers/of/fdt_address.c
@@ -17,23 +17,10 @@
 #include <linux/of_fdt.h>
 #include <linux/sizes.h>
 
-/* Max address size we deal with */
-#define OF_MAX_ADDR_CELLS	4
-#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-			(ns) > 0)
-
-/* Debug utility */
-#ifdef DEBUG
-static void __init of_dump_addr(const char *s, const __be32 *addr, int na)
-{
-	pr_debug("%s", s);
-	while(na--)
-		pr_cont(" %08x", be32_to_cpu(*(addr++)));
-	pr_cont("\n");
-}
-#else
-static void __init of_dump_addr(const char *s, const __be32 *addr, int na) { }
-#endif
+/* Uncomment me to enable of_dump_addr() debugging output */
+// #define DEBUG
+
+#include "of_private.h"
 
 /* Callbacks for bus specific translators */
 struct of_bus {
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 3433ccd330e84fd3a4b54638e0e922069757c8f0..f3e1193c8ded4899f39677a76da073e2266a1b9a 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -190,4 +190,22 @@ void __init fdt_scan_reserved_mem_reg_nodes(void);
 
 bool of_fdt_device_is_available(const void *blob, unsigned long node);
 
+/* Max address size we deal with */
+#define OF_MAX_ADDR_CELLS	4
+#define OF_CHECK_ADDR_COUNT(na)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
+#define OF_CHECK_COUNTS(na, ns)	(OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
+
+/* Debug utility */
+#ifdef DEBUG
+static void __maybe_unused of_dump_addr(const char *s, const __be32 *addr, int na)
+{
+	pr_debug("%s", s);
+	while (na--)
+		pr_cont(" %08x", be32_to_cpu(*(addr++)));
+	pr_cont("\n");
+}
+#else
+static void __maybe_unused of_dump_addr(const char *s, const __be32 *addr, int na) { }
+#endif
+
 #endif /* _LINUX_OF_PRIVATE_H */

-- 
2.34.1

Re: [PATCH v4 08/14] of: Remove a duplicated code block

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:26:59 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> address.c has a same code block with fdt_address.c.
> 
> Remove a copy by moving the duplicated code block into of_private.h.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/address.c     | 21 +++------------------
>  drivers/of/fdt_address.c | 21 ++++-----------------
>  drivers/of/of_private.h  | 18 ++++++++++++++++++
>  3 files changed, 25 insertions(+), 35 deletions(-)
> 

Applied, thanks!

[PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

According to DT spec, size of property 'alignment' is based on parent
node’s #size-cells property.

But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
the property obviously.

Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.

Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/of_reserved_mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
 
 	prop = of_get_flat_dt_prop(node, "alignment", &len);
 	if (prop) {
-		if (len != dt_root_addr_cells * sizeof(__be32)) {
+		if (len != dt_root_size_cells * sizeof(__be32)) {
 			pr_err("invalid alignment property in '%s' node.\n",
 				uname);
 			return -EINVAL;
 		}
-		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
+		align = dt_mem_next_cell(dt_root_size_cells, &prop);
 	}
 
 	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;

-- 
2.34.1

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> According to DT spec, size of property 'alignment' is based on parent
> node’s #size-cells property.
> 
> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> the property obviously.
> 
> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.

I wonder if changing this might break someone. It's been this way for 
a long time. It might be better to change the spec or just read 
'alignment' as whatever size it happens to be (len / 4). It's not really 
the kernel's job to validate the DT. We should first have some 
validation in place to *know* if there are any current .dts files that 
would break. That would probably be easier to implement in dtc than 
dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
but that was the default for OpenFirmware.

As the alignment is the base address alignment, it can be argued that 
"#address-cells" makes more sense to use than "#size-cells". So maybe 
the spec was a copy-n-paste error.

> 
> Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>  
>  	prop = of_get_flat_dt_prop(node, "alignment", &len);
>  	if (prop) {
> -		if (len != dt_root_addr_cells * sizeof(__be32)) {
> +		if (len != dt_root_size_cells * sizeof(__be32)) {
>  			pr_err("invalid alignment property in '%s' node.\n",
>  				uname);
>  			return -EINVAL;
>  		}
> -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
>  	}
>  
>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

Hi Zijun and Rob,

On 01/13/2025, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> > From: Zijun Hu <quic_zijuhu@quicinc.com>
> > 
> > According to DT spec, size of property 'alignment' is based on parent
> > node’s #size-cells property.
> > 
> > But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> > the property obviously.
> > 
> > Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> 
> I wonder if changing this might break someone. It's been this way for 
> a long time. It might be better to change the spec or just read 
> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> the kernel's job to validate the DT. We should first have some 
> validation in place to *know* if there are any current .dts files that 
> would break. That would probably be easier to implement in dtc than 
> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> but that was the default for OpenFirmware.
> 
> As the alignment is the base address alignment, it can be argued that 
> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> the spec was a copy-n-paste error.

Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
tree has cases where #address-cells != #size-cells.

I would prefer to not have this change, but if that's not possible, could we
not backport it to all the stable branches? That way we can just force new
devices to fix this instead of existing devices on older LTS kernels?

Thanks,
Will

> 
> > 
> > Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> > ---
> >  drivers/of/of_reserved_mem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
> > --- a/drivers/of/of_reserved_mem.c
> > +++ b/drivers/of/of_reserved_mem.c
> > @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
> >  
> >  	prop = of_get_flat_dt_prop(node, "alignment", &len);
> >  	if (prop) {
> > -		if (len != dt_root_addr_cells * sizeof(__be32)) {
> > +		if (len != dt_root_size_cells * sizeof(__be32)) {
> >  			pr_err("invalid alignment property in '%s' node.\n",
> >  				uname);
> >  			return -EINVAL;
> >  		}
> > -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
> >  	}
> >  
> >  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> > 
> > -- 
> > 2.34.1
> > 

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2/25/2025 9:18 AM, William McVicker wrote:
> Hi Zijun and Rob,
> 
> On 01/13/2025, Rob Herring wrote:
>> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> According to DT spec, size of property 'alignment' is based on parent
>>> node’s #size-cells property.
>>>
>>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
>>> the property obviously.
>>>
>>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
>>
>> I wonder if changing this might break someone. It's been this way for 
>> a long time. It might be better to change the spec or just read 
>> 'alignment' as whatever size it happens to be (len / 4). It's not really 
>> the kernel's job to validate the DT. We should first have some 
>> validation in place to *know* if there are any current .dts files that 
>> would break. That would probably be easier to implement in dtc than 
>> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
>> but that was the default for OpenFirmware.
>>
>> As the alignment is the base address alignment, it can be argued that 
>> "#address-cells" makes more sense to use than "#size-cells". So maybe 
>> the spec was a copy-n-paste error.
> 
> Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> tree has cases where #address-cells != #size-cells.
> 

it seems upstream upstream Pixel 6 has no property 'alignment'
git grep alignment arch/arm64/boot/dts/exynos/google/
so it should not be broken.

> I would prefer to not have this change, but if that's not possible, could we
> not backport it to all the stable branches? That way we can just force new
> devices to fix this instead of existing devices on older LTS kernels?
> 

the fix have stable and fix tags. not sure if we can control its
backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.


> Thanks,
> Will
> 
>>
>>>
>>> Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> ---
>>>  drivers/of/of_reserved_mem.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>>> index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
>>> --- a/drivers/of/of_reserved_mem.c
>>> +++ b/drivers/of/of_reserved_mem.c
>>> @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>>>  
>>>  	prop = of_get_flat_dt_prop(node, "alignment", &len);
>>>  	if (prop) {
>>> -		if (len != dt_root_addr_cells * sizeof(__be32)) {
>>> +		if (len != dt_root_size_cells * sizeof(__be32)) {
>>>  			pr_err("invalid alignment property in '%s' node.\n",
>>>  				uname);
>>>  			return -EINVAL;
>>>  		}
>>> -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
>>> +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
>>>  	}
>>>  
>>>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>>>
>>> -- 
>>> 2.34.1
>>>

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

On 02/25/2025, Zijun Hu wrote:
> On 2/25/2025 9:18 AM, William McVicker wrote:
> > Hi Zijun and Rob,
> > 
> > On 01/13/2025, Rob Herring wrote:
> >> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> >>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>
> >>> According to DT spec, size of property 'alignment' is based on parent
> >>> node’s #size-cells property.
> >>>
> >>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> >>> the property obviously.
> >>>
> >>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> >>
> >> I wonder if changing this might break someone. It's been this way for 
> >> a long time. It might be better to change the spec or just read 
> >> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> >> the kernel's job to validate the DT. We should first have some 
> >> validation in place to *know* if there are any current .dts files that 
> >> would break. That would probably be easier to implement in dtc than 
> >> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> >> but that was the default for OpenFirmware.
> >>
> >> As the alignment is the base address alignment, it can be argued that 
> >> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> >> the spec was a copy-n-paste error.
> > 
> > Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> > tree has cases where #address-cells != #size-cells.
> > 
> 
> it seems upstream upstream Pixel 6 has no property 'alignment'
> git grep alignment arch/arm64/boot/dts/exynos/google/
> so it should not be broken.

That's right. I was responding to Rob's statement about #address-cells !=
#size-cells being pretty rare. And wanted to give credance to the idea that
this change could possible break someone.

> 
> > I would prefer to not have this change, but if that's not possible, could we
> > not backport it to all the stable branches? That way we can just force new
> > devices to fix this instead of existing devices on older LTS kernels?
> > 
> 
> the fix have stable and fix tags. not sure if we can control its
> backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.

Right, I think it's already backported to the LTS kernels, but if it breaks any
in-tree users then we'd have to revert it. I just like Rob's idea to instead
change the spec for obvious reasons :)

Regards,
Will

<snip>

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Tue, Feb 25, 2025 at 09:46:54AM -0800, William McVicker wrote:
> On 02/25/2025, Zijun Hu wrote:
> > On 2/25/2025 9:18 AM, William McVicker wrote:
> > > Hi Zijun and Rob,
> > > 
> > > On 01/13/2025, Rob Herring wrote:
> > >> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> > >>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> > >>>
> > >>> According to DT spec, size of property 'alignment' is based on parent
> > >>> node’s #size-cells property.
> > >>>
> > >>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> > >>> the property obviously.
> > >>>
> > >>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> > >>
> > >> I wonder if changing this might break someone. It's been this way for 
> > >> a long time. It might be better to change the spec or just read 
> > >> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> > >> the kernel's job to validate the DT. We should first have some 
> > >> validation in place to *know* if there are any current .dts files that 
> > >> would break. That would probably be easier to implement in dtc than 
> > >> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> > >> but that was the default for OpenFirmware.
> > >>
> > >> As the alignment is the base address alignment, it can be argued that 
> > >> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> > >> the spec was a copy-n-paste error.
> > > 
> > > Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> > > tree has cases where #address-cells != #size-cells.
> > >

I thought downstream kept kernels and DTs in sync, so the dts could be 
fixed?
 
> > 
> > it seems upstream upstream Pixel 6 has no property 'alignment'
> > git grep alignment arch/arm64/boot/dts/exynos/google/
> > so it should not be broken.
> 
> That's right. I was responding to Rob's statement about #address-cells !=
> #size-cells being pretty rare. And wanted to give credance to the idea that
> this change could possible break someone.
> 
> > 
> > > I would prefer to not have this change, but if that's not possible, could we
> > > not backport it to all the stable branches? That way we can just force new
> > > devices to fix this instead of existing devices on older LTS kernels?
> > > 
> > 
> > the fix have stable and fix tags. not sure if we can control its
> > backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.
> 
> Right, I think it's already backported to the LTS kernels, but if it breaks any
> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> change the spec for obvious reasons :)

While if it is downstream, it doesn't exist, I'm reverting this for now. 
We need the tools to check this and look at other projects to see what 
they expect. Then we can think about changing the spec.

Rob

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 03:45, Rob Herring wrote:
>> Right, I think it's already backported to the LTS kernels, but if it breaks any
>> in-tree users then we'd have to revert it. I just like Rob's idea to instead
>> change the spec for obvious reasons 🙂
> While if it is downstream, it doesn't exist, I'm reverting this for now. 

perhaps, it is better for us to slow down here.

1) This change does not break any upstream code.
   is there downstream code which is publicly visible and is broken by
   this change ?

2) IMO, the spec may be right.
   The type of size is enough to express any alignment wanted.
   For several kernel allocators. type of 'alignment' should be the type
   of 'size', NOT the type of 'address'


> We need the tools to check this and look at other projects to see what 
> they expect. Then we can think about changing the spec.

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Wed, Feb 26, 2025 at 2:31 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2025/2/27 03:45, Rob Herring wrote:
> >> Right, I think it's already backported to the LTS kernels, but if it breaks any
> >> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> >> change the spec for obvious reasons 🙂
> > While if it is downstream, it doesn't exist, I'm reverting this for now.
>
> perhaps, it is better for us to slow down here.
>
> 1) This change does not break any upstream code.
>    is there downstream code which is publicly visible and is broken by
>    this change ?

We don't know that unless you tested every dts file. We only know that
no one has reported an issue yet.

Even if we did test everything, there are DT's that aren't in the
kernel tree. It's not like this downstream DT is using some
undocumented binding or questionable things. It's a standard binding.

Every time this code is touched, it breaks. This is not even the only
breakage right now[1].

> 2) IMO, the spec may be right.
>    The type of size is enough to express any alignment wanted.
>    For several kernel allocators. type of 'alignment' should be the type
>    of 'size', NOT the type of 'address'

As I said previously, it can be argued either way.

Rob

[1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

> I thought downstream kept kernels and DTs in sync, so the dts could be
> fixed?

For Pixel the kernel and DT are in sync, but I'm not sure about other devices.
The problem in general though is now everyone would need to do a special
coordination when updating to the newer LTS version to make sure their kernel
matches the new DT.

On 02/26/2025, Rob Herring wrote:
> On Wed, Feb 26, 2025 at 2:31 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >
> > On 2025/2/27 03:45, Rob Herring wrote:
> > >> Right, I think it's already backported to the LTS kernels, but if it breaks any
> > >> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> > >> change the spec for obvious reasons 🙂
> > > While if it is downstream, it doesn't exist, I'm reverting this for now.
> >
> > perhaps, it is better for us to slow down here.
> >
> > 1) This change does not break any upstream code.
> >    is there downstream code which is publicly visible and is broken by
> >    this change ?
> 
> We don't know that unless you tested every dts file. We only know that
> no one has reported an issue yet.
> 
> Even if we did test everything, there are DT's that aren't in the
> kernel tree. It's not like this downstream DT is using some
> undocumented binding or questionable things. It's a standard binding.
> 
> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].

You can find the Pixel 6/7/8/9 device trees on android.googlesource.com.
You can see for zuma based devices (Pixel 9 for example) they have this [1]:

  &reserved_memory {
        #address-cells = <2>;
        #size-cells = <1>;
        vstream: vstream {
                compatible = "shared-dma-pool";
                reusable;
                size = <0x4800000>;
                alignment = <0x0 0x00010000>;
                alloc-ranges = <0x9 0x80000000 0x80000000>,
                               <0x9 0x00000000 0x80000000>,
                               <0x8 0x80000000 0x80000000>,
                               <0x0 0x80000000 0x80000000>;
        };

I understand this code is downstream, but as a general principle we shouldn't
break backwards compatibilty.

Thanks,
Will

[1] https://android.googlesource.com/kernel/devices/google/zuma/+/refs/heads/android-gs-shusky-6.1-android16-dp/dts/gs101-dma-heap.dtsi#147

> 
> > 2) IMO, the spec may be right.
> >    The type of size is enough to express any alignment wanted.
> >    For several kernel allocators. type of 'alignment' should be the type
> >    of 'size', NOT the type of 'address'
> 
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:36, William McVicker wrote:
>> Every time this code is touched, it breaks. This is not even the only
>> breakage right now[1].
> You can find the Pixel 6/7/8/9 device trees on android.googlesource.com.
> You can see for zuma based devices (Pixel 9 for example) they have this [1]:
> 
>   &reserved_memory {
>         #address-cells = <2>;
>         #size-cells = <1>;
>         vstream: vstream {
>                 compatible = "shared-dma-pool";
>                 reusable;
>                 size = <0x4800000>;
>                 alignment = <0x0 0x00010000>;
>                 alloc-ranges = <0x9 0x80000000 0x80000000>,
>                                <0x9 0x00000000 0x80000000>,
>                                <0x8 0x80000000 0x80000000>,
>                                <0x0 0x80000000 0x80000000>;
>         };
> 
> I understand this code is downstream, but as a general principle we shouldn't
> break backwards compatibilty.

this is not backward compatibility issue. it is a downstream bug instead.

normally, you need to write DTS according to relevant DT binding spec or
DT spec.

i can't access the link you shared due to my country's GFW.
does google kernel have extra binding spec about size of property
'alignment'?

IMO, downstream maintainers may needs to fix this issue by if the
upstream fix is picked up.
- alignment = <0x0 0x00010000>;
+ alignment = <0x00010000>;

actually, "The importance of getting code into the mainline" within
Documentation/process/1.Intro.rst encourages upstream your code to
avoid such issue.

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:30, Rob Herring wrote:
>>    this change ?
> We don't know that unless you tested every dts file. We only know that
> no one has reported an issue yet.
> 

Sorry, my mistake to post the question here for convenience.

actually, i want to ask William this question, and he/she shared applet
of the downstream code.

> Even if we did test everything, there are DT's that aren't in the
> kernel tree. It's not like this downstream DT is using some
> undocumented binding or questionable things. It's a standard binding.
> 

IMO, that may be a downstream bug since they don't refer to binding spec
to set property 'alignment'.

> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].
> 

indeed.

>> 2) IMO, the spec may be right.
>>    The type of size is enough to express any alignment wanted.
>>    For several kernel allocators. type of 'alignment' should be the type
>>    of 'size', NOT the type of 'address'
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:30, Rob Herring wrote:
> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].
> 
>> 2) IMO, the spec may be right.
>>    The type of size is enough to express any alignment wanted.
>>    For several kernel allocators. type of 'alignment' should be the type
>>    of 'size', NOT the type of 'address'
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

this issue may be a downstream issue as well.

based on comments. they expects the CMA area is within 4G range.

but the wrong alloc-ranges config make the range (0x42000000 +
0xc0000000) > 4G across 4G boundary.

[PATCH v4 10/14] of: reserved-memory: Do not make kmemleak ignore freed address

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

early_init_dt_alloc_reserved_memory_arch() will free address @base when
suffers memblock_mark_nomap() error, but it still makes kmemleak ignore
the freed address @base via kmemleak_ignore_phys().

That is unnecessary, besides, also causes unnecessary warning messages:

kmemleak_ignore_phys()
 -> make_black_object()
    -> paint_ptr()
       -> kmemleak_warn() // warning message here.

Fix by avoiding kmemleak_ignore_phys() when suffer the error.

Fixes: 658aafc8139c ("memblock: exclude MEMBLOCK_NOMAP regions from kmemleak")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/of_reserved_mem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index d2753756d7c30adcbd52f57338e281c16d821488..03a8f03ed1da165d6d7bf907d931857260888225 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -52,7 +52,8 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
 			memblock_phys_free(base, size);
 	}
 
-	kmemleak_ignore_phys(base);
+	if (!err)
+		kmemleak_ignore_phys(base);
 
 	return err;
 }

-- 
2.34.1

Re: [PATCH v4 10/14] of: reserved-memory: Do not make kmemleak ignore freed address

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:27:01 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> early_init_dt_alloc_reserved_memory_arch() will free address @base when
> suffers memblock_mark_nomap() error, but it still makes kmemleak ignore
> the freed address @base via kmemleak_ignore_phys().
> 
> That is unnecessary, besides, also causes unnecessary warning messages:
> 
> kmemleak_ignore_phys()
>  -> make_black_object()
>     -> paint_ptr()
>        -> kmemleak_warn() // warning message here.
> 
> Fix by avoiding kmemleak_ignore_phys() when suffer the error.
> 
> Fixes: 658aafc8139c ("memblock: exclude MEMBLOCK_NOMAP regions from kmemleak")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Applied, thanks!

[PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For child node of /reserved-memory, its property 'reg' may contain
multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
into account the first region, and miss remaining regions.

Give warning message when missing remaining regions.

Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/of_reserved_mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 03a8f03ed1da165d6d7bf907d931857260888225..e2da88d7706ab3c208386e29f31350178e67cd14 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -263,6 +263,11 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
 			       uname);
 			continue;
 		}
+
+		if (len > t_len)
+			pr_warn("%s() ignores %d regions in node '%s'\n",
+				__func__, len / t_len - 1, uname);
+
 		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
 		size = dt_mem_next_cell(dt_root_size_cells, &prop);
 

-- 
2.34.1

Re: [PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Rob Herring]

On Thu, Jan 09, 2025 at 09:27:02PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For child node of /reserved-memory, its property 'reg' may contain
> multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
> into account the first region, and miss remaining regions.
> 
> Give warning message when missing remaining regions.

Can't we just fix it to support more than 1 entry?

> 
> Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 03a8f03ed1da165d6d7bf907d931857260888225..e2da88d7706ab3c208386e29f31350178e67cd14 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -263,6 +263,11 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
>  			       uname);
>  			continue;
>  		}
> +
> +		if (len > t_len)
> +			pr_warn("%s() ignores %d regions in node '%s'\n",
> +				__func__, len / t_len - 1, uname);
> +
>  		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
>  		size = dt_mem_next_cell(dt_root_size_cells, &prop);
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Zijun Hu]

On 2025/1/14 07:16, Rob Herring wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For child node of /reserved-memory, its property 'reg' may contain
>> multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
>> into account the first region, and miss remaining regions.
>>
>> Give warning message when missing remaining regions.
> Can't we just fix it to support more than 1 entry?

actually, i ever considered supporting more entries.
and find out there are no simple approach to achieve this aim.

it may need to change 'struct reserved_mem' to support multiple regions
that may also involve 4 RESERVEDMEM_OF_DECLARE instances.

RESERVEDMEM_OF_DECLARE(tegra210_emc_table, "nvidia,tegra210-emc-table",
                       tegra210_emc_table_init);
RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup);
RESERVEDMEM_OF_DECLARE(cma, "shared-dma-pool", rmem_cma_setup);
RESERVEDMEM_OF_DECLARE(dma, "restricted-dma-pool", rmem_swiotlb_setup);

so i finally chose this warning way due to simplification and low risk.

[PATCH v4 12/14] of: reserved-memory: Move an assignment to effective place in __reserved_mem_alloc_size()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

The assignment '@base = 0' in __reserved_mem_alloc_size() is meaningless
since @base was already initialized to 0.

Move the assignment to effective and proper place.

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

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index e2da88d7706ab3c208386e29f31350178e67cd14..75e819f66a56139a800dba5e2e0044d0bbeb065e 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -441,13 +441,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
 			return -EINVAL;
 		}
 
-		base = 0;
-
 		while (len > 0) {
 			start = dt_mem_next_cell(dt_root_addr_cells, &prop);
 			end = start + dt_mem_next_cell(dt_root_size_cells,
 						       &prop);
 
+			base = 0;
 			ret = __reserved_mem_alloc_in_range(size, align,
 					start, end, nomap, &base);
 			if (ret == 0) {

-- 
2.34.1

Re: [PATCH v4 12/14] of: reserved-memory: Move an assignment to effective place in __reserved_mem_alloc_size()

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:27:03 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> The assignment '@base = 0' in __reserved_mem_alloc_size() is meaningless
> since @base was already initialized to 0.
> 
> Move the assignment to effective and proper place.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Applied, thanks!

[PATCH v4 13/14] of/fdt: Check fdt_get_mem_rsv() error in early_init_fdt_scan_reserved_mem()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

early_init_fdt_scan_reserved_mem() invoks fdt_get_mem_rsv(), and it will
use uninitialized variables @base and @size once the callee suffers error.

Fix by checking fdt_get_mem_rsv() error as other callers do.

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

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4b1e9f101ce34d7212cc8de99c7e7761a2636866..c93a99d4a1e08c0d4cccf9e5ae16f7e4950ee801 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -497,6 +497,7 @@ static void __init fdt_reserve_elfcorehdr(void)
 void __init early_init_fdt_scan_reserved_mem(void)
 {
 	int n;
+	int res;
 	u64 base, size;
 
 	if (!initial_boot_params)
@@ -507,7 +508,11 @@ void __init early_init_fdt_scan_reserved_mem(void)
 
 	/* Process header /memreserve/ fields */
 	for (n = 0; ; n++) {
-		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
+		res = fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
+		if (res) {
+			pr_err("Invalid memory reservation block index %d\n", n);
+			break;
+		}
 		if (!size)
 			break;
 		memblock_reserve(base, size);

-- 
2.34.1

Re: [PATCH v4 13/14] of/fdt: Check fdt_get_mem_rsv() error in early_init_fdt_scan_reserved_mem()

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:27:04 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> early_init_fdt_scan_reserved_mem() invoks fdt_get_mem_rsv(), and it will
> use uninitialized variables @base and @size once the callee suffers error.
> 
> Fix by checking fdt_get_mem_rsv() error as other callers do.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/fdt.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 

Applied, thanks!

[PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

__of_add_property_sysfs() hard codes string "security-" length as 9, but
that is not obvious for readers.

Improve its readability by using strlen().

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

diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index aa887166f0d21030d620d43c864ca76cde1c6d05..44bfe50c6ea6503e3940578de1dfc7fe0583dfb3 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -62,10 +62,11 @@ static const char *safe_name(const struct kobject *kobj, const char *orig_name)
 
 int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 {
+	const char *security_prefix = "security-";
 	int rc;
 
 	/* Important: Don't leak passwords */
-	bool secure = strncmp(pp->name, "security-", 9) == 0;
+	bool secure = strncmp(pp->name, security_prefix, strlen(security_prefix)) == 0;
 
 	if (!IS_ENABLED(CONFIG_SYSFS))
 		return 0;

-- 
2.34.1

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Rob Herring]

On Thu, Jan 09, 2025 at 09:27:05PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> __of_add_property_sysfs() hard codes string "security-" length as 9, but
> that is not obvious for readers.
> 
> Improve its readability by using strlen().

Does the compiler optimize the strlen call away? Maybe, maybe not. If 
not, that's N calls to strlen() where N is the number of properties in 
your DT. That's in the 1000s easily.

Do you really want to go test enough compiler versions we support to 
feel confident this is optimized away. I don't.

Rob

> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/kobj.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
> index aa887166f0d21030d620d43c864ca76cde1c6d05..44bfe50c6ea6503e3940578de1dfc7fe0583dfb3 100644
> --- a/drivers/of/kobj.c
> +++ b/drivers/of/kobj.c
> @@ -62,10 +62,11 @@ static const char *safe_name(const struct kobject *kobj, const char *orig_name)
>  
>  int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>  {
> +	const char *security_prefix = "security-";
>  	int rc;
>  
>  	/* Important: Don't leak passwords */
> -	bool secure = strncmp(pp->name, "security-", 9) == 0;
> +	bool secure = strncmp(pp->name, security_prefix, strlen(security_prefix)) == 0;
>  
>  	if (!IS_ENABLED(CONFIG_SYSFS))
>  		return 0;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

On 2025/1/11 04:41, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:27:05PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> __of_add_property_sysfs() hard codes string "security-" length as 9, but
>> that is not obvious for readers.
>>
>> Improve its readability by using strlen().
> 
> Does the compiler optimize the strlen call away? Maybe, maybe not. If 
> not, that's N calls to strlen() where N is the number of properties in 
> your DT. That's in the 1000s easily.
> 
> Do you really want to go test enough compiler versions we support to 
> feel confident this is optimized away. I don't.
> 

i understand your concern about performance.
perhaps, such impact about performance may be ignored for linux OS.

what about below solution ?

const char security_prefix[] = "security-";
use 'sizeof(security_prefix) - 1' for the length of string.


> Rob
> 
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/of/kobj.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
>> index aa887166f0d21030d620d43c864ca76cde1c6d05..44bfe50c6ea6503e3940578de1dfc7fe0583dfb3 100644
>> --- a/drivers/of/kobj.c
>> +++ b/drivers/of/kobj.c
>> @@ -62,10 +62,11 @@ static const char *safe_name(const struct kobject *kobj, const char *orig_name)
>>  
>>  int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>>  {
>> +	const char *security_prefix = "security-";
>>  	int rc;
>>  
>>  	/* Important: Don't leak passwords */
>> -	bool secure = strncmp(pp->name, "security-", 9) == 0;
>> +	bool secure = strncmp(pp->name, security_prefix, strlen(security_prefix)) == 0;
>>  
>>  	if (!IS_ENABLED(CONFIG_SYSFS))
>>  		return 0;
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Krzysztof Kozlowski]

On Sat, Jan 11, 2025 at 07:48:48AM +0800, Zijun Hu wrote:
> On 2025/1/11 04:41, Rob Herring wrote:
> > On Thu, Jan 09, 2025 at 09:27:05PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> __of_add_property_sysfs() hard codes string "security-" length as 9, but
> >> that is not obvious for readers.
> >>
> >> Improve its readability by using strlen().
> > 
> > Does the compiler optimize the strlen call away? Maybe, maybe not. If 
> > not, that's N calls to strlen() where N is the number of properties in 
> > your DT. That's in the 1000s easily.
> > 
> > Do you really want to go test enough compiler versions we support to 
> > feel confident this is optimized away. I don't.
> > 
> 
> i understand your concern about performance.
> perhaps, such impact about performance may be ignored for linux OS.
> 
> what about below solution ?
> 
> const char security_prefix[] = "security-";
> use 'sizeof(security_prefix) - 1' for the length of string.

Code is still not equivalent - just de-assemble it and you will see
some overhead.

Maybe just introduce builtin_strlen() to string.h and use such? It would
be the pretty obvious code.


Best regards,
Krzysztof

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

On 2025/1/11 17:17, Krzysztof Kozlowski wrote:
> On Sat, Jan 11, 2025 at 07:48:48AM +0800, Zijun Hu wrote:
>> On 2025/1/11 04:41, Rob Herring wrote:
>>> On Thu, Jan 09, 2025 at 09:27:05PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> __of_add_property_sysfs() hard codes string "security-" length as 9, but
>>>> that is not obvious for readers.
>>>>
>>>> Improve its readability by using strlen().
>>>
>>> Does the compiler optimize the strlen call away? Maybe, maybe not. If 
>>> not, that's N calls to strlen() where N is the number of properties in 
>>> your DT. That's in the 1000s easily.
>>>
>>> Do you really want to go test enough compiler versions we support to 
>>> feel confident this is optimized away. I don't.
>>>
>>
>> i understand your concern about performance.
>> perhaps, such impact about performance may be ignored for linux OS.
>>
>> what about below solution ?
>>
>> const char security_prefix[] = "security-";
>> use 'sizeof(security_prefix) - 1' for the length of string.
> 
> Code is still not equivalent - just de-assemble it and you will see
> some overhead.
> 

Thank you Krzysztof Kozlowski for code review

the overhead may be that use of stack is increased a bit.
it may not almost impact performance since 'sizeof(security_prefix) - 1'
may be still a compile time constant.

> Maybe just introduce builtin_strlen() to string.h and use such? It would
> be the pretty obvious code.
> 

it is a good idea to introduce API for this common issue, it seems the
main strncmp() usages in current kernel tree is to check if a string
begin with the other string a shown below:

// "s2_string" is wrote twice, and also impact performance
strncmp(s1, "s2_string", strlen("s2_string"))

// it is not obvious for readers that "s2_string" length is 9
strncmp(s1, "s2_string", 9)

//"s2_string" is wrote twice, same as strcmp(), may be wrong
strncmp(s1, "s2_string", sizeof("s2_string"))

//"s2_string" is wrote twice
strncmp(s1, "s2_string", sizeof("s2_string") - 1)

what about a new API such as ?

// check if @s1 begins with @s2
bool str_begin_with(const char *cs, const char *ct)
{
	unsigned char c1, c2;
	
	if (!cs || !ct)
		return false;
	
	do {
		c1 = *cs++;
		c2 = *ct++;
		
		if (c1 != c2)
			return c2 == '\0';
	} while (c1 != '\0');
	
	return true;
}

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Krzysztof Kozlowski]

On Sat, Jan 11, 2025 at 08:44:34PM +0800, Zijun Hu wrote:
> On 2025/1/11 17:17, Krzysztof Kozlowski wrote:
> > On Sat, Jan 11, 2025 at 07:48:48AM +0800, Zijun Hu wrote:
> >> On 2025/1/11 04:41, Rob Herring wrote:
> >>> On Thu, Jan 09, 2025 at 09:27:05PM +0800, Zijun Hu wrote:
> >>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>
> >>>> __of_add_property_sysfs() hard codes string "security-" length as 9, but
> >>>> that is not obvious for readers.
> >>>>
> >>>> Improve its readability by using strlen().
> >>>
> >>> Does the compiler optimize the strlen call away? Maybe, maybe not. If 
> >>> not, that's N calls to strlen() where N is the number of properties in 
> >>> your DT. That's in the 1000s easily.
> >>>
> >>> Do you really want to go test enough compiler versions we support to 
> >>> feel confident this is optimized away. I don't.
> >>>
> >>
> >> i understand your concern about performance.
> >> perhaps, such impact about performance may be ignored for linux OS.
> >>
> >> what about below solution ?
> >>
> >> const char security_prefix[] = "security-";
> >> use 'sizeof(security_prefix) - 1' for the length of string.
> > 
> > Code is still not equivalent - just de-assemble it and you will see
> > some overhead.
> > 
> 
> Thank you Krzysztof Kozlowski for code review
> 
> the overhead may be that use of stack is increased a bit.
> it may not almost impact performance since 'sizeof(security_prefix) - 1'
> may be still a compile time constant.

No, I was talking about initialization. Look how it is done.

> 
> > Maybe just introduce builtin_strlen() to string.h and use such? It would
> > be the pretty obvious code.
> > 
> 
> it is a good idea to introduce API for this common issue, it seems the
> main strncmp() usages in current kernel tree is to check if a string
> begin with the other string a shown below:
> 
> // "s2_string" is wrote twice, and also impact performance
> strncmp(s1, "s2_string", strlen("s2_string"))
> 
> // it is not obvious for readers that "s2_string" length is 9
> strncmp(s1, "s2_string", 9)
> 
> //"s2_string" is wrote twice, same as strcmp(), may be wrong
> strncmp(s1, "s2_string", sizeof("s2_string"))
> 
> //"s2_string" is wrote twice
> strncmp(s1, "s2_string", sizeof("s2_string") - 1)
> 
> what about a new API such as ?
> 
> // check if @s1 begins with @s2
> bool str_begin_with(const char *cs, const char *ct)

Nothing related to my proposal, so no clue why you ask me. Anyway, as it
is one more unsafe str-like function, I don't see how it makes things
much better.

Best regards,
Krzysztof

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

On 2025/1/11 17:17, Krzysztof Kozlowski wrote:
>> const char security_prefix[] = "security-";
>> use 'sizeof(security_prefix) - 1' for the length of string.
> Code is still not equivalent - just de-assemble it and you will see
> some overhead.
> 
> Maybe just introduce builtin_strlen() to string.h and use such? It would
> be the pretty obvious code.

strncmp(s1, "s2_string", builtin_strlen("s2_string")) is similar as
strncmp(s1, "s2_string", sizeof("s2_string") - 1).

so perhaps, it is not worthy of a new builtin_strlen().

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Krzysztof Kozlowski]

On 14/01/2025 16:20, Zijun Hu wrote:
> On 2025/1/11 17:17, Krzysztof Kozlowski wrote:
>>> const char security_prefix[] = "security-";
>>> use 'sizeof(security_prefix) - 1' for the length of string.
>> Code is still not equivalent - just de-assemble it and you will see
>> some overhead.
>>
>> Maybe just introduce builtin_strlen() to string.h and use such? It would
>> be the pretty obvious code.
> 
> strncmp(s1, "s2_string", builtin_strlen("s2_string")) is similar as
> strncmp(s1, "s2_string", sizeof("s2_string") - 1).

Above yes, but maybe builtin_strlen() could be made to work on the
pointer, thus you would have "s2_string" in only one place.

> 
> so perhaps, it is not worthy of a new builtin_strlen().

Best regards,
Krzysztof

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

On 2025/1/11 04:41, Rob Herring wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> __of_add_property_sysfs() hard codes string "security-" length as 9, but
>> that is not obvious for readers.
>>
>> Improve its readability by using strlen().
> Does the compiler optimize the strlen call away? Maybe, maybe not. If 
> not, that's N calls to strlen() where N is the number of properties in 
> your DT. That's in the 1000s easily.
> 
> Do you really want to go test enough compiler versions we support to 
> feel confident this is optimized away. I don't.

i understand your concern about performance.

what about below solution ?

 int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 {
+#define SECURITY_PREFIX "security-"
        int rc;

        /* Important: Don't leak passwords */
-       bool secure = strncmp(pp->name, "security-", 9) == 0;
+       bool secure = strncmp(pp->name, SECURITY_PREFIX,
sizeof(SECURITY_PREFIX) - 1) == 0;

        if (!IS_ENABLED(CONFIG_SYSFS))
                return 0;

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Rob Herring]

On Mon, Jan 13, 2025 at 11:00:12PM +0800, Zijun Hu wrote:
> On 2025/1/11 04:41, Rob Herring wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> __of_add_property_sysfs() hard codes string "security-" length as 9, but
> >> that is not obvious for readers.
> >>
> >> Improve its readability by using strlen().
> > Does the compiler optimize the strlen call away? Maybe, maybe not. If 
> > not, that's N calls to strlen() where N is the number of properties in 
> > your DT. That's in the 1000s easily.
> > 
> > Do you really want to go test enough compiler versions we support to 
> > feel confident this is optimized away. I don't.
> 
> i understand your concern about performance.
> 
> what about below solution ?

See 72921427d46b ("string.h: Add str_has_prefix() helper function"). 

Though we already had strstarts(), but that lacks __always_inline which 
seems is important for eliminating the strlen(). Also, since that 
commit, clang has become more common and need to make sure the same 
optimization happens on it.

> 
>  int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>  {
> +#define SECURITY_PREFIX "security-"
>         int rc;
> 
>         /* Important: Don't leak passwords */
> -       bool secure = strncmp(pp->name, "security-", 9) == 0;
> +       bool secure = strncmp(pp->name, SECURITY_PREFIX,
> sizeof(SECURITY_PREFIX) - 1) == 0;
> 
>         if (!IS_ENABLED(CONFIG_SYSFS))
>                 return 0;
> 
> 
> 

Re: [PATCH v4 14/14] of: Improve __of_add_property_sysfs() readability

[Zijun Hu]

On 2025/1/14 07:46, Rob Herring wrote:
>> i understand your concern about performance.
>>
>> what about below solution ?
> See 72921427d46b ("string.h: Add str_has_prefix() helper function"). 
> 
> Though we already had strstarts(), but that lacks __always_inline which 
> seems is important for eliminating the strlen(). Also, since that 
> commit, clang has become more common and need to make sure the same 
> optimization happens on it.

than you Rob for sharing with these useful info.

i believe your proposal is feasible to improve  strstarts() performance
to achieve performance level of
strncmp(str, "const", sizeof("const") - 1).


this is a common issue and i would like to start a new thread to discuss
this.