Some of the PCI host controllers (such as generic PCI host controller)
use "interrupt-map" DT property to describe the mapping between PCI
endpoints and PCI interrupt pins. This is the only case where the
interrupts are not described in DT.
Currently, there is no fw_devlink created based on "interrupt-map"
DT property so interrupt controller is not guaranteed to be probed
before the PCI host controller. This affects every platform where
both PCI host controller and interrupt controllers are probed as
regular platform devices.
This creates fw_devlink between consumers (PCI host controller) and
supplier (interrupt controller) based on "interrupt-map" DT property.
Signed-off-by: Anup Patel <[email protected]>
Reviewed-by: Saravana Kannan <[email protected]>
---
Changes since v3:
- Added a comment about of_irq_parse_raw()
- Removed redundant NULL assignments to sup_args.np
Changes since v2:
- No need for a loop to find #interrupt-cells property value
- Fix node de-reference leak when index is greater than number
of entries in interrupt-map property
Changes since v1:
- Updated commit description based on Rob's suggestion
- Use of_irq_parse_raw() for parsing interrupt-map DT property
---
drivers/of/property.c | 52 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index a6358ee99b74..2d749a18b037 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1311,6 +1311,57 @@ static struct device_node *parse_interrupts(struct device_node *np,
return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
}
+static struct device_node *parse_interrupt_map(struct device_node *np,
+ const char *prop_name, int index)
+{
+ const __be32 *imap, *imap_end, *addr;
+ struct of_phandle_args sup_args;
+ u32 addrcells, intcells;
+ int i, imaplen;
+
+ if (!IS_ENABLED(CONFIG_OF_IRQ))
+ return NULL;
+
+ if (strcmp(prop_name, "interrupt-map"))
+ return NULL;
+
+ if (of_property_read_u32(np, "#interrupt-cells", &intcells))
+ return NULL;
+ addrcells = of_bus_n_addr_cells(np);
+
+ imap = of_get_property(np, "interrupt-map", &imaplen);
+ if (!imap || imaplen <= (addrcells + intcells))
+ return NULL;
+ imap_end = imap + imaplen;
+
+ while (imap < imap_end) {
+ addr = imap;
+ imap += addrcells;
+
+ 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;
+
+ /*
+ * 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))
+ return NULL;
+
+ if (!index)
+ return sup_args.np;
+
+ of_node_put(sup_args.np);
+ imap += sup_args.args_count + 1;
+ index--;
+ }
+
+ return NULL;
+}
+
static struct device_node *parse_remote_endpoint(struct device_node *np,
const char *prop_name,
int index)
@@ -1359,6 +1410,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_msi_parent, },
{ .parse_prop = parse_gpio_compat, },
{ .parse_prop = parse_interrupts, },
+ { .parse_prop = parse_interrupt_map, },
{ .parse_prop = parse_regulators, },
{ .parse_prop = parse_gpio, },
{ .parse_prop = parse_gpios, },
--
2.34.1
On Thu, 09 May 2024 17:38:20 +0530, Anup Patel wrote:
> Some of the PCI host controllers (such as generic PCI host controller)
> use "interrupt-map" DT property to describe the mapping between PCI
> endpoints and PCI interrupt pins. This is the only case where the
> interrupts are not described in DT.
>
> Currently, there is no fw_devlink created based on "interrupt-map"
> DT property so interrupt controller is not guaranteed to be probed
> before the PCI host controller. This affects every platform where
> both PCI host controller and interrupt controllers are probed as
> regular platform devices.
>
> This creates fw_devlink between consumers (PCI host controller) and
> supplier (interrupt controller) based on "interrupt-map" DT property.
>
> Signed-off-by: Anup Patel <[email protected]>
> Reviewed-by: Saravana Kannan <[email protected]>
> ---
> Changes since v3:
> - Added a comment about of_irq_parse_raw()
> - Removed redundant NULL assignments to sup_args.np
> Changes since v2:
> - No need for a loop to find #interrupt-cells property value
> - Fix node de-reference leak when index is greater than number
> of entries in interrupt-map property
> Changes since v1:
> - Updated commit description based on Rob's suggestion
> - Use of_irq_parse_raw() for parsing interrupt-map DT property
> ---
> drivers/of/property.c | 52 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
Applied, thanks!
Hello:
This patch was applied to riscv/linux.git (fixes)
by Rob Herring (Arm) <[email protected]>:
On Thu, 9 May 2024 17:38:20 +0530 you wrote:
> Some of the PCI host controllers (such as generic PCI host controller)
> use "interrupt-map" DT property to describe the mapping between PCI
> endpoints and PCI interrupt pins. This is the only case where the
> interrupts are not described in DT.
>
> Currently, there is no fw_devlink created based on "interrupt-map"
> DT property so interrupt controller is not guaranteed to be probed
> before the PCI host controller. This affects every platform where
> both PCI host controller and interrupt controllers are probed as
> regular platform devices.
>
> [...]
Here is the summary with links:
- [v4] of: property: Add fw_devlink support for interrupt-map property
https://git.kernel.org/riscv/c/d976c6f4b32c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Thu, 09 May 2024 13:08:20 +0100,
Anup Patel <[email protected]> wrote:
>
> Some of the PCI host controllers (such as generic PCI host controller)
> use "interrupt-map" DT property to describe the mapping between PCI
> endpoints and PCI interrupt pins. This is the only case where the
> interrupts are not described in DT.
>
> Currently, there is no fw_devlink created based on "interrupt-map"
> DT property so interrupt controller is not guaranteed to be probed
> before the PCI host controller. This affects every platform where
> both PCI host controller and interrupt controllers are probed as
> regular platform devices.
>
> This creates fw_devlink between consumers (PCI host controller) and
> supplier (interrupt controller) based on "interrupt-map" DT property.
>
> Signed-off-by: Anup Patel <[email protected]>
> Reviewed-by: Saravana Kannan <[email protected]>
> ---
> Changes since v3:
> - Added a comment about of_irq_parse_raw()
> - Removed redundant NULL assignments to sup_args.np
> Changes since v2:
> - No need for a loop to find #interrupt-cells property value
> - Fix node de-reference leak when index is greater than number
> of entries in interrupt-map property
> Changes since v1:
> - Updated commit description based on Rob's suggestion
> - Use of_irq_parse_raw() for parsing interrupt-map DT property
This patch breaks badly on my M1 Mini, with a continuous stream of
boot time warnings lasting about 100 seconds:
[ 97.832335] ------------[ cut here ]------------
[ 97.836955] /soc/pcie@690000000/pci@2,0 interrupt-map failed, using interrupt-controller
[ 97.845072] WARNING: CPU: 0 PID: 1 at drivers/of/irq.c:277 of_irq_parse_raw+0x620/0x730
[ 97.853087] Modules linked in:
[ 97.856139] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 6.10.0-rc1 #2915
[ 97.864163] Hardware name: Apple Mac mini (M1, 2020) (DT)
[ 97.869570] pstate: 61400009 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[ 97.876546] pc : of_irq_parse_raw+0x620/0x730
[ 97.880907] lr : of_irq_parse_raw+0x620/0x730
[ 97.885267] sp : ffffc000800538a0
[ 97.888581] x29: ffffc000800538a0 x28: 0000000000000000 x27: ffffffffff5ffc68
[ 97.895732] x26: ffffc000800539a4 x25: ffffffffff5ffbbc x24: ffffc00080053a48
[ 97.902883] x23: ffffe3ff73284e68 x22: ffffb7889aede6d0 x21: ffffe3ff735f9320
[ 97.910034] x20: ffffc00080053964 x19: ffffb7889aede6d0 x18: ffffffffffffffff
[ 97.917185] x17: 7075727265746e69 x16: 20676e697375202c x15: 64656c6961662070
[ 97.924336] x14: 616d2d7470757272 x13: 72656c6c6f72746e x12: 6f632d7470757272
[ 97.931487] x11: 65746e6920676e69 x10: ffffe3ff740bcb68 x9 : ffffe3ff72335b38
[ 97.938639] x8 : 00000001000028a4 x7 : ffffe3ff740afc08 x6 : 00000000000038a4
[ 97.945789] x5 : 00000000000067b0 x4 : c0000001000028a4 x3 : 0000000000000000
[ 97.952940] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffb784c16ade00
[ 97.960092] Call trace:
[ 97.962534] of_irq_parse_raw+0x620/0x730
[ 97.966545] parse_interrupt_map+0xfc/0x188
[ 97.970731] of_fwnode_add_links+0x170/0x1e0
[ 97.975004] fw_devlink_parse_fwtree+0x44/0x98
[ 97.979452] fw_devlink_parse_fwtree+0x6c/0x98
[ 97.983899] fw_devlink_parse_fwtree+0x6c/0x98
[ 97.988347] device_add+0x610/0x6a8
[ 97.991836] of_device_add+0x4c/0x70
[ 97.995411] of_platform_device_create_pdata+0xa0/0x160
[ 98.000644] of_platform_bus_create+0x184/0x370
[ 98.005178] of_platform_populate+0x68/0x160
[ 98.009451] of_platform_default_populate_init+0xf4/0x118
[ 98.014859] do_one_initcall+0x4c/0x320
[ 98.018695] do_initcalls+0xf4/0x1d8
[ 98.022271] kernel_init_freeable+0x12c/0x280
[ 98.026632] kernel_init+0x2c/0x1f8
[ 98.030120] ret_from_fork+0x10/0x20
[ 98.033696] ---[ end trace 0000000000000000 ]---
which comes from 10a20b34d735f ("of/irq: Don't ignore
interrupt-controller when interrupt-map failed").
Each of the 3 PCIe ports are described as such:
port02: pci@2,0 {
device_type = "pci";
reg = <0x1000 0x0 0x0 0x0 0x0>;
reset-gpios = <&pinctrl_ap 33 GPIO_ACTIVE_LOW>;
#address-cells = <3>;
#size-cells = <2>;
ranges;
interrupt-controller;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 7>;
interrupt-map = <0 0 0 1 &port02 0 0 0 0>,
<0 0 0 2 &port02 0 0 0 1>,
<0 0 0 3 &port02 0 0 0 2>,
<0 0 0 4 &port02 0 0 0 3>;
status = "disabled";
};
and get probed *972 times*, which seem... excessive, given that there
are only 4 entries per port.
> ---
> drivers/of/property.c | 52 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index a6358ee99b74..2d749a18b037 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1311,6 +1311,57 @@ static struct device_node *parse_interrupts(struct device_node *np,
> return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
> }
>
> +static struct device_node *parse_interrupt_map(struct device_node *np,
> + const char *prop_name, int index)
> +{
> + const __be32 *imap, *imap_end, *addr;
> + struct of_phandle_args sup_args;
> + u32 addrcells, intcells;
> + int i, imaplen;
> +
> + if (!IS_ENABLED(CONFIG_OF_IRQ))
> + return NULL;
> +
> + if (strcmp(prop_name, "interrupt-map"))
> + return NULL;
> +
> + if (of_property_read_u32(np, "#interrupt-cells", &intcells))
> + return NULL;
> + addrcells = of_bus_n_addr_cells(np);
> +
> + imap = of_get_property(np, "interrupt-map", &imaplen);
> + if (!imap || imaplen <= (addrcells + intcells))
This is "interesting". You compare a number of *bytes* with a number
of cells. Only off by a factor of 4...
Also, you need a minimum of one extra cell to hold the phandle, and a
yet unknown number of cells for whatever follows the phandle.
> + return NULL;
> + imap_end = imap + imaplen;
Same problem, with pointer arithmetic this time.
> +
> + while (imap < imap_end) {
> + addr = imap;
> + imap += addrcells;
> +
> + 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;
> +
> + /*
> + * 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))
> + return NULL;
> +
> + if (!index)
> + return sup_args.np;
> +
> + of_node_put(sup_args.np);
> + imap += sup_args.args_count + 1;
This really doesn't map (pun intended) to the way the interrupt-map
entries are built. You need to account for the parent address size as
well.
I'll post a patch fixing both issues.
M.
--
Without deviation from the norm, progress is not possible.