[PATCH 0/3] of: Reimplement of_(bus_)?n_(size|addr)_cells()

[PATCH 0/3] of: Reimplement of_(bus_)?n_(size|addr)_cells()

[Rob Herring (Arm)]

This series reworks the of_(bus_)?n_(size|addr)_cells() functions. They 
fail to hold the DT spinlock while accessing 'parent' pointer and don't 
hold a reference to the parent node. Neither is likely a real issue as 
most nodes are static.

With these issues fixed, we can then replace the open coded version in 
of_irq_parse_raw().

This series depends on the fixes from this series[1].

Rob

[1] https://lore.kernel.org/lkml/20240529-dt-interrupt-map-fix-v2-0-ef86dc5bcd2a@kernel.org/

---
Rob Herring (Arm) (3):
      of: Add an iterator to walk up parent nodes
      of: Add missing locking to of_(bus_)?n_(size|addr)_cells()
      of/irq: Use of_bus_n_addr_cells() to retrieve "#address-cells"

 drivers/of/base.c  | 18 ++++++++----------
 drivers/of/irq.c   | 15 +++------------
 include/linux/of.h |  5 +++++
 3 files changed, 16 insertions(+), 22 deletions(-)
---
base-commit: e7985f43609c782132f8f5794ee6cc4cdb66ca75
change-id: 20240530-dt-interrupt-map-fix-6946101b1391

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

[PATCH 1/3] of: Add an iterator to walk up parent nodes

[Rob Herring (Arm)]

Similar to other node iterators, add one for walking up parent
nodes. The iterator starts on the current node, not the immediate
parent as that seems to be the common case and starting with the parent
node can be implemented like this:

for_each_parent_of_node_scoped(parent, of_get_parent(node))

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 include/linux/of.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index a0bedd038a05..c322802dfc2b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1482,6 +1482,11 @@ static inline int of_property_read_s32(const struct device_node *np,
 	     child != NULL;						\
 	     child = of_get_next_available_child(parent, child))
 
+#define for_each_parent_of_node_scoped(parent, node) \
+	for (struct device_node *parent __free(device_node) = \
+	     of_node_get(node) ; \
+	     parent; parent = of_get_next_parent(parent))
+
 #define for_each_of_cpu_node(cpu) \
 	for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
 	     cpu = of_get_next_cpu_node(cpu))

-- 
2.43.0

[PATCH 2/3] of: Add missing locking to of_(bus_)?n_(size|addr)_cells()

[Rob Herring (Arm)]

When accessing parent/child/sibling pointers the DT spinlock needs to
be held. The of_(bus_)?n_(size|addr)_cells() functions are missing that
when walking up the parent nodes. In reality, it rarely matters as most
nodes are static.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/base.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20603d3c9931..61fff13bbee5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -91,8 +91,8 @@ int of_bus_n_addr_cells(struct device_node *np)
 {
 	u32 cells;
 
-	for (; np; np = np->parent)
-		if (!of_property_read_u32(np, "#address-cells", &cells))
+	for_each_parent_of_node_scoped(parent, np)
+		if (!of_property_read_u32(parent, "#address-cells", &cells))
 			return cells;
 
 	/* No #address-cells property for the root node */
@@ -101,10 +101,9 @@ int of_bus_n_addr_cells(struct device_node *np)
 
 int of_n_addr_cells(struct device_node *np)
 {
-	if (np->parent)
-		np = np->parent;
+	struct device_node *parent __free(device_node) = of_get_parent(np);
 
-	return of_bus_n_addr_cells(np);
+	return of_bus_n_addr_cells(parent);
 }
 EXPORT_SYMBOL(of_n_addr_cells);
 
@@ -112,8 +111,8 @@ int of_bus_n_size_cells(struct device_node *np)
 {
 	u32 cells;
 
-	for (; np; np = np->parent)
-		if (!of_property_read_u32(np, "#size-cells", &cells))
+	for_each_parent_of_node_scoped(parent, np)
+		if (!of_property_read_u32(parent, "#size-cells", &cells))
 			return cells;
 
 	/* No #size-cells property for the root node */
@@ -122,10 +121,9 @@ int of_bus_n_size_cells(struct device_node *np)
 
 int of_n_size_cells(struct device_node *np)
 {
-	if (np->parent)
-		np = np->parent;
+	struct device_node *parent __free(device_node) = of_get_parent(np);
 
-	return of_bus_n_size_cells(np);
+	return of_bus_n_size_cells(parent);
 }
 EXPORT_SYMBOL(of_n_size_cells);
 

-- 
2.43.0

Re: [PATCH 2/3] of: Add missing locking to of_(bus_)?n_(size|addr)_cells()

[Rob Herring]

On Thu, May 30, 2024 at 08:03:28PM -0500, Rob Herring (Arm) wrote:
> When accessing parent/child/sibling pointers the DT spinlock needs to
> be held. The of_(bus_)?n_(size|addr)_cells() functions are missing that
> when walking up the parent nodes. In reality, it rarely matters as most
> nodes are static.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/base.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20603d3c9931..61fff13bbee5 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -91,8 +91,8 @@ int of_bus_n_addr_cells(struct device_node *np)
>  {
>  	u32 cells;
>  
> -	for (; np; np = np->parent)
> -		if (!of_property_read_u32(np, "#address-cells", &cells))
> +	for_each_parent_of_node_scoped(parent, np)
> +		if (!of_property_read_u32(parent, "#address-cells", &cells))
>  			return cells;
>  
>  	/* No #address-cells property for the root node */
> @@ -101,10 +101,9 @@ int of_bus_n_addr_cells(struct device_node *np)
>  
>  int of_n_addr_cells(struct device_node *np)
>  {
> -	if (np->parent)
> -		np = np->parent;

This isn't going to work... This drops of_n_addr_cells working for the 
root node. Callers wanting to get root node's properties need to use the 
of_bus_n variant instead, so those callers will have to be checked and 
fixed first.

Rob

[PATCH 3/3] of/irq: Use of_bus_n_addr_cells() to retrieve "#address-cells"

[Rob Herring (Arm)]

of_irq_parse_raw() is open coding what of_bus_n_addr_cells() does, so
replace it with of_bus_n_addr_cells().

Note that the original code would use 2 cells if #address-cells was not
found. That doesn't match the default of 1 for anything but Sparc which
doesn't use this code.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/irq.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 462375b293e4..d81ee880a553 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -165,10 +165,10 @@ const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_ph
  */
 int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 {
-	struct device_node *ipar, *tnode, *old = NULL;
+	struct device_node *ipar, *tnode;
 	__be32 initial_match_array[MAX_PHANDLE_ARGS];
 	const __be32 *match_array = initial_match_array;
-	const __be32 *tmp, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
+	const __be32 dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
 	u32 intsize = 1, addrsize;
 	int i, rc = -EINVAL;
 
@@ -202,16 +202,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 	/* Look for this #address-cells. We have to implement the old linux
 	 * trick of looking for the parent here as some device-trees rely on it
 	 */
-	old = of_node_get(ipar);
-	do {
-		tmp = of_get_property(old, "#address-cells", NULL);
-		tnode = of_get_parent(old);
-		of_node_put(old);
-		old = tnode;
-	} while (old && tmp == NULL);
-	of_node_put(old);
-	old = NULL;
-	addrsize = (tmp == NULL) ? 2 : be32_to_cpu(*tmp);
+	addrsize = of_bus_n_addr_cells(ipar);
 
 	pr_debug(" -> addrsize=%d\n", addrsize);
 

-- 
2.43.0