* [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs
@ 2019-03-11 14:52 Liu Xiang
2019-03-11 15:55 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Liu Xiang @ 2019-03-11 14:52 UTC (permalink / raw)
To: tglx; +Cc: jason, marc.zyngier, linux-kernel, liuxiang_1999, Liu Xiang
For secondary GICs, the start irq number should skip over SGIs
and PPIs. Its value should be 32. So we should pass hwirq_base to
irq_alloc_descs() rather than a constant number 16.
Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
---
drivers/irqchip/irq-gic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ba2a37a..351f576 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1157,7 +1157,7 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
- irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
+ irq_base = irq_alloc_descs(irq_start, hwirq_base, gic_irqs,
numa_node_id());
if (irq_base < 0) {
WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs
2019-03-11 14:52 [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs Liu Xiang
@ 2019-03-11 15:55 ` Marc Zyngier
2019-03-12 13:59 ` Liu Xiang
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2019-03-11 15:55 UTC (permalink / raw)
To: Liu Xiang, tglx; +Cc: jason, linux-kernel, liuxiang_1999
On 11/03/2019 14:52, Liu Xiang wrote:
> For secondary GICs, the start irq number should skip over SGIs
> and PPIs. Its value should be 32. So we should pass hwirq_base to
> irq_alloc_descs() rather than a constant number 16.
>
> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
> ---
> drivers/irqchip/irq-gic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index ba2a37a..351f576 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1157,7 +1157,7 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
>
> gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>
> - irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
> + irq_base = irq_alloc_descs(irq_start, hwirq_base, gic_irqs,
> numa_node_id());
> if (irq_base < 0) {
> WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>
I suggest you look at __irq_alloc_descs(), and understand what the
various parameters mean. What you're doing here has absolutely no
effect.
The right thing to do would be to get rid of this altogether, except
that we have exactly *one* platform in the tree that is still non-DT
(some unmaintained Cavium piece of junk). But we can still simplify it,
as this guy doesn't have a secondary GIC (it is braindead enough).
What I'm suggesting instead is:
From b41fdc4a7bf9045e4871c5b15905ea732ffd044f Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 11 Mar 2019 15:38:10 +0000
Subject: [PATCH] irqchip/gic: Drop support for secondary GIC in non-DT systems
We do not have any in-tree platform with this pathological setup,
and only a single system (Cavium's cns3xxx) isn't DT aware.
Let's drop the secondary GIC support for now, until we remove
the above horror altogether.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/mach-cns3xxx/core.c | 2 +-
drivers/irqchip/irq-gic.c | 45 ++++++++++++---------------------
include/linux/irqchip/arm-gic.h | 3 +--
3 files changed, 18 insertions(+), 32 deletions(-)
diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c
index 7d5a44a06648..f676592d8402 100644
--- a/arch/arm/mach-cns3xxx/core.c
+++ b/arch/arm/mach-cns3xxx/core.c
@@ -90,7 +90,7 @@ void __init cns3xxx_map_io(void)
/* used by entry-macro.S */
void __init cns3xxx_init_irq(void)
{
- gic_init(0, 29, IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
+ gic_init(IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
IOMEM(CNS3XXX_TC11MP_GIC_CPU_BASE_VIRT));
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ba2a37a27a54..fd3110c171ba 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1089,11 +1089,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev,
#endif
}
-static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
+static int gic_init_bases(struct gic_chip_data *gic,
struct fwnode_handle *handle)
{
- irq_hw_number_t hwirq_base;
- int gic_irqs, irq_base, ret;
+ int gic_irqs, ret;
if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) {
/* Frankein-GIC without banked registers... */
@@ -1145,28 +1144,21 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
} else { /* Legacy support */
/*
* For primary GICs, skip over SGIs.
- * For secondary GICs, skip over PPIs, too.
+ * No secondary GIC support whatsoever.
*/
- if (gic == &gic_data[0] && (irq_start & 31) > 0) {
- hwirq_base = 16;
- if (irq_start != -1)
- irq_start = (irq_start & ~31) + 16;
- } else {
- hwirq_base = 32;
- }
+ int irq_base;
- gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
+ gic_irqs -= 16; /* calculate # of irqs to allocate */
- irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
+ irq_base = irq_alloc_descs(16, 16, gic_irqs,
numa_node_id());
if (irq_base < 0) {
- WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
- irq_start);
- irq_base = irq_start;
+ WARN(1, "Cannot allocate irq_descs @ IRQ16, assuming pre-allocated\n");
+ irq_base = 16;
}
gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base,
- hwirq_base, &gic_irq_domain_ops, gic);
+ 16, &gic_irq_domain_ops, gic);
}
if (WARN_ON(!gic->domain)) {
@@ -1195,7 +1187,6 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
}
static int __init __gic_init_bases(struct gic_chip_data *gic,
- int irq_start,
struct fwnode_handle *handle)
{
char *name;
@@ -1231,32 +1222,28 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
gic_init_chip(gic, NULL, name, false);
}
- ret = gic_init_bases(gic, irq_start, handle);
+ ret = gic_init_bases(gic, handle);
if (ret)
kfree(name);
return ret;
}
-void __init gic_init(unsigned int gic_nr, int irq_start,
- void __iomem *dist_base, void __iomem *cpu_base)
+void __init gic_init(void __iomem *dist_base, void __iomem *cpu_base)
{
struct gic_chip_data *gic;
- if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
- return;
-
/*
* Non-DT/ACPI systems won't run a hypervisor, so let's not
* bother with these...
*/
static_branch_disable(&supports_deactivate_key);
- gic = &gic_data[gic_nr];
+ gic = &gic_data[0];
gic->raw_dist_base = dist_base;
gic->raw_cpu_base = cpu_base;
- __gic_init_bases(gic, irq_start, NULL);
+ __gic_init_bases(gic, NULL);
}
static void gic_teardown(struct gic_chip_data *gic)
@@ -1399,7 +1386,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
if (ret)
return ret;
- ret = gic_init_bases(*gic, -1, &dev->of_node->fwnode);
+ ret = gic_init_bases(*gic, &dev->of_node->fwnode);
if (ret) {
gic_teardown(*gic);
return ret;
@@ -1459,7 +1446,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base))
static_branch_disable(&supports_deactivate_key);
- ret = __gic_init_bases(gic, -1, &node->fwnode);
+ ret = __gic_init_bases(gic, &node->fwnode);
if (ret) {
gic_teardown(gic);
return ret;
@@ -1650,7 +1637,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
return -ENOMEM;
}
- ret = __gic_init_bases(gic, -1, domain_handle);
+ ret = __gic_init_bases(gic, domain_handle);
if (ret) {
pr_err("Failed to initialise GIC\n");
irq_domain_free_fwnode(domain_handle);
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 626179077bb0..0f049b384ccd 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -158,8 +158,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq);
* Legacy platforms not converted to DT yet must use this to init
* their GIC
*/
-void gic_init(unsigned int nr, int start,
- void __iomem *dist , void __iomem *cpu);
+void gic_init(void __iomem *dist , void __iomem *cpu);
int gicv2m_init(struct fwnode_handle *parent_handle,
struct irq_domain *parent);
--
2.20.1
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re:Re: [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs
2019-03-11 15:55 ` Marc Zyngier
@ 2019-03-12 13:59 ` Liu Xiang
0 siblings, 0 replies; 3+ messages in thread
From: Liu Xiang @ 2019-03-12 13:59 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Liu Xiang, tglx, jason, linux-kernel
Hi, Marc
Thanks for your reply！
At 2019-03-11 23:55:11, "Marc Zyngier" <marc.zyngier@arm.com> wrote:
>On 11/03/2019 14:52, Liu Xiang wrote:
>> For secondary GICs, the start irq number should skip over SGIs
>> and PPIs. Its value should be 32. So we should pass hwirq_base to
>> irq_alloc_descs() rather than a constant number 16.
>>
>> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
>> ---
>> drivers/irqchip/irq-gic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index ba2a37a..351f576 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1157,7 +1157,7 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
>>
>> gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>>
>> - irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>> + irq_base = irq_alloc_descs(irq_start, hwirq_base, gic_irqs,
>> numa_node_id());
>> if (irq_base < 0) {
>> WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>>
>
>I suggest you look at __irq_alloc_descs(), and understand what the
>various parameters mean. What you're doing here has absolutely no
>effect.
>
>The right thing to do would be to get rid of this altogether, except
>that we have exactly *one* platform in the tree that is still non-DT
>(some unmaintained Cavium piece of junk). But we can still simplify it,
>as this guy doesn't have a secondary GIC (it is braindead enough).
>
>What I'm suggesting instead is:
>
>From b41fdc4a7bf9045e4871c5b15905ea732ffd044f Mon Sep 17 00:00:00 2001
>From: Marc Zyngier <marc.zyngier@arm.com>
>Date: Mon, 11 Mar 2019 15:38:10 +0000
>Subject: [PATCH] irqchip/gic: Drop support for secondary GIC in non-DT systems
>
>We do not have any in-tree platform with this pathological setup,
>and only a single system (Cavium's cns3xxx) isn't DT aware.
>
>Let's drop the secondary GIC support for now, until we remove
>the above horror altogether.
>
>Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>---
> arch/arm/mach-cns3xxx/core.c | 2 +-
> drivers/irqchip/irq-gic.c | 45 ++++++++++++---------------------
> include/linux/irqchip/arm-gic.h | 3 +--
> 3 files changed, 18 insertions(+), 32 deletions(-)
>
>diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c
>index 7d5a44a06648..f676592d8402 100644
>--- a/arch/arm/mach-cns3xxx/core.c
>+++ b/arch/arm/mach-cns3xxx/core.c
>@@ -90,7 +90,7 @@ void __init cns3xxx_map_io(void)
> /* used by entry-macro.S */
> void __init cns3xxx_init_irq(void)
> {
>- gic_init(0, 29, IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
>+ gic_init(IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
> IOMEM(CNS3XXX_TC11MP_GIC_CPU_BASE_VIRT));
> }
>
>diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>index ba2a37a27a54..fd3110c171ba 100644
>--- a/drivers/irqchip/irq-gic.c
>+++ b/drivers/irqchip/irq-gic.c
>@@ -1089,11 +1089,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev,
> #endif
> }
>
>-static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
>+static int gic_init_bases(struct gic_chip_data *gic,
> struct fwnode_handle *handle)
> {
>- irq_hw_number_t hwirq_base;
>- int gic_irqs, irq_base, ret;
>+ int gic_irqs, ret;
>
> if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) {
> /* Frankein-GIC without banked registers... */
>@@ -1145,28 +1144,21 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
> } else { /* Legacy support */
> /*
> * For primary GICs, skip over SGIs.
>- * For secondary GICs, skip over PPIs, too.
>+ * No secondary GIC support whatsoever.
> */
>- if (gic == &gic_data[0] && (irq_start & 31) > 0) {
>- hwirq_base = 16;
>- if (irq_start != -1)
>- irq_start = (irq_start & ~31) + 16;
>- } else {
>- hwirq_base = 32;
>- }
>+ int irq_base;
>
>- gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>+ gic_irqs -= 16; /* calculate # of irqs to allocate */
>
>- irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>+ irq_base = irq_alloc_descs(16, 16, gic_irqs,
> numa_node_id());
> if (irq_base < 0) {
>- WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>- irq_start);
>- irq_base = irq_start;
>+ WARN(1, "Cannot allocate irq_descs @ IRQ16, assuming pre-allocated\n");
>+ irq_base = 16;
> }
>
> gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base,
>- hwirq_base, &gic_irq_domain_ops, gic);
>+ 16, &gic_irq_domain_ops, gic);
> }
>
> if (WARN_ON(!gic->domain)) {
>@@ -1195,7 +1187,6 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
> }
>
> static int __init __gic_init_bases(struct gic_chip_data *gic,
>- int irq_start,
> struct fwnode_handle *handle)
> {
> char *name;
>@@ -1231,32 +1222,28 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
> gic_init_chip(gic, NULL, name, false);
> }
>
>- ret = gic_init_bases(gic, irq_start, handle);
>+ ret = gic_init_bases(gic, handle);
> if (ret)
> kfree(name);
>
> return ret;
> }
>
>-void __init gic_init(unsigned int gic_nr, int irq_start,
>- void __iomem *dist_base, void __iomem *cpu_base)
>+void __init gic_init(void __iomem *dist_base, void __iomem *cpu_base)
> {
> struct gic_chip_data *gic;
>
>- if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
>- return;
>-
> /*
> * Non-DT/ACPI systems won't run a hypervisor, so let's not
> * bother with these...
> */
> static_branch_disable(&supports_deactivate_key);
>
>- gic = &gic_data[gic_nr];
>+ gic = &gic_data[0];
> gic->raw_dist_base = dist_base;
> gic->raw_cpu_base = cpu_base;
>
>- __gic_init_bases(gic, irq_start, NULL);
>+ __gic_init_bases(gic, NULL);
> }
>
> static void gic_teardown(struct gic_chip_data *gic)
>@@ -1399,7 +1386,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
> if (ret)
> return ret;
>
>- ret = gic_init_bases(*gic, -1, &dev->of_node->fwnode);
>+ ret = gic_init_bases(*gic, &dev->of_node->fwnode);
> if (ret) {
> gic_teardown(*gic);
> return ret;
>@@ -1459,7 +1446,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
> if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base))
> static_branch_disable(&supports_deactivate_key);
>
>- ret = __gic_init_bases(gic, -1, &node->fwnode);
>+ ret = __gic_init_bases(gic, &node->fwnode);
> if (ret) {
> gic_teardown(gic);
> return ret;
>@@ -1650,7 +1637,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> return -ENOMEM;
> }
>
>- ret = __gic_init_bases(gic, -1, domain_handle);
>+ ret = __gic_init_bases(gic, domain_handle);
> if (ret) {
> pr_err("Failed to initialise GIC\n");
> irq_domain_free_fwnode(domain_handle);
>diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
>index 626179077bb0..0f049b384ccd 100644
>--- a/include/linux/irqchip/arm-gic.h
>+++ b/include/linux/irqchip/arm-gic.h
>@@ -158,8 +158,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq);
> * Legacy platforms not converted to DT yet must use this to init
> * their GIC
> */
>-void gic_init(unsigned int nr, int start,
>- void __iomem *dist , void __iomem *cpu);
>+void gic_init(void __iomem *dist , void __iomem *cpu);
>
> int gicv2m_init(struct fwnode_handle *parent_handle,
> struct irq_domain *parent);
>--
>2.20.1
>
>Thanks,
>
> M.
>--
>Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-12 13:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-11 14:52 [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs Liu Xiang
2019-03-11 15:55 ` Marc Zyngier
2019-03-12 13:59 ` Liu Xiang
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).