irqchip: gic: Don't complain in gic_get_cpumask() if UP system

PATCH In review Stephen Boyd 5 participants 15 messages Jul 5, 2013 – Aug 23, 2013

Subsystems (2)

irqchip drivers Arm generic interrupt controller drivers

lore

Share & tools

lore

Stephen Boyd

author

2013-07-05 23:39 UTC #0 lore raw

PATCH 1/1 irqchip: gic: Don't complain in gic_get_cpumask() if UP system

In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying

GIC CPU mask not found - kernel will fail to boot.

if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Skip this check if we're running a UP kernel or if we detect that the hardware only supports a single processor.

Cc Nicolas Pitre <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

Maybe we should just drop the check entirely? It looks like it may just be debug code that won't ever trigger in practice, even on the 11MPCore that caused this code to be introduced.

 drivers/irqchip/irq-gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic.c +1 -1

index 19ceaa6..589c760 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Javi Merino

2013-07-12 11:13 UTC #1 lore raw

Quoted 11 lines · Stephen Boyd

In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying
GIC CPU mask not found - kernel will fail to boot.
if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Skip this check if we're running a UP kernel or if we detect that the hardware only supports a single processor.

Quoted 8 lines

Cc Nicolas Pitre <[email protected]>
Cc Russell King <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Maybe we should just drop the check entirely? It looks like it may just be debug code that won't ever trigger in practice, even on the 11MPCore that caused this code to be introduced.

I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.

Stephen Boyd

author

2013-07-12 12:10 UTC #2 lore raw

Quoted 17 lines · Javi Merino

Quoted 16 lines · Stephen Boyd
In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying
GIC CPU mask not found - kernel will fail to boot.
if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Skip this check if we're running a UP kernel or if we detect that the hardware only supports a single processor.
Cc Nicolas Pitre <[email protected]>
Cc Russell King <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---

Quoted 8 lines

Quoted 3 lines
Maybe we should just drop the check entirely? It looks like it may just be debug code that won't ever trigger in practice, even on the 11MPCore that caused this code to be introduced.
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.

Ok. How about this?

----8<-----
Subject: [PATCH v2] irqchip: gic: Don't complain in gic_get_cpumask()

In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying

GIC CPU mask not found - kernel will fail to boot.

if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Remove this check because if you're on a multiprocessor system it's unlikely to be printed unless your DT is wrong or your hardware is broken.

Cc Nicolas Pitre <[email protected]>

Cc Russell King <[email protected]>

Cc Javi Merino <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/irqchip/irq-gic.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/irqchip/irq-gic.c +0 -3

index 19ceaa60..4fbcea9 100644
@@ -368,9 +368,6 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
-		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
-
 	return mask;
 }

Signature · 8 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2013-07-17 21:53 UTC #3 lore raw

Quoted 6 lines · Stephen Boyd

Quoted 4 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
Ok. How about this?

Any comments?

Quoted 38 lines

----8<-----
Subject: [PATCH v2] irqchip: gic: Don't complain in gic_get_cpumask()
In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying
GIC CPU mask not found - kernel will fail to boot.
if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Remove this check because if you're on a multiprocessor system it's unlikely to be printed unless your DT is wrong or your hardware is broken.
Cc Nicolas Pitre <[email protected]>
Cc Russell King <[email protected]>
Cc Javi Merino <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/irqchip/irq-gic.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/irqchip/irq-gic.c +0 -3
index 19ceaa60..4fbcea9 100644
@@ -368,9 +368,6 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
-		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
-
 	return mask;
 }
 

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Nicolas Pitre

2013-07-17 22:34 UTC #4 lore raw

Quoted 5 lines · Stephen Boyd

Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.

And that's a likely outcome in the real world.

Quoted 4 lines

Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?

What about this instead:

drivers/irqchip/irq-gic.c +1 -1

index 19ceaa60e0..86d21bc6cb 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (is_smp() && !mask)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;

Nicolas

Stephen Boyd

author

2013-07-17 22:36 UTC #5 lore raw

Quoted 4 lines · Nicolas Pitre

Quoted 2 lines · Stephen Boyd
Quoted 1 line · Stephen Boyd
On 07/12, Javi Merino wrote:

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:

Unfortunately arm64 doesn't have SMP_ON_UP. It sounds like you preferred the first patch using num_possible_cpus()

Quoted 17 lines

drivers/irqchip/irq-gic.c +1 -1

index 19ceaa60e0..86d21bc6cb 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (is_smp() && !mask)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;

Nicolas

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Nicolas Pitre

2013-07-17 22:53 UTC #6 lore raw

