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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Wed, Oct 07 2020 at 13:45, Marc Zyngier wrote:
> +/**
> + * irq_domain_trim_hierarchy - Trim the uninitialized part of a irq hierarchy
> + * @virq:	IRQ number to trim where the hierarchy is to be trimmed
> + *
> + * Drop the partial irq_data hierarchy from the level where the
> + * irq_data->chip is NULL.
> + *
> + * Its only use is to be able to trim levels of hierarchy that do not
> + * have any real meaning for this interrupt, and that the driver leaves
> + * uninitialized in its .alloc() callback.
> + */
> +static void irq_domain_trim_hierarchy(unsigned int virq)
> +{
> +	struct irq_data *tail, *irq_data = irq_get_irq_data(virq);
> +
> +	/* It really needs to be a hierarchy, and not a single entry */
> +	if (!irq_data->parent_data)
> +		return;
> +
> +	/* Skip until we find a parent irq_data without a populated chip */
> +	while (irq_data->parent_data && irq_data->parent_data->chip)
> +		irq_data = irq_data->parent_data;
> +
> +	/* All levels populated */
> +	if (!irq_data->parent_data)
> +		return;
> +
> +	pr_info("IRQ%d: trimming hierarchy from %s\n",
> +		virq, irq_data->parent_data->domain->name);
> +
> +	/* Sever the inner part of the hierarchy...  */
> +	tail = irq_data->parent_data;
> +	irq_data->parent_data = NULL;
> +	__irq_domain_free_hierarchy(tail);
> +}

I like that way more than the previous version, but there are still
quite some dangeroos waiting to bite.

Just for robustness sake we should do the following:

 Let the alloc() callback which decides to break the hierarchy tell the
 core code about it.  Conveying this through an error return might be
 tedious, but the alloc() callback should call:

static inline void irq_disconnect_hierarchy(struct irq_data *irqd)
{
    irqd->chip = ERR_PTR(-ENOTCONN);
}

to signal that this is intenionally the end of the hierarchy.

Then the above function would not only trim, but also sanity check the
hierarchy.

	trim = NULL;
        
        for (irqd = irq_data; irqd; irqd = irqd->parent_data) {
                  if (!irqd->chip && !trim)
                  	return -EINVAL;

		  if (trim && irqd->chip)
                  	return -EINVAL;
                 
                  if (IS_ERR(irqd->chip) {
                  	if (PTR_ERR(irqd->chip) != -ENOTCONN)
                        	return -EINVAL;
                        trim = irqd;
                  }
        }

        for (irqd = irq_data; trim && irqd; irqd = irqd->parent_data) {
        	if (trim == irqd->parent_data) {
                	irqd->parent_data = NULL;
                        free_stuff(trim);
                }
        }

	return 0;

or some less convoluted variant of it :)

That way we catch cases which do outright stupid crap and we let the
allocation fail which needs to be handled at the outmost caller anyway
instead of crashing later in the middle of the irq chip chain.

Thanks,

        tglx