| Message ID | 1458224359-32665-12-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jon Hunter |
| Headers | show |
On Thu, 17 Mar 2016 14:19:15 +0000 Jon Hunter <jonathanh@nvidia.com> wrote: > If the GIC initialisation fails, then currently we do not return an error > or clean-up afterwards. Although for root controllers, this failure may be > fatal anyway, for secondary controllers, it may not be fatal and so return > an error on failure and clean-up. > > For non-banked GIC controllers, make sure that we free any memory > allocated if we fail to initialise the IRQ domain. Please note that > free_percpu() only frees memory if the pointer passed to it is not NULL > and so it is unnecessary to check if both pointers are valid or not. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/irqchip/irq-gic.c | 57 ++++++++++++++++++++++++++++++++++------------- > 1 file changed, 41 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index b0a781f8c450..42a1412b5186 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -999,13 +999,13 @@ static const struct irq_domain_ops gic_irq_domain_ops = { > .unmap = gic_irq_domain_unmap, > }; > > -static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > +static int __init __gic_init_bases(unsigned int gic_nr, int irq_start, > void __iomem *dist_base, void __iomem *cpu_base, > u32 percpu_offset, struct fwnode_handle *handle) > { > irq_hw_number_t hwirq_base; > struct gic_chip_data *gic; > - int gic_irqs, irq_base, i; > + int gic_irqs, irq_base, i, ret; > > BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR); > > @@ -1030,17 +1030,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > gic->chip.irq_set_affinity = gic_set_affinity; > #endif > > -#ifdef CONFIG_GIC_NON_BANKED > - if (percpu_offset) { /* Frankein-GIC without banked registers... */ > + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) { > + /* Frankein-GIC without banked registers... */ > unsigned int cpu; > > gic->dist_base.percpu_base = alloc_percpu(void __iomem *); > gic->cpu_base.percpu_base = alloc_percpu(void __iomem *); > if (WARN_ON(!gic->dist_base.percpu_base || > !gic->cpu_base.percpu_base)) { > - free_percpu(gic->dist_base.percpu_base); > - free_percpu(gic->cpu_base.percpu_base); > - return; > + ret = -ENOMEM; > + goto error; > } > > for_each_possible_cpu(cpu) { > @@ -1052,9 +1051,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > } > > gic_set_base_accessor(gic, gic_get_percpu_base); > - } else > -#endif > - { /* Normal, sane GIC... */ > + } else { > + /* Normal, sane GIC... */ > WARN(percpu_offset, > "GIC_NON_BANKED not enabled, ignoring %08x offset!", > percpu_offset); > @@ -1104,8 +1102,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > hwirq_base, &gic_irq_domain_ops, gic); > } > > - if (WARN_ON(!gic->domain)) > - return; > + if (WARN_ON(!gic->domain)) { > + ret = -ENODEV; > + goto error; > + } > > if (gic_nr == 0) { > /* > @@ -1127,6 +1127,18 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > gic_dist_init(gic); > gic_cpu_init(gic); > gic_pm_init(gic); > + > + return 0; > + > +error: > + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) { > + free_percpu(gic->dist_base.percpu_base); > + free_percpu(gic->cpu_base.percpu_base); > + } > + > + kfree(gic->chip.name); Ah, this is where Linus' remark about "GICv2" clashes. Either you keep the allocation in the previous patch, or you guard this with a test on the EOImode. I'll leave it up to you. > + > + return ret; > } > > void __init gic_init(unsigned int gic_nr, int irq_start, > @@ -1187,7 +1199,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) > void __iomem *cpu_base; > void __iomem *dist_base; > u32 percpu_offset; > - int irq; > + int irq, ret; > > if (WARN_ON(!node)) > return -ENODEV; > @@ -1212,8 +1224,14 @@ gic_of_init(struct device_node *node, struct device_node *parent) > if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) > percpu_offset = 0; > > - __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, > + ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, > &node->fwnode); > + if (ret) { > + iounmap(dist_base); > + iounmap(cpu_base); > + return ret; > + } > + > if (!gic_cnt) > gic_init_physaddr(node); > > @@ -1302,7 +1320,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, > struct acpi_madt_generic_distributor *dist; > void __iomem *cpu_base, *dist_base; > struct fwnode_handle *domain_handle; > - int count; > + int count, ret; > > /* Collect CPU base addresses */ > count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, > @@ -1345,7 +1363,14 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, > return -ENOMEM; > } > > - __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); > + ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); > + if (ret) { > + pr_err("Failed to initialise GIC\n"); > + irq_domain_free_fwnode(domain_handle); > + iounmap(cpu_base); > + iounmap(dist_base); > + return ret; > + } > > acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle); > Otherwise looks good. M.
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index b0a781f8c450..42a1412b5186 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -999,13 +999,13 @@ static const struct irq_domain_ops gic_irq_domain_ops = { .unmap = gic_irq_domain_unmap, }; -static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, +static int __init __gic_init_bases(unsigned int gic_nr, int irq_start, void __iomem *dist_base, void __iomem *cpu_base, u32 percpu_offset, struct fwnode_handle *handle) { irq_hw_number_t hwirq_base; struct gic_chip_data *gic; - int gic_irqs, irq_base, i; + int gic_irqs, irq_base, i, ret; BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR); @@ -1030,17 +1030,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, gic->chip.irq_set_affinity = gic_set_affinity; #endif -#ifdef CONFIG_GIC_NON_BANKED - if (percpu_offset) { /* Frankein-GIC without banked registers... */ + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) { + /* Frankein-GIC without banked registers... */ unsigned int cpu; gic->dist_base.percpu_base = alloc_percpu(void __iomem *); gic->cpu_base.percpu_base = alloc_percpu(void __iomem *); if (WARN_ON(!gic->dist_base.percpu_base || !gic->cpu_base.percpu_base)) { - free_percpu(gic->dist_base.percpu_base); - free_percpu(gic->cpu_base.percpu_base); - return; + ret = -ENOMEM; + goto error; } for_each_possible_cpu(cpu) { @@ -1052,9 +1051,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, } gic_set_base_accessor(gic, gic_get_percpu_base); - } else -#endif - { /* Normal, sane GIC... */ + } else { + /* Normal, sane GIC... */ WARN(percpu_offset, "GIC_NON_BANKED not enabled, ignoring %08x offset!", percpu_offset); @@ -1104,8 +1102,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, hwirq_base, &gic_irq_domain_ops, gic); } - if (WARN_ON(!gic->domain)) - return; + if (WARN_ON(!gic->domain)) { + ret = -ENODEV; + goto error; + } if (gic_nr == 0) { /* @@ -1127,6 +1127,18 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, gic_dist_init(gic); gic_cpu_init(gic); gic_pm_init(gic); + + return 0; + +error: + if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) { + free_percpu(gic->dist_base.percpu_base); + free_percpu(gic->cpu_base.percpu_base); + } + + kfree(gic->chip.name); + + return ret; } void __init gic_init(unsigned int gic_nr, int irq_start, @@ -1187,7 +1199,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) void __iomem *cpu_base; void __iomem *dist_base; u32 percpu_offset; - int irq; + int irq, ret; if (WARN_ON(!node)) return -ENODEV; @@ -1212,8 +1224,14 @@ gic_of_init(struct device_node *node, struct device_node *parent) if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) percpu_offset = 0; - __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, + ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, &node->fwnode); + if (ret) { + iounmap(dist_base); + iounmap(cpu_base); + return ret; + } + if (!gic_cnt) gic_init_physaddr(node); @@ -1302,7 +1320,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, struct acpi_madt_generic_distributor *dist; void __iomem *cpu_base, *dist_base; struct fwnode_handle *domain_handle; - int count; + int count, ret; /* Collect CPU base addresses */ count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, @@ -1345,7 +1363,14 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, return -ENOMEM; } - __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); + ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); + if (ret) { + pr_err("Failed to initialise GIC\n"); + irq_domain_free_fwnode(domain_handle); + iounmap(cpu_base); + iounmap(dist_base); + return ret; + } acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
If the GIC initialisation fails, then currently we do not return an error or clean-up afterwards. Although for root controllers, this failure may be fatal anyway, for secondary controllers, it may not be fatal and so return an error on failure and clean-up. For non-banked GIC controllers, make sure that we free any memory allocated if we fail to initialise the IRQ domain. Please note that free_percpu() only frees memory if the pointer passed to it is not NULL and so it is unnecessary to check if both pointers are valid or not. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/irqchip/irq-gic.c | 57 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 16 deletions(-)