[PATCH v2 0/2] of: Fix interrupt-map for fw_devlink

[PATCH v2 0/2] of: Fix interrupt-map for fw_devlink

[Rob Herring (Arm)]

The duplicated parsing continued to bother me, so I've refactored things 
to avoid that for parsing the interrupt parent and args in the 
interrupt-map.

It passes testing with unittests on QEMU virt platform, but I don't 
think that catches the problematic cases. So please test.

v1: https://lore.kernel.org/all/20240528164132.2451685-1-maz@kernel.org/
 - Refactor existing interrupt-map parsing code and use it for 
   fw_devlink

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Marc Zyngier (1):
      of: property: Fix fw_devlink handling of interrupt-map

Rob Herring (Arm) (1):
      of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()

 drivers/of/irq.c        | 127 +++++++++++++++++++++++++++++-------------------
 drivers/of/of_private.h |   3 ++
 drivers/of/property.c   |  30 ++++--------
 3 files changed, 89 insertions(+), 71 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240529-dt-interrupt-map-fix-a37b9aff5ca0

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

[PATCH v2 1/2] of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()

[Rob Herring (Arm)]

Factor out the parsing of interrupt-map interrupt parent phandle and its
arg cells to a separate function, of_irq_parse_imap_parent(), so that it
can be used in other parsing scenarios (e.g. fw_devlink).

There was a refcount leak on non-matching entries when iterating thru
"interrupt-map" which is fixed.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/irq.c        | 127 +++++++++++++++++++++++++++++-------------------
 drivers/of/of_private.h |   3 ++
 2 files changed, 79 insertions(+), 51 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 174900072c18..a7cdb892991e 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -25,6 +25,8 @@
 #include <linux/string.h>
 #include <linux/slab.h>
 
