lkml.org 
[lkml]   [2018]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 01/10] irqdomain: Add interface to request an irq domain
From
Date
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

\
 
 \ /
  Last update: 2018-11-08 11:20    [W:0.164 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site