Re: [PATCH 3/3] soc/tegra: pmc: Don't create fake interrupt hierarchy levels
From: Marc Zyngier
Date: Mon Oct 05 2020 - 09:10:17 EST
On 2020-10-05 12:33, Thierry Reding wrote:
On Mon, Oct 05, 2020 at 12:14:43PM +0100, Marc Zyngier wrote:
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 allow the hierarchy to be trimmed to 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@xxxxxxxxxx>
---
drivers/soc/tegra/pmc.c | 79
+++++++++++++++--------------------------
1 file changed, 29 insertions(+), 50 deletions(-)
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 9960f7c18431..4eea3134fb3e 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1993,6 +1993,30 @@ static int tegra_pmc_irq_translate(struct
irq_domain *domain,
return 0;
}
+/* Trim the irq hierarchy from a particular irq domain */
+static void trim_hierarchy(unsigned int virq, struct irq_domain
*domain)
+{
+ struct irq_data *tail, *irq_data = irq_get_irq_data(virq);
+
+ /* The PMC doesn't generate any interrupt by itself */
+ if (WARN_ON(!irq_data->parent_data))
+ return;
+
+ /* Skip until we find the right domain */
+ while (irq_data->parent_data && irq_data->parent_data->domain !=
domain)
+ irq_data = irq_data->parent_data;
+
+ /* Sever the inner part of the hierarchy... */
+ tail = irq_data->parent_data;
+ irq_data->parent_data = NULL;
+
+ /* ... and free it */
+ for (irq_data = tail; irq_data; irq_data = tail) {
+ tail = irq_data->parent_data;
+ kfree(irq_data);
+ };
+}
That kind of looks like what I originally wanted to do and (naively)
thought that passing the (0, NULL, NULL) triplet would achieve.
Given that this is fairly low-level stuff that deals with the inner
workings of the IRQ infrastructure, should we eventually pull this out
of the driver and make it into a core helper? I don't seriously expect
this to be widely useful, but putting it into the core might help keep
it more maintainable.
That's the ultimate plan, but I wanted to give it some soaking time
on Tegra before exposing it to the outside world
(irq_domain_free_irq_data()
could be rewritten in terms of this primitive, for example).
I volunteer to do that work if you think it's a good idea.
Sure, once we know we're good to go with this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...