[PATCH 00/10] irqdomain, gic-v3-its: Implement late irq domain initialization

[PATCH 00/10] irqdomain, gic-v3-its: Implement late irq domain initialization

[Robert Richter]

This patch series is a rework of the gic-v3-its initialization. It is
in preparation of a further series that uses CMA memory allocation for
ITS tables. For this the CMA framework must be available and thus ITS
needs to be initialized after the arch_initcalls. This requires two
major reworks.

First we must remove all irq domain init order dependencies (patches
#1-#5), second the ITS initialization must be split into an early
probe and a later init part (patches #6-#10).

Patch #1 introduces a new interface to request an irq domain, see the
patch description for details. In patches #2-#5 all ITS related irq
domains are converted to use the new interface. There are no order
dependencies for parent and client irq domain initialization anymore
which allows us to initialize all ITS irq domains in the same initcall
(moving to the later subsys_initcall). Note that I do not have fsl
hardware available, the code should work, but is only carefully
reviewed and compile tested, please test on that hardware.

The remaining patches #6-#10 are an incremental rework of the ITS
initialization, basically splitting probe and init (patch #7) and
separating it from gic-v3 code (patch #8). Patches #9 and #10 change
initcall levels for various drivers.

Patches have been tested with Cavium ThunderX and ThunderX2. I have an
implementation of CMA ITS table allocation on top of this series
available, I will send out patches for this in a couple of days.

Robert Richter (10):
  irqdomain: Add interface to request an irq domain
  irqchip/gic-v3-its-platform-msi: Remove domain init order dependencies
  irqchip/irq-gic-v3-its-pci-msi: Remove domain init order dependencies
  irqchip/irq-gic-v3-its-fsl-mc-msi: Remove domain init order
    dependencies
  fsl-mc/dprc-driver: Remove domain init order dependencies
  irqchip/gic-v3-its: Initialize its nodes in probe order
  irqchip/gic-v3-its: Split probing from its node initialization
  irqchip/gic-v3-its: Decouple its initialization from gic
  irqchip/gic-v3-its: Initialize its nodes later
  irqchip/gic-v3-its: Initialize MSIs with subsys_initcalls

 drivers/bus/fsl-mc/dprc-driver.c              |  41 +++++++
 drivers/bus/fsl-mc/fsl-mc-msi.c               |   6 +-
 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c   |  49 +++++---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c      |  68 ++++++-----
 drivers/irqchip/irq-gic-v3-its-platform-msi.c |  56 ++++++---
 drivers/irqchip/irq-gic-v3-its.c              | 160 +++++++++++++++++---------
 drivers/irqchip/irq-gic-v3.c                  |  12 +-
 include/linux/cpuhotplug.h                    |   1 +
 include/linux/irqchip/arm-gic-v3.h            |   5 +-
 include/linux/irqdomain.h                     |  15 +++
 kernel/irq/irqdomain.c                        | 158 +++++++++++++++++++++++++
 11 files changed, 441 insertions(+), 130 deletions(-)

-- 
2.11.0

[PATCH 01/10] irqdomain: Add interface to request an irq domain

[Robert Richter]

This patch introduces a new interface to allow irq domain
initialization regardless of order dependencies. This is done by
requesting a domain and registering a callback function that is called
as soon as a domain becomes available.

A typical irq domain initialization code is the following:

Parent initialization:

	...
	domain = msi_create_irq_domain(fwnode, info, parent);
	if (domain)
		irq_domain_update_bus_token(domain, bus_token);
	...

Child initialization:

	...
	parent = irq_find_matching_fwnode(fwnode, bus_token);
	if (!parent)
		...
	create_irq_domain(parent, ...);
	...

In case the parent is not yet available, the child initialization
fails. Thus, the current implementation requires the parent domain
being initialized before the child domain. With a complex irq domain
hierarchy it becomes more and more difficult to grant that order as
irq domains are enabled in separate subsystems. Care must be taken
when initializing parent and child domains in the same initcall
level. E.g. Arm's gic-v3-its implementation might have the following
tree and dependencies:

	gic-v3
	├── its-node-0
	│   ├── pci-host-0
	│   ├── platform-bus
	│   ...
	├── its-node-1
	│   ├── pci-host-1
	...

All domains must be initialized in top-down order of the tree.

This patch introduces an interface that allows domain initialization
without any order requirements, e.g. to be able to initialize the irq
domains in the same initcall level. The following functions have been
introduced to allow the registration of a callback handler:

	irq_domain_request_fwnode()
	irq_domain_request_host()

Instead of using the irq_find_matching_fwnode() function and it's
variants the child code replaces them with the new functions and
looks e.g. like the following:

	...
	irq_domain_request_fwnode(fwnode, bus_token,
				create_irq_domain, name, priv);
	...

Here, the callback function create_irq_domain() is called as soon as
the parent becomes available. All registered handlers are stored in a
list. With each update of the bus token using irq_domain_update_bus_
token(), the list is checked if that domain is requested by a handler
and if that is the case it's callback function is called and the
request removed from the list.

With a late_initcall all requests from the list should already have
been handled, otherwise all remaining requests are removed with an
error reported.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 include/linux/irqdomain.h |  15 +++++
 kernel/irq/irqdomain.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 068aa46f0d55..27e83803627d 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -311,6 +311,21 @@ static inline struct irq_domain *irq_find_host(struct device_node *node)
 	return d;
 }
 
+typedef int (*irq_domain_callback_t)(struct irq_domain *, void *);
+int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
+			enum irq_domain_bus_token bus_token,
+			irq_domain_callback_t callback,
+			const char *name, void *priv);
+
+static inline int irq_domain_request_host(struct device_node *node,
+					enum irq_domain_bus_token bus_token,
+					irq_domain_callback_t callback,
+					void *priv)
+{
+	return irq_domain_request_fwnode(of_node_to_fwnode(node), bus_token,
+					callback, node->full_name, priv);
+}
+
 /**
  * irq_domain_add_linear() - Allocate and register a linear revmap irq_domain.
  * @of_node: pointer to interrupt controller's device tree node.
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3366d11c3e02..9e33d873d8f6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -21,6 +21,7 @@
 #include <linux/fs.h>
 
 static LIST_HEAD(irq_domain_list);
+static LIST_HEAD(irq_domain_requests);
 static DEFINE_MUTEX(irq_domain_mutex);
 
 static struct irq_domain *irq_default_domain;
@@ -45,6 +46,106 @@ static inline void debugfs_remove_domain_dir(struct irq_domain *d) { }
 const struct fwnode_operations irqchip_fwnode_ops;
 EXPORT_SYMBOL_GPL(irqchip_fwnode_ops);
 
+struct irq_domain_request {
+	struct list_head		list;
+	struct fwnode_handle		*fwnode;
+	enum irq_domain_bus_token	bus_token;
+	irq_domain_callback_t		callback;
+	char				*name;
+	void				*priv;
+};
+
+static void irq_domain_call_handler(struct irq_domain *domain,
+		irq_domain_callback_t callback, const char *name, void *priv)
+{
+	int ret;
+
+	ret = callback(domain, priv);
+	if (ret)
+		pr_err("%s: Domain request handler failed: %d\n",
+			name, ret);
+
+	of_node_put(irq_domain_get_of_node(domain));
+}
+
+static void irq_domain_free_request(struct irq_domain_request *request)
+{
+	kfree(request->name);
+	kfree(request);
+}
+
+static void irq_domain_handle_requests(struct fwnode_handle *fwnode,
+                        enum irq_domain_bus_token bus_token)
+{
+	struct irq_domain *domain;
+	struct irq_domain_request *request;
+
+	if (!fwnode)
+		return;
+redo:
+	domain = irq_find_matching_fwnode(fwnode, bus_token);
+	if (!domain)
+		return;
+
+	mutex_lock(&irq_domain_mutex);
+
+	if ((domain->fwnode != fwnode) && (domain->bus_token != bus_token)) {
+		mutex_unlock(&irq_domain_mutex);
+		goto redo;
+	}
+
+	list_for_each_entry(request, &irq_domain_requests, list) {
+		if (request->fwnode != fwnode ||
+		    request->bus_token != bus_token)
+			continue;
+
+		list_del(&request->list);
+		mutex_unlock(&irq_domain_mutex);
+
+		irq_domain_call_handler(domain, request->callback,
+					request->name, request->priv);
+		irq_domain_free_request(request);
+
+		goto redo;
+	}
+
+	mutex_unlock(&irq_domain_mutex);
+}
+
+static int __init irq_domain_drain_requests(void)
+{
+	struct irq_domain_request *request;
+	struct irq_domain *domain;
+	int ret = 0;
+redo:
+	mutex_lock(&irq_domain_mutex);
+
+	list_for_each_entry(request, &irq_domain_requests, list) {
+		list_del(&request->list);
+		mutex_unlock(&irq_domain_mutex);
+
+		domain = irq_find_matching_fwnode(request->fwnode,
+						request->bus_token);
+		if (domain) {
+			irq_domain_call_handler(domain, request->callback,
+						request->name, request->priv);
+		} else {
+			ret = -ENODEV;
+			pr_err("%s-%d: Unhandled domain request\n",
+				request->name, request->bus_token);
+		}
+
+		irq_domain_free_request(request);
+
+		goto redo;
+	}
+
+	mutex_unlock(&irq_domain_mutex);
+
+	return ret;
+}
+late_initcall(irq_domain_drain_requests);
+
 /**
  * irq_domain_alloc_fwnode - Allocate a fwnode_handle suitable for
  *                           identifying an irq domain
@@ -293,6 +394,8 @@ void irq_domain_update_bus_token(struct irq_domain *domain,
 	debugfs_add_domain_dir(domain);
 
 	mutex_unlock(&irq_domain_mutex);
+
+	irq_domain_handle_requests(domain->fwnode, bus_token);
 }
 
 /**
@@ -417,6 +520,61 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);
 
 /**
+ * irq_domain_request_fwnode() - Requests a domain for a given fwspec
+ * @fwspec: FW specifier for an interrupt
+ * @bus_token: domain-specific data
+ * @callback: function to be called once domain becomes available
+ * @name: name to be used for fwnode
+ * @priv: private data to be passed to callback
+ *
+ * The callback function is called as soon as the domain is available.
+ */
+int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
+			enum irq_domain_bus_token bus_token,
+			irq_domain_callback_t callback,
+			const char *name, void *priv)
+{
+	struct irq_domain *parent;
+	struct irq_domain_request *request;
+
+	if (!fwnode || bus_token == DOMAIN_BUS_ANY || !callback || !name)
+		return -EINVAL;
+
+	parent = irq_find_matching_fwnode(fwnode, bus_token);
+	if (parent) {
+		irq_domain_call_handler(parent, callback, name, priv);
+		return 0;
+	}
+
+	request = kzalloc(sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	request->fwnode = fwnode;
+	request->bus_token = bus_token;
+	request->callback = callback;
+	request->name = kstrdup(name, GFP_KERNEL);
+	request->priv = priv;
+	INIT_LIST_HEAD(&request->list);
+
+	if (!request->name) {
+		kfree(request);
+		return -ENOMEM;
+	}
+
+	of_node_get(to_of_node(fwnode));
+
+	mutex_lock(&irq_domain_mutex);
+	list_add_tail(&request->list, &irq_domain_requests);
+	mutex_unlock(&irq_domain_mutex);
+
+	/* recheck in case list changed */
+	irq_domain_handle_requests(fwnode, bus_token);
+
+	return 0;
+}
+
+/**
  * irq_domain_check_msi_remap - Check whether all MSI irq domains implement
  * IRQ remapping
  *
-- 
2.11.0

[PATCH 02/10] irqchip/gic-v3-its-platform-msi: Remove domain init order dependencies

[Robert Richter]

Use new irq_domain_request_host_*() interface which allows independent
parent and child initialization using an irq domain request handler.
This makes it possible to move its initialization to a later point
during boot. All domains can be initialized in the same initcall level
then.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-platform-msi.c | 54 +++++++++++++++++++++------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 7b8e87b493fe..1f2849bc58c4 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -94,25 +94,56 @@ static const struct of_device_id its_device_id[] = {
 	{},
 };
 
-static int __init its_pmsi_init_one(struct fwnode_handle *fwnode,
-				const char *name)
+static int __init its_pmsi_create_irq_domain(struct irq_domain *parent,
+					void *priv)
 {
-	struct irq_domain *parent;
+	const char *name = priv;
+	int err = 0;
 
-	parent = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_NEXUS);
-	if (!parent || !msi_get_domain_info(parent)) {
-		pr_err("%s: unable to locate ITS domain\n", name);
-		return -ENXIO;
+	if (!msi_get_domain_info(parent)) {
+		err = -ENODEV;
+		goto out;
 	}
 
-	if (!platform_msi_create_irq_domain(fwnode, &its_pmsi_domain_info,
+	if (!platform_msi_create_irq_domain(parent->fwnode, &its_pmsi_domain_info,
 					    parent)) {
-		pr_err("%s: unable to create platform domain\n", name);
-		return -ENXIO;
+		err = -ENXIO;
+		goto out;
 	}
 
 	pr_info("Platform MSI: %s domain created\n", name);
-	return 0;
+out:
+	if (err)
+		pr_err("Platform MSI: Failed to create %s domain\n", name);
+
+	kfree(name);
+	return err;
+}
+
+static int __init its_pmsi_init_one(struct fwnode_handle *fwnode,
+				const char *name)
+{
+	void *priv = kstrdup(name, GFP_KERNEL);
+	int err;
+
+	if (!name) {
+		err = -EINVAL;
+		goto fail;
+	}
+
+	if (!priv) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	err = irq_domain_request_fwnode(fwnode, DOMAIN_BUS_NEXUS,
+					its_pmsi_create_irq_domain, name, priv);
+	if (!err)
+		return 0;
+fail:
+	pr_err("Platform MSI: Failed to register %s domain\n", name);
+	kfree(priv);
+	return err;
 }
 
 #ifdef CONFIG_ACPI
@@ -135,7 +166,6 @@ its_pmsi_parse_madt(struct acpi_subtable_header *header,
 	}
 
 	err = its_pmsi_init_one(domain_handle, node_name);
-
 out:
 	kfree(node_name);
 	return err;
-- 
2.11.0

[PATCH 03/10] irqchip/irq-gic-v3-its-pci-msi: Remove domain init order dependencies

[Robert Richter]

Use new irq_domain_request_host_*() interface which allows independent
parent and child initialization using an irq domain request handler.
This makes it possible to move its initialization to a later point
during boot. All domains can be initialized in the same initcall level
then.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 66 ++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index 8d6d009d1d58..7d7366d55d34 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -116,27 +116,50 @@ static struct of_device_id its_device_id[] = {
 	{},
 };
 
-static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
-				       const char *name)
+static int __init its_pci_create_irq_domain(struct irq_domain *parent,
+					void *priv)
 {
-	struct irq_domain *parent;
+	char *name = priv;
+	int err = 0;
 
-	parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
-	if (!parent || !msi_get_domain_info(parent)) {
-		pr_err("%s: Unable to locate ITS domain\n", name);
-		return -ENXIO;
+	if (!msi_get_domain_info(parent)) {
+		err = -ENODEV;
+		goto out;
 	}
 
-	if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
+	if (!pci_msi_create_irq_domain(parent->fwnode, &its_pci_msi_domain_info,
 				       parent)) {
-		pr_err("%s: Unable to create PCI domain\n", name);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto out;
 	}
 
-	return 0;
+	pr_info("PCI/MSI: %s domain created\n", name);
+out:
+	if (err)
+		pr_err("PCI/MSI: Failed to create %s domain: %d\n", name, err);
+
+	kfree(name);
+	return err;
+}
+
+static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
+				       const char *name)
+{
+	void *priv = kstrdup(name, GFP_KERNEL);
+	int err;
+
+	err = irq_domain_request_fwnode(handle, DOMAIN_BUS_NEXUS,
+					its_pci_create_irq_domain, name, priv);
+	if (err) {
+		pr_err("PCI/MSI: Failed to register %s domain: %d\n",
+			name, err);
+		kfree(priv);
+	}
+
+	return err;
 }
 
-static int __init its_pci_of_msi_init(void)
+static void __init its_pci_of_msi_init(void)
 {
 	struct device_node *np;
 
@@ -147,13 +170,8 @@ static int __init its_pci_of_msi_init(void)
 		if (!of_property_read_bool(np, "msi-controller"))
 			continue;
 
-		if (its_pci_msi_init_one(of_node_to_fwnode(np), np->full_name))
-			continue;
-
-		pr_info("PCI/MSI: %pOF domain created\n", np);
+		its_pci_msi_init_one(of_node_to_fwnode(np), np->full_name);
 	}
-
-	return 0;
 }
 
 #ifdef CONFIG_ACPI
@@ -177,32 +195,24 @@ its_pci_msi_parse_madt(struct acpi_subtable_header *header,
 	}
 
 	err = its_pci_msi_init_one(dom_handle, node_name);
-	if (!err)
-		pr_info("PCI/MSI: %s domain created\n", node_name);
-
 out:
 	kfree(node_name);
 	return err;
 }
 
-static int __init its_pci_acpi_msi_init(void)
+static void __init its_pci_acpi_msi_init(void)
 {
 	acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
 			      its_pci_msi_parse_madt, 0);
-	return 0;
 }
 #else
-static int __init its_pci_acpi_msi_init(void)
-{
-	return 0;
-}
+static void __init its_pci_acpi_msi_init(void) { }
 #endif
 
 static int __init its_pci_msi_init(void)
 {
 	its_pci_of_msi_init();
 	its_pci_acpi_msi_init();
-
 	return 0;
 }
 early_initcall(its_pci_msi_init);
-- 
2.11.0

[PATCH 04/10] irqchip/irq-gic-v3-its-fsl-mc-msi: Remove domain init order dependencies

[Robert Richter]

Use new irq_domain_request_host_*() interface which allows independent
parent and child initialization using an irq domain request handler.
This makes it possible to move its initialization to a later point
during boot. All domains can be initialized in the same initcall level
then.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 47 ++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
index 606efa64adff..0e9b31f13618 100644
--- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
@@ -66,11 +66,34 @@ static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static int __init its_fsl_create_irq_domain(struct irq_domain *parent,
+					void *priv)
+{
+	struct device_node *np = irq_domain_get_of_node(parent);
+	struct irq_domain *mc_msi_domain;
+
+	if (!msi_get_domain_info(parent)) {
+		pr_err("%pOF: unable to locate ITS domain\n", np);
+		return -ENXIO;
+	}
+
+	mc_msi_domain = fsl_mc_msi_create_irq_domain(parent->fwnode,
+						&its_fsl_mc_msi_domain_info,
+						parent);
+	if (!mc_msi_domain) {
+		pr_err("%pOF: unable to create fsl-mc domain\n", np);
+		return -ENOMEM;
+	}
+
+	pr_info("fsl-mc MSI: %pOF domain created\n", np);
+
+	return 0;
+}
+
 static int __init its_fsl_mc_msi_init(void)
 {
 	struct device_node *np;
-	struct irq_domain *parent;
-	struct irq_domain *mc_msi_domain;
+	int ret = 0;
 
 	for (np = of_find_matching_node(NULL, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
@@ -79,22 +102,10 @@ static int __init its_fsl_mc_msi_init(void)
 		if (!of_property_read_bool(np, "msi-controller"))
 			continue;
 
-		parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
-		if (!parent || !msi_get_domain_info(parent)) {
-			pr_err("%pOF: unable to locate ITS domain\n", np);
-			continue;
-		}
-
-		mc_msi_domain = fsl_mc_msi_create_irq_domain(
-						 of_node_to_fwnode(np),
-						 &its_fsl_mc_msi_domain_info,
-						 parent);
-		if (!mc_msi_domain) {
-			pr_err("%pOF: unable to create fsl-mc domain\n", np);
-			continue;
-		}
-
-		pr_info("fsl-mc MSI: %pOF domain created\n", np);
+		ret = irq_domain_request_host(np, DOMAIN_BUS_NEXUS,
+						its_fsl_create_irq_domain, NULL);
+		if (ret)
+			pr_err("%pOF: unable to register ITS domain\n", np);
 	}
 
 	return 0;
-- 
2.11.0

[PATCH 05/10] fsl-mc/dprc-driver: Remove domain init order dependencies

[Robert Richter]

Use new irq_domain_request_host_*() interface which allows independent
parent and child initialization using an irq domain request handler.
This makes it possible to move its initialization to a later point
during boot. All domains can be initialized in the same initcall level
then.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/bus/fsl-mc/dprc-driver.c | 41 ++++++++++++++++++++++++++++++++++++++++
 drivers/bus/fsl-mc/fsl-mc-msi.c  |  6 +-----
 2 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 52c7e15143d6..2f41886c8e00 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -10,7 +10,9 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/irqdomain.h>
 #include <linux/msi.h>
+#include <linux/of.h>
 #include <linux/fsl/mc.h>
 
 #include "fsl-mc-private.h"
@@ -575,6 +577,40 @@ static int dprc_setup_irq(struct fsl_mc_device *mc_dev)
 	return error;
 }
 
+static int dprc_probe(struct fsl_mc_device *mc_dev);
+
+static int dprc_probe_late(struct irq_domain *parent, void *priv)
+{
+	struct fsl_mc_device *mc_dev = priv;
+
+	of_node_put(mc_dev->dev.parent->of_node);
+	of_node_put(irq_domain_get_of_node(parent));
+
+	return dprc_probe(mc_dev);
+}
+
+static int dprc_register_msi_domain(struct fsl_mc_device *mc_dev)
+{
+	struct device_node *mc_of_node, *msi_np;
+	int err = -EINVAL;
+
+	mc_of_node = of_node_get(mc_dev->dev.parent->of_node);
+
+	msi_np = of_parse_phandle(mc_of_node, "msi-parent", 0);
+	if (msi_np && !of_property_read_bool(msi_np, "#msi-cells"))
+		err = irq_domain_request_host(msi_np, DOMAIN_BUS_FSL_MC_MSI,
+			dprc_probe_late, mc_dev);
+
+	if (err) {
+		pr_err("Unable to find fsl-mc MSI domain for %pOF\n",
+			mc_of_node);
+		of_node_put(msi_np);
+		of_node_put(mc_of_node);
+	}
+
+	return err;
+}
+
 /**
  * dprc_probe - callback invoked when a DPRC is being bound to this driver
  *
@@ -641,6 +677,11 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 
 		error = fsl_mc_find_msi_domain(parent_dev,
 					       &mc_msi_domain);
+
+		if (error == -ENOENT)
+			/* initialize later */
+			return dprc_register_msi_domain(mc_dev);
+
 		if (error < 0) {
 			dev_warn(&mc_dev->dev,
 				 "WARNING: MC bus without interrupt support\n");
diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c
index 8b9c66d7c4ff..550d2ed07f69 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -185,12 +185,8 @@ int fsl_mc_find_msi_domain(struct device *mc_platform_dev,
 
 	msi_domain = of_msi_get_domain(mc_platform_dev, mc_of_node,
 				       DOMAIN_BUS_FSL_MC_MSI);
-	if (!msi_domain) {
-		pr_err("Unable to find fsl-mc MSI domain for %pOF\n",
-		       mc_of_node);
-
+	if (!msi_domain)
 		return -ENOENT;
-	}
 
 	*mc_msi_domain = msi_domain;
 	return 0;
-- 
2.11.0

[PATCH 06/10] irqchip/gic-v3-its: Initialize its nodes in probe order

[Robert Richter]

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 <rrichter@cavium.com>
---
 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 db20e992a40f..4033f71f5181 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3602,7 +3602,7 @@ static int __init its_probe_one(struct resource *res,
 		goto out_free_tables;
 
 	raw_spin_lock(&its_lock);
-	list_add(&its->entry, &its_nodes);
+	list_add_tail(&its->entry, &its_nodes);
 	raw_spin_unlock(&its_lock);
 
 	return 0;
-- 
2.11.0

[PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization

[Robert Richter]

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 <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 135 +++++++++++++++++++++++--------------
 drivers/irqchip/irq-gic-v3.c       |   2 +-
 include/linux/irqchip/arm-gic-v3.h |   4 +-
 3 files changed, 87 insertions(+), 54 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 4033f71f5181..c28f4158ff70 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -103,6 +103,7 @@ struct its_node {
 	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];
@@ -3375,7 +3376,7 @@ static struct syscore_ops its_syscore_ops = {
 	.resume = its_restore_enable,
 };
 
-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;
@@ -3384,7 +3385,8 @@ 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_handle,
+					&its_domain_ops, its);
 	if (!inner_domain) {
 		kfree(info);
 		return -ENOMEM;
@@ -3441,8 +3443,7 @@ static int its_init_vpe_domain(void)
 	return 0;
 }
 
-static int __init its_compute_its_list_map(struct resource *res,
-					   void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
 {
 	int its_number;
 	u32 ctlr;
@@ -3456,15 +3457,15 @@ static int __init its_compute_its_list_map(struct resource *res,
 	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
 	if (its_number >= GICv4_ITS_LIST_MAX) {
 		pr_err("ITS@%pa: No ITSList entry available!\n",
-		       &res->start);
+		       &its->phys_base);
 		return -EINVAL;
 	}
 
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	ctlr &= ~GITS_CTLR_ITS_NUMBER;
 	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
-	writel_relaxed(ctlr, its_base + GITS_CTLR);
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	writel_relaxed(ctlr, its->base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
 		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
 		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -3472,83 +3473,110 @@ static int __init its_compute_its_list_map(struct resource *res,
 
 	if (test_and_set_bit(its_number, &its_list_map)) {
 		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
-		       &res->start, its_number);
+		       &its->phys_base, its_number);
 		return -EINVAL;
 	}
 
 	return its_number;
 }
 
+static void its_free(struct its_node *its)
+{
+	raw_spin_lock(&its_lock);
+	list_del(&its->entry);
+	raw_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 = handle;
+	its->phys_base = res->start;
+	its->phys_size = resource_size(res);
+	its->numa_node = numa_node;
+
+	raw_spin_lock(&its_lock);
+	list_add_tail(&its->entry, &its_nodes);
+	raw_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, ctlr;
 	u64 baser, tmp, typer;
 	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);
+		pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
 		goto out_unmap;
 	}
 
-	pr_info("ITS %pR\n", res);
-
-	its = kzalloc(sizeof(*its), GFP_KERNEL);
-	if (!its) {
-		err = -ENOMEM;
-		goto out_unmap;
-	}
-
-	raw_spin_lock_init(&its->lock);
-	INIT_LIST_HEAD(&its->entry);
-	INIT_LIST_HEAD(&its->its_device_list);
 	typer = gic_read_typer(its_base + GITS_TYPER);
 	its->base = its_base;
-	its->phys_base = res->start;
 	its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
 	its->device_ids = GITS_TYPER_DEVBITS(typer);
 	its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
 	if (its->is_v4) {
 		if (!(typer & GITS_TYPER_VMOVP)) {
-			err = its_compute_its_list_map(res, its_base);
+			err = its_compute_its_list_map(its);
 			if (err < 0)
-				goto out_free_its;
+				goto out_unmap;
 
 			its->list_nr = err;
 
 			pr_info("ITS@%pa: Using ITS number %d\n",
-				&res->start, err);
+				&its->phys_base, err);
 		} else {
-			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+			pr_info("ITS@%pa: Single VMOVP capable\n",
+				&its->phys_base);
 		}
 	}
 
-	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;
-	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
 	its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
 
@@ -3597,13 +3625,11 @@ static int __init its_probe_one(struct resource *res,
 	if (GITS_TYPER_HCC(typer))
 		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
 
-	err = its_init_domain(handle, its);
+	err = its_init_domain(its);
 	if (err)
 		goto out_free_tables;
 
-	raw_spin_lock(&its_lock);
-	list_add_tail(&its->entry, &its_nodes);
-	raw_spin_unlock(&its_lock);
+	pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
 
 	return 0;
 
@@ -3611,11 +3637,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;
 }
 
@@ -3888,13 +3913,12 @@ 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;
-	struct its_node *its;
-	bool has_v4 = false;
-	int err;
 
 	its_parent = parent_domain;
 	of_node = to_of_node(handle);
@@ -3903,13 +3927,22 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	else
 		its_acpi_probe();
 
+	gic_rdists = rdists;
+
+	return its_init();
+}
+
+static int __init its_init(void)
+{
+	struct its_node *its;
+	bool has_v4 = false;
+	int err;
+
 	if (list_empty(&its_nodes)) {
 		pr_warn("ITS: No ITS available, not enabling LPIs\n");
 		return -ENXIO;
 	}
 
-	gic_rdists = rdists;
-
 	err = allocate_lpi_tables();
 	if (err)
 		return err;
@@ -3917,10 +3950,10 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	list_for_each_entry(its, &its_nodes, entry)
 		has_v4 |= its->is_v4;
 
-	if (has_v4 & rdists->has_vlpis) {
+	if (has_v4 & gic_rdists->has_vlpis) {
 		if (its_init_vpe_domain() ||
-		    its_init_v4(parent_domain, &its_vpe_domain_ops)) {
-			rdists->has_vlpis = false;
+		    its_init_v4(its_parent, &its_vpe_domain_ops)) {
+			gic_rdists->has_vlpis = false;
 			pr_err("ITS: Disabling GICv4 support\n");
 		}
 	}
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 8f87f40c9460..e04108b7c6b7 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1132,7 +1132,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
 	gic_cpu_pm_init();
 
 	if (gic_dist_supports_lpis()) {
-		its_init(handle, &gic_data.rdists, gic_data.domain);
+		its_probe(handle, &gic_data.rdists, gic_data.domain);
 		its_cpu_init();
 	}
 
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 071b4cbdf010..a6fdb2910f73 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -603,8 +603,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);
 int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
 
 static inline bool gic_enable_sre(void)
-- 
2.11.0

[PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization from gic

[Robert Richter]

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 <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 58 +++++++++++++++++++++++++-------------
 drivers/irqchip/irq-gic-v3.c       | 14 ++++-----
 include/linux/cpuhotplug.h         |  1 +
 include/linux/irqchip/arm-gic-v3.h |  2 +-
 4 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index c28f4158ff70..fd8561fcfdf3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -167,6 +167,7 @@ static struct {
 } vpe_proxy;
 
 static LIST_HEAD(its_nodes);
+static LIST_HEAD(its_probed);
 static DEFINE_RAW_SPINLOCK(its_lock);
 static struct rdists *gic_rdists;
 static struct irq_domain *its_parent;
@@ -3482,20 +3483,13 @@ static int __init its_compute_its_list_map(struct its_node *its)
 
 static void its_free(struct its_node *its)
 {
-	raw_spin_lock(&its_lock);
-	list_del(&its->entry);
-	raw_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)
@@ -3510,16 +3504,12 @@ static int __init its_probe_one(struct resource *res,
 	its->numa_node = numa_node;
 
 	raw_spin_lock(&its_lock);
-	list_add_tail(&its->entry, &its_nodes);
+	list_add_tail(&its->entry, &its_probed);
 	raw_spin_unlock(&its_lock);
 
 	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)
@@ -3717,7 +3707,7 @@ static int redist_disable_lpis(void)
 	return 0;
 }
 
-int its_cpu_init(void)
+static int its_cpu_init(unsigned int cpu)
 {
 	if (!list_empty(&its_nodes)) {
 		int ret;
@@ -3913,8 +3903,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)
 {
@@ -3929,23 +3917,51 @@ 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)
 {
 	struct its_node *its;
 	bool has_v4 = false;
 	int err;
 
+	if (list_empty(&its_probed))
+		return 0;
+
+	raw_spin_lock(&its_lock);
+redo:
+	list_for_each_entry(its, &its_probed, entry) {
+		list_del_init(&its->entry);
+
+		raw_spin_unlock(&its_lock);
+
+		/* Needs to be called in non-atomic context */
+		err = its_init_one(its);
+		if (err)
+			its_free(its);
+
+		raw_spin_lock(&its_lock);
+
+		if (!err)
+			list_add_tail(&its->entry, &its_nodes);
+
+		goto redo;
+	}
+
+	raw_spin_unlock(&its_lock);
+
 	if (list_empty(&its_nodes)) {
 		pr_warn("ITS: No ITS available, not enabling LPIs\n");
 		return -ENXIO;
 	}
 
 	err = allocate_lpi_tables();
-	if (err)
+	if (err) {
+		pr_warn("ITS: Failed to initialize (%d), not enabling LPIs\n",
+			err);
 		return err;
+	}
 
 	list_for_each_entry(its, &its_nodes, entry)
 		has_v4 |= its->is_v4;
@@ -3960,5 +3976,9 @@ static int __init its_init(void)
 
 	register_syscore_ops(&its_syscore_ops);
 
+	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 e04108b7c6b7..d2942efdb6d5 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -685,9 +685,6 @@ static int gic_starting_cpu(unsigned int cpu)
 {
 	gic_cpu_init();
 
-	if (gic_dist_supports_lpis())
-		its_cpu_init();
-
 	return 0;
 }
 
@@ -815,7 +812,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 */
@@ -1131,10 +1128,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
 	gic_cpu_init();
 	gic_cpu_pm_init();
 
-	if (gic_dist_supports_lpis()) {
+	if (gic_dist_supports_lpis())
 		its_probe(handle, &gic_data.rdists, gic_data.domain);
-		its_cpu_init();
-	}
 
 	return 0;
 
@@ -1327,6 +1322,9 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 
 	if (static_branch_likely(&supports_deactivate_key))
 		gic_of_setup_kvm_info(node);
+
+	its_init();
+
 	return 0;
 
 out_unmap_rdist:
@@ -1630,6 +1628,8 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
 	if (static_branch_likely(&supports_deactivate_key))
 		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 e0cd2baa8380..584f73585142 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -96,6 +96,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 a6fdb2910f73..f4348fa4260a 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -602,9 +602,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);
 int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
 
 static inline bool gic_enable_sre(void)
-- 
2.11.0

[PATCH 09/10] irqchip/gic-v3-its: Initialize its nodes later

[Robert Richter]

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 <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 3 ++-
 drivers/irqchip/irq-gic-v3.c       | 4 ----
 include/linux/irqchip/arm-gic-v3.h | 1 -
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fd8561fcfdf3..13cf56c66483 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3920,7 +3920,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;
 	bool has_v4 = false;
@@ -3982,3 +3982,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 d2942efdb6d5..01538876ad15 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1323,8 +1323,6 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	if (static_branch_likely(&supports_deactivate_key))
 		gic_of_setup_kvm_info(node);
 
-	its_init();
-
 	return 0;
 
 out_unmap_rdist:
@@ -1628,8 +1626,6 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
 	if (static_branch_likely(&supports_deactivate_key))
 		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 f4348fa4260a..885d5a4e239a 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -604,7 +604,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);
 int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
 
 static inline bool gic_enable_sre(void)
-- 
2.11.0

[PATCH 10/10] irqchip/gic-v3-its: Initialize MSIs with subsys_initcalls

[Robert Richter]

Since ITS is initialized with with the subsys_initcall now, we don't
need to enable ITS children earlier. Due to the use of irq_domain_
request_host_*() there are no order dependencies when initializing irq
domains.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c   | 2 +-
 drivers/irqchip/irq-gic-v3-its-pci-msi.c      | 2 +-
 drivers/irqchip/irq-gic-v3-its-platform-msi.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
index 0e9b31f13618..74d63fdff411 100644
--- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
@@ -111,4 +111,4 @@ static int __init its_fsl_mc_msi_init(void)
 	return 0;
 }
 
-early_initcall(its_fsl_mc_msi_init);
+subsys_initcall(its_fsl_mc_msi_init);
diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index 7d7366d55d34..9c4a0ebdab0b 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -215,4 +215,4 @@ static int __init its_pci_msi_init(void)
 	its_pci_acpi_msi_init();
 	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 1f2849bc58c4..76f8a2e85375 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -201,4 +201,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

Re: [PATCH 01/10] irqdomain: Add interface to request an irq domain

[Julien Thierry]

Hi Robert,

On 07/11/18 22:03, Robert Richter wrote:
> This patch introduces a new interface to allow irq domain
> initialization regardless of order dependencies. This is done by
> requesting a domain and registering a callback function that is called
> as soon as a domain becomes available.
> 
> A typical irq domain initialization code is the following:
> 
> Parent initialization:
> 
> 	...
> 	domain = msi_create_irq_domain(fwnode, info, parent);
> 	if (domain)
> 		irq_domain_update_bus_token(domain, bus_token);
> 	...
> 
> Child initialization:
> 
> 	...
> 	parent = irq_find_matching_fwnode(fwnode, bus_token);
> 	if (!parent)
> 		...
> 	create_irq_domain(parent, ...);

What is that function?

> 	...
> 
> In case the parent is not yet available, the child initialization
> fails. Thus, the current implementation requires the parent domain
> being initialized before the child domain. With a complex irq domain
> hierarchy it becomes more and more difficult to grant that order as
> irq domains are enabled in separate subsystems. Care must be taken
> when initializing parent and child domains in the same initcall
> level. E.g. Arm's gic-v3-its implementation might have the following
> tree and dependencies:
> 
> 	gic-v3
> 	├── its-node-0
> 	│   ├── pci-host-0
> 	│   ├── platform-bus
> 	│   ...
> 	├── its-node-1
> 	│   ├── pci-host-1
> 	...
> 
> All domains must be initialized in top-down order of the tree.
> 
> This patch introduces an interface that allows domain initialization
> without any order requirements, e.g. to be able to initialize the irq
> domains in the same initcall level. The following functions have been
> introduced to allow the registration of a callback handler:
> 
> 	irq_domain_request_fwnode()
> 	irq_domain_request_host()
> 
> Instead of using the irq_find_matching_fwnode() function and it's
> variants the child code replaces them with the new functions and
> looks e.g. like the following:
> 
> 	...
> 	irq_domain_request_fwnode(fwnode, bus_token,
> 				create_irq_domain, name, priv);
> 	...
> 
> Here, the callback function create_irq_domain() is called as soon as
> the parent becomes available. All registered handlers are stored in a
> list. With each update of the bus token using irq_domain_update_bus_
> token(), the list is checked if that domain is requested by a handler
> and if that is the case it's callback function is called and the
> request removed from the list.
> 
> With a late_initcall all requests from the list should already have
> been handled, otherwise all remaining requests are removed with an
> error reported.
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>   include/linux/irqdomain.h |  15 +++++
>   kernel/irq/irqdomain.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 173 insertions(+)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 068aa46f0d55..27e83803627d 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -311,6 +311,21 @@ static inline struct irq_domain *irq_find_host(struct device_node *node)
>   	return d;
>   }
>   
> +typedef int (*irq_domain_callback_t)(struct irq_domain *, void *);
> +int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
> +			enum irq_domain_bus_token bus_token,
> +			irq_domain_callback_t callback,
> +			const char *name, void *priv);
> +
> +static inline int irq_domain_request_host(struct device_node *node,
> +					enum irq_domain_bus_token bus_token,
> +					irq_domain_callback_t callback,
> +					void *priv)
> +{
> +	return irq_domain_request_fwnode(of_node_to_fwnode(node), bus_token,
> +					callback, node->full_name, priv);
> +}
> +

A little documentation here? Why do we need both irq_domain_request_host 
and irq_domain_request_fwnode?

Also, you introduced this for domain creation, but the callback here is 
very generic. Is it intended to allow any kind of action once a fwnode 
is available?

>   /**
>    * irq_domain_add_linear() - Allocate and register a linear revmap irq_domain.
>    * @of_node: pointer to interrupt controller's device tree node.
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 3366d11c3e02..9e33d873d8f6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -21,6 +21,7 @@
>   #include <linux/fs.h>
>   
>   static LIST_HEAD(irq_domain_list);
> +static LIST_HEAD(irq_domain_requests);
>   static DEFINE_MUTEX(irq_domain_mutex);
>   
>   static struct irq_domain *irq_default_domain;
> @@ -45,6 +46,106 @@ static inline void debugfs_remove_domain_dir(struct irq_domain *d) { }
>   const struct fwnode_operations irqchip_fwnode_ops;
>   EXPORT_SYMBOL_GPL(irqchip_fwnode_ops);
>   
> +struct irq_domain_request {
> +	struct list_head		list;
> +	struct fwnode_handle		*fwnode;
> +	enum irq_domain_bus_token	bus_token;
> +	irq_domain_callback_t		callback;
> +	char				*name;
> +	void				*priv;
> +};
> +
> +static void irq_domain_call_handler(struct irq_domain *domain,
> +		irq_domain_callback_t callback, const char *name, void *priv)
> +{
> +	int ret;
> +
> +	ret = callback(domain, priv);
> +	if (ret)
> +		pr_err("%s: Domain request handler failed: %d\n",
> +			name, ret);
> +
> +	of_node_put(irq_domain_get_of_node(domain));
> +}
> +
> +static void irq_domain_free_request(struct irq_domain_request *request)
> +{
> +	kfree(request->name);
> +	kfree(request);
> +}
> +
> +static void irq_domain_handle_requests(struct fwnode_handle *fwnode,
> +                        enum irq_domain_bus_token bus_token)
> +{
> +	struct irq_domain *domain;
> +	struct irq_domain_request *request;
> +
> +	if (!fwnode)
> +		return;
> +redo:
> +	domain = irq_find_matching_fwnode(fwnode, bus_token);
> +	if (!domain)
> +		return;
> +
> +	mutex_lock(&irq_domain_mutex);
> +

Why do we need to take the mutex before checking the domain fields? 
Can't we delay it?

> +	if ((domain->fwnode != fwnode) && (domain->bus_token != bus_token)) {

Why do we even need that check?

Isn't the point of passing fwnode and bus_token to 
irq_find_matching_fwnode to find a domain with those properties?

> +		mutex_unlock(&irq_domain_mutex);
> +		goto redo;
> +	}
> +
> +	list_for_each_entry(request, &irq_domain_requests, list) {

Shouldn't you use list_for_each_safe if you want to remove elements of 
the list inside the loop?

> +		if (request->fwnode != fwnode ||
> +		    request->bus_token != bus_token)
> +			continue;
> +
> +		list_del(&request->list);
> +		mutex_unlock(&irq_domain_mutex);
> +
> +		irq_domain_call_handler(domain, request->callback,
> +					request->name, request->priv);
> +		irq_domain_free_request(request);
> +
> +		goto redo;
> +	}
> +
> +	mutex_unlock(&irq_domain_mutex);
> +}
> +
> +static int __init irq_domain_drain_requests(void)
> +{
> +	struct irq_domain_request *request;
> +	struct irq_domain *domain;
> +	int ret = 0;
> +redo:
> +	mutex_lock(&irq_domain_mutex);
> +
> +	list_for_each_entry(request, &irq_domain_requests, list) {

Same remark.

> +		list_del(&request->list);
> +		mutex_unlock(&irq_domain_mutex);
> +
> +		domain = irq_find_matching_fwnode(request->fwnode,
> +						request->bus_token);
> +		if (domain) {
> +			irq_domain_call_handler(domain, request->callback,
> +						request->name, request->priv);
> +		} else {
> +			ret = -ENODEV;
> +			pr_err("%s-%d: Unhandled domain request\n",
> +				request->name, request->bus_token);
> +		}
> +
> +		irq_domain_free_request(request);
> +
> +		goto redo;

Hmmm, are you starting a loop to break out of it at each iteration?

Wouldn't it be much simpler to have something like the following?

	while (!list_empty(&irq_domain_requests) {
		mutex_lock(&irq_domain_mutex);
		request = list_first_entry_or_null(&irq_domain_requests,
					struct irq_domain_request,
					list);
		if (request)
			list_del(&request->list);
		mutex_unlock(&irq_domain_mutex);

		<...>
	}

If you really need to consider possible additions to the list while 
draining it.

Otherwise, just consider doing:

	mutex_lock(&irq_domain_mutex);
	while (!list_empty(&irq_domain_requests)) {
		request = list_first_entry(&irq_domain_requests,
					struct irq_domain_request,
					list);
		list_del(&request->list);
	}
	mutex_unlock(&irq_domain_mutex);

> +	}
> +
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	return ret;
> +}
> +late_initcall(irq_domain_drain_requests);

Overall the code for irq_domain_drain_requests and 
irq_domain_handle_requests feels very similar.

The only difference is that one filters the elements it processes based 
on an irq_domain while the other just goes through all of them.

You can probably factor that code.

Cheers,

> +
>   /**
>    * irq_domain_alloc_fwnode - Allocate a fwnode_handle suitable for
>    *                           identifying an irq domain
> @@ -293,6 +394,8 @@ void irq_domain_update_bus_token(struct irq_domain *domain,
>   	debugfs_add_domain_dir(domain);
>   
>   	mutex_unlock(&irq_domain_mutex);
> +
> +	irq_domain_handle_requests(domain->fwnode, bus_token);
>   }
>   
>   /**
> @@ -417,6 +520,61 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
>   EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);
>   
>   /**
> + * irq_domain_request_fwnode() - Requests a domain for a given fwspec
> + * @fwspec: FW specifier for an interrupt
> + * @bus_token: domain-specific data
> + * @callback: function to be called once domain becomes available
> + * @name: name to be used for fwnode
> + * @priv: private data to be passed to callback
> + *
> + * The callback function is called as soon as the domain is available.
> + */
> +int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
> +			enum irq_domain_bus_token bus_token,
> +			irq_domain_callback_t callback,
> +			const char *name, void *priv)
> +{
> +	struct irq_domain *parent;
> +	struct irq_domain_request *request;
> +
> +	if (!fwnode || bus_token == DOMAIN_BUS_ANY || !callback || !name)
> +		return -EINVAL;
> +
> +	parent = irq_find_matching_fwnode(fwnode, bus_token);
> +	if (parent) {
> +		irq_domain_call_handler(parent, callback, name, priv);
> +		return 0;
> +	}
> +
> +	request = kzalloc(sizeof(*request), GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	request->fwnode = fwnode;
> +	request->bus_token = bus_token;
> +	request->callback = callback;
> +	request->name = kstrdup(name, GFP_KERNEL);
> +	request->priv = priv;
> +	INIT_LIST_HEAD(&request->list);
> +
> +	if (!request->name) {
> +		kfree(request);
> +		return -ENOMEM;
> +	}
> +
> +	of_node_get(to_of_node(fwnode));
> +
> +	mutex_lock(&irq_domain_mutex);
> +	list_add_tail(&request->list, &irq_domain_requests);
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	/* recheck in case list changed */
> +	irq_domain_handle_requests(fwnode, bus_token);
> +
> +	return 0;
> +}
> +
> +/**
>    * irq_domain_check_msi_remap - Check whether all MSI irq domains implement
>    * IRQ remapping
>    *
> 

-- 
Julien Thierry

Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization

[Julien Thierry]

On 07/11/18 22:03, Robert Richter wrote:
> 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 <rrichter@cavium.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c   | 135 +++++++++++++++++++++++--------------
>   drivers/irqchip/irq-gic-v3.c       |   2 +-
>   include/linux/irqchip/arm-gic-v3.h |   4 +-
>   3 files changed, 87 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 4033f71f5181..c28f4158ff70 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -103,6 +103,7 @@ struct its_node {
>   	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];
> @@ -3375,7 +3376,7 @@ static struct syscore_ops its_syscore_ops = {
>   	.resume = its_restore_enable,
>   };
>   
> -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;
> @@ -3384,7 +3385,8 @@ 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_handle,
> +					&its_domain_ops, its);

Separate change?

>   	if (!inner_domain) {
>   		kfree(info);
>   		return -ENOMEM;
> @@ -3441,8 +3443,7 @@ static int its_init_vpe_domain(void)
>   	return 0;
>   }
>   
> -static int __init its_compute_its_list_map(struct resource *res,
> -					   void __iomem *its_base)
> +static int __init its_compute_its_list_map(struct its_node *its)
>   {
>   	int its_number;
>   	u32 ctlr;
> @@ -3456,15 +3457,15 @@ static int __init its_compute_its_list_map(struct resource *res,
>   	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
>   	if (its_number >= GICv4_ITS_LIST_MAX) {
>   		pr_err("ITS@%pa: No ITSList entry available!\n",
> -		       &res->start);
> +		       &its->phys_base);
>   		return -EINVAL;
>   	}
>   
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>   	ctlr &= ~GITS_CTLR_ITS_NUMBER;
>   	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> -	writel_relaxed(ctlr, its_base + GITS_CTLR);
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	writel_relaxed(ctlr, its->base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);

This (removal of its_base parameter) also feel like a separate change.

Also, I would define a local variable its_base to avoid dereferencing 
its every time in order to get the base address.

>   	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
>   		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
>   		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> @@ -3472,83 +3473,110 @@ static int __init its_compute_its_list_map(struct resource *res,
>   
>   	if (test_and_set_bit(its_number, &its_list_map)) {
>   		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> -		       &res->start, its_number);
> +		       &its->phys_base, its_number);
>   		return -EINVAL;
>   	}
>   
>   	return its_number;
>   }
>   
> +static void its_free(struct its_node *its)
> +{
> +	raw_spin_lock(&its_lock);
> +	list_del(&its->entry);
> +	raw_spin_unlock(&its_lock);
> +
> +	kfree(its);
> +}
> +
> +static int __init its_init_one(struct its_node *its);

You might as well define its_init_one here, no?

> +
>   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 = handle;
> +	its->phys_base = res->start;
> +	its->phys_size = resource_size(res);
> +	its->numa_node = numa_node;
> +
> +	raw_spin_lock(&its_lock);
> +	list_add_tail(&its->entry, &its_nodes);
> +	raw_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, ctlr;
>   	u64 baser, tmp, typer;
>   	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);
> +		pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
>   		goto out_unmap;
>   	}
>   
> -	pr_info("ITS %pR\n", res);
> -
> -	its = kzalloc(sizeof(*its), GFP_KERNEL);
> -	if (!its) {
> -		err = -ENOMEM;
> -		goto out_unmap;
> -	}
> -
> -	raw_spin_lock_init(&its->lock);
> -	INIT_LIST_HEAD(&its->entry);
> -	INIT_LIST_HEAD(&its->its_device_list);
>   	typer = gic_read_typer(its_base + GITS_TYPER);
>   	its->base = its_base;
> -	its->phys_base = res->start;
>   	its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
>   	its->device_ids = GITS_TYPER_DEVBITS(typer);
>   	its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
>   	if (its->is_v4) {
>   		if (!(typer & GITS_TYPER_VMOVP)) {
> -			err = its_compute_its_list_map(res, its_base);
> +			err = its_compute_its_list_map(its);
>   			if (err < 0)
> -				goto out_free_its;
> +				goto out_unmap;
>   
>   			its->list_nr = err;
>   
>   			pr_info("ITS@%pa: Using ITS number %d\n",
> -				&res->start, err);
> +				&its->phys_base, err);
>   		} else {
> -			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> +			pr_info("ITS@%pa: Single VMOVP capable\n",
> +				&its->phys_base);
>   		}
>   	}
>   
> -	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;
> -	its->fwnode_handle = handle;
>   	its->get_msi_base = its_irq_get_msi_base;
>   	its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
>   
> @@ -3597,13 +3625,11 @@ static int __init its_probe_one(struct resource *res,
>   	if (GITS_TYPER_HCC(typer))
>   		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>   
> -	err = its_init_domain(handle, its);
> +	err = its_init_domain(its);

I'm not sure what is the logic for "this goes in probe, this goes in init?".

>   	if (err)
>   		goto out_free_tables;
>   
> -	raw_spin_lock(&its_lock);
> -	list_add_tail(&its->entry, &its_nodes);
> -	raw_spin_unlock(&its_lock);
> +	pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
>   
>   	return 0;
>   
> @@ -3611,11 +3637,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;
>   }
>   
> @@ -3888,13 +3913,12 @@ 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;
> -	struct its_node *its;
> -	bool has_v4 = false;
> -	int err;
>   
>   	its_parent = parent_domain;
>   	of_node = to_of_node(handle);
> @@ -3903,13 +3927,22 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>   	else
>   		its_acpi_probe();
>   
> +	gic_rdists = rdists;
> +
> +	return its_init();
> +}
> +
> +static int __init its_init(void)
> +{
> +	struct its_node *its;
> +	bool has_v4 = false;
> +	int err;
> +
>   	if (list_empty(&its_nodes)) {
>   		pr_warn("ITS: No ITS available, not enabling LPIs\n");
>   		return -ENXIO;
>   	}
>   
> -	gic_rdists = rdists;
> -
>   	err = allocate_lpi_tables();
>   	if (err)
>   		return err;
> @@ -3917,10 +3950,10 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>   	list_for_each_entry(its, &its_nodes, entry)
>   		has_v4 |= its->is_v4;
>   
> -	if (has_v4 & rdists->has_vlpis) {
> +	if (has_v4 & gic_rdists->has_vlpis) {
>   		if (its_init_vpe_domain() ||
> -		    its_init_v4(parent_domain, &its_vpe_domain_ops)) {
> -			rdists->has_vlpis = false;
> +		    its_init_v4(its_parent, &its_vpe_domain_ops)) {
> +			gic_rdists->has_vlpis = false;
>   			pr_err("ITS: Disabling GICv4 support\n");
>   		}
>   	}
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 8f87f40c9460..e04108b7c6b7 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1132,7 +1132,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
>   	gic_cpu_pm_init();
>   
>   	if (gic_dist_supports_lpis()) {
> -		its_init(handle, &gic_data.rdists, gic_data.domain);
> +		its_probe(handle, &gic_data.rdists, gic_data.domain);
>   		its_cpu_init();
>   	}
>   
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 071b4cbdf010..a6fdb2910f73 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -603,8 +603,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);
>   int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
>   
>   static inline bool gic_enable_sre(void)
> 

-- 
Julien Thierry

Re: [PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization from gic

[Julien Thierry]

On 07/11/18 22:03, Robert Richter wrote:
> 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 <rrichter@cavium.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c   | 58 +++++++++++++++++++++++++-------------
>   drivers/irqchip/irq-gic-v3.c       | 14 ++++-----
>   include/linux/cpuhotplug.h         |  1 +
>   include/linux/irqchip/arm-gic-v3.h |  2 +-
>   4 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index c28f4158ff70..fd8561fcfdf3 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -167,6 +167,7 @@ static struct {
>   } vpe_proxy;
>   
>   static LIST_HEAD(its_nodes);
> +static LIST_HEAD(its_probed);
>   static DEFINE_RAW_SPINLOCK(its_lock);
>   static struct rdists *gic_rdists;
>   static struct irq_domain *its_parent;
> @@ -3482,20 +3483,13 @@ static int __init its_compute_its_list_map(struct its_node *its)
>   
>   static void its_free(struct its_node *its)
>   {
> -	raw_spin_lock(&its_lock);
> -	list_del(&its->entry);
> -	raw_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)
> @@ -3510,16 +3504,12 @@ static int __init its_probe_one(struct resource *res,
>   	its->numa_node = numa_node;
>   
>   	raw_spin_lock(&its_lock);
> -	list_add_tail(&its->entry, &its_nodes);
> +	list_add_tail(&its->entry, &its_probed);
>   	raw_spin_unlock(&its_lock);
>   
>   	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)
> @@ -3717,7 +3707,7 @@ static int redist_disable_lpis(void)
>   	return 0;
>   }
>   
> -int its_cpu_init(void)
> +static int its_cpu_init(unsigned int cpu)
>   {
>   	if (!list_empty(&its_nodes)) {
>   		int ret;
> @@ -3913,8 +3903,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)
>   {
> @@ -3929,23 +3917,51 @@ 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)
>   {
>   	struct its_node *its;
>   	bool has_v4 = false;
>   	int err;
>   
> +	if (list_empty(&its_probed))
> +		return 0;
> +
> +	raw_spin_lock(&its_lock);
> +redo:
> +	list_for_each_entry(its, &its_probed, entry) {
> +		list_del_init(&its->entry);
> +
> +		raw_spin_unlock(&its_lock);
> +
> +		/* Needs to be called in non-atomic context */
> +		err = its_init_one(its);
> +		if (err)
> +			its_free(its);
> +
> +		raw_spin_lock(&its_lock);
> +
> +		if (!err)
> +			list_add_tail(&its->entry, &its_nodes);
> +
> +		goto redo;

Again, you're starting a loop only to work on the first element and then 
restarting the loop.

Just do a while (!list_empty()), and without gotos...

> +	}
> +
> +	raw_spin_unlock(&its_lock);
> +
>   	if (list_empty(&its_nodes)) {
>   		pr_warn("ITS: No ITS available, not enabling LPIs\n");
>   		return -ENXIO;
>   	}
>   
>   	err = allocate_lpi_tables();
> -	if (err)
> +	if (err) {
> +		pr_warn("ITS: Failed to initialize (%d), not enabling LPIs\n",
> +			err);
>   		return err;
> +	}
>   
>   	list_for_each_entry(its, &its_nodes, entry)
>   		has_v4 |= its->is_v4;
> @@ -3960,5 +3976,9 @@ static int __init its_init(void)
>   
>   	register_syscore_ops(&its_syscore_ops);
>   
> +	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 e04108b7c6b7..d2942efdb6d5 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -685,9 +685,6 @@ static int gic_starting_cpu(unsigned int cpu)
>   {
>   	gic_cpu_init();
>   
> -	if (gic_dist_supports_lpis())
> -		its_cpu_init();
> -
>   	return 0;
>   }
>   
> @@ -815,7 +812,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 */
> @@ -1131,10 +1128,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>   	gic_cpu_init();
>   	gic_cpu_pm_init();
>   
> -	if (gic_dist_supports_lpis()) {
> +	if (gic_dist_supports_lpis())
>   		its_probe(handle, &gic_data.rdists, gic_data.domain);
> -		its_cpu_init();
> -	}
>   
>   	return 0;
>   
> @@ -1327,6 +1322,9 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>   
>   	if (static_branch_likely(&supports_deactivate_key))
>   		gic_of_setup_kvm_info(node);
> +
> +	its_init();
> +
>   	return 0;
>   
>   out_unmap_rdist:
> @@ -1630,6 +1628,8 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
>   	if (static_branch_likely(&supports_deactivate_key))
>   		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 e0cd2baa8380..584f73585142 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -96,6 +96,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 a6fdb2910f73..f4348fa4260a 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -602,9 +602,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);
>   int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
>   
>   static inline bool gic_enable_sre(void)
> 

