* Re: [PATCH v2] irqchip: irq-mips-gic:- Avoiding Kernel panics due to Error.
[not found] <1484546822-10199-1-git-send-email-arvind.yadav.cs@gmail.com>
@ 2017-01-17 8:38 ` Thomas Gleixner
2017-01-17 9:45 ` Matt Redfearn
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Gleixner @ 2017-01-17 8:38 UTC (permalink / raw)
To: Arvind Yadav; +Cc: Jason Cooper, Marc Zyngier, LKML, Paul Burton, linux-mips
On Mon, 16 Jan 2017, Arvind Yadav wrote:
Cc'ing the MIPS folks.
> Eliminating kernel panic due to NULL pointer dereferencing and
> other failure in __gic_init.
> Here, __gic_init can fail. This error check will avoid NULL pointer
> dereference and kernel panic.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> drivers/irqchip/irq-mips-gic.c | 40 +++++++++++++++++++++++++++++++---------
> 1 file changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index c01c09e..bf0816f 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -968,7 +968,7 @@ int gic_ipi_domain_match(struct irq_domain *d, struct device_node *node,
> .match = gic_ipi_domain_match,
> };
>
> -static void __init __gic_init(unsigned long gic_base_addr,
> +static int __init __gic_init(unsigned long gic_base_addr,
> unsigned long gic_addrspace_size,
> unsigned int cpu_vec, unsigned int irqbase,
> struct device_node *node)
> @@ -979,6 +979,10 @@ static void __init __gic_init(unsigned long gic_base_addr,
> __gic_base_addr = gic_base_addr;
>
> gic_base = ioremap_nocache(gic_base_addr, gic_addrspace_size);
> + if (!gic_base) {
> + pr_err("Failed to map GIC memory");
> + return -ENOMEM;
> + }
>
> gicconfig = gic_read(GIC_REG(SHARED, GIC_SH_CONFIG));
> gic_shared_intrs = (gicconfig & GIC_SH_CONFIG_NUMINTRS_MSK) >>
> @@ -1035,23 +1039,29 @@ static void __init __gic_init(unsigned long gic_base_addr,
> gic_irq_domain = irq_domain_add_simple(node, GIC_NUM_LOCAL_INTRS +
> gic_shared_intrs, irqbase,
> &gic_irq_domain_ops, NULL);
> - if (!gic_irq_domain)
> - panic("Failed to add GIC IRQ domain");
> + if (!gic_irq_domain) {
> + pr_err("Failed to add GIC IRQ domain");
> + goto iounmap;
> + }
> gic_irq_domain->name = "mips-gic-irq";
>
> gic_dev_domain = irq_domain_add_hierarchy(gic_irq_domain, 0,
> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
> node, &gic_dev_domain_ops, NULL);
> - if (!gic_dev_domain)
> - panic("Failed to add GIC DEV domain");
> + if (!gic_dev_domain) {
> + pr_err("Failed to add GIC DEV domain");
> + goto iounmap;
> + }
> gic_dev_domain->name = "mips-gic-dev";
>
> gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain,
> IRQ_DOMAIN_FLAG_IPI_PER_CPU,
> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
> node, &gic_ipi_domain_ops, NULL);
> - if (!gic_ipi_domain)
> - panic("Failed to add GIC IPI domain");
> + if (!gic_ipi_domain) {
> + pr_err("Failed to add GIC IPI domain");
> + goto iounmap;
> + }
>
> gic_ipi_domain->name = "mips-gic-ipi";
> gic_ipi_domain->bus_token = DOMAIN_BUS_IPI;
> @@ -1067,13 +1077,22 @@ static void __init __gic_init(unsigned long gic_base_addr,
> }
>
> gic_basic_init();
> + return 0;
> +iounmap:
> + iounmap(gic_base);
> + return -ENOMEM;
> }
>
> void __init gic_init(unsigned long gic_base_addr,
> unsigned long gic_addrspace_size,
> unsigned int cpu_vec, unsigned int irqbase)
> {
> - __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase, NULL);
> + int ret;
> +
> + ret = __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase,
> + NULL);
> + if (ret)
> + pr_err("Failed to initialize GIC");
> }
>
> static int __init gic_of_init(struct device_node *node,
> @@ -1083,6 +1102,7 @@ static int __init gic_of_init(struct device_node *node,
> unsigned int cpu_vec, i = 0, reserved = 0;
> phys_addr_t gic_base;
> size_t gic_len;
> + int ret;
>
> /* Find the first available CPU vector. */
> while (!of_property_read_u32_index(node, "mti,reserved-cpu-vectors",
> @@ -1119,7 +1139,9 @@ static int __init gic_of_init(struct device_node *node,
> write_gcr_gic_base(gic_base | CM_GCR_GIC_BASE_GICEN_MSK);
> gic_present = true;
>
> - __gic_init(gic_base, gic_len, cpu_vec, 0, node);
> + ret = __gic_init(gic_base, gic_len, cpu_vec, 0, node);
> + if (ret)
> + return ret;
>
> return 0;
> }
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] irqchip: irq-mips-gic:- Avoiding Kernel panics due to Error.
2017-01-17 8:38 ` [PATCH v2] irqchip: irq-mips-gic:- Avoiding Kernel panics due to Error Thomas Gleixner
@ 2017-01-17 9:45 ` Matt Redfearn
2017-01-17 9:45 ` Matt Redfearn
0 siblings, 1 reply; 3+ messages in thread
From: Matt Redfearn @ 2017-01-17 9:45 UTC (permalink / raw)
To: Thomas Gleixner, Arvind Yadav
Cc: Jason Cooper, Marc Zyngier, LKML, Paul Burton, linux-mips
On 17/01/17 08:38, Thomas Gleixner wrote:
> On Mon, 16 Jan 2017, Arvind Yadav wrote:
>
> Cc'ing the MIPS folks.
>
>> Eliminating kernel panic due to NULL pointer dereferencing and
>> other failure in __gic_init.
>> Here, __gic_init can fail. This error check will avoid NULL pointer
>> dereference and kernel panic.
Hi Aravind,
While panicing due to a null dereference is not ideal, I don't think
simply printing an error an attempting to continue on without the GIC is
the way forward. If the GIC fails to initialize, there will be no
interrupts, from devices, timers, IPI's etc, in other words the system
will be completely broken.
The driver should panic since the system will not be usable if the set
up fails.
Your v1 https://lkml.org/lkml/2017/1/9/106 would be fine.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>> drivers/irqchip/irq-mips-gic.c | 40 +++++++++++++++++++++++++++++++---------
>> 1 file changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
>> index c01c09e..bf0816f 100644
>> --- a/drivers/irqchip/irq-mips-gic.c
>> +++ b/drivers/irqchip/irq-mips-gic.c
>> @@ -968,7 +968,7 @@ int gic_ipi_domain_match(struct irq_domain *d, struct device_node *node,
>> .match = gic_ipi_domain_match,
>> };
>>
>> -static void __init __gic_init(unsigned long gic_base_addr,
>> +static int __init __gic_init(unsigned long gic_base_addr,
>> unsigned long gic_addrspace_size,
>> unsigned int cpu_vec, unsigned int irqbase,
>> struct device_node *node)
>> @@ -979,6 +979,10 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> __gic_base_addr = gic_base_addr;
>>
>> gic_base = ioremap_nocache(gic_base_addr, gic_addrspace_size);
>> + if (!gic_base) {
>> + pr_err("Failed to map GIC memory");
Just panic here and drop the rest.
Thanks,
Matt
>> + return -ENOMEM;
>> + }
>>
>> gicconfig = gic_read(GIC_REG(SHARED, GIC_SH_CONFIG));
>> gic_shared_intrs = (gicconfig & GIC_SH_CONFIG_NUMINTRS_MSK) >>
>> @@ -1035,23 +1039,29 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> gic_irq_domain = irq_domain_add_simple(node, GIC_NUM_LOCAL_INTRS +
>> gic_shared_intrs, irqbase,
>> &gic_irq_domain_ops, NULL);
>> - if (!gic_irq_domain)
>> - panic("Failed to add GIC IRQ domain");
>> + if (!gic_irq_domain) {
>> + pr_err("Failed to add GIC IRQ domain");
>> + goto iounmap;
>> + }
>> gic_irq_domain->name = "mips-gic-irq";
>>
>> gic_dev_domain = irq_domain_add_hierarchy(gic_irq_domain, 0,
>> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
>> node, &gic_dev_domain_ops, NULL);
>> - if (!gic_dev_domain)
>> - panic("Failed to add GIC DEV domain");
>> + if (!gic_dev_domain) {
>> + pr_err("Failed to add GIC DEV domain");
>> + goto iounmap;
>> + }
>> gic_dev_domain->name = "mips-gic-dev";
>>
>> gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain,
>> IRQ_DOMAIN_FLAG_IPI_PER_CPU,
>> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
>> node, &gic_ipi_domain_ops, NULL);
>> - if (!gic_ipi_domain)
>> - panic("Failed to add GIC IPI domain");
>> + if (!gic_ipi_domain) {
>> + pr_err("Failed to add GIC IPI domain");
>> + goto iounmap;
>> + }
>>
>> gic_ipi_domain->name = "mips-gic-ipi";
>> gic_ipi_domain->bus_token = DOMAIN_BUS_IPI;
>> @@ -1067,13 +1077,22 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> }
>>
>> gic_basic_init();
>> + return 0;
>> +iounmap:
>> + iounmap(gic_base);
>> + return -ENOMEM;
>> }
>>
>> void __init gic_init(unsigned long gic_base_addr,
>> unsigned long gic_addrspace_size,
>> unsigned int cpu_vec, unsigned int irqbase)
>> {
>> - __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase, NULL);
>> + int ret;
>> +
>> + ret = __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase,
>> + NULL);
>> + if (ret)
>> + pr_err("Failed to initialize GIC");
>> }
>>
>> static int __init gic_of_init(struct device_node *node,
>> @@ -1083,6 +1102,7 @@ static int __init gic_of_init(struct device_node *node,
>> unsigned int cpu_vec, i = 0, reserved = 0;
>> phys_addr_t gic_base;
>> size_t gic_len;
>> + int ret;
>>
>> /* Find the first available CPU vector. */
>> while (!of_property_read_u32_index(node, "mti,reserved-cpu-vectors",
>> @@ -1119,7 +1139,9 @@ static int __init gic_of_init(struct device_node *node,
>> write_gcr_gic_base(gic_base | CM_GCR_GIC_BASE_GICEN_MSK);
>> gic_present = true;
>>
>> - __gic_init(gic_base, gic_len, cpu_vec, 0, node);
>> + ret = __gic_init(gic_base, gic_len, cpu_vec, 0, node);
>> + if (ret)
>> + return ret;
>>
>> return 0;
>> }
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] irqchip: irq-mips-gic:- Avoiding Kernel panics due to Error.
2017-01-17 9:45 ` Matt Redfearn
@ 2017-01-17 9:45 ` Matt Redfearn
0 siblings, 0 replies; 3+ messages in thread
From: Matt Redfearn @ 2017-01-17 9:45 UTC (permalink / raw)
To: Thomas Gleixner, Arvind Yadav
Cc: Jason Cooper, Marc Zyngier, LKML, Paul Burton, linux-mips
On 17/01/17 08:38, Thomas Gleixner wrote:
> On Mon, 16 Jan 2017, Arvind Yadav wrote:
>
> Cc'ing the MIPS folks.
>
>> Eliminating kernel panic due to NULL pointer dereferencing and
>> other failure in __gic_init.
>> Here, __gic_init can fail. This error check will avoid NULL pointer
>> dereference and kernel panic.
Hi Aravind,
While panicing due to a null dereference is not ideal, I don't think
simply printing an error an attempting to continue on without the GIC is
the way forward. If the GIC fails to initialize, there will be no
interrupts, from devices, timers, IPI's etc, in other words the system
will be completely broken.
The driver should panic since the system will not be usable if the set
up fails.
Your v1 https://lkml.org/lkml/2017/1/9/106 would be fine.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>> drivers/irqchip/irq-mips-gic.c | 40 +++++++++++++++++++++++++++++++---------
>> 1 file changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
>> index c01c09e..bf0816f 100644
>> --- a/drivers/irqchip/irq-mips-gic.c
>> +++ b/drivers/irqchip/irq-mips-gic.c
>> @@ -968,7 +968,7 @@ int gic_ipi_domain_match(struct irq_domain *d, struct device_node *node,
>> .match = gic_ipi_domain_match,
>> };
>>
>> -static void __init __gic_init(unsigned long gic_base_addr,
>> +static int __init __gic_init(unsigned long gic_base_addr,
>> unsigned long gic_addrspace_size,
>> unsigned int cpu_vec, unsigned int irqbase,
>> struct device_node *node)
>> @@ -979,6 +979,10 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> __gic_base_addr = gic_base_addr;
>>
>> gic_base = ioremap_nocache(gic_base_addr, gic_addrspace_size);
>> + if (!gic_base) {
>> + pr_err("Failed to map GIC memory");
Just panic here and drop the rest.
Thanks,
Matt
>> + return -ENOMEM;
>> + }
>>
>> gicconfig = gic_read(GIC_REG(SHARED, GIC_SH_CONFIG));
>> gic_shared_intrs = (gicconfig & GIC_SH_CONFIG_NUMINTRS_MSK) >>
>> @@ -1035,23 +1039,29 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> gic_irq_domain = irq_domain_add_simple(node, GIC_NUM_LOCAL_INTRS +
>> gic_shared_intrs, irqbase,
>> &gic_irq_domain_ops, NULL);
>> - if (!gic_irq_domain)
>> - panic("Failed to add GIC IRQ domain");
>> + if (!gic_irq_domain) {
>> + pr_err("Failed to add GIC IRQ domain");
>> + goto iounmap;
>> + }
>> gic_irq_domain->name = "mips-gic-irq";
>>
>> gic_dev_domain = irq_domain_add_hierarchy(gic_irq_domain, 0,
>> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
>> node, &gic_dev_domain_ops, NULL);
>> - if (!gic_dev_domain)
>> - panic("Failed to add GIC DEV domain");
>> + if (!gic_dev_domain) {
>> + pr_err("Failed to add GIC DEV domain");
>> + goto iounmap;
>> + }
>> gic_dev_domain->name = "mips-gic-dev";
>>
>> gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain,
>> IRQ_DOMAIN_FLAG_IPI_PER_CPU,
>> GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
>> node, &gic_ipi_domain_ops, NULL);
>> - if (!gic_ipi_domain)
>> - panic("Failed to add GIC IPI domain");
>> + if (!gic_ipi_domain) {
>> + pr_err("Failed to add GIC IPI domain");
>> + goto iounmap;
>> + }
>>
>> gic_ipi_domain->name = "mips-gic-ipi";
>> gic_ipi_domain->bus_token = DOMAIN_BUS_IPI;
>> @@ -1067,13 +1077,22 @@ static void __init __gic_init(unsigned long gic_base_addr,
>> }
>>
>> gic_basic_init();
>> + return 0;
>> +iounmap:
>> + iounmap(gic_base);
>> + return -ENOMEM;
>> }
>>
>> void __init gic_init(unsigned long gic_base_addr,
>> unsigned long gic_addrspace_size,
>> unsigned int cpu_vec, unsigned int irqbase)
>> {
>> - __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase, NULL);
>> + int ret;
>> +
>> + ret = __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase,
>> + NULL);
>> + if (ret)
>> + pr_err("Failed to initialize GIC");
>> }
>>
>> static int __init gic_of_init(struct device_node *node,
>> @@ -1083,6 +1102,7 @@ static int __init gic_of_init(struct device_node *node,
>> unsigned int cpu_vec, i = 0, reserved = 0;
>> phys_addr_t gic_base;
>> size_t gic_len;
>> + int ret;
>>
>> /* Find the first available CPU vector. */
>> while (!of_property_read_u32_index(node, "mti,reserved-cpu-vectors",
>> @@ -1119,7 +1139,9 @@ static int __init gic_of_init(struct device_node *node,
>> write_gcr_gic_base(gic_base | CM_GCR_GIC_BASE_GICEN_MSK);
>> gic_present = true;
>>
>> - __gic_init(gic_base, gic_len, cpu_vec, 0, node);
>> + ret = __gic_init(gic_base, gic_len, cpu_vec, 0, node);
>> + if (ret)
>> + return ret;
>>
>> return 0;
>> }
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-01-17 9:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1484546822-10199-1-git-send-email-arvind.yadav.cs@gmail.com>
2017-01-17 8:38 ` [PATCH v2] irqchip: irq-mips-gic:- Avoiding Kernel panics due to Error Thomas Gleixner
2017-01-17 9:45 ` Matt Redfearn
2017-01-17 9:45 ` Matt Redfearn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).