Quoted 8 lines · Stephen Boyd

Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.

Quoted 7 lines

Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.

And why does that matter?

Quoted 1 line

It sounds like you preferred the first patch using num_possible_cpus()

Probably, yes. I didn't follow the early conversation though.

Nicolas

Stephen Boyd

author

2013-07-17 23:06 UTC #7 lore raw

Quoted 10 lines · Nicolas Pitre

Quoted 8 lines · Stephen Boyd
Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.

Quoted 7 lines

Quoted 6 lines
Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.
And why does that matter?

Because the gic driver is compiled on both arm and arm64? I suppose we could define is_smp() to 1 on arm64 but its probably better to rely on generic kernel things instead of arch specific functions.

Quoted 3 lines

Quoted 2 lines
It sounds like you preferred the first patch using num_possible_cpus()
Probably, yes. I didn't follow the early conversation though.

This was the first patch:

---8<----

drivers/irqchip/irq-gic.c +1 -1

index 19ceaa6..589c760 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2013-08-22 18:43 UTC #8 lore raw

Quoted 36 lines · Stephen Boyd

Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Stephen Boyd
Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.
And why does that matter?
Because the gic driver is compiled on both arm and arm64? I suppose we could define is_smp() to 1 on arm64 but its probably better to rely on generic kernel things instead of arch specific functions.
Quoted 3 lines
Quoted 2 lines
It sounds like you preferred the first patch using num_possible_cpus()
Probably, yes. I didn't follow the early conversation though.
This was the first patch:
---8<----
drivers/irqchip/irq-gic.c +0 -0
index 19ceaa6..589c760 100644

drivers/irqchip/irq-gic.c static u8 gic_get_cpumask(struct gic_chip_data *gic) · line 374

Quoted 8 lines

drivers/irqchip/irq-gic.c +1 -1

@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;

Can one of these two patches be picked up?

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Nicolas Pitre

2013-08-22 18:59 UTC #9 lore raw

Quoted 40 lines · Stephen Boyd

Quoted 39 lines · Stephen Boyd
Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Stephen Boyd
Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.
And why does that matter?
Because the gic driver is compiled on both arm and arm64? I suppose we could define is_smp() to 1 on arm64 but its probably better to rely on generic kernel things instead of arch specific functions.
Quoted 3 lines
Quoted 2 lines
It sounds like you preferred the first patch using num_possible_cpus()
Probably, yes. I didn't follow the early conversation though.
This was the first patch:
---8<----
drivers/irqchip/irq-gic.c +0 -0
index 19ceaa6..589c760 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 

Quoted 7 lines

drivers/irqchip/irq-gic.c static u8 gic_get_cpumask(struct gic_chip_data *gic) · line 374
Quoted 5 lines
drivers/irqchip/irq-gic.c +1 -1
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;
Can one of these two patches be picked up?

Sure. Just send it to RMK's patch system with my ACK.

Nicolas

Stephen Boyd

author

2013-08-23 04:35 UTC #10 lore raw

Quoted 46 lines · Nicolas Pitre

Quoted 44 lines · Stephen Boyd
Quoted 43 lines · Stephen Boyd
Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Stephen Boyd
Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.
And why does that matter?
Because the gic driver is compiled on both arm and arm64? I suppose we could define is_smp() to 1 on arm64 but its probably better to rely on generic kernel things instead of arch specific functions.
Quoted 3 lines
Quoted 2 lines
It sounds like you preferred the first patch using num_possible_cpus()
Probably, yes. I didn't follow the early conversation though.
This was the first patch:
---8<----
drivers/irqchip/irq-gic.c +1 -1
index 19ceaa6..589c760 100644
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 

Quoted 6 lines

Quoted 3 lines
drivers/irqchip/irq-gic.c static u8 gic_get_cpumask(struct gic_chip_data *gic) · line 374
Quoted 1 line
drivers/irqchip/irq-gic.c +0 -0
 	return mask;
Can one of these two patches be picked up?
Sure. Just send it to RMK's patch system with my ACK.

I'm confused on that. MAINTAINERS says this patch should go through Thomas Gleixner's irq/core branch but it looks like only arm-soc has been taking patches for the current location.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Nicolas Pitre

2013-08-23 04:51 UTC #11 lore raw

Quoted 51 lines · Stephen Boyd

