Re: [PATCH v2 1/4] genirq/irqdomain: Allow partial trimming of irq_data hierarchy

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Oct 06 2020 at 11:11, Marc Zyngier wrote:
> It appears that some HW is ugly enough that not all the interrupts
> connected to a particular interrupt controller end up with the same
> hierarchy repth (some of them are terminated early). This leaves

  depth?

> the irqchip hacker with only two choices, both equally bad:
>
> - create discrete domain chains, one for each "hierarchy depth",
>   which is very hard to maintain
>
> - create fake hierarchy levels for the shallow paths, leading
>   to all kind of problems (what are the safe hwirq values for these
>   fake levels?)
>
> Instead, let's offer the possibility to cut short a single interrupt

s/let's offer/implement/

> hierarchy, exactly representing the HW. This can only be done from
> the .alloc() callback, before mappings can be established.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  include/linux/irqdomain.h |  3 +++
>  kernel/irq/irqdomain.c    | 56 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index b37350c4fe37..c6901c1bb981 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -509,6 +509,9 @@ extern void irq_domain_free_irqs_parent(struct irq_domain *domain,
>  					unsigned int irq_base,
>  					unsigned int nr_irqs);
>  
> +extern int irq_domain_trim_hierarchy(unsigned int virq,
> +				     struct irq_domain *domain);
> +
>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>  {
>  	return domain->flags & IRQ_DOMAIN_FLAG_HIERARCHY;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 76cd7ebd1178..d0adaeea70b6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1136,6 +1136,17 @@ static struct irq_data *irq_domain_insert_irq_data(struct irq_domain *domain,
>  	return irq_data;
>  }
>  
> +static void __irq_domain_free_hierarchy(struct irq_data *irq_data)
> +{
> +	struct irq_data *tmp;
> +
> +	while (irq_data) {
> +		tmp = irq_data;
> +		irq_data = irq_data->parent_data;
> +		kfree(tmp);
> +	}
> +}
> +
>  static void irq_domain_free_irq_data(unsigned int virq, unsigned int nr_irqs)
>  {
>  	struct irq_data *irq_data, *tmp;
> @@ -1147,14 +1158,49 @@ static void irq_domain_free_irq_data(unsigned int virq, unsigned int nr_irqs)
>  		irq_data->parent_data = NULL;
>  		irq_data->domain = NULL;
>  
> -		while (tmp) {
> -			irq_data = tmp;
> -			tmp = tmp->parent_data;
> -			kfree(irq_data);
> -		}
> +		__irq_domain_free_hierarchy(tmp);
>  	}
>  }
>  
> +/**
> + * irq_domain_trim_hierarchy - Trim the irq hierarchy from a particular
> + *			       irq domain
> + * @virq:	IRQ number to trim where the hierarchy is to be trimmed
> + * @domain:	domain from which the hierarchy gets discarded for this
> + *		interrupt
> + *
> + * Drop the partial irq_data hierarchy from @domain (included) onward.
> + *
> + * This is only meant to be called from a .alloc() callback, when no
> + * actual mapping in the respective domains has been established yet.
> + * Its only use is to be able to trim levels of hierarchy that do not
> + * have any real meaning for this interrupt.
> + */
> +int irq_domain_trim_hierarchy(unsigned int virq, struct irq_domain *domain)
> +{
> +	struct irq_data *tail, *irq_data = irq_get_irq_data(virq);
> +
> +	/* It really needs to be a hierarchy, and not a single entry */
> +	if (WARN_ON(!irq_data->parent_data))
> +		return -EINVAL;
> +
> +	/* Skip until we find the right domain */
> +	while (irq_data->parent_data && irq_data->parent_data->domain != domain)
> +		irq_data = irq_data->parent_data;
> +
> +	/* The domain doesn't exist in the hierarchy, which is pretty bad */
> +	if (WARN_ON(!irq_data->parent_data))
> +		return -ENOENT;
> +
> +	/* Sever the inner part of the hierarchy...  */
> +	tail = irq_data->parent_data;
> +	irq_data->parent_data = NULL;
> +	__irq_domain_free_hierarchy(tail);

This is butt ugly, really. Especially the use case where the tegra PMC
domain removes itself from the hierarchy from .alloc()

That said, I don't have a better idea either. Sigh...

Thanks,

        tglx