| Message ID | 20201006101137.1393797-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc/tegra: Prevent the PMC driver from corrupting interrupt routing | expand |
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
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.
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.
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.
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
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) {
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(-)