[PATCH v3 0/4] soc/tegra: Prevent the PMC driver from corrupting interrupt routing

[PATCH v3 0/4] soc/tegra: Prevent the PMC driver from corrupting interrupt routing

[Marc Zyngier]

This is another respin of the initial version posted at [1] (the cover
letter describes the rational for doing this).

Jon, Thierry: I still haven't applied your TB tags as the series has
changed significantly again. Please let me know if they are still
valid.

If everybody is OK with this, I'll stick it in irq/irqchip-next.

* From v2 [2]:
  - Made the hierarchy trimming an internal functionnality, not
    requiring any intervention from driver code
  - Spelling fixes

* From v1 [1]:
  - Moved the hierarchy trimming part to its own patch, living in
    irqdomain.c
  - Reduced the PMC irqchip patch to the bare minimal in order to
    reduce the risk of merge conflicts

[1] https://lore.kernel.org/r/20201005111443.1390096-1-maz@kernel.org
[2] https://lore.kernel.org/r/20201006101137.1393797-1-maz@kernel.org

Marc Zyngier (4):
  genirq/irqdomain: Allow partial trimming of irq_data hierarchy
  gpio: tegra186: Allow optional irq parent callbacks
  soc/tegra: pmc: Allow optional irq parent callbacks
  soc/tegra: pmc: Don't create fake interrupt hierarchy levels

 drivers/gpio/gpio-tegra186.c | 15 ++++++-
 drivers/soc/tegra/pmc.c      | 86 ++++++++++++++----------------------
 kernel/irq/irqdomain.c       | 58 +++++++++++++++++++++---
 3 files changed, 97 insertions(+), 62 deletions(-)

-- 
2.28.0

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

[Marc Zyngier]

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 depth (some of them are terminated early). This leaves
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?)

Implement the ability to cut short a single interrupt hierarchy
from the first level that doesn't have a corresponding irqchip.
As this is never a valid option (we have the no_irq_chip chip
for the "do nothing" case), the hierarchy can be trimmed from
that level.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 kernel/irq/irqdomain.c | 58 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 76cd7ebd1178..375eb2b79fe5 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,47 @@ 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 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);
+}
+
+
 static int irq_domain_alloc_irq_data(struct irq_domain *domain,
 				     unsigned int virq, unsigned int nr_irqs)
 {
@@ -1362,8 +1406,10 @@ 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++) {
+		irq_domain_trim_hierarchy(virq + i);
 		irq_domain_insert_irq(virq + i);
+	}
 	mutex_unlock(&irq_domain_mutex);
 
 	return virq;
-- 
2.28.0

[PATCH v3 2/4] gpio: tegra186: Allow optional irq parent callbacks

[Marc Zyngier]

Make the tegra186 GPIO driver resistent to variable depth
interrupt hierarchy, which we are about to introduce.

No functionnal change yet.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/gpio/gpio-tegra186.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 178e9128ded0..9500074b1f1b 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -430,7 +430,18 @@ static int tegra186_irq_set_type(struct irq_data *data, unsigned int type)
 	else
 		irq_set_handler_locked(data, handle_edge_irq);
 
-	return irq_chip_set_type_parent(data, type);
+	if (data->parent_data)
+		return irq_chip_set_type_parent(data, type);
+
+	return 0;
+}
+
+static int tegra186_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	if (data->parent_data)
+		return irq_chip_set_wake_parent(data, on);
+
+	return 0;
 }
 
 static void tegra186_gpio_irq(struct irq_desc *desc)
@@ -678,7 +689,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->intc.irq_mask = tegra186_irq_mask;
 	gpio->intc.irq_unmask = tegra186_irq_unmask;
 	gpio->intc.irq_set_type = tegra186_irq_set_type;
-	gpio->intc.irq_set_wake = irq_chip_set_wake_parent;
+	gpio->intc.irq_set_wake = tegra186_irq_set_wake;
 
 	irq = &gpio->gpio.irq;
 	irq->chip = &gpio->intc;
-- 
2.28.0

