[PATCH 0/5] of: Trivial cleanup

[PATCH 0/5] of: Trivial cleanup

[Zijun Hu]

This patch series is to do some trivial cleanup.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (5):
      of: Compare property names by of_prop_cmp() in of_alias_scan()
      of: Introduce and apply private is_pseudo_property()
      of: Correct property name comparison in __of_add_property()
      of: Do not change property state under __of_add_property() failure
      of/platform: Do not use of_get_property() to test property presence

 drivers/of/base.c       | 12 +++++-------
 drivers/of/of_private.h |  7 +++++++
 drivers/of/overlay.c    |  4 +---
 drivers/of/platform.c   |  2 +-
 drivers/of/resolver.c   |  4 +---
 5 files changed, 15 insertions(+), 14 deletions(-)
---
base-commit: 7526e4fe550f51bd8c41eb51492436117917e3f1
change-id: 20250120-of_bugfix-16581919a446

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

[PATCH 1/5] of: Compare property names by of_prop_cmp() in of_alias_scan()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For these pseudo property names 'name', 'phandle' and 'linux,phandle':

Use dedicated property name comparison macro of_prop_cmp() instead of
strcmp() in of_alias_scan() to:

- Make property name comparison consistent.
- Prepare for introducing private is_pseudo_property() later.

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 af6c68bbb4277e4f66deef886a2df8d1b6f114cf..d2d41601136bc8ee2b97e31b83af1b361ba03261 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1855,9 +1855,9 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		int id, len;
 
 		/* Skip those we do not want to proceed */
-		if (!strcmp(pp->name, "name") ||
-		    !strcmp(pp->name, "phandle") ||
-		    !strcmp(pp->name, "linux,phandle"))
+		if (!of_prop_cmp(pp->name, "name") ||
+		    !of_prop_cmp(pp->name, "phandle") ||
+		    !of_prop_cmp(pp->name, "linux,phandle"))
 			continue;
 
 		np = of_find_node_by_path(pp->value);

-- 
2.34.1

Re: [PATCH 1/5] of: Compare property names by of_prop_cmp() in of_alias_scan()

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 22:27:57 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For these pseudo property names 'name', 'phandle' and 'linux,phandle':
> 
> Use dedicated property name comparison macro of_prop_cmp() instead of
> strcmp() in of_alias_scan() to:
> 
> - Make property name comparison consistent.
> - Prepare for introducing private is_pseudo_property() later.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Applied, thanks!

[PATCH 2/5] of: Introduce and apply private is_pseudo_property()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

There are several places which check if a property name is one of
'name'|'phandle'|'linux,phandle'.

Introduce and apply private is_pseudo_property() for the check.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c       | 4 +---
 drivers/of/of_private.h | 7 +++++++
 drivers/of/overlay.c    | 4 +---
 drivers/of/resolver.c   | 4 +---
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d2d41601136bc8ee2b97e31b83af1b361ba03261..001ff6ce4abf85c07f13649d5a9f691f549a8ccc 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1855,9 +1855,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		int id, len;
 
 		/* Skip those we do not want to proceed */
