This patch series implements ITS nodes as kernel devices. The
advantage of that is that CMA can be used to allocate large ITS
tables, where standard memory and page allocation fails (above
MAX_ORDER - 1). This approach was suggested first here:
https://marc.info/?i=56D44812.6000309%40arm.com
Another advantage is that all memory resources of the device are now
device managed, thus the code for releasing the device becomes much
easier.
The following is required to implement this:
* each ITS node must be assigned to a struct device each,
* the ITS initialization must be moved to a later point during boot,
* the ITS enablement must be separated from the GIC code (the GIC
still needs to be enabled early),
* the early ITS node probing by DT or ACPI must be separated from the
ITS initialization,
* ITS table allocation must be changed to use dma_alloc_coherent()
which uses CMA for allocation of large memory ranges.
This is an update of my last patch submission here:
https://lkml.org/lkml/2017/3/6/570
V3:
* rebased onto v4.13-rc1,
* fixed use after free in error path in its_init_one(),
* added comment for arch_setup_dma_ops() to describe coherency,
V2:
* rebased onto v4.11-rc1,
* fixed syntax error in its_init() function (split probing patch),
* added comment in its_create_device(),
* fixed GITS_BASER_PAGE_SIZE_MASK usage in its_setup_baser()
Robert Richter (8):
irqchip/gic-v3-its: Initialize its nodes in probe order
irqchip/gic-v3-its: Initialize MSIs with subsys_initcalls
irqchip/gic-v3-its: Split probing from its node initialization
irqchip/gic-v3-its: Decouple its initialization from gic
irqchip/gic-v3-its: Prevent its init ordering dependencies
irqchip/gic-v3-its: Initialize its nodes later
irqchip/gic-v3-its: Handle its nodes as kernel devices
irqchip, gicv3-its, cma: Use CMA for allocation of large device tables
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 6 +-
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 6 +-
drivers/irqchip/irq-gic-v3-its.c | 285 ++++++++++++++++----------
drivers/irqchip/irq-gic-v3.c | 8 +-
include/linux/cpuhotplug.h | 1 +
include/linux/irqchip/arm-gic-v3.h | 5 +-
6 files changed, 195 insertions(+), 116 deletions(-)
--
2.11.0
To initialize the its nodes at a later point during boot, we need to
split probing from initialization. Collect all information required
for initialization in struct its_node. We can then use the its node
list for initialization.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 95 ++++++++++++++++++++++++++------------
drivers/irqchip/irq-gic-v3.c | 2 +-
include/linux/irqchip/arm-gic-v3.h | 4 +-
3 files changed, 68 insertions(+), 33 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 92c66c86a63f..b51a1208588b 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -75,10 +75,12 @@ struct its_baser {
* list of devices writing to it.
*/
struct its_node {
+ struct fwnode_handle *fwnode;
raw_spinlock_t lock;
struct list_head entry;
void __iomem *base;
phys_addr_t phys_base;
+ phys_addr_t phys_size;
struct its_cmd_block *cmd_base;
struct its_cmd_block *cmd_write;
struct its_baser tables[GITS_BASER_NR_REGS];
@@ -1647,7 +1649,7 @@ static void its_enable_quirks(struct its_node *its)
gic_enable_quirks(iidr, its_quirks, its);
}
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
{
struct irq_domain *inner_domain;
struct msi_domain_info *info;
@@ -1656,7 +1658,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
if (!info)
return -ENOMEM;
- inner_domain = irq_domain_create_tree(handle, &its_domain_ops, its);
+ inner_domain = irq_domain_create_tree(its->fwnode, &its_domain_ops, its);
if (!inner_domain) {
kfree(info);
return -ENOMEM;
@@ -1672,55 +1674,83 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
return 0;
}
+static void its_free(struct its_node *its)
+{
+ spin_lock(&its_lock);
+ list_del(&its->entry);
+ spin_unlock(&its_lock);
+
+ kfree(its);
+}
+
+static int __init its_init_one(struct its_node *its);
+
static int __init its_probe_one(struct resource *res,
struct fwnode_handle *handle, int numa_node)
{
struct its_node *its;
+ int err;
+
+ its = kzalloc(sizeof(*its), GFP_KERNEL);
+ if (!its)
+ return -ENOMEM;
+
+ raw_spin_lock_init(&its->lock);
+ INIT_LIST_HEAD(&its->entry);
+ INIT_LIST_HEAD(&its->its_device_list);
+ its->fwnode = handle;
+ its->phys_base = res->start;
+ its->phys_size = resource_size(res);
+ its->numa_node = numa_node;
+
+ spin_lock(&its_lock);
+ list_add_tail(&its->entry, &its_nodes);
+ spin_unlock(&its_lock);
+
+ pr_info("ITS %pR\n", res);
+
+ err = its_init_one(its);
+ if (err)
+ its_free(its);
+
+ return err;
+}
+
+static int __init its_init_one(struct its_node *its)
+{
void __iomem *its_base;
u32 val;
u64 baser, tmp;
int err;
- its_base = ioremap(res->start, resource_size(res));
+ its_base = ioremap(its->phys_base, its->phys_size);
if (!its_base) {
- pr_warn("ITS@%pa: Unable to map ITS registers\n", &res->start);
- return -ENOMEM;
+ pr_warn("ITS@%pa: Unable to map ITS registers\n", &its->phys_base);
+ err = -ENOMEM;
+ goto fail;
}
val = readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK;
if (val != 0x30 && val != 0x40) {
- pr_warn("ITS@%pa: No ITS detected, giving up\n", &res->start);
+ pr_warn("ITS@%pa: No ITS detected, giving up\n", &its->phys_base);
err = -ENODEV;
goto out_unmap;
}
err = its_force_quiescent(its_base);
if (err) {
- pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &res->start);
- goto out_unmap;
- }
-
- pr_info("ITS %pR\n", res);
-
- its = kzalloc(sizeof(*its), GFP_KERNEL);
- if (!its) {
- err = -ENOMEM;
+ pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
goto out_unmap;
}
- raw_spin_lock_init(&its->lock);
- INIT_LIST_HEAD(&its->entry);
- INIT_LIST_HEAD(&its->its_device_list);
its->base = its_base;
- its->phys_base = res->start;
its->ite_size = ((gic_read_typer(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
- its->numa_node = numa_node;
its->cmd_base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
get_order(ITS_CMD_QUEUE_SZ));
if (!its->cmd_base) {
err = -ENOMEM;
- goto out_free_its;
+ goto out_unmap;
}
its->cmd_write = its->cmd_base;
@@ -1762,13 +1792,11 @@ static int __init its_probe_one(struct resource *res,
gits_write_cwriter(0, its->base + GITS_CWRITER);
writel_relaxed(GITS_CTLR_ENABLE, its->base + GITS_CTLR);
- err = its_init_domain(handle, its);
+ err = its_init_domain(its);
if (err)
goto out_free_tables;
- spin_lock(&its_lock);
- list_add_tail(&its->entry, &its_nodes);
- spin_unlock(&its_lock);
+ pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
return 0;
@@ -1776,11 +1804,10 @@ static int __init its_probe_one(struct resource *res,
its_free_tables(its);
out_free_cmd:
free_pages((unsigned long)its->cmd_base, get_order(ITS_CMD_QUEUE_SZ));
-out_free_its:
- kfree(its);
out_unmap:
iounmap(its_base);
- pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+fail:
+ pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
return err;
}
@@ -1956,8 +1983,10 @@ static void __init its_acpi_probe(void)
static void __init its_acpi_probe(void) { }
#endif
-int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
- struct irq_domain *parent_domain)
+static int __init its_init(void);
+
+int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
+ struct irq_domain *parent_domain)
{
struct device_node *of_node;
@@ -1974,5 +2003,11 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
}
gic_rdists = rdists;
+
+ return its_init();
+}
+
+static int __init its_init(void)
+{
return its_alloc_lpi_tables();
}
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index dbffb7ab6203..886e70ab2159 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -952,7 +952,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
set_handle_irq(gic_handle_irq);
if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
- its_init(handle, &gic_data.rdists, gic_data.domain);
+ its_probe(handle, &gic_data.rdists, gic_data.domain);
gic_smp_init();
gic_dist_init();
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 6a1f87ff94e2..c4f9c968728f 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -492,8 +492,8 @@ struct rdists {
struct irq_domain;
struct fwnode_handle;
int its_cpu_init(void);
-int its_init(struct fwnode_handle *handle, struct rdists *rdists,
- struct irq_domain *domain);
+int its_probe(struct fwnode_handle *handle, struct rdists *rdists,
+ struct irq_domain *domain);
static inline bool gic_enable_sre(void)
{
--
2.11.0
Right now its_init() must be called before pci and platform init.
Remove ordering dependencies to allow all initialization functions
being called with the same initcall type.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 4 +++-
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index 5940fdf0036e..7a8fbb74f2eb 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -72,6 +72,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
return -EINVAL;
msi_info = msi_get_domain_info(domain->parent);
+ if (!msi_info)
+ return -ENODEV;
pdev = to_pci_dev(dev);
/*
@@ -112,7 +114,7 @@ static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
struct irq_domain *parent;
parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
- if (!parent || !msi_get_domain_info(parent)) {
+ if (!parent) {
pr_err("%s: Unable to locate ITS domain\n", name);
return -ENXIO;
}
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 6ebc871ac63f..999d3bdc17a6 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -61,6 +61,8 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
int ret;
msi_info = msi_get_domain_info(domain->parent);
+ if (!msi_info)
+ return -ENODEV;
if (dev->of_node)
ret = of_pmsi_get_dev_id(domain, dev, &dev_id);
@@ -97,7 +99,7 @@ static int __init its_pmsi_init_one(struct fwnode_handle *fwnode,
struct irq_domain *parent;
parent = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_NEXUS);
- if (!parent || !msi_get_domain_info(parent)) {
+ if (!parent) {
pr_err("%s: unable to locate ITS domain\n", name);
return -ENXIO;
}
--
2.11.0
This patch separates its initialization from the gic. Probing and
initialization of its nodes is separate now. There is an own cpu
notifier for its now.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 61 +++++++++++++++++++++++++++-----------
drivers/irqchip/irq-gic-v3.c | 11 +++----
include/linux/cpuhotplug.h | 1 +
include/linux/irqchip/arm-gic-v3.h | 2 +-
4 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b51a1208588b..5e2d4f2876d8 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1676,20 +1676,13 @@ static int its_init_domain(struct its_node *its)
static void its_free(struct its_node *its)
{
- spin_lock(&its_lock);
- list_del(&its->entry);
- spin_unlock(&its_lock);
-
kfree(its);
}
-static int __init its_init_one(struct its_node *its);
-
static int __init its_probe_one(struct resource *res,
struct fwnode_handle *handle, int numa_node)
{
struct its_node *its;
- int err;
its = kzalloc(sizeof(*its), GFP_KERNEL);
if (!its)
@@ -1709,11 +1702,7 @@ static int __init its_probe_one(struct resource *res,
pr_info("ITS %pR\n", res);
- err = its_init_one(its);
- if (err)
- its_free(its);
-
- return err;
+ return 0;
}
static int __init its_init_one(struct its_node *its)
@@ -1816,7 +1805,7 @@ static bool gic_rdists_supports_plpis(void)
return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS);
}
-int its_cpu_init(void)
+static int its_cpu_init(unsigned int cpu)
{
if (!list_empty(&its_nodes)) {
if (!gic_rdists_supports_plpis()) {
@@ -1983,8 +1972,6 @@ static void __init its_acpi_probe(void)
static void __init its_acpi_probe(void) { }
#endif
-static int __init its_init(void);
-
int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
struct irq_domain *parent_domain)
{
@@ -2004,10 +1991,48 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
gic_rdists = rdists;
- return its_init();
+ return 0;
}
-static int __init its_init(void)
+int __init its_init(void)
{
- return its_alloc_lpi_tables();
+ struct its_node *its, *tmp;
+ int err = 0, err2;
+
+ if (list_empty(&its_nodes))
+ return 0;
+
+ spin_lock(&its_lock);
+
+ list_for_each_entry(its, &its_nodes, entry) {
+ err2 = its_init_one(its);
+ if (!err && err2)
+ err = err2;
+ }
+
+ if (!err)
+ goto unlock;
+
+ list_for_each_entry_safe(its, tmp, &its_nodes, entry) {
+ list_del(&its->entry);
+ its_free(its);
+ }
+
+unlock:
+ spin_unlock(&its_lock);
+
+ if (!err)
+ err = its_alloc_lpi_tables();
+
+ if (err) {
+ pr_warn("ITS: Failed to initialize (%d), not enabling LPIs\n",
+ err);
+ return err;
+ }
+
+ cpuhp_setup_state(CPUHP_AP_IRQ_GIC_ITS_STARTING,
+ "irqchip/arm/gicv3-its:starting",
+ its_cpu_init, NULL);
+
+ return 0;
}
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 886e70ab2159..1476ce12b3b8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -536,10 +536,6 @@ static void gic_cpu_init(void)
gic_cpu_config(rbase, gic_redist_wait_for_rwp);
- /* Give LPIs a spin */
- if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
- its_cpu_init();
-
/* initialise system registers */
gic_cpu_sys_reg_init();
}
@@ -675,7 +671,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
#else
#define gic_set_affinity NULL
#define gic_smp_init() do { } while(0)
-#endif
+#endif /* CONFIG_SMP */
#ifdef CONFIG_CPU_PM
/* Check whether it's single security state view */
@@ -1171,6 +1167,9 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
gic_populate_ppi_partitions(node);
gic_of_setup_kvm_info(node);
+
+ its_init();
+
return 0;
out_unmap_rdist:
@@ -1460,6 +1459,8 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
gic_acpi_setup_kvm_info();
+ its_init();
+
return 0;
out_fwhandle_free:
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index b56573bf440d..6b2fc0e22efc 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -78,6 +78,7 @@ enum cpuhp_state {
CPUHP_AP_SCHED_STARTING,
CPUHP_AP_RCUTREE_DYING,
CPUHP_AP_IRQ_GIC_STARTING,
+ CPUHP_AP_IRQ_GIC_ITS_STARTING,
CPUHP_AP_IRQ_HIP04_STARTING,
CPUHP_AP_IRQ_ARMADA_XP_STARTING,
CPUHP_AP_IRQ_BCM2836_STARTING,
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index c4f9c968728f..15cbedbd82f1 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -491,9 +491,9 @@ struct rdists {
struct irq_domain;
struct fwnode_handle;
-int its_cpu_init(void);
int its_probe(struct fwnode_handle *handle, struct rdists *rdists,
struct irq_domain *domain);
+int its_init(void);
static inline bool gic_enable_sre(void)
{
--
2.11.0
The gicv3-its device table may have a size of up to 16MB. With 4k
pagesize the maximum size of memory allocation is 4MB. Use CMA for
allocation of large tables.
We use the device managed version of dma_alloc_coherent(). Thus, we
don't need to release it manually on device removal.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 74 ++++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 29 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b7d853dd6b75..8da9b423f9e1 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -22,6 +22,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/dma-iommu.h>
+#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/irqdomain.h>
#include <linux/log2.h>
@@ -866,6 +867,7 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
u64 type = GITS_BASER_TYPE(val);
u32 alloc_pages;
void *base;
+ dma_addr_t dma_handle;
u64 tmp;
retry_alloc_baser:
@@ -878,13 +880,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
order = get_order(GITS_BASER_PAGES_MAX * psz);
}
- base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO,
- order);
- if (!base)
+ base = dmam_alloc_coherent(&its->dev,
+ PAGE_ORDER_TO_SIZE(order),
+ &dma_handle,
+ GFP_KERNEL | __GFP_ZERO);
+
+ if (!base && order >= MAX_ORDER) {
+ order = MAX_ORDER - 1;
+ dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n",
+ its->device_ids,
+ ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz));
+ goto retry_alloc_baser;
+ }
+
+ if (!base) {
+ dev_err(&its->dev, "Failed to allocate device table\n");
return -ENOMEM;
+ }
retry_baser:
- val = (virt_to_phys(base) |
+ val = (dma_handle |
(type << GITS_BASER_TYPE_SHIFT) |
((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT) |
((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT) |
@@ -925,29 +940,28 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
goto retry_baser;
}
- if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
- /*
- * Page size didn't stick. Let's try a smaller
- * size and retry. If we reach 4K, then
- * something is horribly wrong...
- */
- devm_free_pages(&its->dev, (unsigned long)base);
- baser->base = NULL;
-
- switch (psz) {
- case SZ_16K:
- psz = SZ_4K;
- goto retry_alloc_baser;
- case SZ_64K:
- psz = SZ_16K;
- goto retry_alloc_baser;
+ if (val != tmp) {
+ dmam_free_coherent(&its->dev, PAGE_ORDER_TO_SIZE(order),
+ base, dma_handle);
+
+ if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
+ /*
+ * Page size didn't stick. Let's try a smaller
+ * size and retry. If we reach 4K, then
+ * something is horribly wrong...
+ */
+ switch (psz) {
+ case SZ_16K:
+ psz = SZ_4K;
+ goto retry_alloc_baser;
+ case SZ_64K:
+ psz = SZ_16K;
+ goto retry_alloc_baser;
+ }
}
- }
- if (val != tmp) {
dev_err(&its->dev, "%s doesn't stick: %llx %llx\n",
its_base_type_string[type], val, tmp);
- devm_free_pages(&its->dev, (unsigned long)base);
return -ENXIO;
}
@@ -1005,12 +1019,6 @@ static bool its_parse_baser_device(struct its_node *its, struct its_baser *baser
* feature is not supported by hardware.
*/
new_order = max_t(u32, get_order(esz << ids), new_order);
- if (new_order >= MAX_ORDER) {
- new_order = MAX_ORDER - 1;
- ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz);
- dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u\n",
- its->device_ids, ids);
- }
*order = new_order;
@@ -1716,6 +1724,14 @@ static int __init its_init_one(struct its_node *its)
return err;
}
+ /*
+ * Setup dma_ops to be used with dmam_alloc_coherent() for its
+ * device table allocation. Since the device table is
+ * exclusively used by the device only we can mark this mem as
+ * coherent.
+ */
+ arch_setup_dma_ops(&its->dev, 0, 0, NULL, true);
+
its_base = devm_ioremap(&its->dev, its->phys_base, its->phys_size);
if (!its_base) {
dev_warn(&its->dev, "Unable to map ITS registers\n");
--
2.11.0
Manage its nodes as kernel devices. We can then use the kernel's
device resource management for memory allocation. Freeing memory
becomes much easier now. This also allows us to use CMA for the
allocation of large its tables.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 118 ++++++++++++++++++++-------------------
1 file changed, 60 insertions(+), 58 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 488f811d5978..b7d853dd6b75 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -20,6 +20,7 @@
#include <linux/bitmap.h>
#include <linux/cpu.h>
#include <linux/delay.h>
+#include <linux/device.h>
#include <linux/dma-iommu.h>
#include <linux/interrupt.h>
#include <linux/irqdomain.h>
@@ -75,6 +76,7 @@ struct its_baser {
* list of devices writing to it.
*/
struct its_node {
+ struct device dev;
struct fwnode_handle *fwnode;
raw_spinlock_t lock;
struct list_head entry;
@@ -400,7 +402,7 @@ static struct its_cmd_block *its_allocate_entry(struct its_node *its)
while (its_queue_full(its)) {
count--;
if (!count) {
- pr_err_ratelimited("ITS queue not draining\n");
+ dev_err_ratelimited(&its->dev, "ITS queue not draining\n");
return NULL;
}
cpu_relax();
@@ -460,7 +462,7 @@ static void its_wait_for_range_completion(struct its_node *its,
count--;
if (!count) {
- pr_err_ratelimited("ITS queue timeout\n");
+ dev_err_ratelimited(&its->dev, "ITS queue timeout\n");
return;
}
cpu_relax();
@@ -480,7 +482,7 @@ static void its_send_single_command(struct its_node *its,
cmd = its_allocate_entry(its);
if (!cmd) { /* We're soooooo screewed... */
- pr_err_ratelimited("ITS can't allocate, dropping command\n");
+ dev_err_ratelimited(&its->dev, "ITS can't allocate, dropping command\n");
raw_spin_unlock_irqrestore(&its->lock, flags);
return;
}
@@ -490,7 +492,7 @@ static void its_send_single_command(struct its_node *its,
if (sync_col) {
sync_cmd = its_allocate_entry(its);
if (!sync_cmd) {
- pr_err_ratelimited("ITS can't SYNC, skipping\n");
+ dev_err_ratelimited(&its->dev, "ITS can't SYNC, skipping\n");
goto post;
}
its_encode_cmd(sync_cmd, GITS_CMD_SYNC);
@@ -869,14 +871,15 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
retry_alloc_baser:
alloc_pages = (PAGE_ORDER_TO_SIZE(order) / psz);
if (alloc_pages > GITS_BASER_PAGES_MAX) {
- pr_warn("ITS@%pa: %s too large, reduce ITS pages %u->%u\n",
- &its->phys_base, its_base_type_string[type],
- alloc_pages, GITS_BASER_PAGES_MAX);
+ dev_warn(&its->dev, "%s too large, reduce ITS pages %u->%u\n",
+ its_base_type_string[type], alloc_pages,
+ GITS_BASER_PAGES_MAX);
alloc_pages = GITS_BASER_PAGES_MAX;
order = get_order(GITS_BASER_PAGES_MAX * psz);
}
- base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
+ base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO,
+ order);
if (!base)
return -ENOMEM;
@@ -928,7 +931,7 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
* size and retry. If we reach 4K, then
* something is horribly wrong...
*/
- free_pages((unsigned long)base, order);
+ devm_free_pages(&its->dev, (unsigned long)base);
baser->base = NULL;
switch (psz) {
@@ -942,10 +945,9 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
}
if (val != tmp) {
- pr_err("ITS@%pa: %s doesn't stick: %llx %llx\n",
- &its->phys_base, its_base_type_string[type],
- val, tmp);
- free_pages((unsigned long)base, order);
+ dev_err(&its->dev, "%s doesn't stick: %llx %llx\n",
+ its_base_type_string[type], val, tmp);
+ devm_free_pages(&its->dev, (unsigned long)base);
return -ENXIO;
}
@@ -954,8 +956,8 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
baser->psz = psz;
tmp = indirect ? GITS_LVL1_ENTRY_SIZE : esz;
- pr_info("ITS@%pa: allocated %d %s @%lx (%s, esz %d, psz %dK, shr %d)\n",
- &its->phys_base, (int)(PAGE_ORDER_TO_SIZE(order) / (int)tmp),
+ dev_info(&its->dev, "allocated %d %s @%lx (%s, esz %d, psz %dK, shr %d)\n",
+ (int)(PAGE_ORDER_TO_SIZE(order) / (int)tmp),
its_base_type_string[type],
(unsigned long)virt_to_phys(base),
indirect ? "indirect" : "flat", (int)esz,
@@ -1006,8 +1008,8 @@ static bool its_parse_baser_device(struct its_node *its, struct its_baser *baser
if (new_order >= MAX_ORDER) {
new_order = MAX_ORDER - 1;
ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz);
- pr_warn("ITS@%pa: Device Table too large, reduce ids %u->%u\n",
- &its->phys_base, its->device_ids, ids);
+ dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u\n",
+ its->device_ids, ids);
}
*order = new_order;
@@ -1015,19 +1017,6 @@ static bool its_parse_baser_device(struct its_node *its, struct its_baser *baser
return indirect;
}
-static void its_free_tables(struct its_node *its)
-{
- int i;
-
- for (i = 0; i < GITS_BASER_NR_REGS; i++) {
- if (its->tables[i].base) {
- free_pages((unsigned long)its->tables[i].base,
- its->tables[i].order);
- its->tables[i].base = NULL;
- }
- }
-}
-
static int its_alloc_tables(struct its_node *its)
{
u64 typer = gic_read_typer(its->base + GITS_TYPER);
@@ -1062,10 +1051,8 @@ static int its_alloc_tables(struct its_node *its)
indirect = its_parse_baser_device(its, baser, psz, &order);
err = its_setup_baser(its, baser, cache, shr, psz, order, indirect);
- if (err < 0) {
- its_free_tables(its);
+ if (err < 0)
return err;
- }
/* Update settings which will be used for next BASERn */
psz = baser->psz;
@@ -1349,6 +1336,9 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
gic_flush_dcache_to_poc(itt, sz);
+ /* prevent its from being released */
+ get_device(&its->dev);
+
dev->its = its;
dev->itt = itt;
dev->nr_ites = nr_ites;
@@ -1674,8 +1664,9 @@ static int its_init_domain(struct its_node *its)
return 0;
}
-static void its_free(struct its_node *its)
-{
+static void its_device_release(struct device *dev) {
+ struct its_node *its = container_of(dev, struct its_node, dev);
+
kfree(its);
}
@@ -1712,34 +1703,47 @@ static int __init its_init_one(struct its_node *its)
u64 baser, tmp;
int err;
- its_base = ioremap(its->phys_base, its->phys_size);
+ /* On error always use put_device() to free devices */
+ device_initialize(&its->dev);
+ its->dev.release = its_device_release;
+
+ err = dev_set_name(&its->dev, "its@%pa", &its->phys_base);
+ if (!err)
+ err = device_add(&its->dev);
+
+ if (err) {
+ pr_warn("ITS@%pa: Unable to register device\n", &its->phys_base);
+ return err;
+ }
+
+ its_base = devm_ioremap(&its->dev, its->phys_base, its->phys_size);
if (!its_base) {
- pr_warn("ITS@%pa: Unable to map ITS registers\n", &its->phys_base);
+ dev_warn(&its->dev, "Unable to map ITS registers\n");
err = -ENOMEM;
goto fail;
}
val = readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK;
if (val != 0x30 && val != 0x40) {
- pr_warn("ITS@%pa: No ITS detected, giving up\n", &its->phys_base);
+ dev_warn(&its->dev, "No ITS detected, giving up\n");
err = -ENODEV;
- goto out_unmap;
+ goto fail;
}
err = its_force_quiescent(its_base);
if (err) {
- pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
- goto out_unmap;
+ dev_warn(&its->dev, "Failed to quiesce, giving up\n");
+ goto fail;
}
its->base = its_base;
its->ite_size = ((gic_read_typer(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
- its->cmd_base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
- get_order(ITS_CMD_QUEUE_SZ));
+ its->cmd_base = (void *)devm_get_free_pages(&its->dev,
+ GFP_KERNEL | __GFP_ZERO, get_order(ITS_CMD_QUEUE_SZ));
if (!its->cmd_base) {
err = -ENOMEM;
- goto out_unmap;
+ goto fail;
}
its->cmd_write = its->cmd_base;
@@ -1747,11 +1751,11 @@ static int __init its_init_one(struct its_node *its)
err = its_alloc_tables(its);
if (err)
- goto out_free_cmd;
+ goto fail;
err = its_alloc_collections(its);
if (err)
- goto out_free_tables;
+ goto fail;
baser = (virt_to_phys(its->cmd_base) |
GITS_CBASER_RaWaWb |
@@ -1774,7 +1778,7 @@ static int __init its_init_one(struct its_node *its)
baser |= GITS_CBASER_nC;
gits_write_cbaser(baser, its->base + GITS_CBASER);
}
- pr_info("ITS: using cache flushing for cmd queue\n");
+ dev_info(&its->dev, "using cache flushing for cmd queue\n");
its->flags |= ITS_FLAGS_CMDQ_NEEDS_FLUSHING;
}
@@ -1783,20 +1787,14 @@ static int __init its_init_one(struct its_node *its)
err = its_init_domain(its);
if (err)
- goto out_free_tables;
+ goto fail;
- pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
+ dev_info(&its->dev, "ITS node added\n");
return 0;
-
-out_free_tables:
- its_free_tables(its);
-out_free_cmd:
- free_pages((unsigned long)its->cmd_base, get_order(ITS_CMD_QUEUE_SZ));
-out_unmap:
- iounmap(its_base);
fail:
- pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
+ dev_err(&its->dev, "failed probing (%d)\n", err);
+ device_del(&its->dev);
return err;
}
@@ -2004,6 +2002,10 @@ static int __init its_init(void)
spin_lock(&its_lock);
+ /*
+ * Call this for all entries. We can then use put_device() to
+ * release the nodes on error.
+ */
list_for_each_entry(its, &its_nodes, entry) {
err2 = its_init_one(its);
if (!err && err2)
@@ -2015,7 +2017,7 @@ static int __init its_init(void)
list_for_each_entry_safe(its, tmp, &its_nodes, entry) {
list_del(&its->entry);
- its_free(its);
+ put_device(&its->dev);
}
unlock:
--
2.11.0
Use an initcall to initialize its. This allows us to use the device
framework during initialization that is up at this point. We use
subsys_initcall() here since we need the arch to be initialized
first. It is before pci and platform device probe where devices are
bound to msi interrupts.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 3 ++-
drivers/irqchip/irq-gic-v3.c | 5 -----
include/linux/irqchip/arm-gic-v3.h | 1 -
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 5e2d4f2876d8..488f811d5978 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
return 0;
}
-int __init its_init(void)
+static int __init its_init(void)
{
struct its_node *its, *tmp;
int err = 0, err2;
@@ -2036,3 +2036,4 @@ int __init its_init(void)
return 0;
}
+subsys_initcall(its_init);
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1476ce12b3b8..32bdfd656ea7 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1167,9 +1167,6 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
gic_populate_ppi_partitions(node);
gic_of_setup_kvm_info(node);
-
- its_init();
-
return 0;
out_unmap_rdist:
@@ -1459,8 +1456,6 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
gic_acpi_setup_kvm_info();
- its_init();
-
return 0;
out_fwhandle_free:
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 15cbedbd82f1..3c375c593800 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -493,7 +493,6 @@ struct irq_domain;
struct fwnode_handle;
int its_probe(struct fwnode_handle *handle, struct rdists *rdists,
struct irq_domain *domain);
-int its_init(void);
static inline bool gic_enable_sre(void)
{
--
2.11.0
This allows us to use kernel core functionality (e.g. cma) for ITS
initialization. MSIs must be up before the device_initcalls (pci and
platform device probe) and after arch_initcalls (dma init), so
subsys_initcall is fine.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 2 +-
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index 77931214d954..5940fdf0036e 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -193,4 +193,4 @@ static int __init its_pci_msi_init(void)
return 0;
}
-early_initcall(its_pci_msi_init);
+subsys_initcall(its_pci_msi_init);
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 249240d9a425..6ebc871ac63f 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -166,4 +166,4 @@ static int __init its_pmsi_init(void)
its_pmsi_acpi_init();
return 0;
}
-early_initcall(its_pmsi_init);
+subsys_initcall(its_pmsi_init);
--
2.11.0
ATM the last discovered node is initialized first. Though this order
should work too, change the initialization of nodes to probe order as
one would expect it.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 68932873eebc..92c66c86a63f 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1767,7 +1767,7 @@ static int __init its_probe_one(struct resource *res,
goto out_free_tables;
spin_lock(&its_lock);
- list_add(&its->entry, &its_nodes);
+ list_add_tail(&its->entry, &its_nodes);
spin_unlock(&its_lock);
return 0;
--
2.11.0
Marc,
On 08.08.17 14:22:50, Robert Richter wrote:
> This patch series implements ITS nodes as kernel devices. The
> advantage of that is that CMA can be used to allocate large ITS
> tables, where standard memory and page allocation fails (above
> MAX_ORDER - 1). This approach was suggested first here:
>
> https://marc.info/?i=56D44812.6000309%40arm.com
>
> Another advantage is that all memory resources of the device are now
> device managed, thus the code for releasing the device becomes much
> easier.
>
> The following is required to implement this:
>
> * each ITS node must be assigned to a struct device each,
>
> * the ITS initialization must be moved to a later point during boot,
>
> * the ITS enablement must be separated from the GIC code (the GIC
> still needs to be enabled early),
>
> * the early ITS node probing by DT or ACPI must be separated from the
> ITS initialization,
>
> * ITS table allocation must be changed to use dma_alloc_coherent()
> which uses CMA for allocation of large memory ranges.
>
> This is an update of my last patch submission here:
>
> https://lkml.org/lkml/2017/3/6/570
>
> V3:
> * rebased onto v4.13-rc1,
> * fixed use after free in error path in its_init_one(),
> * added comment for arch_setup_dma_ops() to describe coherency,
what is your opinion on this series?
There might be conflicts with your gicv4 series you sent around. Any
advice how we can move forward with this one?
Thanks,
-Robert
> V2:
> * rebased onto v4.11-rc1,
> * fixed syntax error in its_init() function (split probing patch),
> * added comment in its_create_device(),
> * fixed GITS_BASER_PAGE_SIZE_MASK usage in its_setup_baser()
>
> Robert Richter (8):
> irqchip/gic-v3-its: Initialize its nodes in probe order
> irqchip/gic-v3-its: Initialize MSIs with subsys_initcalls
> irqchip/gic-v3-its: Split probing from its node initialization
> irqchip/gic-v3-its: Decouple its initialization from gic
> irqchip/gic-v3-its: Prevent its init ordering dependencies
> irqchip/gic-v3-its: Initialize its nodes later
> irqchip/gic-v3-its: Handle its nodes as kernel devices
> irqchip, gicv3-its, cma: Use CMA for allocation of large device tables
>
> drivers/irqchip/irq-gic-v3-its-pci-msi.c | 6 +-
> drivers/irqchip/irq-gic-v3-its-platform-msi.c | 6 +-
> drivers/irqchip/irq-gic-v3-its.c | 285 ++++++++++++++++----------
> drivers/irqchip/irq-gic-v3.c | 8 +-
> include/linux/cpuhotplug.h | 1 +
> include/linux/irqchip/arm-gic-v3.h | 5 +-
> 6 files changed, 195 insertions(+), 116 deletions(-)
>
> --
> 2.11.0
>
On Tue, Aug 08 2017 at 2:22:52 pm BST, Robert Richter <[email protected]> wrote:
> This allows us to use kernel core functionality (e.g. cma) for ITS
> initialization. MSIs must be up before the device_initcalls (pci and
> platform device probe) and after arch_initcalls (dma init), so
> subsys_initcall is fine.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its-pci-msi.c | 2 +-
> drivers/irqchip/irq-gic-v3-its-platform-msi.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index 77931214d954..5940fdf0036e 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -193,4 +193,4 @@ static int __init its_pci_msi_init(void)
>
> return 0;
> }
> -early_initcall(its_pci_msi_init);
> +subsys_initcall(its_pci_msi_init);
> diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> index 249240d9a425..6ebc871ac63f 100644
> --- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> @@ -166,4 +166,4 @@ static int __init its_pmsi_init(void)
> its_pmsi_acpi_init();
> return 0;
> }
> -early_initcall(its_pmsi_init);
> +subsys_initcall(its_pmsi_init);
You've missed the fsl-mc stuff in staging, which has the same behaviour,
except that it is a postcore_initcall.
More importantly, what guarantees do we have that this doesn't break
anything else? Sure, the ITS itself will work fine, but what about the
subsystems that depend on it? Just on the PCI side, I can see a bunch of
initcalls that are much earlier than subsys, and that says nothing of
other subsystems (such as the above fsl stuff).
I'm not saying that this is wrong, but I'd like some evidence that
you've done the required due diligence, ensuring this will not cause any
regression.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
+Lorenzo
On 08/08/17 13:22, Robert Richter wrote:
> Use an initcall to initialize its. This allows us to use the device
> framework during initialization that is up at this point. We use
> subsys_initcall() here since we need the arch to be initialized
> first. It is before pci and platform device probe where devices are
> bound to msi interrupts.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 3 ++-
> drivers/irqchip/irq-gic-v3.c | 5 -----
> include/linux/irqchip/arm-gic-v3.h | 1 -
> 3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5e2d4f2876d8..488f811d5978 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
> return 0;
> }
>
> -int __init its_init(void)
> +static int __init its_init(void)
> {
> struct its_node *its, *tmp;
> int err = 0, err2;
> @@ -2036,3 +2036,4 @@ int __init its_init(void)
>
> return 0;
> }
> +subsys_initcall(its_init);
*ding*!
I'm sorry, but that's a total NAK. We're trading hard to maintain
hardcoded dependencies for even more difficult to deal with, link-order
driven dependencies. That's exactly what Lorenzo and I have been
fighting against in the XGene ACPI case, and I'm not going to introduce
more of this.
We need to break the dependency between the HW and the associated MSI
domains, not making it tighter. Let the HW be probed at some time, and
the MSI domains lazily created as needed once the end-points are probed.
I'm also pretty worried that (as mentioned in my reply to patch #2) that
this "make it happen later" games with the initcalls are breaking stuff
(see how platform devices are instantiated on the back of an
arch_initcall_sync). As far as I can tell, non-PCI MSI is dead after
patch #2.
I think there is a number of things to fix in the core code before we
can start playing that kind of tricks. The major issue I see is that MSI
domains are expected to be available way too early, at device discovery
time rather than at driver probe time. This has a cascading effect which
we should solve first.
I'll try to prototype something...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Marc,
thanks for your review.
On 21.08.17 09:30:24, Marc Zyngier wrote:
> +Lorenzo
>
> On 08/08/17 13:22, Robert Richter wrote:
> > Use an initcall to initialize its. This allows us to use the device
> > framework during initialization that is up at this point. We use
> > subsys_initcall() here since we need the arch to be initialized
> > first. It is before pci and platform device probe where devices are
> > bound to msi interrupts.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/irqchip/irq-gic-v3-its.c | 3 ++-
> > drivers/irqchip/irq-gic-v3.c | 5 -----
> > include/linux/irqchip/arm-gic-v3.h | 1 -
> > 3 files changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 5e2d4f2876d8..488f811d5978 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1994,7 +1994,7 @@ int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
> > return 0;
> > }
> >
> > -int __init its_init(void)
> > +static int __init its_init(void)
> > {
> > struct its_node *its, *tmp;
> > int err = 0, err2;
> > @@ -2036,3 +2036,4 @@ int __init its_init(void)
> >
> > return 0;
> > }
> > +subsys_initcall(its_init);
>
> *ding*!
>
> I'm sorry, but that's a total NAK. We're trading hard to maintain
> hardcoded dependencies for even more difficult to deal with, link-order
> driven dependencies. That's exactly what Lorenzo and I have been
> fighting against in the XGene ACPI case, and I'm not going to introduce
> more of this.
>
> We need to break the dependency between the HW and the associated MSI
> domains, not making it tighter. Let the HW be probed at some time, and
> the MSI domains lazily created as needed once the end-points are probed.
I see your point here. the ordering of initialization functions using
initcalls is fragile.
> I'm also pretty worried that (as mentioned in my reply to patch #2) that
> this "make it happen later" games with the initcalls are breaking stuff
> (see how platform devices are instantiated on the back of an
> arch_initcall_sync). As far as I can tell, non-PCI MSI is dead after
> patch #2.
Yes, I missed that as you pointed out in #2. Even if we change it to a
subsys_initcall (not sure if that would generally work for this),
there still would be link order dependencies.
I have tested it and even pci msi has order dependencies that are not
guaranteed:
ffff000008c14e60 t __initcall_its_init4
ffff000008c14e68 t __initcall_its_pci_msi_init4
ffff000008c14e70 t __initcall_its_pmsi_init4
With a different order the intialization would fail here.
> I think there is a number of things to fix in the core code before we
> can start playing that kind of tricks. The major issue I see is that MSI
> domains are expected to be available way too early, at device discovery
> time rather than at driver probe time. This has a cascading effect which
> we should solve first.
>
> I'll try to prototype something...
I will look into this too.
Thanks,
-Robert