Messages in this thread Patch in this message | | | Subject | Re: [PATCH 01/10] irqdomain: Add interface to request an irq domain | From | Julien Thierry <> | Date | Thu, 8 Nov 2018 10:19:52 +0000 |
| |
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
| |