* [PATCH v2 1/2] of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw()
2024-05-29 19:59 [PATCH v2 0/2] of: Fix interrupt-map for fw_devlink Rob Herring (Arm)
@ 2024-05-29 19:59 ` Rob Herring (Arm)
2024-05-30 13:46 ` Marc Zyngier
2024-05-29 19:59 ` [PATCH v2 2/2] of: property: Fix fw_devlink handling of interrupt-map Rob Herring (Arm)
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Rob Herring (Arm) @ 2024-05-29 19:59 UTC (permalink / raw)
To: Saravana Kannan, Anup Patel, Marc Zyngier
Cc: devicetree, linux-kernel, linux-arm-kernel, linux-riscv
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/2] of: property: Fix fw_devlink handling of interrupt-map
2024-05-29 19:59 [PATCH v2 0/2] of: Fix interrupt-map for fw_devlink Rob Herring (Arm)
2024-05-29 19:59 ` [PATCH v2 1/2] of/irq: Factor out parsing of interrupt-map parent phandle+args from of_irq_parse_raw() Rob Herring (Arm)
@ 2024-05-29 19:59 ` Rob Herring (Arm)
2024-05-30 13:54 ` [PATCH v2 0/2] of: Fix interrupt-map for fw_devlink Marc Zyngier
2024-05-30 14:52 ` Anup Patel
3 siblings, 0 replies; 6+ messages in thread
From: Rob Herring (Arm) @ 2024-05-29 19:59 UTC (permalink / raw)
To: Saravana Kannan, Anup Patel, Marc Zyngier
Cc: devicetree, linux-kernel, linux-arm-kernel, linux-riscv
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
^ permalink raw reply related [flat|nested] 6+ messages in thread