-- 
Julien Thierry

Re: [PATCH 01/10] irqdomain: Add interface to request an irq domain

[Richter, Robert]

Julien,

thanks for your review. See my comments below.

On 08.11.18 10:19:52, Julien Thierry wrote:
> On 07/11/18 22:03, Robert Richter wrote:
> >This patch introduces a new interface to allow irq domain
> >initialization regardless of order dependencies. This is done by
> >requesting a domain and registering a callback function that is called
> >as soon as a domain becomes available.
> >
> >A typical irq domain initialization code is the following:
> >
> >Parent initialization:
> >
> >      ...
> >      domain = msi_create_irq_domain(fwnode, info, parent);
> >      if (domain)
> >              irq_domain_update_bus_token(domain, bus_token);
> >      ...
> >
> >Child initialization:
> >
> >      ...
> >      parent = irq_find_matching_fwnode(fwnode, bus_token);
> >      if (!parent)
> >              ...
> >      create_irq_domain(parent, ...);
> 
> What is that function?

This functions is just an example, all others are part of the api.  I
will rename it for more clarity, e.g. do_some_irq_domain_setup().

> 
> >      ...
> >
> >In case the parent is not yet available, the child initialization
> >fails. Thus, the current implementation requires the parent domain
> >being initialized before the child domain. With a complex irq domain
> >hierarchy it becomes more and more difficult to grant that order as
> >irq domains are enabled in separate subsystems. Care must be taken
> >when initializing parent and child domains in the same initcall
> >level. E.g. Arm's gic-v3-its implementation might have the following
> >tree and dependencies:
> >
> >      gic-v3
> >      ├── its-node-0
> >      │   ├── pci-host-0
> >      │   ├── platform-bus
> >      │   ...
> >      ├── its-node-1
> >      │   ├── pci-host-1
> >      ...
> >
> >All domains must be initialized in top-down order of the tree.
> >
> >This patch introduces an interface that allows domain initialization
> >without any order requirements, e.g. to be able to initialize the irq
> >domains in the same initcall level. The following functions have been
> >introduced to allow the registration of a callback handler:
> >
> >      irq_domain_request_fwnode()
> >      irq_domain_request_host()
> >
> >Instead of using the irq_find_matching_fwnode() function and it's
> >variants the child code replaces them with the new functions and
> >looks e.g. like the following:
> >
> >      ...
> >      irq_domain_request_fwnode(fwnode, bus_token,
> >                              create_irq_domain, name, priv);
> >      ...
> >
> >Here, the callback function create_irq_domain() is called as soon as
> >the parent becomes available. All registered handlers are stored in a
> >list. With each update of the bus token using irq_domain_update_bus_
> >token(), the list is checked if that domain is requested by a handler
> >and if that is the case it's callback function is called and the
> >request removed from the list.
> >
> >With a late_initcall all requests from the list should already have
> >been handled, otherwise all remaining requests are removed with an
> >error reported.
> >
> >Signed-off-by: Robert Richter <rrichter@cavium.com>
> >---
> >  include/linux/irqdomain.h |  15 +++++
> >  kernel/irq/irqdomain.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 173 insertions(+)
> >
> >diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> >index 068aa46f0d55..27e83803627d 100644
> >--- a/include/linux/irqdomain.h
> >+++ b/include/linux/irqdomain.h
> >@@ -311,6 +311,21 @@ static inline struct irq_domain *irq_find_host(struct device_node *node)
> >      return d;
> >  }
> >
> >+typedef int (*irq_domain_callback_t)(struct irq_domain *, void *);
> >+int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
> >+                     enum irq_domain_bus_token bus_token,
> >+                     irq_domain_callback_t callback,
> >+                     const char *name, void *priv);
> >+
> >+static inline int irq_domain_request_host(struct device_node *node,
> >+                                     enum irq_domain_bus_token bus_token,
> >+                                     irq_domain_callback_t callback,
> >+                                     void *priv)
> >+{
> >+     return irq_domain_request_fwnode(of_node_to_fwnode(node), bus_token,
> >+                                     callback, node->full_name, priv);
> >+}
> >+
> 
> A little documentation here? Why do we need both irq_domain_request_host
> and irq_domain_request_fwnode?