-		if (!of_prop_cmp(pp->name, "name") ||
-		    !of_prop_cmp(pp->name, "phandle") ||
-		    !of_prop_cmp(pp->name, "linux,phandle"))
+		if (is_pseudo_property(pp->name))
 			continue;
 
 		np = of_find_node_by_path(pp->value);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index f3e1193c8ded4899f39677a76da073e2266a1b9a..b0c077867bf4abc045ca332ebacb988cdead90fc 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -208,4 +208,11 @@ static void __maybe_unused of_dump_addr(const char *s, const __be32 *addr, int n
 static void __maybe_unused of_dump_addr(const char *s, const __be32 *addr, int na) { }
 #endif
 
+static inline bool is_pseudo_property(const char *prop_name)
+{
+	return !of_prop_cmp(prop_name, "name") ||
+		!of_prop_cmp(prop_name, "phandle") ||
+		!of_prop_cmp(prop_name, "linux,phandle");
+}
+
 #endif /* _LINUX_OF_PRIVATE_H */
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 434f6dd6a86c1ffad2b0d490b2b612a5147994c5..5a51c52b9729af2ab77b5a9365cb72d30740f3b0 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -304,9 +304,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
 	int ret = 0;
 
 	if (target->in_livetree)
-		if (!of_prop_cmp(overlay_prop->name, "name") ||
-		    !of_prop_cmp(overlay_prop->name, "phandle") ||
-		    !of_prop_cmp(overlay_prop->name, "linux,phandle"))
+		if (is_pseudo_property(overlay_prop->name))
 			return 0;
 
 	if (target->in_livetree)
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 779db058c42f5b8198ee3417dfaab80c81b43e4c..31eb80d894ec569e5b7538cbc07895803ca7d402 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -161,9 +161,7 @@ static int adjust_local_phandle_references(const struct device_node *local_fixup
 	for_each_property_of_node(local_fixups, prop_fix) {
 
 		/* skip properties added automatically */
-		if (!of_prop_cmp(prop_fix->name, "name") ||
-		    !of_prop_cmp(prop_fix->name, "phandle") ||
-		    !of_prop_cmp(prop_fix->name, "linux,phandle"))
+		if (is_pseudo_property(prop_fix->name))
 			continue;
 
 		if ((prop_fix->length % 4) != 0 || prop_fix->length == 0)

-- 
2.34.1

Re: [PATCH 2/5] of: Introduce and apply private is_pseudo_property()

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 22:27:58 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> There are several places which check if a property name is one of
> 'name'|'phandle'|'linux,phandle'.
> 
> Introduce and apply private is_pseudo_property() for the check.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c       | 4 +---
>  drivers/of/of_private.h | 7 +++++++
>  drivers/of/overlay.c    | 4 +---
>  drivers/of/resolver.c   | 4 +---
>  4 files changed, 10 insertions(+), 9 deletions(-)
> 

Applied, thanks!

[PATCH 3/5] of: Correct property name comparison in __of_add_property()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

__of_add_property() compares property name by strcmp(), and that is
improper for SPARC which wants strcasecmp().

Fix by using dedicated property name comparison macro of_prop_cmp().

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 001ff6ce4abf85c07f13649d5a9f691f549a8ccc..c810014957e81171675b63f25eaabe391cc903e4 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1651,7 +1651,7 @@ int __of_add_property(struct device_node *np, struct property *prop)
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
-		if (strcmp(prop->name, (*next)->name) == 0) {
+		if (of_prop_cmp(prop->name, (*next)->name) == 0) {
 			/* duplicate ! don't insert it */
 			rc = -EEXIST;
 			goto out_unlock;

-- 
2.34.1

Re: [PATCH 3/5] of: Correct property name comparison in __of_add_property()

[Rob Herring]

On Mon, Feb 24, 2025 at 10:27:59PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> __of_add_property() compares property name by strcmp(), and that is
> improper for SPARC which wants strcasecmp().

Except that 'name' is the nodename (usually, with a few rare 
exceptions). Sparc node names are case sensitive, so strcmp was correct. 

My hope is to get rid of case insensitive comparisions, so if nothing 
cares that we're doing a case sensitive comparision, I want to keep 
that. 

I also want to get rid of storing both 'name' as a property and 
device_node.name. The name property is an ABI issue though if no one is 
looking, then it's not an ABI issue. Also, we should be able to generate 
device_node.name from device_node.full_name. There's still a bunch of 
direct users of device_node.name which have to be fixed. Mostly in clock 
drivers from what I remember.

> Fix by using dedicated property name comparison macro of_prop_cmp().
> 
> 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 001ff6ce4abf85c07f13649d5a9f691f549a8ccc..c810014957e81171675b63f25eaabe391cc903e4 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1651,7 +1651,7 @@ int __of_add_property(struct device_node *np, struct property *prop)
>  	prop->next = NULL;
>  	next = &np->properties;
>  	while (*next) {
> -		if (strcmp(prop->name, (*next)->name) == 0) {
> +		if (of_prop_cmp(prop->name, (*next)->name) == 0) {
>  			/* duplicate ! don't insert it */
>  			rc = -EEXIST;
>  			goto out_unlock;
> 
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] of: Correct property name comparison in __of_add_property()

[Zijun Hu]

On 2025/2/25 22:38, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 10:27:59PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> __of_add_property() compares property name by strcmp(), and that is
>> improper for SPARC which wants strcasecmp().
> Except that 'name' is the nodename (usually, with a few rare 
> exceptions). Sparc node names are case sensitive, so strcmp was correct. 

Here, it is property's name and NOT node's name.

arch/sparc/include/asm/prom.h:

#define of_compat_cmp(s1, s2, l) strncmp((s1), (s2), (l))
#define of_prop_cmp(s1, s2)	strcasecmp((s1), (s2)) //SPARC HERE
#define of_node_cmp(s1, s2)	strcmp((s1), (s2))


include/linux/of.h:
/* Default string compare functions, Allow arch asm/prom.h to override */
#if !defined(of_compat_cmp)
#define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2))
#define of_prop_cmp(s1, s2)	strcmp((s1), (s2))      // others HERE
#define of_node_cmp(s1, s2)	strcasecmp((s1), (s2))
#endif

actually, later __of_update_property() and __of_find_property() use
of_prop_cmp() instead of strcmp() to compare property name.

perhaps, compatible and node name also have similar issues in current
OF codes.

Re: [PATCH 3/5] of: Correct property name comparison in __of_add_property()

[Rob Herring]

On Tue, Feb 25, 2025 at 11:04:55PM +0800, Zijun Hu wrote:
> On 2025/2/25 22:38, Rob Herring wrote:
> > On Mon, Feb 24, 2025 at 10:27:59PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> __of_add_property() compares property name by strcmp(), and that is
> >> improper for SPARC which wants strcasecmp().
> > Except that 'name' is the nodename (usually, with a few rare 
> > exceptions). Sparc node names are case sensitive, so strcmp was correct. 
> 
> Here, it is property's name and NOT node's name.

Sigh, indeed... Applied.

Rob

[PATCH 4/5] of: Do not change property state under __of_add_property() failure

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Do not remove the property from list @np->deadprops if
__of_add_property() encounters -EEXIST failure.

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 c810014957e81171675b63f25eaabe391cc903e4..47cae6e48a48a7e1312c25fc5267bcf39102bbe9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1646,9 +1646,6 @@ int __of_add_property(struct device_node *np, struct property *prop)
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 
-	__of_remove_property_from_list(&np->deadprops, prop);
-
-	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
 		if (of_prop_cmp(prop->name, (*next)->name) == 0) {
@@ -1660,6 +1657,9 @@ int __of_add_property(struct device_node *np, struct property *prop)
 	}
 	*next = prop;
 
+	__of_remove_property_from_list(&np->deadprops, prop);
+	prop->next = NULL;
+
 out_unlock:
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	if (rc)

-- 
2.34.1

Re: [PATCH 4/5] of: Do not change property state under __of_add_property() failure

[Rob Herring]

On Mon, Feb 24, 2025 at 10:28:00PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Do not remove the property from list @np->deadprops if
> __of_add_property() encounters -EEXIST failure.

A property can never be on both np->deadprops and np->props.

> 
> 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 c810014957e81171675b63f25eaabe391cc903e4..47cae6e48a48a7e1312c25fc5267bcf39102bbe9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1646,9 +1646,6 @@ int __of_add_property(struct device_node *np, struct property *prop)
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  
> -	__of_remove_property_from_list(&np->deadprops, prop);
> -
> -	prop->next = NULL;
>  	next = &np->properties;
>  	while (*next) {
>  		if (of_prop_cmp(prop->name, (*next)->name) == 0) {
> @@ -1660,6 +1657,9 @@ int __of_add_property(struct device_node *np, struct property *prop)
>  	}
>  	*next = prop;
>  
> +	__of_remove_property_from_list(&np->deadprops, prop);
> +	prop->next = NULL;
> +
>  out_unlock:
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	if (rc)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 4/5] of: Do not change property state under __of_add_property() failure

[Zijun Hu]

On 2025/2/25 22:41, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 10:28:00PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Do not remove the property from list @np->deadprops if
>> __of_add_property() encounters -EEXIST failure.
> A property can never be on both np->deadprops and np->props.

i made this patch based on convention that

This change is made base on convention that tasks which have
been done successfully needs to be undid if fails to do the
remaining task.


It is okay to drop this patch based on condition you mentioned.

thank you. (^^)(^^)

Re: [PATCH 4/5] of: Do not change property state under __of_add_property() failure

[Zijun Hu]

On 2025/2/25 22:41, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 10:28:00PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Do not remove the property from list @np->deadprops if
>> __of_add_property() encounters -EEXIST failure.
> A property can never be on both np->deadprops and np->props.

i made this patch based on convention that

partial task which has been done successfully needs to be undid if fails
to do the remaining task.

It is okay to drop this patch based on condition you mentioned.
thank you. (^^)(^^)

[PATCH 5/5] of/platform: Do not use of_get_property() to test property presence

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Use of_property_present() instead of of_get_property() to test property
'compatible' presence in of_platform_bus_create().

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

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c6d8afb284e88061eb6fb0ba02e429cec702664c..242172e4b8757eec9a7ccb413764b475046dbae8 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -334,7 +334,7 @@ static int of_platform_bus_create(struct device_node *bus,
 	int rc = 0;
 
 	/* Make sure it has a compatible property */
-	if (strict && (!of_get_property(bus, "compatible", NULL))) {
+	if (strict && (!of_property_present(bus, "compatible"))) {
 		pr_debug("%s() - skipping %pOF, no compatible prop\n",
 			 __func__, bus);
 		return 0;

-- 
2.34.1

Re: [PATCH 5/5] of/platform: Do not use of_get_property() to test property presence

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 22:28:01 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Use of_property_present() instead of of_get_property() to test property
> 'compatible' presence in of_platform_bus_create().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks!