| Message ID | 20201007124544.1397322-2-maz@kernel.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | soc/tegra: Prevent the PMC driver from corrupting interrupt routing | expand |
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
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);
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
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.
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;
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(-)