[PATCH v3 3/4] soc/tegra: pmc: Allow optional irq parent callbacks

[Marc Zyngier]

Make the PMC driver resistent to variable depth interrupt hierarchy,
which we are about to introduce.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/soc/tegra/pmc.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index d332e5d9abac..b39536c68f45 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -2184,6 +2184,34 @@ static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
 	return 0;
 }
 
+static void tegra_irq_mask_parent(struct irq_data *data)
+{
+	if (data->parent_data)
+		irq_chip_mask_parent(data);
+}
+
+static void tegra_irq_unmask_parent(struct irq_data *data)
+{
+	if (data->parent_data)
+		irq_chip_unmask_parent(data);
+}
+
+static void tegra_irq_eoi_parent(struct irq_data *data)
+{
+	if (data->parent_data)
+		irq_chip_eoi_parent(data);
+}
+
+static int tegra_irq_set_affinity_parent(struct irq_data *data,
+					 const struct cpumask *dest,
+					 bool force)
+{
+	if (data->parent_data)
+		return irq_chip_set_affinity_parent(data, dest, force);
+
+	return -EINVAL;
+}
+
 static int tegra_pmc_irq_init(struct tegra_pmc *pmc)
 {
 	struct irq_domain *parent = NULL;
@@ -2199,10 +2227,10 @@ static int tegra_pmc_irq_init(struct tegra_pmc *pmc)
 		return 0;
 
 	pmc->irq.name = dev_name(pmc->dev);
-	pmc->irq.irq_mask = irq_chip_mask_parent;
-	pmc->irq.irq_unmask = irq_chip_unmask_parent;
-	pmc->irq.irq_eoi = irq_chip_eoi_parent;
-	pmc->irq.irq_set_affinity = irq_chip_set_affinity_parent;
+	pmc->irq.irq_mask = tegra_irq_mask_parent;
+	pmc->irq.irq_unmask = tegra_irq_unmask_parent;
+	pmc->irq.irq_eoi = tegra_irq_eoi_parent;
+	pmc->irq.irq_set_affinity = tegra_irq_set_affinity_parent;
 	pmc->irq.irq_set_type = pmc->soc->irq_set_type;
 	pmc->irq.irq_set_wake = pmc->soc->irq_set_wake;
 
-- 
2.28.0

[PATCH v3 4/4] soc/tegra: pmc: Don't create fake interrupt hierarchy levels

[Marc Zyngier]

The Tegra PMC driver does ungodly things with the interrupt hierarchy,
repeatedly corrupting it by pulling hwirq numbers out of thin air,
overriding existing IRQ mappings and changing the handling flow
of unsuspecting users.

All of this is done in the name of preserving the interrupt hierarchy
even when these levels do not exist in the HW. Together with the use
of proper IRQs for IPIs, this leads to an unbootable system as the
rescheduling IPI gets repeatedly repurposed for random drivers...

Instead, let's simply not initialize the levels that do not make sense
for the HW, and let the core code remove them from the hierarchy.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/soc/tegra/pmc.c | 50 -----------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index b39536c68f45..2395c84ef83a 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1989,46 +1989,10 @@ static int tegra_pmc_irq_alloc(struct irq_domain *domain, unsigned int virq,
 			err = irq_domain_set_hwirq_and_chip(domain, virq,
 							    event->id,
 							    &pmc->irq, pmc);
-
-			/*
-			 * GPIOs don't have an equivalent interrupt in the
-			 * parent controller (GIC). However some code, such
-			 * as the one in irq_get_irqchip_state(), require a
-			 * valid IRQ chip to be set. Make sure that's the
-			 * case by passing NULL here, which will install a
-			 * dummy IRQ chip for the interrupt in the parent
-			 * domain.
-			 */
-			if (domain->parent)
-				irq_domain_set_hwirq_and_chip(domain->parent,
-							      virq, 0, NULL,
-							      NULL);
-
 			break;
 		}
 	}
 
