Re: [PATCH 2/4 v2] irqdomain: augment add_simple() to allocate descs

[Grant Likely]

On Mon, 1 Oct 2012 09:35:22 +0200, Linus Walleij <linus.walleij@xxxxxxxxxxxxxx> wrote:
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> 
> Currently we rely on all IRQ chip instances to dynamically
> allocate their IRQ descriptors unless they use the linear
> IRQ domain. So for irqdomain_add_legacy() and
> irqdomain_add_simple() the caller need to make sure that
> descriptors are allocated.
> 
> Let's slightly augment the yet unused irqdomain_add_simple()
> to also allocate descriptors as a means to simplify usage
> and avoid code duplication throughout the kernel.
> 
> We warn if descriptors cannot be allocated, e.g. if a
> platform has the bad habit of hogging descriptors at boot
> time.
> 
> Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
> Cc: Paul Mundt <lethal@xxxxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Switch descriptor allocation on IS_ENABLED(CONFIG_SPARSE_IRQ)
>   so it won't attempt to allocate descriptors in the non-sparse
>   case.
> - Use of_node_to_nid() to make sure we work on platforms with their
>   own node concept.
> - Specify irq_alloc_descs(first_irq, first_irq ...) to emulate
>   irq_alloc_desc_at().
> ---
>  kernel/irq/irqdomain.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 49a7772..4e69e24 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -148,7 +148,8 @@ static unsigned int irq_domain_legacy_revmap(struct irq_domain *domain,
>   * @host_data: Controller private data pointer
>   *
>   * Allocates a legacy irq_domain if irq_base is positive or a linear
> - * domain otherwise.
> + * domain otherwise. For the legacy domain, IRQ descriptors will also
> + * be allocated.
>   *
>   * This is intended to implement the expected behaviour for most
>   * interrupt controllers which is that a linear mapping should
> @@ -162,11 +163,33 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
>  					 const struct irq_domain_ops *ops,
>  					 void *host_data)
>  {
> -	if (first_irq > 0)
> -		return irq_domain_add_legacy(of_node, size, first_irq, 0,
> +	if (first_irq > 0) {
> +		int irq_base;
> +
> +		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> +			/*
> +			 * Set the descriptor allocator to search for a
> +			 * 1-to-1 mapping, such as irq_alloc_desc_at().
> +			 * Use of_node_to_nid() which is defined to
> +			 * numa_node_id() on platforms that have no custom
> +			 * implementation.
> +			 */
> +			irq_base = irq_alloc_descs(first_irq, first_irq, size,
> +						   of_node_to_nid(of_node));
> +			if (irq_base < 0) {
> +				WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> +				     first_irq);
> +				irq_base = first_irq;

As I just commented on the previous version, WARN() is probably too
verbose (and scary). Make it an informational.

However, I see another problem. What is the requested range straddles
the boundary between reserved and non-reserved IRQs? It would be good to
give some information about which irq range was requested and maybe
report which ones were available.... or check to see if the request is
inside or partially inside the reserved region?

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/