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

[Marc Zyngier]

On 2020-10-08 12:22, Thomas Gleixner wrote:
> 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:

[...]

Here's what I have now, with the pmc driver calling
irq_domain_disconnect_hierarchy() at the right spots.

        M.

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index b37350c4fe37..a52b095bd404 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_disconnect_hierarchy(struct irq_domain *domain,
+					   unsigned int virq);
+
 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..316f5baa9cd9 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,12 +1158,81 @@ 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);
+	}
+}
+
+int irq_domain_disconnect_hierarchy(struct irq_domain *domain,
+				    unsigned int virq)
+{
+	struct irq_data *irqd;
+
+	irqd = irq_domain_get_irq_data(domain, virq);
+	if (!irqd)
+		return -EINVAL;
+
+	irqd->chip = ERR_PTR(-ENOTCONN);
+	return 0;
+}
+
+/**
+ * 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 a trim marker (PTR_ERR(-ENOTCONN)).
+ *
+ * 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 marks
+ * as such from its .alloc() callback.
+ */
+static int irq_domain_trim_hierarchy(unsigned int virq)
+{
+	struct irq_data *tail, *irqd, *irq_data;
+
+	irq_data = irq_get_irq_data(virq);
+	tail = NULL;
+
+	/* The first entry must have a valid irqchip */
+	if (!irq_data->chip || IS_ERR(irq_data->chip))
+		return -EINVAL;
+
+	/*
+	 * Validate that the irq_data chain is sane in the presence of
+	 * a hierarchy trimming marker.
+	 */
+	for (irqd = irq_data->parent_data; irqd; irq_data = irqd, irqd = 
irqd->parent_data) {
+		/* Can't have a valid irqchip after a trim marker */
+		if (irqd->chip && tail)
+			return -EINVAL;
+
+		/* Can't have an empty irqchip before a trim marker */
+		if (!irqd->chip && !tail)
+			return -EINVAL;
+
+		if (IS_ERR(irqd->chip)) {
+			/* Only -ENOTCONN is a valid trim marker */
+			if (PTR_ERR(irqd->chip) != -ENOTCONN)
+				return -EINVAL;
+
+			tail = irq_data;
 		}
 	}
+
+	/* No trim marker, nothing to do */
+	if (!tail)
+		return 0;
+
+	pr_info("IRQ%d: trimming hierarchy from %s\n",
+		virq, tail->parent_data->domain->name);
+
+	/* Sever the inner part of the hierarchy...  */
+	irqd = tail;
+	tail = tail->parent_data;
+	irqd->parent_data = NULL;
+	__irq_domain_free_hierarchy(tail);
+
+	return 0;
 }

 static int irq_domain_alloc_irq_data(struct irq_domain *domain,
@@ -1362,11 +1442,16 @@ int __irq_domain_alloc_irqs(struct irq_domain 
*domain, int irq_base,
 		mutex_unlock(&irq_domain_mutex);
 		goto out_free_irq_data;
 	}
-	for (i = 0; i < nr_irqs; i++)
+	for (i = 0; i < nr_irqs; i++) {
+		ret = irq_domain_trim_hierarchy(virq + i);
+		if (ret)
+			break;
 		irq_domain_insert_irq(virq + i);
+	}
 	mutex_unlock(&irq_domain_mutex);

-	return virq;
+	if (!ret)
+		return virq;

 out_free_irq_data:
 	irq_domain_free_irq_data(virq, nr_irqs);

--
Jazz is not dead. It just smells funny...