Quoted 50 lines · Nicolas Pitre
Quoted 47 lines · Stephen Boyd
Quoted 36 lines · Stephen Boyd
Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Stephen Boyd
Quoted 7 lines · Nicolas Pitre
Quoted 5 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd
Quoted 3 lines · Javi Merino
I agree, we should drop the check. It's annoying in uniprocessors and unlikely to be found in the real world unless your gic entry in the dt is wrong.
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
And that's a likely outcome in the real world.
Quoted 2 lines
Quoted 1 line
Ok. How about this?
Any comments?
What about this instead:
Unfortunately arm64 doesn't have SMP_ON_UP.
And why does that matter?
Because the gic driver is compiled on both arm and arm64? I suppose we could define is_smp() to 1 on arm64 but its probably better to rely on generic kernel things instead of arch specific functions.
Quoted 3 lines
Quoted 2 lines
It sounds like you preferred the first patch using num_possible_cpus()
Probably, yes. I didn't follow the early conversation though.
This was the first patch:
---8<----
drivers/irqchip/irq-gic.c +0 -0
index 19ceaa6..589c760 100644
drivers/irqchip/irq-gic.c static u8 gic_get_cpumask(struct gic_chip_data *gic) · line 374
Quoted 8 lines
drivers/irqchip/irq-gic.c +1 -1
@@ -368,7 +368,7 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 			break;
 	}
 
-	if (!mask)
+	if (!mask && num_possible_cpus() > 1)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
 	return mask;
Can one of these two patches be picked up?

Quoted 6 lines

Quoted 2 lines
Sure. Just send it to RMK's patch system with my ACK.
I'm confused on that. MAINTAINERS says this patch should go through Thomas Gleixner's irq/core branch but it looks like only arm-soc has been taking patches for the current location.

Blah. OK then, just send it to Thomas.

Initially this code was written and committed by RMK which is why I suggested you send him the fix.

Nicolas

Russell King - ARM Linux

2013-08-23 09:15 UTC #12 lore raw

Quoted 4 lines · Nicolas Pitre

Blah. OK then, just send it to Thomas.
Initially this code was written and committed by RMK which is why I suggested you send him the fix.

It _should_, because the author of the file presumably knows how the driver is supposed to work much better than the maintainer of the subsystem. So driver authors _should_ always be involved in the handling of the patch.

Unfortunately, that rarely happens, and I've given up any hope of the old kernel process(es) remaining where authors were responsible for the code they wrote. Somehow, kernel maintanence has been perversed so that subsystem maintainers get to decide whether patches to drivers that they don't have hardware for are to be applied irrespective of whether the driver author has any input to it.

Bedia, Vaibhav

2013-07-20 07:27 UTC #13 lore raw

In reply to Stephen Boyd

Hi,

Quoted 11 lines · Stephen Boyd

In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying
GIC CPU mask not found - kernel will fail to boot.
if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Skip this check if we're running a UP kernel or if we detect that the hardware only supports a single processor.

Quoted 9 lines

Cc Nicolas Pitre <[email protected]>
Cc Russell King <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Maybe we should just drop the check entirely? It looks like it may just be debug code that won't ever trigger in practice, even on the 11MPCore that caused this code to be introduced.

Apologies if I missed v2 of this patch if there was one. I could not locate it in my inbox or the list archives.

We have a yet to be released single-core A9 MPCore system (AM437x) which needs to co-exist in omap2plus_defconfig wherein NR_CPUS is set to 2. This currently leads to the "GIC CPU mask not found..." being printed twice during bootup. If the check is really some debug code, can it be completely dropped?

Regards,
Vaibhav B.

Stephen Boyd

author

2013-07-23 17:01 UTC #14 lore raw

Quoted 24 lines · Vaibhav

Hi,
Quoted 11 lines · Stephen Boyd
In a uniprocessor implementation the interrupt processor targets registers are read-as-zero/write-ignored (RAZ/WI). Unfortunately gic_get_cpumask() will print a critical message saying
GIC CPU mask not found - kernel will fail to boot.
if these registers all read as zero, but there won't actually be a problem on uniprocessor systems and the kernel will boot just fine. Skip this check if we're running a UP kernel or if we detect that the hardware only supports a single processor.
Quoted 9 lines
Cc Nicolas Pitre <[email protected]>
Cc Russell King <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Maybe we should just drop the check entirely? It looks like it may just be debug code that won't ever trigger in practice, even on the 11MPCore that caused this code to be introduced.

Quoted 8 lines

Apologies if I missed v2 of this patch if there was one. I could not locate it in my inbox or the list archives.
We have a yet to be released single-core A9 MPCore system (AM437x) which needs to co-exist in omap2plus_defconfig wherein NR_CPUS is set to 2. This currently leads to the "GIC CPU mask not found..." being printed twice during bootup. If the check is really some debug code, can it be completely dropped?

I have sent both versions of the patch in hopes that one or the other will be accepted. Either way works for me.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation