| Message ID | 1416406451-4578-2-git-send-email-yingjoe.chen@mediatek.com |
|---|---|
| State | Accepted |
| Commit | 9a1091ef0017c40ab63e7fc0326b2dcfd4dde3a4 |
| Headers | show |
Hi Yingjoe, On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote: > Add support to use gic as a parent for stacked irq domain. > > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com> > --- > drivers/irqchip/Kconfig | 1 + > drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++--------------- > 2 files changed, 55 insertions(+), 24 deletions(-) > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index b21f12f..7f34138 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -5,6 +5,7 @@ config IRQCHIP > config ARM_GIC > bool > select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > select MULTI_IRQ_HANDLER > > config GIC_NON_BANKED > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 38493ff..464dd53 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, > { > if (hw < 32) { > irq_set_percpu_devid(irq); > - irq_set_chip_and_handler(irq, &gic_chip, > - handle_percpu_devid_irq); > + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, > + handle_percpu_devid_irq, NULL, NULL); > set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN); > } else { > - irq_set_chip_and_handler(irq, &gic_chip, > - handle_fasteoi_irq); > + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, > + handle_fasteoi_irq, NULL, NULL); > set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); > > gic_routable_irq_domain_ops->map(d, irq, hw); > } > - irq_set_chip_data(irq, d->host_data); > return 0; > } > > @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = { > }; > #endif > > +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + int i, ret; > + irq_hw_number_t hwirq; > + unsigned int type = IRQ_TYPE_NONE; > + struct of_phandle_args *irq_data = arg; > + > + ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, > + irq_data->args_count, &hwirq, &type); > + if (ret) > + return ret; > + > + for (i = 0; i < nr_irqs; i++) > + gic_irq_domain_map(domain, virq+i, hwirq+i); nit: spacing around '+'. > + > + return 0; > +} > + > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { > + .xlate = gic_irq_domain_xlate, > + .alloc = gic_irq_domain_alloc, > + .free = irq_domain_free_irqs_top, I'm convinced that irq_domain_free_irqs_top is the wrong function to call here, because you're calling it from the bottom, not the top-level (it has no parent). I cannot verify this with your code as I don't a working platform with GICv2m, but if I enable something similar on GICv3, it dies a very painful way: Unable to handle kernel NULL pointer dereference at virtual address 00000018 pgd = ffffffc03d059000 [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 Internal error: Oops: 96000006 [#1] SMP Modules linked in: CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 PC is at irq_domain_free_irqs_recursive+0x1c/0x80 LR is at irq_domain_free_irqs_common+0x88/0x9c pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 [...] [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c [...] and I cannot see how this could work on the standard GIC either. Thomas, Jiang: could you please confirm or infirm my suspicions? My understanding is that irq_domain_free_irqs_top can only be called from the top-level domain. > +}; > + > static const struct irq_domain_ops gic_irq_domain_ops = { > .map = gic_irq_domain_map, > .unmap = gic_irq_domain_unmap, > @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > gic_cpu_map[i] = 0xff; > > /* > - * For primary GICs, skip over SGIs. > - * For secondary GICs, skip over PPIs, too. > - */ > - if (gic_nr == 0 && (irq_start & 31) > 0) { > - hwirq_base = 16; > - if (irq_start != -1) > - irq_start = (irq_start & ~31) + 16; > - } else { > - hwirq_base = 32; > - } > - > - /* > * Find out how many interrupts are supported. > * The GIC only supports up to 1020 interrupt sources. > */ > @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > gic_irqs = 1020; > gic->gic_irqs = gic_irqs; > > - gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ > + if (node) { /* DT case */ > + const struct irq_domain_ops *ops = > + &gic_irq_domain_hierarchy_ops; nit: please put this on the same line, even if it is longer than 80 characters. > + > + if (!of_property_read_u32(node, "arm,routable-irqs", > + &nr_routable_irqs)) { > + ops = &gic_irq_domain_ops; > + gic_irqs = nr_routable_irqs; > + } > + > + gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic); > + } else { /* Non-DT case */ > + /* > + * For primary GICs, skip over SGIs. > + * For secondary GICs, skip over PPIs, too. > + */ > + if (gic_nr == 0 && (irq_start & 31) > 0) { > + hwirq_base = 16; > + if (irq_start != -1) > + irq_start = (irq_start & ~31) + 16; > + } else { > + hwirq_base = 32; > + } > + > + gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ > > - if (of_property_read_u32(node, "arm,routable-irqs", > - &nr_routable_irqs)) { > irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, > numa_node_id()); > if (IS_ERR_VALUE(irq_base)) { > @@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, > hwirq_base, &gic_irq_domain_ops, gic); > - } else { > - gic->domain = irq_domain_add_linear(node, nr_routable_irqs, > - &gic_irq_domain_ops, > - gic); > } > > if (WARN_ON(!gic->domain)) Thanks, M.
Hi Marc, On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote: > > +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > + unsigned int nr_irqs, void *arg) > > +{ > > + int i, ret; > > + irq_hw_number_t hwirq; > > + unsigned int type = IRQ_TYPE_NONE; > > + struct of_phandle_args *irq_data = arg; > > + > > + ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, > > + irq_data->args_count, &hwirq, &type); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < nr_irqs; i++) > > + gic_irq_domain_map(domain, virq+i, hwirq+i); > > nit: spacing around '+'. OK. > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { > > + .xlate = gic_irq_domain_xlate, > > + .alloc = gic_irq_domain_alloc, > > + .free = irq_domain_free_irqs_top, > > I'm convinced that irq_domain_free_irqs_top is the wrong function to > call here, because you're calling it from the bottom, not the top-level > (it has no parent). Base on the name, I though this is helper function for top level irq_domain? > I cannot verify this with your code as I don't a working platform with > GICv2m, but if I enable something similar on GICv3, it dies a very > painful way: > > Unable to handle kernel NULL pointer dereference at virtual address 00000018 > pgd = ffffffc03d059000 > [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 > Internal error: Oops: 96000006 [#1] SMP > Modules linked in: > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 > PC is at irq_domain_free_irqs_recursive+0x1c/0x80 > LR is at irq_domain_free_irqs_common+0x88/0x9c > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 > [...] > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c > [...] > > and I cannot see how this could work on the standard GIC either. I'm sorry, I just realize my testcase was too simple, irqs are populated by device tree and never got freed. I'll add that and test it again. > Thomas, Jiang: could you please confirm or infirm my suspicions? My > understanding is that irq_domain_free_irqs_top can only be called from > the top-level domain. > > > +}; > > + > > static const struct irq_domain_ops gic_irq_domain_ops = { > > .map = gic_irq_domain_map, > > .unmap = gic_irq_domain_unmap, > > @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > gic_cpu_map[i] = 0xff; > > > > /* > > - * For primary GICs, skip over SGIs. > > - * For secondary GICs, skip over PPIs, too. > > - */ > > - if (gic_nr == 0 && (irq_start & 31) > 0) { > > - hwirq_base = 16; > > - if (irq_start != -1) > > - irq_start = (irq_start & ~31) + 16; > > - } else { > > - hwirq_base = 32; > > - } > > - > > - /* > > * Find out how many interrupts are supported. > > * The GIC only supports up to 1020 interrupt sources. > > */ > > @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > gic_irqs = 1020; > > gic->gic_irqs = gic_irqs; > > > > - gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ > > + if (node) { /* DT case */ > > + const struct irq_domain_ops *ops = > > + &gic_irq_domain_hierarchy_ops; > > nit: please put this on the same line, even if it is longer than 80 > characters. ok. Joe.C -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/11/20 1:18, Marc Zyngier wrote: > Hi Yingjoe, > > On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote: >> Add support to use gic as a parent for stacked irq domain. >> >> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com> >> --- >> drivers/irqchip/Kconfig | 1 + >> drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++--------------- >> 2 files changed, 55 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >> index b21f12f..7f34138 100644 >> --- a/drivers/irqchip/Kconfig >> +++ b/drivers/irqchip/Kconfig >> @@ -5,6 +5,7 @@ config IRQCHIP >> config ARM_GIC >> bool >> select IRQ_DOMAIN >> + select IRQ_DOMAIN_HIERARCHY >> select MULTI_IRQ_HANDLER >> >> config GIC_NON_BANKED >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index 38493ff..464dd53 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, >> { >> if (hw < 32) { >> irq_set_percpu_devid(irq); >> - irq_set_chip_and_handler(irq, &gic_chip, >> - handle_percpu_devid_irq); >> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, >> + handle_percpu_devid_irq, NULL, NULL); >> set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN); >> } else { >> - irq_set_chip_and_handler(irq, &gic_chip, >> - handle_fasteoi_irq); >> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, >> + handle_fasteoi_irq, NULL, NULL); >> set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); >> >> gic_routable_irq_domain_ops->map(d, irq, hw); >> } >> - irq_set_chip_data(irq, d->host_data); >> return 0; >> } >> >> @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = { >> }; >> #endif >> >> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, >> + unsigned int nr_irqs, void *arg) >> +{ >> + int i, ret; >> + irq_hw_number_t hwirq; >> + unsigned int type = IRQ_TYPE_NONE; >> + struct of_phandle_args *irq_data = arg; >> + >> + ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, >> + irq_data->args_count, &hwirq, &type); >> + if (ret) >> + return ret; >> + >> + for (i = 0; i < nr_irqs; i++) >> + gic_irq_domain_map(domain, virq+i, hwirq+i); > > nit: spacing around '+'. > >> + >> + return 0; >> +} >> + >> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { >> + .xlate = gic_irq_domain_xlate, >> + .alloc = gic_irq_domain_alloc, >> + .free = irq_domain_free_irqs_top, > > I'm convinced that irq_domain_free_irqs_top is the wrong function to > call here, because you're calling it from the bottom, not the top-level > (it has no parent). > > I cannot verify this with your code as I don't a working platform with > GICv2m, but if I enable something similar on GICv3, it dies a very > painful way: > > Unable to handle kernel NULL pointer dereference at virtual address 00000018 > pgd = ffffffc03d059000 > [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 > Internal error: Oops: 96000006 [#1] SMP > Modules linked in: > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 > PC is at irq_domain_free_irqs_recursive+0x1c/0x80 > LR is at irq_domain_free_irqs_common+0x88/0x9c > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 > [...] > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c > [...] > > and I cannot see how this could work on the standard GIC either. > > Thomas, Jiang: could you please confirm or infirm my suspicions? My > understanding is that irq_domain_free_irqs_top can only be called from > the top-level domain. Hi Marc, It indicates that irq_domain_free_irqs_top() is not a good name. We have: 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and handler_data; 3) irq_domain_reset_irq_data() resets irq_chip and chip_data. 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls parent domain's domain_ops.free() callback. 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler, handler_data and call parent domain's domain_ops.free() callback. So there two possible improvements here: 1) Rename irq_domain_free_irqs_top() with better name, any suggestions? It's named as is because it's always called by the outer-most irqdomains on x86. 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top() to call parent domain's domain_ops.free() callback only if parent exists. By this way, they could be used for inner-most irqdomains. If OK, I will respin a version 4 patch set based on tip/irq/irqdomain. Thoughts? Regards! Gerry > >> +}; >> + >> static const struct irq_domain_ops gic_irq_domain_ops = { >> .map = gic_irq_domain_map, >> .unmap = gic_irq_domain_unmap, >> @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> gic_cpu_map[i] = 0xff; >> >> /* >> - * For primary GICs, skip over SGIs. >> - * For secondary GICs, skip over PPIs, too. >> - */ >> - if (gic_nr == 0 && (irq_start & 31) > 0) { >> - hwirq_base = 16; >> - if (irq_start != -1) >> - irq_start = (irq_start & ~31) + 16; >> - } else { >> - hwirq_base = 32; >> - } >> - >> - /* >> * Find out how many interrupts are supported. >> * The GIC only supports up to 1020 interrupt sources. >> */ >> @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> gic_irqs = 1020; >> gic->gic_irqs = gic_irqs; >> >> - gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ >> + if (node) { /* DT case */ >> + const struct irq_domain_ops *ops = >> + &gic_irq_domain_hierarchy_ops; > > nit: please put this on the same line, even if it is longer than 80 > characters. > >> + >> + if (!of_property_read_u32(node, "arm,routable-irqs", >> + &nr_routable_irqs)) { >> + ops = &gic_irq_domain_ops; >> + gic_irqs = nr_routable_irqs; >> + } >> + >> + gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic); >> + } else { /* Non-DT case */ >> + /* >> + * For primary GICs, skip over SGIs. >> + * For secondary GICs, skip over PPIs, too. >> + */ >> + if (gic_nr == 0 && (irq_start & 31) > 0) { >> + hwirq_base = 16; >> + if (irq_start != -1) >> + irq_start = (irq_start & ~31) + 16; >> + } else { >> + hwirq_base = 32; >> + } >> + >> + gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ >> >> - if (of_property_read_u32(node, "arm,routable-irqs", >> - &nr_routable_irqs)) { >> irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, >> numa_node_id()); >> if (IS_ERR_VALUE(irq_base)) { >> @@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> >> gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >> hwirq_base, &gic_irq_domain_ops, gic); >> - } else { >> - gic->domain = irq_domain_add_linear(node, nr_routable_irqs, >> - &gic_irq_domain_ops, >> - gic); >> } >> >> if (WARN_ON(!gic->domain)) > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marc, On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote: > On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote: > > > + > > > + return 0; > > > +} > > > + > > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { > > > + .xlate = gic_irq_domain_xlate, > > > + .alloc = gic_irq_domain_alloc, > > > + .free = irq_domain_free_irqs_top, > > > > I'm convinced that irq_domain_free_irqs_top is the wrong function to > > call here, because you're calling it from the bottom, not the top-level > > (it has no parent). > > Base on the name, I though this is helper function for top level > irq_domain? > > > I cannot verify this with your code as I don't a working platform with > > GICv2m, but if I enable something similar on GICv3, it dies a very > > painful way: > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000018 > > pgd = ffffffc03d059000 > > [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 > > Internal error: Oops: 96000006 [#1] SMP > > Modules linked in: > > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 > > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 > > PC is at irq_domain_free_irqs_recursive+0x1c/0x80 > > LR is at irq_domain_free_irqs_common+0x88/0x9c > > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 > > [...] > > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 > > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() > > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 > > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 > > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c > > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 > > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c > > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c > > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 > > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c > > [...] > > > > and I cannot see how this could work on the standard GIC either. > > I'm sorry, I just realize my testcase was too simple, irqs are populated > by device tree and never got freed. I'll add that and test it again. On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops is only used when we use DT, so we probably will never use the free function. Is it OK to remove the free support here? Joe.C -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 20 2014 at 4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote: Hi Jiang, > On 2014/11/20 1:18, Marc Zyngier wrote: >> Hi Yingjoe, >> >> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen >> <yingjoe.chen@mediatek.com> wrote: >>> + >>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { >>> + .xlate = gic_irq_domain_xlate, >>> + .alloc = gic_irq_domain_alloc, >>> + .free = irq_domain_free_irqs_top, >> >> I'm convinced that irq_domain_free_irqs_top is the wrong function to >> call here, because you're calling it from the bottom, not the top-level >> (it has no parent). >> >> I cannot verify this with your code as I don't a working platform with >> GICv2m, but if I enable something similar on GICv3, it dies a very >> painful way: >> >> Unable to handle kernel NULL pointer dereference at virtual address 00000018 >> pgd = ffffffc03d059000 >> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 >> Internal error: Oops: 96000006 [#1] SMP >> Modules linked in: >> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 >> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 >> PC is at irq_domain_free_irqs_recursive+0x1c/0x80 >> LR is at irq_domain_free_irqs_common+0x88/0x9c >> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 >> [...] >> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 >> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c >> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c >> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c >> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 >> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c >> [...] >> >> and I cannot see how this could work on the standard GIC either. >> >> Thomas, Jiang: could you please confirm or infirm my suspicions? My >> understanding is that irq_domain_free_irqs_top can only be called from >> the top-level domain. > Hi Marc, > It indicates that irq_domain_free_irqs_top() is not a good name. > We have: > 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data > 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and > handler_data; > 3) irq_domain_reset_irq_data() resets irq_chip and chip_data. > 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls > parent domain's domain_ops.free() callback. > 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler, > handler_data and call parent domain's domain_ops.free() callback. Yes, and this "call parent domain's free callback" is where the problem lies. Here, it is called from the innermost domain, with no parent. > So there two possible improvements here: > 1) Rename irq_domain_free_irqs_top() with better name, any suggestions? > It's named as is because it's always called by the outer-most > irqdomains on x86. > 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top() > to call parent domain's domain_ops.free() callback only if parent > exists. By this way, they could be used for inner-most irqdomains. > If OK, I will respin a version 4 patch set based on tip/irq/irqdomain. > Thoughts? Checking the parent is probably a safe solution (this is not a hot path anyway). I don't care much about the name though, and I the only thing I can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's not even funny. I'll let the matter rest in your capable hands! ;-) Thanks, M.
Hi Joe, On Thu, Nov 20 2014 at 9:41:51 am GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote: > Hi Marc, > > On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote: >> On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote: >> > > + >> > > + return 0; >> > > +} >> > > + >> > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { >> > > + .xlate = gic_irq_domain_xlate, >> > > + .alloc = gic_irq_domain_alloc, >> > > + .free = irq_domain_free_irqs_top, >> > >> > I'm convinced that irq_domain_free_irqs_top is the wrong function to >> > call here, because you're calling it from the bottom, not the top-level >> > (it has no parent). >> >> Base on the name, I though this is helper function for top level >> irq_domain? >> >> > I cannot verify this with your code as I don't a working platform with >> > GICv2m, but if I enable something similar on GICv3, it dies a very >> > painful way: >> > >> > Unable to handle kernel NULL pointer dereference at virtual address 00000018 >> > pgd = ffffffc03d059000 >> > [00000018] *pgd=0000000081356003, *pud=0000000081356003, >> > *pmd=0000000000000000 >> > Internal error: Oops: 96000006 [#1] SMP >> > Modules linked in: >> > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 >> > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 >> > PC is at irq_domain_free_irqs_recursive+0x1c/0x80 >> > LR is at irq_domain_free_irqs_common+0x88/0x9c >> > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 >> > [...] >> > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 >> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c >> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- >> > gic_domain.free() >> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 >> > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 >> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c >> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c >> > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 >> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 >> > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c >> > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c >> > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 >> > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c >> > [...] >> > >> > and I cannot see how this could work on the standard GIC either. >> >> I'm sorry, I just realize my testcase was too simple, irqs are populated >> by device tree and never got freed. I'll add that and test it again. > > On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops > is only used when we use DT, so we probably will never use the free > function. Is it OK to remove the free support here? Well, such thing is coming with GICv2m (SPIs are allocated out of DT). You can drop it if you want, but I will then have to add it back (which seems like a waste of time). I'd prefer if you kept it in with the rest of the conversion. Thanks, M.
Hi, On Thu, 2014-11-20 at 10:07 +0000, Marc Zyngier wrote: > On Thu, Nov 20 2014 at 4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote: > > Hi Jiang, > > > On 2014/11/20 1:18, Marc Zyngier wrote: > >> Hi Yingjoe, > >> > >> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen > >> <yingjoe.chen@mediatek.com> wrote: > >>> + > >>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { > >>> + .xlate = gic_irq_domain_xlate, > >>> + .alloc = gic_irq_domain_alloc, > >>> + .free = irq_domain_free_irqs_top, > >> > >> I'm convinced that irq_domain_free_irqs_top is the wrong function to > >> call here, because you're calling it from the bottom, not the top-level > >> (it has no parent). > >> > >> I cannot verify this with your code as I don't a working platform with > >> GICv2m, but if I enable something similar on GICv3, it dies a very > >> painful way: > >> > >> Unable to handle kernel NULL pointer dereference at virtual address 00000018 > >> pgd = ffffffc03d059000 > >> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000 > >> Internal error: Oops: 96000006 [#1] SMP > >> Modules linked in: > >> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311 > >> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000 > >> PC is at irq_domain_free_irqs_recursive+0x1c/0x80 > >> LR is at irq_domain_free_irqs_common+0x88/0x9c > >> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145 > >> [...] > >> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80 > >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free() > >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > >> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20 > >> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250 > >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c > >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c > >> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88 > >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80 > >> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c > >> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c > >> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0 > >> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c > >> [...] > >> > >> and I cannot see how this could work on the standard GIC either. > >> > >> Thomas, Jiang: could you please confirm or infirm my suspicions? My > >> understanding is that irq_domain_free_irqs_top can only be called from > >> the top-level domain. > > Hi Marc, > > It indicates that irq_domain_free_irqs_top() is not a good name. > > We have: > > 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data > > 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and > > handler_data; > > 3) irq_domain_reset_irq_data() resets irq_chip and chip_data. > > 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls > > parent domain's domain_ops.free() callback. > > 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler, > > handler_data and call parent domain's domain_ops.free() callback. > > Yes, and this "call parent domain's free callback" is where the problem > lies. Here, it is called from the innermost domain, with no parent. > > > So there two possible improvements here: > > 1) Rename irq_domain_free_irqs_top() with better name, any suggestions? > > It's named as is because it's always called by the outer-most > > irqdomains on x86. > > 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top() > > to call parent domain's domain_ops.free() callback only if parent > > exists. By this way, they could be used for inner-most irqdomains. > > If OK, I will respin a version 4 patch set based on tip/irq/irqdomain. > > Thoughts? > > Checking the parent is probably a safe solution (this is not a hot path > anyway). I don't care much about the name though, and I the only thing I > can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's > not even funny. I'll let the matter rest in your capable hands! ;-) I've applied Jiang's "irqdomain: Enhance irq_domain_free_irqs_common() to support parentless irqdomain" patch and it did fix the crash. Thanks Jiang, Marc Joe.C -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index b21f12f..7f34138 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -5,6 +5,7 @@ config IRQCHIP config ARM_GIC bool select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY select MULTI_IRQ_HANDLER config GIC_NON_BANKED diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 38493ff..464dd53 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, { if (hw < 32) { irq_set_percpu_devid(irq); - irq_set_chip_and_handler(irq, &gic_chip, - handle_percpu_devid_irq); + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, + handle_percpu_devid_irq, NULL, NULL); set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN); } else { - irq_set_chip_and_handler(irq, &gic_chip, - handle_fasteoi_irq); + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data, + handle_fasteoi_irq, NULL, NULL); set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); gic_routable_irq_domain_ops->map(d, irq, hw); } - irq_set_chip_data(irq, d->host_data); return 0; } @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = { }; #endif +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + int i, ret; + irq_hw_number_t hwirq; + unsigned int type = IRQ_TYPE_NONE; + struct of_phandle_args *irq_data = arg; + + ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, + irq_data->args_count, &hwirq, &type); + if (ret) + return ret; + + for (i = 0; i < nr_irqs; i++) + gic_irq_domain_map(domain, virq+i, hwirq+i); + + return 0; +} + +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = { + .xlate = gic_irq_domain_xlate, + .alloc = gic_irq_domain_alloc, + .free = irq_domain_free_irqs_top, +}; + static const struct irq_domain_ops gic_irq_domain_ops = { .map = gic_irq_domain_map, .unmap = gic_irq_domain_unmap, @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic_cpu_map[i] = 0xff; /* - * For primary GICs, skip over SGIs. - * For secondary GICs, skip over PPIs, too. - */ - if (gic_nr == 0 && (irq_start & 31) > 0) { - hwirq_base = 16; - if (irq_start != -1) - irq_start = (irq_start & ~31) + 16; - } else { - hwirq_base = 32; - } - - /* * Find out how many interrupts are supported. * The GIC only supports up to 1020 interrupt sources. */ @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic_irqs = 1020; gic->gic_irqs = gic_irqs; - gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ + if (node) { /* DT case */ + const struct irq_domain_ops *ops = + &gic_irq_domain_hierarchy_ops; + + if (!of_property_read_u32(node, "arm,routable-irqs", + &nr_routable_irqs)) { + ops = &gic_irq_domain_ops; + gic_irqs = nr_routable_irqs; + } + + gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic); + } else { /* Non-DT case */ + /* + * For primary GICs, skip over SGIs. + * For secondary GICs, skip over PPIs, too. + */ + if (gic_nr == 0 && (irq_start & 31) > 0) { + hwirq_base = 16; + if (irq_start != -1) + irq_start = (irq_start & ~31) + 16; + } else { + hwirq_base = 32; + } + + gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ - if (of_property_read_u32(node, "arm,routable-irqs", - &nr_routable_irqs)) { irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, numa_node_id()); if (IS_ERR_VALUE(irq_base)) { @@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, hwirq_base, &gic_irq_domain_ops, gic); - } else { - gic->domain = irq_domain_add_linear(node, nr_routable_irqs, - &gic_irq_domain_ops, - gic); } if (WARN_ON(!gic->domain))
Add support to use gic as a parent for stacked irq domain. Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com> --- drivers/irqchip/Kconfig | 1 + drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 55 insertions(+), 24 deletions(-)