+#include "of_private.h"
+
 /**
  * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
  * @dev: Device node of the device whose interrupt is to be mapped
@@ -96,6 +98,59 @@ static const char * const of_irq_imap_abusers[] = {
 	NULL,
 };
 
+const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_phandle_args *out_irq)
+{
+	u32 intsize, addrsize;
+	struct device_node *np;
+
+	/* Get the interrupt parent */
+	if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
+		np = of_node_get(of_irq_dflt_pic);
+	else
+		np = of_find_node_by_phandle(be32_to_cpup(imap));
+	imap++;
+
+	/* Check if not found */
+	if (!np) {
+		pr_debug(" -> imap parent not found !\n");
+		return NULL;
+	}
+
+	/* Get #interrupt-cells and #address-cells of new
+		* parent
+		*/
+	if (of_property_read_u32(np, "#interrupt-cells",
+					&intsize)) {
+		pr_debug(" -> parent lacks #interrupt-cells!\n");
+		of_node_put(np);
+		return NULL;
+	}
+	if (of_property_read_u32(np, "#address-cells",
+					&addrsize))
+		addrsize = 0;
+
+	pr_debug(" -> intsize=%d, addrsize=%d\n",
+		intsize, addrsize);
+
+	/* Check for malformed properties */
+	if (WARN_ON(addrsize + intsize > MAX_PHANDLE_ARGS)
+		|| (len < (addrsize + intsize))) {
+		of_node_put(np);
+		return NULL;
+	}
+
+	pr_debug(" -> imaplen=%d\n", len);
+
+	imap += addrsize + intsize;
+
+	out_irq->np = np;
+	for (int i = 0; i < intsize; i++)
+		out_irq->args[i] = be32_to_cpup(imap - intsize + i);
+	out_irq->args_count = intsize;
+
+	return imap;
+}
+
 /**
  * of_irq_parse_raw - Low level interrupt tree parsing
  * @addr:	address specifier (start of "reg" property of the device) in be32 format
@@ -112,12 +167,12 @@ static const char * const of_irq_imap_abusers[] = {
  */
 int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 {
-	struct device_node *ipar, *tnode, *old = NULL, *newpar = NULL;
+	struct device_node *ipar, *tnode, *old = NULL;
 	__be32 initial_match_array[MAX_PHANDLE_ARGS];
 	const __be32 *match_array = initial_match_array;
-	const __be32 *tmp, *imap, *imask, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
-	u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0;
-	int imaplen, match, i, rc = -EINVAL;
+	const __be32 *tmp, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
+	u32 intsize = 1, addrsize;
+	int i, rc = -EINVAL;
 
 #ifdef DEBUG
 	of_print_phandle_args("of_irq_parse_raw: ", out_irq);
@@ -176,6 +231,9 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 
 	/* Now start the actual "proper" walk of the interrupt tree */
 	while (ipar != NULL) {
+		int imaplen, match;
+		const __be32 *imap, *oldimap, *imask;
+		struct device_node *newpar;
 		/*
 		 * Now check if cursor is an interrupt-controller and
 		 * if it is then we are done, unless there is an
@@ -216,7 +274,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 
 		/* Parse interrupt-map */
 		match = 0;
-		while (imaplen > (addrsize + intsize + 1) && !match) {
+		while (imaplen > (addrsize + intsize + 1)) {
 			/* Compare specifiers */
 			match = 1;
 			for (i = 0; i < (addrsize + intsize); i++, imaplen--)
@@ -224,48 +282,17 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 
 			pr_debug(" -> match=%d (imaplen=%d)\n", match, imaplen);
 
-			/* Get the interrupt parent */
-			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
-				newpar = of_node_get(of_irq_dflt_pic);
-			else
-				newpar = of_find_node_by_phandle(be32_to_cpup(imap));
-			imap++;
-			--imaplen;
-
-			/* Check if not found */
-			if (newpar == NULL) {
-				pr_debug(" -> imap parent not found !\n");
-				goto fail;
-			}
-
-			if (!of_device_is_available(newpar))
-				match = 0;
-
-			/* Get #interrupt-cells and #address-cells of new
-			 * parent
-			 */
-			if (of_property_read_u32(newpar, "#interrupt-cells",
-						 &newintsize)) {
-				pr_debug(" -> parent lacks #interrupt-cells!\n");
-				goto fail;
-			}
-			if (of_property_read_u32(newpar, "#address-cells",
-						 &newaddrsize))
-				newaddrsize = 0;
-
-			pr_debug(" -> newintsize=%d, newaddrsize=%d\n",
-			    newintsize, newaddrsize);
-
-			/* Check for malformed properties */
-			if (WARN_ON(newaddrsize + newintsize > MAX_PHANDLE_ARGS)
-			    || (imaplen < (newaddrsize + newintsize))) {
-				rc = -EFAULT;
+			oldimap = imap;
+			imap = of_irq_parse_imap_parent(oldimap, imaplen, out_irq);
+			if (!imap)
 				goto fail;
-			}
 
-			imap += newaddrsize + newintsize;
-			imaplen -= newaddrsize + newintsize;
+			match &= of_device_is_available(out_irq->np);
+			if (match)
+				break;
 
+			of_node_put(out_irq->np);
+			imaplen -= imap - oldimap;
 			pr_debug(" -> imaplen=%d\n", imaplen);
 		}
 		if (!match) {
@@ -287,11 +314,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		 * Successfully parsed an interrupt-map translation; copy new
 		 * interrupt specifier into the out_irq structure
 		 */
-		match_array = imap - newaddrsize - newintsize;
-		for (i = 0; i < newintsize; i++)
-			out_irq->args[i] = be32_to_cpup(imap - newintsize + i);
-		out_irq->args_count = intsize = newintsize;
-		addrsize = newaddrsize;
+		match_array = oldimap + 1;
+
+		newpar = out_irq->np;
+		intsize = out_irq->args_count;
+		addrsize = (imap - match_array) - intsize;
 
 		if (ipar == newpar) {
 			pr_debug("%pOF interrupt-map entry to self\n", ipar);
@@ -300,7 +327,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 
 	skiplevel:
 		/* Iterate again with new parent */
-		out_irq->np = newpar;
 		pr_debug(" -> new parent: %pOF\n", newpar);
 		of_node_put(ipar);
 		ipar = newpar;
@@ -310,7 +336,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 
  fail:
 	of_node_put(ipar);
-	of_node_put(newpar);
 
 	return rc;
 }
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 94fc0aa07af9..04aa2a91f851 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -159,6 +159,9 @@ extern void __of_sysfs_remove_bin_file(struct device_node *np,
 extern int of_bus_n_addr_cells(struct device_node *np);
 extern int of_bus_n_size_cells(struct device_node *np);
 
+const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len,
+				       struct of_phandle_args *out_irq);
+
 struct bus_dma_region;
 #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
 int of_dma_get_range(struct device_node *np,

-- 
2.43.0

[PATCH v2 2/2] of: property: Fix fw_devlink handling of interrupt-map

[Rob Herring (Arm)]

From: Marc Zyngier <maz@kernel.org>

Commit d976c6f4b32c ("of: property: Add fw_devlink support for
interrupt-map property") tried to do what it says on the tin,
but failed on a couple of points:

- it confuses bytes and cells. Not a huge deal, except when it
  comes to pointer arithmetic

- it doesn't really handle anything but interrupt-maps that have
  their parent #address-cells set to 0

The combinations of the two leads to some serious fun on my M1
box, with plenty of WARN-ON() firing all over the shop, and
amusing values being generated for interrupt specifiers.

Having 2 versions of parsing code for "interrupt-map" was a bad
idea. Now that the common parsing parts have been refactored
into of_irq_parse_imap_parent(), rework the code here to use it
instead and fix the pointer arithmetic.

Note that the dependency will be a bit different than the original code
when the interrupt-map points to another interrupt-map. In this case,
the original code would resolve to the final interrupt controller. Now
the dependency is the parent interrupt-map (which itself should have a
dependency to the parent). It is possible that a node with an
interrupt-map has no driver.

Fixes: d976c6f4b32c ("of: property: Add fw_devlink support for interrupt-map property")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Cc: Anup Patel <apatel@ventanamicro.com>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/property.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 1c83e68f805b..164d77cb9445 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1306,10 +1306,10 @@ static struct device_node *parse_interrupts(struct device_node *np,
 static struct device_node *parse_interrupt_map(struct device_node *np,
 					       const char *prop_name, int index)
 {
-	const __be32 *imap, *imap_end, *addr;
+	const __be32 *imap, *imap_end;
 	struct of_phandle_args sup_args;
 	u32 addrcells, intcells;
-	int i, imaplen;
+	int imaplen;
 
 	if (!IS_ENABLED(CONFIG_OF_IRQ))
 		return NULL;
@@ -1322,33 +1322,23 @@ 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);
-	if (!imap || imaplen <= (addrcells + intcells))
+	imaplen /= sizeof(*imap);
+	if (!imap)
 		return NULL;
-	imap_end = imap + imaplen;
 
-	while (imap < imap_end) {
-		addr = imap;
-		imap += addrcells;
+	imap_end = imap + imaplen;
 
-		sup_args.np = np;
-		sup_args.args_count = intcells;
-		for (i = 0; i < intcells; i++)
-			sup_args.args[i] = be32_to_cpu(imap[i]);
-		imap += intcells;
+	for (int i = 0; imap + addrcells + intcells + 1 < imap_end; i++) {
+		imap += addrcells + intcells;
 
-		/*
-		 * Upon success, the function of_irq_parse_raw() returns
-		 * interrupt controller DT node pointer in sup_args.np.
-		 */
-		if (of_irq_parse_raw(addr, &sup_args))
+		imap = of_irq_parse_imap_parent(imap, imap_end - imap, &sup_args);
+		if (!imap)
 			return NULL;
 
-		if (!index)
+		if (i == index)
 			return sup_args.np;
 
 		of_node_put(sup_args.np);
-		imap += sup_args.args_count + 1;
-		index--;
 	}
 
 	return NULL;

-- 
2.43.0

Re: [PATCH v2 1/2] of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()

[Marc Zyngier]

On Wed, 29 May 2024 20:59:20 +0100,
"Rob Herring (Arm)" <robh@kernel.org> wrote:
> 
> Factor out the parsing of interrupt-map interrupt parent phandle and its
> arg cells to a separate function, of_irq_parse_imap_parent(), so that it
> can be used in other parsing scenarios (e.g. fw_devlink).
> 
> There was a refcount leak on non-matching entries when iterating thru
> "interrupt-map" which is fixed.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/irq.c        | 127 +++++++++++++++++++++++++++++-------------------
>  drivers/of/of_private.h |   3 ++
>  2 files changed, 79 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 174900072c18..a7cdb892991e 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -25,6 +25,8 @@
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  
> +#include "of_private.h"
> +
>  /**
>   * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
>   * @dev: Device node of the device whose interrupt is to be mapped
> @@ -96,6 +98,59 @@ static const char * const of_irq_imap_abusers[] = {
>  	NULL,
>  };
>  
> +const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_phandle_args *out_irq)
> +{
> +	u32 intsize, addrsize;
> +	struct device_node *np;
> +
> +	/* Get the interrupt parent */
> +	if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
> +		np = of_node_get(of_irq_dflt_pic);
> +	else
> +		np = of_find_node_by_phandle(be32_to_cpup(imap));
> +	imap++;
> +
> +	/* Check if not found */
> +	if (!np) {
> +		pr_debug(" -> imap parent not found !\n");
> +		return NULL;
> +	}
> +
> +	/* Get #interrupt-cells and #address-cells of new
> +		* parent
> +		*/

nit: funky formatting.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/2] of: Fix interrupt-map for fw_devlink

[Marc Zyngier]

On Wed, 29 May 2024 20:59:19 +0100,
"Rob Herring (Arm)" <robh@kernel.org> wrote:
> 
> The duplicated parsing continued to bother me, so I've refactored things 
> to avoid that for parsing the interrupt parent and args in the 
> interrupt-map.
> 
> It passes testing with unittests on QEMU virt platform, but I don't 
> think that catches the problematic cases. So please test.
> 
> v1: https://lore.kernel.org/all/20240528164132.2451685-1-maz@kernel.org/
>  - Refactor existing interrupt-map parsing code and use it for 
>    fw_devlink
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Marc Zyngier (1):
>       of: property: Fix fw_devlink handling of interrupt-map
> 
> Rob Herring (Arm) (1):
>       of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()
> 
>  drivers/of/irq.c        | 127 +++++++++++++++++++++++++++++-------------------
>  drivers/of/of_private.h |   3 ++
>  drivers/of/property.c   |  30 ++++--------
>  3 files changed, 89 insertions(+), 71 deletions(-)

I've just gave it a go on an M1 and as a kvmtool guest, and nothing
caught fire. Must be perfect.

Tested-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/2] of: Fix interrupt-map for fw_devlink

[Anup Patel]

On Thu, May 30, 2024 at 1:29 AM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> The duplicated parsing continued to bother me, so I've refactored things
> to avoid that for parsing the interrupt parent and args in the
> interrupt-map.
>
> It passes testing with unittests on QEMU virt platform, but I don't
> think that catches the problematic cases. So please test.
>
> v1: https://lore.kernel.org/all/20240528164132.2451685-1-maz@kernel.org/
>  - Refactor existing interrupt-map parsing code and use it for
>    fw_devlink
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Marc Zyngier (1):
>       of: property: Fix fw_devlink handling of interrupt-map
>
> Rob Herring (Arm) (1):
>       of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()
>
>  drivers/of/irq.c        | 127 +++++++++++++++++++++++++++++-------------------
>  drivers/of/of_private.h |   3 ++
>  drivers/of/property.c   |  30 ++++--------
>  3 files changed, 89 insertions(+), 71 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240529-dt-interrupt-map-fix-a37b9aff5ca0
>
> Best regards,
> --
> Rob Herring (Arm) <robh@kernel.org>
>

Works well for RISC-V, Thanks!

Tested-by: Anup Patel <apatel@ventanamicro.com>

Regards,
Anup