irq_domain_request_fwnode() is documented in kernel/irq/irqdomain.c.
Functions are equivalent to it's irq_find_matching_*() counterparts.
*_host() is for devicetree only nodes.

> 
> Also, you introduced this for domain creation, but the callback here is
> very generic. Is it intended to allow any kind of action once a fwnode
> is available?

Yes, it is generic, see patch #5 where it is used for late device
probing. Domain creation is the main use case to setup the irq domain
hierarchy. It can be used as a replacement for irq_find_matching_*().
Any users of that functions are suitable for being used with the new
interface.

> 
> >  /**
> >   * irq_domain_add_linear() - Allocate and register a linear revmap irq_domain.
> >   * @of_node: pointer to interrupt controller's device tree node.
> >diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> >index 3366d11c3e02..9e33d873d8f6 100644
> >--- a/kernel/irq/irqdomain.c
> >+++ b/kernel/irq/irqdomain.c
> >@@ -21,6 +21,7 @@
> >  #include <linux/fs.h>
> >
> >  static LIST_HEAD(irq_domain_list);
> >+static LIST_HEAD(irq_domain_requests);
> >  static DEFINE_MUTEX(irq_domain_mutex);
> >
> >  static struct irq_domain *irq_default_domain;
> >@@ -45,6 +46,106 @@ static inline void debugfs_remove_domain_dir(struct irq_domain *d) { }
> >  const struct fwnode_operations irqchip_fwnode_ops;
> >  EXPORT_SYMBOL_GPL(irqchip_fwnode_ops);
> >
> >+struct irq_domain_request {
> >+     struct list_head                list;
> >+     struct fwnode_handle            *fwnode;
> >+     enum irq_domain_bus_token       bus_token;
> >+     irq_domain_callback_t           callback;
> >+     char                            *name;
> >+     void                            *priv;
> >+};
> >+
> >+static void irq_domain_call_handler(struct irq_domain *domain,
> >+             irq_domain_callback_t callback, const char *name, void *priv)
> >+{
> >+     int ret;
> >+
> >+     ret = callback(domain, priv);
> >+     if (ret)
> >+             pr_err("%s: Domain request handler failed: %d\n",
> >+                     name, ret);
> >+
> >+     of_node_put(irq_domain_get_of_node(domain));
> >+}
> >+
> >+static void irq_domain_free_request(struct irq_domain_request *request)
> >+{
> >+     kfree(request->name);
> >+     kfree(request);
> >+}
> >+
> >+static void irq_domain_handle_requests(struct fwnode_handle *fwnode,
> >+                        enum irq_domain_bus_token bus_token)
> >+{
> >+     struct irq_domain *domain;
> >+     struct irq_domain_request *request;
> >+
> >+     if (!fwnode)
> >+             return;
> >+redo:
> >+     domain = irq_find_matching_fwnode(fwnode, bus_token);
> >+     if (!domain)
> >+             return;
> >+
> >+     mutex_lock(&irq_domain_mutex);
> >+
> 
> Why do we need to take the mutex before checking the domain fields?
> Can't we delay it?

The list is protected by the mutex. irq_find_matching_fwnode() also
accesses the list and must use the same mutex.

> 
> >+     if ((domain->fwnode != fwnode) && (domain->bus_token != bus_token)) {
> 
> Why do we even need that check?

The domain token might have changed after irq_find_matching_fwnode()
and before mutex_lock(), we do a recheck here. Some sort of try-lock.

Note: I found the check being wrong here, it needs to be corrected to:

	if ((domain->fwnode != fwnode) || (domain->bus_token != bus_token)) {

> 
> Isn't the point of passing fwnode and bus_token to
> irq_find_matching_fwnode to find a domain with those properties?

Yes, but properties may change and also the list itself.

> 
> >+             mutex_unlock(&irq_domain_mutex);
> >+             goto redo;
> >+     }
> >+
> >+     list_for_each_entry(request, &irq_domain_requests, list) {
> 
> Shouldn't you use list_for_each_safe if you want to remove elements of
> the list inside the loop?

No, we do a complete redo again without further iterating the list. We
need to do this since the handler must be called with the mutex
unlocked (to be able to manipulate the irq domain list in the callback
and to be in non-atomic context). After we unlocked the mutex, we must
restart again as the list may have changed.

> 
> >+             if (request->fwnode != fwnode ||
> >+                 request->bus_token != bus_token)
> >+                     continue;
> >+
> >+             list_del(&request->list);
> >+             mutex_unlock(&irq_domain_mutex);
> >+
> >+             irq_domain_call_handler(domain, request->callback,
> >+                                     request->name, request->priv);
> >+             irq_domain_free_request(request);
> >+
> >+             goto redo;
> >+     }
> >+
> >+     mutex_unlock(&irq_domain_mutex);
> >+}
> >+
> >+static int __init irq_domain_drain_requests(void)
> >+{
> >+     struct irq_domain_request *request;
> >+     struct irq_domain *domain;
> >+     int ret = 0;
> >+redo:
> >+     mutex_lock(&irq_domain_mutex);
> >+
> >+     list_for_each_entry(request, &irq_domain_requests, list) {
> 
> Same remark.

Same here, the difference is that we can directly operate with the
request, no need to check the domain.

> 
> >+             list_del(&request->list);
> >+             mutex_unlock(&irq_domain_mutex);
> >+
> >+             domain = irq_find_matching_fwnode(request->fwnode,
> >+                                             request->bus_token);
> >+             if (domain) {
> >+                     irq_domain_call_handler(domain, request->callback,
> >+                                             request->name, request->priv);
> >+             } else {
> >+                     ret = -ENODEV;
> >+                     pr_err("%s-%d: Unhandled domain request\n",
> >+                             request->name, request->bus_token);
> >+             }
> >+
> >+             irq_domain_free_request(request);
> >+
> >+             goto redo;
> 
> Hmmm, are you starting a loop to break out of it at each iteration?

We have to as the list lock was released which is needed for
irq_find_matching_fwnode() and the callback handler.

> 
> Wouldn't it be much simpler to have something like the following?
> 
>        while (!list_empty(&irq_domain_requests) {
>                mutex_lock(&irq_domain_mutex);
>                request = list_first_entry_or_null(&irq_domain_requests,
>                                        struct irq_domain_request,
>                                        list);
>                if (request)
>                        list_del(&request->list);
>                mutex_unlock(&irq_domain_mutex);

At this point my implmentation has only 5 lines of code and uses one
list command less than your's. I am also not happy using list_empty()
without the lock hold (though it seems to be used that way elsewhere).

> 
>                <...>
>        }
> 
> If you really need to consider possible additions to the list while
> draining it.
> 
> Otherwise, just consider doing:

I let the callback handler run again which might change the list again
(e.g. by adding new handlers). Just using list_for_each_entry() looks
straight forward to me.

> 
>        mutex_lock(&irq_domain_mutex);
>        while (!list_empty(&irq_domain_requests)) {
>                request = list_first_entry(&irq_domain_requests,
>                                        struct irq_domain_request,
>                                        list);
>                list_del(&request->list);
>        }
>        mutex_unlock(&irq_domain_mutex);
> 
> >+     }
> >+
> >+     mutex_unlock(&irq_domain_mutex);
> >+
> >+     return ret;
> >+}
> >+late_initcall(irq_domain_drain_requests);
> 
> Overall the code for irq_domain_drain_requests and
> irq_domain_handle_requests feels very similar.
> 
> The only difference is that one filters the elements it processes based
> on an irq_domain while the other just goes through all of them.
> 
> You can probably factor that code.

Both functions serve different purposes and factoring the small set of
similarity out just adds more complexity.

Thanks,

-Robert

> 
> Cheers,
> 
> >+
> >  /**
> >   * irq_domain_alloc_fwnode - Allocate a fwnode_handle suitable for
> >   *                           identifying an irq domain
> >@@ -293,6 +394,8 @@ void irq_domain_update_bus_token(struct irq_domain *domain,
> >      debugfs_add_domain_dir(domain);
> >
> >      mutex_unlock(&irq_domain_mutex);
> >+
> >+     irq_domain_handle_requests(domain->fwnode, bus_token);
> >  }
> >
> >  /**
> >@@ -417,6 +520,61 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
> >  EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);
> >
> >  /**
> >+ * irq_domain_request_fwnode() - Requests a domain for a given fwspec
> >+ * @fwspec: FW specifier for an interrupt
> >+ * @bus_token: domain-specific data
> >+ * @callback: function to be called once domain becomes available
> >+ * @name: name to be used for fwnode
> >+ * @priv: private data to be passed to callback
> >+ *
> >+ * The callback function is called as soon as the domain is available.
> >+ */
> >+int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
> >+                     enum irq_domain_bus_token bus_token,
> >+                     irq_domain_callback_t callback,
> >+                     const char *name, void *priv)
> >+{
> >+     struct irq_domain *parent;
> >+     struct irq_domain_request *request;
> >+
> >+     if (!fwnode || bus_token == DOMAIN_BUS_ANY || !callback || !name)
> >+             return -EINVAL;
> >+
> >+     parent = irq_find_matching_fwnode(fwnode, bus_token);
> >+     if (parent) {
> >+             irq_domain_call_handler(parent, callback, name, priv);
> >+             return 0;
> >+     }
> >+
> >+     request = kzalloc(sizeof(*request), GFP_KERNEL);
> >+     if (!request)
> >+             return -ENOMEM;
> >+
> >+     request->fwnode = fwnode;
> >+     request->bus_token = bus_token;
> >+     request->callback = callback;
> >+     request->name = kstrdup(name, GFP_KERNEL);
> >+     request->priv = priv;
> >+     INIT_LIST_HEAD(&request->list);
> >+
> >+     if (!request->name) {
> >+             kfree(request);
> >+             return -ENOMEM;
> >+     }
> >+
> >+     of_node_get(to_of_node(fwnode));
> >+
> >+     mutex_lock(&irq_domain_mutex);
> >+     list_add_tail(&request->list, &irq_domain_requests);
> >+     mutex_unlock(&irq_domain_mutex);
> >+
> >+     /* recheck in case list changed */
> >+     irq_domain_handle_requests(fwnode, bus_token);
> >+
> >+     return 0;
> >+}
> >+
> >+/**
> >   * irq_domain_check_msi_remap - Check whether all MSI irq domains implement
> >   * IRQ remapping
> >   *
> >
> 
> --
> Julien Thierry

Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization

[Richter, Robert]

On 08.11.18 11:25:24, Julien Thierry wrote:
> On 07/11/18 22:03, Robert Richter wrote:

> >-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;
> >@@ -3384,7 +3385,8 @@ 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_handle,
> >+                                     &its_domain_ops, its);
> 
> Separate change?
> 
> >      if (!inner_domain) {
> >              kfree(info);
> >              return -ENOMEM;
> >@@ -3441,8 +3443,7 @@ static int its_init_vpe_domain(void)
> >      return 0;
> >  }
> >
> >-static int __init its_compute_its_list_map(struct resource *res,
> >-                                        void __iomem *its_base)
> >+static int __init its_compute_its_list_map(struct its_node *its)
> >  {
> >      int its_number;
> >      u32 ctlr;
> >@@ -3456,15 +3457,15 @@ static int __init its_compute_its_list_map(struct resource *res,
> >      its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
> >      if (its_number >= GICv4_ITS_LIST_MAX) {
> >              pr_err("ITS@%pa: No ITSList entry available!\n",
> >-                    &res->start);
> >+                    &its->phys_base);
> >              return -EINVAL;
> >      }
> >
> >-     ctlr = readl_relaxed(its_base + GITS_CTLR);
> >+     ctlr = readl_relaxed(its->base + GITS_CTLR);
> >      ctlr &= ~GITS_CTLR_ITS_NUMBER;
> >      ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> >-     writel_relaxed(ctlr, its_base + GITS_CTLR);
> >-     ctlr = readl_relaxed(its_base + GITS_CTLR);
> >+     writel_relaxed(ctlr, its->base + GITS_CTLR);
> >+     ctlr = readl_relaxed(its->base + GITS_CTLR);
> 
> This (removal of its_base parameter) also feel like a separate change.

In a separate change the motivation of the change would not be
obvious. While the change of the variable itself is trivial from the
perspective of review and testing, I decided to keep it in the context
of the overall change of this patch.

> 
> Also, I would define a local variable its_base to avoid dereferencing
> its every time in order to get the base address.

Hmm, there is not much difference in reading the code then, while the
use of a local variable just adds more code without benefit. The
compiler does not care as the value is probably stored in a register
anyway. There are also other struct members, should all of them being
mirrored in a local variable?

> 
> >      if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
> >              its_number = ctlr & GITS_CTLR_ITS_NUMBER;
> >              its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> >@@ -3472,83 +3473,110 @@ static int __init its_compute_its_list_map(struct resource *res,
> >
> >      if (test_and_set_bit(its_number, &its_list_map)) {
> >              pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> >-                    &res->start, its_number);
> >+                    &its->phys_base, its_number);
> >              return -EINVAL;
> >      }
> >
> >      return its_number;
> >  }
> >
> >+static void its_free(struct its_node *its)
> >+{
> >+     raw_spin_lock(&its_lock);
> >+     list_del(&its->entry);
> >+     raw_spin_unlock(&its_lock);
> >+
> >+     kfree(its);
> >+}
> >+
> >+static int __init its_init_one(struct its_node *its);
> 
> You might as well define its_init_one here, no?

This is an intermediate definition that will be removed in a later
patch. Moving the whole code here would make the change less readable.

> 
> >+
> >  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 = handle;
> >+     its->phys_base = res->start;
> >+     its->phys_size = resource_size(res);
> >+     its->numa_node = numa_node;
> >+
> >+     raw_spin_lock(&its_lock);
> >+     list_add_tail(&its->entry, &its_nodes);
> >+     raw_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, ctlr;
> >      u64 baser, tmp, typer;
> >      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);
> >+             pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
> >              goto out_unmap;
> >      }
> >
> >-     pr_info("ITS %pR\n", res);
> >-
> >-     its = kzalloc(sizeof(*its), GFP_KERNEL);
> >-     if (!its) {
> >-             err = -ENOMEM;
> >-             goto out_unmap;
> >-     }
> >-
> >-     raw_spin_lock_init(&its->lock);
> >-     INIT_LIST_HEAD(&its->entry);
> >-     INIT_LIST_HEAD(&its->its_device_list);
> >      typer = gic_read_typer(its_base + GITS_TYPER);
> >      its->base = its_base;
> >-     its->phys_base = res->start;
> >      its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
> >      its->device_ids = GITS_TYPER_DEVBITS(typer);
> >      its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
> >      if (its->is_v4) {
> >              if (!(typer & GITS_TYPER_VMOVP)) {
> >-                     err = its_compute_its_list_map(res, its_base);
> >+                     err = its_compute_its_list_map(its);
> >                      if (err < 0)
> >-                             goto out_free_its;
> >+                             goto out_unmap;
> >
> >                      its->list_nr = err;
> >
> >                      pr_info("ITS@%pa: Using ITS number %d\n",
> >-                             &res->start, err);
> >+                             &its->phys_base, err);
> >              } else {
> >-                     pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> >+                     pr_info("ITS@%pa: Single VMOVP capable\n",
> >+                             &its->phys_base);
> >              }
> >      }
> >
> >-     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;
> >-     its->fwnode_handle = handle;
> >      its->get_msi_base = its_irq_get_msi_base;
> >      its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
> >
> >@@ -3597,13 +3625,11 @@ static int __init its_probe_one(struct resource *res,
> >      if (GITS_TYPER_HCC(typer))
> >              its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
> >
> >-     err = its_init_domain(handle, its);
> >+     err = its_init_domain(its);
> 
> I'm not sure what is the logic for "this goes in probe, this goes in init?".

It is fairly simple, gic-its register access is done in init.
Everything that is detected during devicetree or ACPI device discovery
is done in the probe function that collects all data in struct
its_node.

> 
> >      if (err)
> >              goto out_free_tables;
> >
> >-     raw_spin_lock(&its_lock);
> >-     list_add_tail(&its->entry, &its_nodes);
> >-     raw_spin_unlock(&its_lock);
> >+     pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
> >
> >      return 0;

Thanks,

-Robert

Re: [PATCH 08/10] irqchip/gic-v3-its: Decouple its initialization from gic

[Richter, Robert]

On 08.11.18 11:26:02, Julien Thierry wrote:
> On 07/11/18 22:03, Robert Richter wrote:

> >-static int __init its_init(void)
> >+int __init its_init(void)
> >  {
> >      struct its_node *its;
> >      bool has_v4 = false;
> >      int err;
> >
> >+     if (list_empty(&its_probed))
> >+             return 0;
> >+
> >+     raw_spin_lock(&its_lock);
> >+redo:
> >+     list_for_each_entry(its, &its_probed, entry) {
> >+             list_del_init(&its->entry);
> >+
> >+             raw_spin_unlock(&its_lock);
> >+
> >+             /* Needs to be called in non-atomic context */
> >+             err = its_init_one(its);
> >+             if (err)
> >+                     its_free(its);
> >+
> >+             raw_spin_lock(&its_lock);
> >+
> >+             if (!err)
> >+                     list_add_tail(&its->entry, &its_nodes);
> >+
> >+             goto redo;
> 
> Again, you're starting a loop only to work on the first element and then
> restarting the loop.
> 
> Just do a while (!list_empty()), and without gotos...

When writing the code I have looked into an alternative to
list_for_each_entry() and did not find a better way that works with
proper locking. list_first_entry() requires the list being non-empty.
If you set the lock after list_empty() it is not granted the list is
actual empty. See also my other comment in an earlier mail.

-Robert

> 
> >+     }
> >+
> >+     raw_spin_unlock(&its_lock);
> >+
> >      if (list_empty(&its_nodes)) {
> >              pr_warn("ITS: No ITS available, not enabling LPIs\n");
> >              return -ENXIO;
> >      }
> >
> >      err = allocate_lpi_tables();
> >-     if (err)
> >+     if (err) {
> >+             pr_warn("ITS: Failed to initialize (%d), not enabling LPIs\n",
> >+                     err);
> >              return err;
> >+     }

Re: [PATCH 01/10] irqdomain: Add interface to request an irq domain

[Julien Thierry]

Hi Robert,

On 08/11/18 15:05, Richter, Robert wrote:
> Julien,
> 
> thanks for your review. See my comments below.
> 
> On 08.11.18 10:19:52, Julien Thierry wrote:
>> On 07/11/18 22:03, Robert Richter wrote:
>>> This patch introduces a new interface to allow irq domain
>>> initialization regardless of order dependencies. This is done by
>>> requesting a domain and registering a callback function that is called
>>> as soon as a domain becomes available.
>>>
>>> A typical irq domain initialization code is the following:
>>>
>>> Parent initialization:
>>>
>>>       ...
>>>       domain = msi_create_irq_domain(fwnode, info, parent);
>>>       if (domain)
>>>               irq_domain_update_bus_token(domain, bus_token);
>>>       ...
>>>
>>> Child initialization:
>>>
>>>       ...
>>>       parent = irq_find_matching_fwnode(fwnode, bus_token);
>>>       if (!parent)
>>>               ...
>>>       create_irq_domain(parent, ...);
>>
>> What is that function?
> 
> This functions is just an example, all others are part of the api.  I
> will rename it for more clarity, e.g. do_some_irq_domain_setup().
> 
>>
>>>       ...
>>>
>>> In case the parent is not yet available, the child initialization
>>> fails. Thus, the current implementation requires the parent domain
>>> being initialized before the child domain. With a complex irq domain
>>> hierarchy it becomes more and more difficult to grant that order as
>>> irq domains are enabled in separate subsystems. Care must be taken
>>> when initializing parent and child domains in the same initcall
>>> level. E.g. Arm's gic-v3-its implementation might have the following
>>> tree and dependencies:
>>>
>>>       gic-v3
>>>       ├── its-node-0
>>>       │   ├── pci-host-0
>>>       │   ├── platform-bus
>>>       │   ...
>>>       ├── its-node-1
>>>       │   ├── pci-host-1
>>>       ...
>>>
>>> All domains must be initialized in top-down order of the tree.
>>>
>>> This patch introduces an interface that allows domain initialization
>>> without any order requirements, e.g. to be able to initialize the irq
>>> domains in the same initcall level. The following functions have been
>>> introduced to allow the registration of a callback handler:
>>>
>>>       irq_domain_request_fwnode()
>>>       irq_domain_request_host()
>>>
>>> Instead of using the irq_find_matching_fwnode() function and it's
>>> variants the child code replaces them with the new functions and
>>> looks e.g. like the following:
>>>
>>>       ...
>>>       irq_domain_request_fwnode(fwnode, bus_token,
>>>                               create_irq_domain, name, priv);
>>>       ...
>>>
>>> Here, the callback function create_irq_domain() is called as soon as
>>> the parent becomes available. All registered handlers are stored in a
>>> list. With each update of the bus token using irq_domain_update_bus_
>>> token(), the list is checked if that domain is requested by a handler
>>> and if that is the case it's callback function is called and the
>>> request removed from the list.
>>>
>>> With a late_initcall all requests from the list should already have
>>> been handled, otherwise all remaining requests are removed with an
>>> error reported.
>>>
>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>> ---
>>>   include/linux/irqdomain.h |  15 +++++
>>>   kernel/irq/irqdomain.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 173 insertions(+)
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index 068aa46f0d55..27e83803627d 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -311,6 +311,21 @@ static inline struct irq_domain *irq_find_host(struct device_node *node)
>>>       return d;
>>>   }
>>>
>>> +typedef int (*irq_domain_callback_t)(struct irq_domain *, void *);
>>> +int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
>>> +                     enum irq_domain_bus_token bus_token,
>>> +                     irq_domain_callback_t callback,
>>> +                     const char *name, void *priv);
>>> +
>>> +static inline int irq_domain_request_host(struct device_node *node,
>>> +                                     enum irq_domain_bus_token bus_token,
>>> +                                     irq_domain_callback_t callback,
>>> +                                     void *priv)
>>> +{
>>> +     return irq_domain_request_fwnode(of_node_to_fwnode(node), bus_token,
>>> +                                     callback, node->full_name, priv);
>>> +}
>>> +
>>
>> A little documentation here? Why do we need both irq_domain_request_host
>> and irq_domain_request_fwnode?
> 
> irq_domain_request_fwnode() is documented in kernel/irq/irqdomain.c.
> Functions are equivalent to it's irq_find_matching_*() counterparts.
> *_host() is for devicetree only nodes.
> 
>>
>> Also, you introduced this for domain creation, but the callback here is
>> very generic. Is it intended to allow any kind of action once a fwnode
>> is available?
> 
> Yes, it is generic, see patch #5 where it is used for late device
> probing. Domain creation is the main use case to setup the irq domain
> hierarchy. It can be used as a replacement for irq_find_matching_*().
> Any users of that functions are suitable for being used with the new
> interface.
> 
>>
>>>   /**
>>>    * irq_domain_add_linear() - Allocate and register a linear revmap irq_domain.
>>>    * @of_node: pointer to interrupt controller's device tree node.
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index 3366d11c3e02..9e33d873d8f6 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -21,6 +21,7 @@
>>>   #include <linux/fs.h>
>>>
>>>   static LIST_HEAD(irq_domain_list);
>>> +static LIST_HEAD(irq_domain_requests);
>>>   static DEFINE_MUTEX(irq_domain_mutex);
>>>
>>>   static struct irq_domain *irq_default_domain;
>>> @@ -45,6 +46,106 @@ static inline void debugfs_remove_domain_dir(struct irq_domain *d) { }
>>>   const struct fwnode_operations irqchip_fwnode_ops;
>>>   EXPORT_SYMBOL_GPL(irqchip_fwnode_ops);
>>>
>>> +struct irq_domain_request {
>>> +     struct list_head                list;
>>> +     struct fwnode_handle            *fwnode;
>>> +     enum irq_domain_bus_token       bus_token;
>>> +     irq_domain_callback_t           callback;
>>> +     char                            *name;
>>> +     void                            *priv;
>>> +};
>>> +
>>> +static void irq_domain_call_handler(struct irq_domain *domain,
>>> +             irq_domain_callback_t callback, const char *name, void *priv)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = callback(domain, priv);
>>> +     if (ret)
>>> +             pr_err("%s: Domain request handler failed: %d\n",
>>> +                     name, ret);
>>> +
>>> +     of_node_put(irq_domain_get_of_node(domain));
>>> +}
>>> +
>>> +static void irq_domain_free_request(struct irq_domain_request *request)
>>> +{
>>> +     kfree(request->name);
>>> +     kfree(request);
>>> +}
>>> +
>>> +static void irq_domain_handle_requests(struct fwnode_handle *fwnode,
>>> +                        enum irq_domain_bus_token bus_token)
>>> +{
>>> +     struct irq_domain *domain;
>>> +     struct irq_domain_request *request;
>>> +
>>> +     if (!fwnode)
>>> +             return;
>>> +redo:
>>> +     domain = irq_find_matching_fwnode(fwnode, bus_token);
>>> +     if (!domain)
>>> +             return;
>>> +
>>> +     mutex_lock(&irq_domain_mutex);
>>> +
>>
>> Why do we need to take the mutex before checking the domain fields?
>> Can't we delay it?
> 
> The list is protected by the mutex. irq_find_matching_fwnode() also
> accesses the list and must use the same mutex.
> 
>>
>>> +     if ((domain->fwnode != fwnode) && (domain->bus_token != bus_token)) {
>>
>> Why do we even need that check?
> 
> The domain token might have changed after irq_find_matching_fwnode()
> and before mutex_lock(), we do a recheck here. Some sort of try-lock.
> 
> Note: I found the check being wrong here, it needs to be corrected to:
> 
> 	if ((domain->fwnode != fwnode) || (domain->bus_token != bus_token)) {
> 
>>
>> Isn't the point of passing fwnode and bus_token to
>> irq_find_matching_fwnode to find a domain with those properties?
> 
> Yes, but properties may change and also the list itself.

Hmmm, that check is unrelated to the list, you're just checking the 
domain you just retrieved.

Can you clarify which properties may change? If the irq_domain fields 
can change (I guess if e.g. another cpu modifies the domain with 
irq_domain_update_bus_token), they could still be changed between the 
moment you retrieved the domain and the moment you call the handler. Why 
is that not an issue if we worried about properties changing before 
removing the request from the list?

Maybe some comment would help here.

> 
>>
>>> +             mutex_unlock(&irq_domain_mutex);
>>> +             goto redo;
>>> +     }
>>> +
>>> +     list_for_each_entry(request, &irq_domain_requests, list) {
>>
>> Shouldn't you use list_for_each_safe if you want to remove elements of
>> the list inside the loop?
> 
> No, we do a complete redo again without further iterating the list. We
> need to do this since the handler must be called with the mutex
> unlocked (to be able to manipulate the irq domain list in the callback
> and to be in non-atomic context). After we unlocked the mutex, we must
> restart again as the list may have changed.
> 
>>
>>> +             if (request->fwnode != fwnode ||
>>> +                 request->bus_token != bus_token)
>>> +                     continue;
>>> +
>>> +             list_del(&request->list);
>>> +             mutex_unlock(&irq_domain_mutex);
>>> +
>>> +             irq_domain_call_handler(domain, request->callback,
>>> +                                     request->name, request->priv);
>>> +             irq_domain_free_request(request);
>>> +
>>> +             goto redo;
>>> +     }
>>> +
>>> +     mutex_unlock(&irq_domain_mutex);
>>> +}
>>> +
>>> +static int __init irq_domain_drain_requests(void)
>>> +{
>>> +     struct irq_domain_request *request;
>>> +     struct irq_domain *domain;
>>> +     int ret = 0;
>>> +redo:
>>> +     mutex_lock(&irq_domain_mutex);
>>> +
>>> +     list_for_each_entry(request, &irq_domain_requests, list) {
>>
>> Same remark.
> 
> Same here, the difference is that we can directly operate with the
> request, no need to check the domain.
> 
>>
>>> +             list_del(&request->list);
>>> +             mutex_unlock(&irq_domain_mutex);
>>> +
>>> +             domain = irq_find_matching_fwnode(request->fwnode,
>>> +                                             request->bus_token);
>>> +             if (domain) {
>>> +                     irq_domain_call_handler(domain, request->callback,
>>> +                                             request->name, request->priv);
>>> +             } else {
>>> +                     ret = -ENODEV;
>>> +                     pr_err("%s-%d: Unhandled domain request\n",
>>> +                             request->name, request->bus_token);
>>> +             }
>>> +
>>> +             irq_domain_free_request(request);
>>> +
>>> +             goto redo;
>>
>> Hmmm, are you starting a loop to break out of it at each iteration?
> 
> We have to as the list lock was released which is needed for
> irq_find_matching_fwnode() and the callback handler.
> 
>>
>> Wouldn't it be much simpler to have something like the following?
>>
>>         while (!list_empty(&irq_domain_requests) {
>>                 mutex_lock(&irq_domain_mutex);
>>                 request = list_first_entry_or_null(&irq_domain_requests,
>>                                         struct irq_domain_request,
>>                                         list);
>>                 if (request)
>>                         list_del(&request->list);
>>                 mutex_unlock(&irq_domain_mutex);
> 
> At this point my implmentation has only 5 lines of code and uses one
> list command less than your's. I am also not happy using list_empty()
> without the lock hold (though it seems to be used that way elsewhere).

I'm not sure why the number of list commands is relevant. 
"list_for_each_entry" just already combines a bunch of operations, but 
caries a completely different meaning (and probably expands code in the 
function that is never used).

For irq_domain_drain, you take the first element as long as there is one 
and do stuff with it, so having something like:

	mutex_lock();
	while (!list_empty()) {
		request = list_first_entry();
		list_del(request->list);

		// unlock and relock as you please
		// and do stuff
	}
	mutex_unlock();

Or if you are really concerned about the number of list commands:

	mutex_lock();
	while ((request = list_first_entry_or_null()) != NULL) {
		list_del(request->list);

		// unlock and relock as you please
		// and do stuff
	}
	mutex_unlock();

To me this makes it much easier to get what you are trying to do and I 
don't think it is less efficient that your version (but I could be wrong).


For irq_domain_handle_request, I think I agree that it is actually 
different from irq_domain_drain, but it is hard to see in my opinion 
because of how the functions are structured. So I would suggest 
something like:

	while ((domain = irq_domain_find(...)) != NULL) {
		struct irq_domain_request *found = NULL;

		mutex_lock();

		// Do the check on domain if it is needed

		list_for_each_entry(request, ..., list) {
			if (request->fwnode != fwnode ||
			    request->bus_token != bus_token)
				continue;

			list_del(request->list);
			found = request;
			break;
		}
		mutex_unlock();

		if (found) {
			// call handler, etc...
		}
	}


Personally, I find those flow much easier to follow than when using 
gotos to break out of loops.

This is just my suggestion so feel free to disregard if the maintainers 
agree with your current approach.

Thanks,

Julien

> 
>>
>>                 <...>
>>         }
>>
>> If you really need to consider possible additions to the list while
>> draining it.
>>
>> Otherwise, just consider doing:
> 
> I let the callback handler run again which might change the list again
> (e.g. by adding new handlers). Just using list_for_each_entry() looks
> straight forward to me.
> 
>>
>>         mutex_lock(&irq_domain_mutex);
>>         while (!list_empty(&irq_domain_requests)) {
>>                 request = list_first_entry(&irq_domain_requests,
>>                                         struct irq_domain_request,
>>                                         list);
>>                 list_del(&request->list);
>>         }
>>         mutex_unlock(&irq_domain_mutex);
>>
>>> +     }
>>> +
>>> +     mutex_unlock(&irq_domain_mutex);
>>> +
>>> +     return ret;
>>> +}
>>> +late_initcall(irq_domain_drain_requests);
>>
>> Overall the code for irq_domain_drain_requests and
>> irq_domain_handle_requests feels very similar.
>>
>> The only difference is that one filters the elements it processes based
>> on an irq_domain while the other just goes through all of them.
>>
>> You can probably factor that code.
> 
> Both functions serve different purposes and factoring the small set of
> similarity out just adds more complexity.
> 
> Thanks,
> 
> -Robert
> 
>>
>> Cheers,
>>
>>> +
>>>   /**
>>>    * irq_domain_alloc_fwnode - Allocate a fwnode_handle suitable for
>>>    *                           identifying an irq domain
>>> @@ -293,6 +394,8 @@ void irq_domain_update_bus_token(struct irq_domain *domain,
>>>       debugfs_add_domain_dir(domain);
>>>
>>>       mutex_unlock(&irq_domain_mutex);
>>> +
>>> +     irq_domain_handle_requests(domain->fwnode, bus_token);
>>>   }
>>>
>>>   /**
>>> @@ -417,6 +520,61 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
>>>   EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);
>>>
>>>   /**
>>> + * irq_domain_request_fwnode() - Requests a domain for a given fwspec
>>> + * @fwspec: FW specifier for an interrupt
>>> + * @bus_token: domain-specific data
>>> + * @callback: function to be called once domain becomes available
>>> + * @name: name to be used for fwnode
>>> + * @priv: private data to be passed to callback
>>> + *
>>> + * The callback function is called as soon as the domain is available.
>>> + */
>>> +int irq_domain_request_fwnode(struct fwnode_handle *fwnode,
>>> +                     enum irq_domain_bus_token bus_token,
>>> +                     irq_domain_callback_t callback,
>>> +                     const char *name, void *priv)
>>> +{
>>> +     struct irq_domain *parent;
>>> +     struct irq_domain_request *request;
>>> +
>>> +     if (!fwnode || bus_token == DOMAIN_BUS_ANY || !callback || !name)
>>> +             return -EINVAL;
>>> +
>>> +     parent = irq_find_matching_fwnode(fwnode, bus_token);
>>> +     if (parent) {
>>> +             irq_domain_call_handler(parent, callback, name, priv);
>>> +             return 0;
>>> +     }
>>> +
>>> +     request = kzalloc(sizeof(*request), GFP_KERNEL);
>>> +     if (!request)
>>> +             return -ENOMEM;
>>> +
>>> +     request->fwnode = fwnode;
>>> +     request->bus_token = bus_token;
>>> +     request->callback = callback;
>>> +     request->name = kstrdup(name, GFP_KERNEL);
>>> +     request->priv = priv;
>>> +     INIT_LIST_HEAD(&request->list);
>>> +
>>> +     if (!request->name) {
>>> +             kfree(request);
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     of_node_get(to_of_node(fwnode));
>>> +
>>> +     mutex_lock(&irq_domain_mutex);
>>> +     list_add_tail(&request->list, &irq_domain_requests);
>>> +     mutex_unlock(&irq_domain_mutex);
>>> +
>>> +     /* recheck in case list changed */
>>> +     irq_domain_handle_requests(fwnode, bus_token);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/**
>>>    * irq_domain_check_msi_remap - Check whether all MSI irq domains implement
>>>    * IRQ remapping
>>>    *
>>>
>>
>> --
>> Julien Thierry

-- 
Julien Thierry

Re: [PATCH 00/10] irqdomain, gic-v3-its: Implement late irq domain initialization

[John Garry]

On 07/11/2018 22:03, Robert Richter wrote:
> This patch series is a rework of the gic-v3-its initialization. It is
> in preparation of a further series that uses CMA memory allocation for
> ITS tables. For this the CMA framework must be available and thus ITS
> needs to be initialized after the arch_initcalls. This requires two
> major reworks.
>
> First we must remove all irq domain init order dependencies (patches
> #1-#5), second the ITS initialization must be split into an early
> probe and a later init part (patches #6-#10).
>
> Patch #1 introduces a new interface to request an irq domain, see the
> patch description for details. In patches #2-#5 all ITS related irq
> domains are converted to use the new interface. There are no order
> dependencies for parent and client irq domain initialization anymore
> which allows us to initialize all ITS irq domains in the same initcall
> (moving to the later subsys_initcall). Note that I do not have fsl
> hardware available, the code should work, but is only carefully
> reviewed and compile tested, please test on that hardware.
>
> The remaining patches #6-#10 are an incremental rework of the ITS
> initialization, basically splitting probe and init (patch #7) and
> separating it from gic-v3 code (patch #8). Patches #9 and #10 change
> initcall levels for various drivers.
>
> Patches have been tested with Cavium ThunderX and ThunderX2. I have an
> implementation of CMA ITS table allocation on top of this series
> available, I will send out patches for this in a couple of days.

Hi,

For this follow-on patchset, will it conflict with this:
https://lkml.org/lkml/2017/6/25/75

We were planning on reposting with some results.

Thanks,
John

>
> Robert Richter (10):
>   irqdomain: Add interface to request an irq domain
>   irqchip/gic-v3-its-platform-msi: Remove domain init order dependencies
>   irqchip/irq-gic-v3-its-pci-msi: Remove domain init order dependencies
>   irqchip/irq-gic-v3-its-fsl-mc-msi: Remove domain init order
>     dependencies
>   fsl-mc/dprc-driver: Remove domain init order dependencies
>   irqchip/gic-v3-its: Initialize its nodes in probe order
>   irqchip/gic-v3-its: Split probing from its node initialization
>   irqchip/gic-v3-its: Decouple its initialization from gic
>   irqchip/gic-v3-its: Initialize its nodes later
>   irqchip/gic-v3-its: Initialize MSIs with subsys_initcalls
>
>  drivers/bus/fsl-mc/dprc-driver.c              |  41 +++++++
>  drivers/bus/fsl-mc/fsl-mc-msi.c               |   6 +-
>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c   |  49 +++++---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c      |  68 ++++++-----
>  drivers/irqchip/irq-gic-v3-its-platform-msi.c |  56 ++++++---
>  drivers/irqchip/irq-gic-v3-its.c              | 160 +++++++++++++++++---------
>  drivers/irqchip/irq-gic-v3.c                  |  12 +-
>  include/linux/cpuhotplug.h                    |   1 +
>  include/linux/irqchip/arm-gic-v3.h            |   5 +-
>  include/linux/irqdomain.h                     |  15 +++
>  kernel/irq/irqdomain.c                        | 158 +++++++++++++++++++++++++
>  11 files changed, 441 insertions(+), 130 deletions(-)
>

Re: [PATCH 00/10] irqdomain, gic-v3-its: Implement late irq domain initialization

[Richter, Robert]

On 09.11.18 17:19:54, John Garry wrote:
> On 07/11/2018 22:03, Robert Richter wrote:

> >Patches have been tested with Cavium ThunderX and ThunderX2. I have an
> >implementation of CMA ITS table allocation on top of this series
> >available, I will send out patches for this in a couple of days.

> For this follow-on patchset, will it conflict with this:
> https://lkml.org/lkml/2017/6/25/75
> 
> We were planning on reposting with some results.

Both could be implemented, but of course, it changes the same code and
there will be probably conflicts.

-Robert

Re: [PATCH 01/10] irqdomain: Add interface to request an irq domain

[Richter, Robert]

Julien,

On 09.11.18 09:05:11, Julien Thierry wrote:
> On 08/11/18 15:05, Richter, Robert wrote:

> >>>+static void irq_domain_handle_requests(struct fwnode_handle *fwnode,
> >>>+                        enum irq_domain_bus_token bus_token)
> >>>+{
> >>>+     struct irq_domain *domain;
> >>>+     struct irq_domain_request *request;
> >>>+
> >>>+     if (!fwnode)
> >>>+             return;
> >>>+redo:
> >>>+     domain = irq_find_matching_fwnode(fwnode, bus_token);
> >>>+     if (!domain)
> >>>+             return;
> >>>+
> >>>+     mutex_lock(&irq_domain_mutex);
> >>>+
> >>
> >>Why do we need to take the mutex before checking the domain fields?
> >>Can't we delay it?
> >
> >The list is protected by the mutex. irq_find_matching_fwnode() also
> >accesses the list and must use the same mutex.
> >
> >>
> >>>+     if ((domain->fwnode != fwnode) && (domain->bus_token != bus_token)) {
> >>
> >>Why do we even need that check?
> >
> >The domain token might have changed after irq_find_matching_fwnode()
> >and before mutex_lock(), we do a recheck here. Some sort of try-lock.
> >
> >Note: I found the check being wrong here, it needs to be corrected to:
> >
> >      if ((domain->fwnode != fwnode) || (domain->bus_token != bus_token)) {
> >
> >>
> >>Isn't the point of passing fwnode and bus_token to
> >>irq_find_matching_fwnode to find a domain with those properties?
> >
> >Yes, but properties may change and also the list itself.
> 
> Hmmm, that check is unrelated to the list, you're just checking the
> domain you just retrieved.
> 
> Can you clarify which properties may change? If the irq_domain fields
> can change (I guess if e.g. another cpu modifies the domain with
> irq_domain_update_bus_token), they could still be changed between the
> moment you retrieved the domain and the moment you call the handler. Why
> is that not an issue if we worried about properties changing before
> removing the request from the list?
> 
> Maybe some comment would help here.

The check makes sure we can use the irq_domain_requests list for the
serialization of irq domain updates. Suppose the following:

Thread1         Thread2

~~~~~~~~~~~~~~~~~~~~~~~~~       mutex
fwnode
token1
handler1                        request_fwnode()
~~~~~~~~~~~~~~~~~~~~~~~~~
domain1
fwnode
token1                          find_fwnode()
~~~~~~~~~~~~~~~~~~~~~~~~~
                domain1
                fwnode
                token1          find_fwnode()
~~~~~~~~~~~~~~~~~~~~~~~~~
                domain1
                fwnode
                token1
                handler1        call_handler()
~~~~~~~~~~~~~~~~~~~~~~~~~
                domain1
                fwnode
                token2          update_token()
~~~~~~~~~~~~~~~~~~~~~~~~~
                domain2
                fwnode
                token1          update_token()
~~~~~~~~~~~~~~~~~~~~~~~~~
                fwnode
                token1
                handler1        request_fwnode(), reschedule request
~~~~~~~~~~~~~~~~~~~~~~~~~
domain1                         <---- called with wrong domain, should be domain2
fwnode
token1
handler1                        call_handler()
~~~~~~~~~~~~~~~~~~~~~~~~~

The check handles a corner case and as such the conditions for
triggering it are rare and might look a bit constructed, but it *can*
happen. So see the check more like an assertion in the code that does
not hurt much. How about the following comment:?

	/*
	 * For serialization of irq domain updates make sure to handle
	 * (and remove) the request only if the domain still matches
	 * the request.
	 */

> 
> >
> >>
> >>>+             mutex_unlock(&irq_domain_mutex);
> >>>+             goto redo;
> >>>+     }
> >>>+
> >>>+     list_for_each_entry(request, &irq_domain_requests, list) {
> >>
> >>Shouldn't you use list_for_each_safe if you want to remove elements of
> >>the list inside the loop?
> >
> >No, we do a complete redo again without further iterating the list. We
> >need to do this since the handler must be called with the mutex
> >unlocked (to be able to manipulate the irq domain list in the callback
> >and to be in non-atomic context). After we unlocked the mutex, we must
> >restart again as the list may have changed.
> >
> >>
> >>>+             if (request->fwnode != fwnode ||
> >>>+                 request->bus_token != bus_token)
> >>>+                     continue;
> >>>+
> >>>+             list_del(&request->list);
> >>>+             mutex_unlock(&irq_domain_mutex);
> >>>+
> >>>+             irq_domain_call_handler(domain, request->callback,
> >>>+                                     request->name, request->priv);
> >>>+             irq_domain_free_request(request);
> >>>+
> >>>+             goto redo;
> >>>+     }
> >>>+
> >>>+     mutex_unlock(&irq_domain_mutex);
> >>>+}
> >>>+
> >>>+static int __init irq_domain_drain_requests(void)
> >>>+{
> >>>+     struct irq_domain_request *request;
> >>>+     struct irq_domain *domain;
> >>>+     int ret = 0;
> >>>+redo:
> >>>+     mutex_lock(&irq_domain_mutex);
> >>>+
> >>>+     list_for_each_entry(request, &irq_domain_requests, list) {
> >>
> >>Same remark.
> >
> >Same here, the difference is that we can directly operate with the
> >request, no need to check the domain.
> >
> >>
> >>>+             list_del(&request->list);
> >>>+             mutex_unlock(&irq_domain_mutex);
> >>>+
> >>>+             domain = irq_find_matching_fwnode(request->fwnode,
> >>>+                                             request->bus_token);
> >>>+             if (domain) {
> >>>+                     irq_domain_call_handler(domain, request->callback,
> >>>+                                             request->name, request->priv);
> >>>+             } else {
> >>>+                     ret = -ENODEV;
> >>>+                     pr_err("%s-%d: Unhandled domain request\n",
> >>>+                             request->name, request->bus_token);
> >>>+             }
> >>>+
> >>>+             irq_domain_free_request(request);
> >>>+
> >>>+             goto redo;
> >>
> >>Hmmm, are you starting a loop to break out of it at each iteration?
> >
> >We have to as the list lock was released which is needed for
> >irq_find_matching_fwnode() and the callback handler.
> >
> >>
> >>Wouldn't it be much simpler to have something like the following?
> >>
> >>        while (!list_empty(&irq_domain_requests) {
> >>                mutex_lock(&irq_domain_mutex);
> >>                request = list_first_entry_or_null(&irq_domain_requests,
> >>                                        struct irq_domain_request,
> >>                                        list);
> >>                if (request)
> >>                        list_del(&request->list);
> >>                mutex_unlock(&irq_domain_mutex);
> >
> >At this point my implmentation has only 5 lines of code and uses one
> >list command less than your's. I am also not happy using list_empty()
> >without the lock hold (though it seems to be used that way elsewhere).
> 
> I'm not sure why the number of list commands is relevant.

You said "simpler".

> "list_for_each_entry" just already combines a bunch of operations, but
> caries a completely different meaning (and probably expands code in the
> function that is never used).
> 
> For irq_domain_drain, you take the first element as long as there is one
> and do stuff with it, so having something like:
> 
>        mutex_lock();
>        while (!list_empty()) {
>                request = list_first_entry();
>                list_del(request->list);
> 
>                // unlock and relock as you please
>                // and do stuff
>        }
>        mutex_unlock();
> 
> Or if you are really concerned about the number of list commands:
> 
>        mutex_lock();
>        while ((request = list_first_entry_or_null()) != NULL) {
>                list_del(request->list);
> 
>                // unlock and relock as you please
>                // and do stuff
>        }
>        mutex_unlock();
> 
> To me this makes it much easier to get what you are trying to do and I
> don't think it is less efficient that your version (but I could be wrong).

Both is not much far away from what I have now. To me it is just a
flavor. I don't like the assignment in a condition. And if you fill in
the args it doesn't fit into a single line and doesn't look that easy
anymore.

> 
> 
> For irq_domain_handle_request, I think I agree that it is actually
> different from irq_domain_drain, but it is hard to see in my opinion
> because of how the functions are structured. So I would suggest
> something like:
> 
>        while ((domain = irq_domain_find(...)) != NULL) {
>                struct irq_domain_request *found = NULL;
> 
>                mutex_lock();
> 
>                // Do the check on domain if it is needed
> 
>                list_for_each_entry(request, ..., list) {
>                        if (request->fwnode != fwnode ||
>                            request->bus_token != bus_token)
>                                continue;
> 
>                        list_del(request->list);
>                        found = request;
>                        break;
>                }
>                mutex_unlock();
> 
>                if (found) {
>                        // call handler, etc...
>                }
>        }
> 
> 
> Personally, I find those flow much easier to follow than when using
> gotos to break out of loops.

This does not work and ends up in an endless loop, only the request is
removed from the request list, not the node from the node list.

> 
> This is just my suggestion so feel free to disregard if the maintainers
> agree with your current approach.

Yeah, I probably can live with an alternative implementation, but
let's wait for others to comment.

Thanks again,

-Robert