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

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

[Marc Zyngier]

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

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

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

* From v1:
  - 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

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      | 89 +++++++++++++++---------------------
 include/linux/irqdomain.h    |  3 ++
 kernel/irq/irqdomain.c       | 56 +++++++++++++++++++++--
 4 files changed, 103 insertions(+), 60 deletions(-)

-- 
2.28.0

[PATCH v2 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 repth (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?)

Instead, let's offer the possibility to cut short a single interrupt
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);
+
+	return 0;
+}
+
+
 static int irq_domain_alloc_irq_data(struct irq_domain *domain,
 				     unsigned int virq, unsigned int nr_irqs)
 {
-- 
2.28.0

[PATCH v2 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 v2 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 v2 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 trim the hierarchy yo the level that actually makes
sense for the HW, and not any deeper. This avoids having unnecessary
callbacks, overriding mappings, and otherwise keeps the hierarchy sane.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index b39536c68f45..eed500e4b7b6 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1989,45 +1989,14 @@ 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);
-
+			if (!err)
+				err = irq_domain_trim_hierarchy(virq, domain->parent);
 			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);
-	}
+	if (i == soc->num_wake_events)
+		err = irq_domain_trim_hierarchy(virq, domain);
 
 	return err;
 }
@@ -2043,9 +2012,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 +2046,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 +2086,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 +2113,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 v2 0/4] soc/tegra: Prevent the PMC driver from corrupting interrupt routing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Tue, Oct 06, 2020 at 11:11:33AM +0100, Marc Zyngier wrote:
> This is a respin of the initial version posted at [1] (the cover
> letter describes the rational for doing this).
> 
> Jon, Thierry: I haven't applied your TB tags as the series has changed
> significantly. Please let me know if they are still valid.
> 
> If everybody is OK with this, I'll stick it in irq/irqchip-next.

Looks good to me, still booting fine and waking up from suspend via wake-
events, albeit still with the unrelated issues:

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thomas Gleixner]

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

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

[Marc Zyngier]

On 2020-10-06 21:39, Thomas Gleixner wrote:
> 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/

Thanks for that, I'll fix it locally.

[...]

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

I don't disagree at all. It is both horrible and dangerous.

My preference would have been to split the PMC domain into discrete
domains, each one having having its own depth. But that's incredibly
hard to express in DT, and would break the combination of old/new
DT and kernel.

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

A (very minor) improvement would be to turn the trim call in the PMC 
driver into
a flag set in the first invalid irq_data structure, and let 
__irq_domain_alloc_irqs()
do the dirty work.

Still crap, but at least would prevent some form of abuse. Thoughts?

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

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

[Marc Zyngier]

On 2020-10-07 09:05, Marc Zyngier wrote:
> On 2020-10-06 21:39, Thomas Gleixner wrote:
>> 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/
> 
> Thanks for that, I'll fix it locally.
> 
> [...]
> 
>> This is butt ugly, really. Especially the use case where the tegra PMC
>> domain removes itself from the hierarchy from .alloc()
> 
> I don't disagree at all. It is both horrible and dangerous.
> 
> My preference would have been to split the PMC domain into discrete
> domains, each one having having its own depth. But that's incredibly
> hard to express in DT, and would break the combination of old/new
> DT and kernel.
> 
>> That said, I don't have a better idea either. Sigh...
> 
> A (very minor) improvement would be to turn the trim call in the PMC 
> driver into
> a flag set in the first invalid irq_data structure, and let
> __irq_domain_alloc_irqs() do the dirty work.
> 
> Still crap, but at least would prevent some form of abuse. Thoughts?

Actually, I wonder whether we can have a more general approach:

A partial hierarchy that doesn't have an irq_data->chip pointer 
populated
cannot be valid. So I wonder if the least ugly thing to do is to just 
drop
any messing about in the PMC driver, and instead to let 
__irq_domain_alloc_irqs()
do the culling, always, by looking for a NULL pointer in irq_data->chip.

Not any less ugly, but at least doesn't need any driver intervention.

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

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

[Marc Zyngier]

On 2020-10-07 09:53, Marc Zyngier wrote:
> On 2020-10-07 09:05, Marc Zyngier wrote:
>> On 2020-10-06 21:39, Thomas Gleixner wrote:

[...]

>>> This is butt ugly, really. Especially the use case where the tegra 
>>> PMC
>>> domain removes itself from the hierarchy from .alloc()
>> 
>> I don't disagree at all. It is both horrible and dangerous.
>> 
>> My preference would have been to split the PMC domain into discrete
>> domains, each one having having its own depth. But that's incredibly
>> hard to express in DT, and would break the combination of old/new
>> DT and kernel.
>> 
>>> That said, I don't have a better idea either. Sigh...
>> 
>> A (very minor) improvement would be to turn the trim call in the PMC 
>> driver into
>> a flag set in the first invalid irq_data structure, and let
>> __irq_domain_alloc_irqs() do the dirty work.
>> 
>> Still crap, but at least would prevent some form of abuse. Thoughts?
> 
> Actually, I wonder whether we can have a more general approach:
> 
> A partial hierarchy that doesn't have an irq_data->chip pointer 
> populated
> cannot be valid. So I wonder if the least ugly thing to do is to just 
> drop
> any messing about in the PMC driver, and instead to let
> __irq_domain_alloc_irqs()
> do the culling, always, by looking for a NULL pointer in 
> irq_data->chip.
> 
> Not any less ugly, but at least doesn't need any driver intervention.

[still talking to myself...]

I implemented that, and it has the advantage of placing the hack in a
single location. It even booted on a garden variety of systems.

I'll post an updated series, and we can compare the various levels
of ugliness.

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

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

[Thomas Gleixner]

On Wed, Oct 07 2020 at 09:53, Marc Zyngier wrote:
> On 2020-10-07 09:05, Marc Zyngier wrote:
>> On 2020-10-06 21:39, Thomas Gleixner wrote:
>>> This is butt ugly, really. Especially the use case where the tegra PMC
>>> domain removes itself from the hierarchy from .alloc()
>> 
>> I don't disagree at all. It is both horrible and dangerous.
>> 
>> My preference would have been to split the PMC domain into discrete
>> domains, each one having having its own depth. But that's incredibly
>> hard to express in DT, and would break the combination of old/new
>> DT and kernel.

Moo.

>>> That said, I don't have a better idea either. Sigh...
>> 
>> A (very minor) improvement would be to turn the trim call in the PMC
>> driver into a flag set in the first invalid irq_data structure, and
>> let __irq_domain_alloc_irqs() do the dirty work.
>> 
>> Still crap, but at least would prevent some form of abuse. Thoughts?
>
> Actually, I wonder whether we can have a more general approach:
>
> A partial hierarchy that doesn't have an irq_data->chip pointer
> populated cannot be valid. So I wonder if the least ugly thing to do
> is to just drop any messing about in the PMC driver, and instead to
> let __irq_domain_alloc_irqs() do the culling, always, by looking for a
> NULL pointer in irq_data->chip.
>
> Not any less ugly, but at least doesn't need any driver intervention.

I like that approach.

Thanks,

        tglx