-	/*
-	 * For interrupts that don't have associated wake events, assign a
-	 * dummy hardware IRQ number. This is used in the ->irq_set_type()
-	 * and ->irq_set_wake() callbacks to return early for these IRQs.
-	 */
-	if (i == soc->num_wake_events) {
-		err = irq_domain_set_hwirq_and_chip(domain, virq, ULONG_MAX,
-						    &pmc->irq, pmc);
-
-		/*
-		 * Interrupts without a wake event don't have a corresponding
-		 * interrupt in the parent controller (GIC). Pass NULL for the
-		 * chip here, which causes a dummy IRQ chip to be installed
-		 * for the interrupt in the parent domain, to make this
-		 * explicit.
-		 */
-		if (domain->parent)
-			irq_domain_set_hwirq_and_chip(domain->parent, virq, 0,
-						      NULL, NULL);
-	}
-
 	return err;
 }
 
@@ -2043,9 +2007,6 @@ static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
 	unsigned int offset, bit;
 	u32 value;
 
-	if (data->hwirq == ULONG_MAX)
-		return 0;
-
 	offset = data->hwirq / 32;
 	bit = data->hwirq % 32;
 
@@ -2080,9 +2041,6 @@ static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
 	unsigned int offset, bit;
 	u32 value;
 
-	if (data->hwirq == ULONG_MAX)
-		return 0;
-
 	offset = data->hwirq / 32;
 	bit = data->hwirq % 32;
 
@@ -2123,10 +2081,6 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
 	unsigned int offset, bit;
 	u32 value;
 
-	/* nothing to do if there's no associated wake event */
-	if (WARN_ON(data->hwirq == ULONG_MAX))
-		return 0;
-
 	offset = data->hwirq / 32;
 	bit = data->hwirq % 32;
 
@@ -2154,10 +2108,6 @@ static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
 	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
 	u32 value;
 
-	/* nothing to do if there's no associated wake event */
-	if (data->hwirq == ULONG_MAX)
-		return 0;
-
 	value = readl(pmc->wake + WAKE_AOWAKE_CNTRL(data->hwirq));
 
 	switch (type) {
-- 
2.28.0

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

[Thomas Gleixner]

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

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...

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

[Thomas Gleixner]

On Thu, Oct 08 2020 at 14:06, Marc Zyngier wrote:
> On 2020-10-08 12:22, Thomas Gleixner wrote:
> Here's what I have now, with the pmc driver calling
> irq_domain_disconnect_hierarchy() at the right spots.
>
>   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);

You can't do that in one go because in case of an error you leak the
already inserted irqs. You need two loops.

	for (i = 0; i < nr_irqs; i++) {
		ret = irq_domain_trim_hierarchy(virq + i);
		if (ret) {
                	mutex_unlock(&irq_domain_mutex);
			goto out_free_irq_data;
        }

	for (i = 0; i < nr_irqs; i++)
   		irq_domain_insert_irq(virq + i);

  	mutex_unlock(&irq_domain_mutex);
	return virq;

>   out_free_irq_data:
>   	irq_domain_free_irq_data(virq, nr_irqs);

Thanks,

        tglx

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

[Marc Zyngier]

On Thu, 08 Oct 2020 21:47:29 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Thu, Oct 08 2020 at 14:06, Marc Zyngier wrote:
> > On 2020-10-08 12:22, Thomas Gleixner wrote:
> > Here's what I have now, with the pmc driver calling
> > irq_domain_disconnect_hierarchy() at the right spots.
> >
> >   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);
> 
> You can't do that in one go because in case of an error you leak the
> already inserted irqs. You need two loops.
> 
> 	for (i = 0; i < nr_irqs; i++) {
> 		ret = irq_domain_trim_hierarchy(virq + i);
> 		if (ret) {
>                 	mutex_unlock(&irq_domain_mutex);
> 			goto out_free_irq_data;
>         }
> 
> 	for (i = 0; i < nr_irqs; i++)
>    		irq_domain_insert_irq(virq + i);
> 
>   	mutex_unlock(&irq_domain_mutex);
> 	return virq;

Of course you are right. I'll fold that in.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.