Skip to contentSkip to conversation

genirq: Make - vs ?: precedence explicit

PATCH In review Kees Cook 4 participants 4 messages Nov 22, 2017 – Nov 28, 2017 IRQ

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 genirq: Make - vs ?: precedence explicit

Noticed with a Clang build. This improves the readability of the ?:
expression, as it has lower precedence than the - expression. Show explicitly that - is evaluated first.

Cc Thomas Gleixner <[email protected]>
---
 kernel/irq/matrix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/irq/matrix.c +1 -1
index a3cbbc8191c5..7df2480005f8 100644
@@ -384,7 +384,7 @@ unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown)
 {
 	struct cpumap *cm = this_cpu_ptr(m->maps);
 
-	return m->global_available - cpudown ? cm->available : 0;
+	return (m->global_available - cpudown) ? cm->available : 0;
 }
 
 /**
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw

[tip:irq/urgent] genirq/matrix: Make - vs ?: Precedence explicit

genirq/matrix: Make - vs ?: Precedence explicit
Commit-ID:  75f1133873d6a1276d3c19918b7c94975840f990
Gitweb:     https://git.kernel.org/tip/75f1133873d6a1276d3c19918b7c94975840f990
Author:     Kees Cook <[email protected]>
AuthorDate: Wed, 22 Nov 2017 12:56:45 -0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Thu, 23 Nov 2017 20:09:31 +0100

genirq/matrix: Make - vs ?: Precedence explicit

Noticed with a Clang build. This improves the readability of the ?:
expression, as it has lower precedence than the - expression. Show explicitly that - is evaluated first.

---
 kernel/irq/matrix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/irq/matrix.c +1 -1
index a3cbbc8..7df2480 100644
@@ -384,7 +384,7 @@ unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown)
 {
 	struct cpumap *cm = this_cpu_ptr(m->maps);
 
-	return m->global_available - cpudown ? cm->available : 0;
+	return (m->global_available - cpudown) ? cm->available : 0;
 }
 
 /**
#2 lore raw
In reply to Kees Cook
Quoted 15 lines · Kees Cook

Noticed with a Clang build. This improves the readability of the ?:
expression, as it has lower precedence than the - expression. Show explicitly that - is evaluated first.

Cc Thomas Gleixner <[email protected]>
---
 kernel/irq/matrix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/irq/matrix.c +0 -0
index a3cbbc8191c5..7df2480005f8 100644

kernel/irq/matrix.c unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown) · line 388

Quoted 6 lines
kernel/irq/matrix.c +1 -1
@@ -384,7 +384,7 @@ unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown)
 {
 	struct cpumap *cm = this_cpu_ptr(m->maps);
 
-	return m->global_available - cpudown ? cm->available : 0;
+	return (m->global_available - cpudown) ? cm->available : 0;
 }

I see that this got applied, and that doesn't change the semantics of the code. But surely the code is and was buggy, right? From the kernel doc, I'm pretty sure the idea is to subtract cm->available if cpudown is true, otherwise subtract 0, i.e.

  return m->global_available - (cpudown ? cm->available : 0);

Rasmus

#3 lore raw
Quoted 28 lines · Rasmus Villemoes
Quoted 15 lines · Kees Cook

Noticed with a Clang build. This improves the readability of the ?:
expression, as it has lower precedence than the - expression. Show explicitly that - is evaluated first.

Cc Thomas Gleixner <[email protected]>
---
 kernel/irq/matrix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/irq/matrix.c +0 -0
index a3cbbc8191c5..7df2480005f8 100644

kernel/irq/matrix.c unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown) · line 388

Quoted 6 lines
kernel/irq/matrix.c +1 -1
@@ -384,7 +384,7 @@ unsigned int irq_matrix_available(struct irq_matrix *m, bool cpudown)
 {
 	struct cpumap *cm = this_cpu_ptr(m->maps);
 
-	return m->global_available - cpudown ? cm->available : 0;
+	return (m->global_available - cpudown) ? cm->available : 0;
 }

I see that this got applied, and that doesn't change the semantics of the code. But surely the code is and was buggy, right? From the kernel doc, I'm pretty sure the idea is to subtract cm->available if cpudown is true, otherwise subtract 0, i.e.

Quoted 1 line
  return m->global_available - (cpudown ? cm->available : 0);

Yes, you are right. I completely misread it when I merged that patch. Good catch!

Thanks,

tglx