[PATCH 0/3] xics mask_irq fix and other patches

[PATCH 0/3] xics mask_irq fix and other patches

[Milton Miller]

Hi Ben.

I've submitted the fix for the xics mask real irq I mentioned.
During testing I realized current code is passing rtas a linux irq,
which means no interrupt (at least those above NR_IRQs) is being disabled.

I also created a patch to rename irq to hwirq in xics to try to make
the distiction more clear.  For your consideration, I don't care if you
don't merge it especially with the upcoming rewrite.

Also, I am sending a printk to printk_once patch that affects BML.

Thanks,
milton


MAIL FROM: <miltonm@bga.com>
RCPT TO: <miltonm@bga.com>
RCPT TO: <linuxppc-dev@lists.ozlabs.org>
RCPT TO: <buytenh@secretlab.ca>
DATA
From: Milton Miller <miltonm@bga.com>
Subject: [PATCH 1/2] powerpc: xics: fix numberspace mismatch from irq_desc conversion
To: linuxppc-dev@lists.ozlabs.org
Cc: Lennert Buytenhek <buytenh@secretlab.ca>
Message-id: <xics-mask-irq-desc-fix@mdm.bga.com>
In-Reply-To: <patches-for-39-rc1@mdm.bga.com>
References: <patches-for-39-rc1@mdm.bga.com>

commit 79f26c268ebad29bd75d078cfc09d3d82b30ccbd (powerpc:
platforms/pseries irq_data conversion) pushed irq_desc down into many
functions, dererencing the descriptor irq field as late as possible.

But it incorrectly passed a linix virtural irq number to RTAS,
resulting in the interrupt not being disabled and possibly
other bad things, such as another interrupt being disabled and/or
a checkstop.  

In addition this missed the point of xics_mask_unknown_vec and
the seperation of xics_mask_real_irq from xics_mask_irq.  When
xics_mask_unknown_vec is called it's because the hardware delivered an
irq source for which we have no linux irq allocated, and thefore we can
not have an irq_desc allocated.

Revert xics_mask_real_irq to its prior version, naming the argument
hwirq to highlight the difference.

Signed-off-by: Milton Miller <miltonm@bga.com>


--

Lennert can you please review the other patches for similar problems?

Any reference to irq_map[x].hwirq is in a different number domain.

I initially saw the problem pushing irq_desc through unknown_vec
found this due to my knowledge of the split (I added that function)
and only realized the wrong number space when preparing to test the fix.
In fact I tried without the patch and my firmware checkstops the machine
(hardware stops executing instructions)!


diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index 01fea46..5686db9 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -250,26 +250,26 @@ static unsigned int xics_startup(struct irq_data *d)
 	return 0;
 }
 
-static void xics_mask_real_irq(struct irq_data *d)
+static void xics_mask_real_irq(unsigned int hwirq)
 {
 	int call_status;
 
-	if (d->irq == XICS_IPI)
+	if (hwirq == XICS_IPI)
 		return;
 
-	call_status = rtas_call(ibm_int_off, 1, 1, NULL, d->irq);
+	call_status = rtas_call(ibm_int_off, 1, 1, NULL, hwirq);
 	if (call_status != 0) {
 		printk(KERN_ERR "%s: ibm_int_off irq=%u returned %d\n",
-			__func__, d->irq, call_status);
+			__func__, hwirq, call_status);
 		return;
 	}
 
 	/* Have to set XIVE to 0xff to be able to remove a slot */
-	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, d->irq,
+	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, hwirq,
 				default_server, 0xff);
 	if (call_status != 0) {
 		printk(KERN_ERR "%s: ibm_set_xive(0xff) irq=%u returned %d\n",
-			__func__, d->irq, call_status);
+			__func__, hwirq, call_status);
 		return;
 	}
 }
@@ -283,13 +283,13 @@ static void xics_mask_irq(struct irq_data *d)
 	irq = (unsigned int)irq_map[d->irq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
-	xics_mask_real_irq(d);
+	xics_mask_real_irq(irq);
 }
 
 static void xics_mask_unknown_vec(unsigned int vec)
 {
 	printk(KERN_ERR "Interrupt %u (real) is invalid, disabling it.\n", vec);
-	xics_mask_real_irq(irq_get_irq_data(vec));
+	xics_mask_real_irq(vec);
 }
 
 static inline unsigned int xics_xirr_vector(unsigned int xirr)

[PATCH] powerpc: pseries/smp: query-cpu-stopped-state support won't change

[Milton Miller]

If a given firmware doesn't have a token to support query-cpu-stopped-state,
its not likely to change during the lifetime of the kernel.   

Only print this information once, not once per secondary thread.

While here, make the line wrap grep friendly.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/platforms/pseries/smp.c
===================================================================
--- work.git.orig/arch/powerpc/platforms/pseries/smp.c	2011-03-21 11:27:54.000000000 -0500
+++ work.git/arch/powerpc/platforms/pseries/smp.c	2011-03-21 11:29:51.000000000 -0500
@@ -64,8 +64,8 @@ int smp_query_cpu_stopped(unsigned int p
 	int qcss_tok = rtas_token("query-cpu-stopped-state");
 
 	if (qcss_tok == RTAS_UNKNOWN_SERVICE) {
-		printk(KERN_INFO "Firmware doesn't support "
-				"query-cpu-stopped-state\n");
+		printk_once(KERN_INFO
+			"Firmware doesn't support query-cpu-stopped-state\n");
 		return QCSS_HARDWARE_ERROR;
 	}
 

[PATCH 2/2] powerpc: xics: use hwirq for xics domain irq number

[Milton Miller]

To try to avoid future confusion, rename irq to hwirq when it refers
to a xics domain number instead of a linux irq number.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: next.git/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- next.git.orig/arch/powerpc/platforms/pseries/xics.c	2011-03-19 23:21:21.775085058 -0500
+++ next.git/arch/powerpc/platforms/pseries/xics.c	2011-03-20 00:57:33.144560092 -0500
@@ -204,33 +204,33 @@ static int get_irq_server(unsigned int v
 
 static void xics_unmask_irq(struct irq_data *d)
 {
-	unsigned int irq;
+	unsigned int hwirq;
 	int call_status;
 	int server;
 
 	pr_devel("xics: unmask virq %d\n", d->irq);
 
-	irq = (unsigned int)irq_map[d->irq].hwirq;
-	pr_devel(" -> map to hwirq 0x%x\n", irq);
-	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
+	hwirq = (unsigned int)irq_map[d->irq].hwirq;
+	pr_devel(" -> map to hwirq 0x%x\n", hwirq);
+	if (hwirq == XICS_IPI || hwirq == XICS_IRQ_SPURIOUS)
 		return;
 
 	server = get_irq_server(d->irq, d->affinity, 0);
 
-	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
+	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, hwirq, server,
 				DEFAULT_PRIORITY);
 	if (call_status != 0) {
 		printk(KERN_ERR
 			"%s: ibm_set_xive irq %u server %x returned %d\n",
-			__func__, irq, server, call_status);
+			__func__, hwirq, server, call_status);
 		return;
 	}
 
 	/* Now unmask the interrupt (often a no-op) */
-	call_status = rtas_call(ibm_int_on, 1, 1, NULL, irq);
+	call_status = rtas_call(ibm_int_on, 1, 1, NULL, hwirq);
 	if (call_status != 0) {
 		printk(KERN_ERR "%s: ibm_int_on irq=%u returned %d\n",
-			__func__, irq, call_status);
+			__func__, hwirq, call_status);
 		return;
 	}
 }
@@ -276,14 +276,14 @@ static void xics_mask_real_irq(unsigned 
 
 static void xics_mask_irq(struct irq_data *d)
 {
-	unsigned int irq;
+	unsigned int hwirq;
 
 	pr_devel("xics: mask virq %d\n", d->irq);
 
-	irq = (unsigned int)irq_map[d->irq].hwirq;
-	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
+	hwirq = (unsigned int)irq_map[d->irq].hwirq;
+	if (hwirq == XICS_IPI || hwirq == XICS_IRQ_SPURIOUS)
 		return;
-	xics_mask_real_irq(irq);
+	xics_mask_real_irq(hwirq);
 }
 
 static void xics_mask_unknown_vec(unsigned int vec)
@@ -373,37 +373,37 @@ static unsigned char pop_cppr(void)
 
 static void xics_eoi_direct(struct irq_data *d)
 {
-	unsigned int irq = (unsigned int)irq_map[d->irq].hwirq;
+	unsigned int hwirq = (unsigned int)irq_map[d->irq].hwirq;
 
 	iosync();
-	direct_xirr_info_set((pop_cppr() << 24) | irq);
+	direct_xirr_info_set((pop_cppr() << 24) | hwirq);
 }
 
 static void xics_eoi_lpar(struct irq_data *d)
 {
-	unsigned int irq = (unsigned int)irq_map[d->irq].hwirq;
+	unsigned int hwirq = (unsigned int)irq_map[d->irq].hwirq;
 
 	iosync();
-	lpar_xirr_info_set((pop_cppr() << 24) | irq);
+	lpar_xirr_info_set((pop_cppr() << 24) | hwirq);
 }
 
 static int
 xics_set_affinity(struct irq_data *d, const struct cpumask *cpumask, bool force)
 {
-	unsigned int irq;
+	unsigned int hwirq;
 	int status;
 	int xics_status[2];
 	int irq_server;
 
-	irq = (unsigned int)irq_map[d->irq].hwirq;
-	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
+	hwirq = (unsigned int)irq_map[d->irq].hwirq;
+	if (hwirq == XICS_IPI || hwirq == XICS_IRQ_SPURIOUS)
 		return -1;
 
-	status = rtas_call(ibm_get_xive, 1, 3, xics_status, irq);
+	status = rtas_call(ibm_get_xive, 1, 3, xics_status, hwirq);
 
 	if (status) {
 		printk(KERN_ERR "%s: ibm,get-xive irq=%u returns %d\n",
-			__func__, irq, status);
+			__func__, hwirq, status);
 		return -1;
 	}
 
@@ -418,11 +418,11 @@ xics_set_affinity(struct irq_data *d, co
 	}
 
 	status = rtas_call(ibm_set_xive, 3, 1, NULL,
-				irq, irq_server, xics_status[1]);
+				hwirq, irq_server, xics_status[1]);
 
 	if (status) {
 		printk(KERN_ERR "%s: ibm,set-xive irq=%u returns %d\n",
-			__func__, irq, status);
+			__func__, hwirq, status);
 		return -1;
 	}
 
@@ -874,7 +874,7 @@ void xics_kexec_teardown_cpu(int seconda
 void xics_migrate_irqs_away(void)
 {
 	int cpu = smp_processor_id(), hw_cpu = hard_smp_processor_id();
-	unsigned int irq, virq;
+	int virq;
 
 	/* If we used to be the default server, move to the new "boot_cpuid" */
 	if (hw_cpu == default_server)
@@ -892,6 +892,7 @@ void xics_migrate_irqs_away(void)
 	for_each_irq(virq) {
 		struct irq_desc *desc;
 		struct irq_chip *chip;
+		unsigned int hwirq;
 		int xics_status[2];
 		int status;
 		unsigned long flags;
@@ -901,9 +902,9 @@ void xics_migrate_irqs_away(void)
 			continue;
 		if (irq_map[virq].host != xics_host)
 			continue;
-		irq = (unsigned int)irq_map[virq].hwirq;
+		hwirq = (unsigned int)irq_map[virq].hwirq;
 		/* We need to get IPIs still. */
-		if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
+		if (hwirq == XICS_IPI || hwirq == XICS_IRQ_SPURIOUS)
 			continue;
 
 		desc = irq_to_desc(virq);
@@ -918,10 +919,10 @@ void xics_migrate_irqs_away(void)
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
 
-		status = rtas_call(ibm_get_xive, 1, 3, xics_status, irq);
+		status = rtas_call(ibm_get_xive, 1, 3, xics_status, hwirq);
 		if (status) {
 			printk(KERN_ERR "%s: ibm,get-xive irq=%u returns %d\n",
-					__func__, irq, status);
+					__func__, hwirq, status);
 			goto unlock;
 		}
 

[PATCH 1/2] powerpc: xics: fix numberspace mismatch from irq_desc conversion

[Milton Miller]

commit 79f26c268ebad29bd75d078cfc09d3d82b30ccbd (powerpc:
platforms/pseries irq_data conversion) pushed irq_desc down into many
functions, dererencing the descriptor irq field as late as possible.

But it incorrectly passed a linix virtural irq number to RTAS,
resulting in the interrupt not being disabled and possibly
other bad things, such as another interrupt being disabled and/or
a checkstop.  

In addition this missed the point of xics_mask_unknown_vec and
the seperation of xics_mask_real_irq from xics_mask_irq.  When
xics_mask_unknown_vec is called it's because the hardware delivered an
irq source for which we have no linux irq allocated, and thefore we can
not have an irq_desc allocated.

Revert xics_mask_real_irq to its prior version, naming the argument
hwirq to highlight the difference.

Signed-off-by: Milton Miller <miltonm@bga.com>


--

Lennert can you please review the other patches for similar problems?

Any reference to irq_map[x].hwirq is in a different number domain.

I initially saw the problem pushing irq_desc through unknown_vec
found this due to my knowledge of the split (I added that function)
and only realized the wrong number space when preparing to test the fix.
In fact I tried without the patch and my firmware checkstops the machine
(hardware stops executing instructions)!


diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index 01fea46..5686db9 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -250,26 +250,26 @@ static unsigned int xics_startup(struct irq_data *d)
 	return 0;
 }
 
-static void xics_mask_real_irq(struct irq_data *d)
+static void xics_mask_real_irq(unsigned int hwirq)
 {
 	int call_status;
 
-	if (d->irq == XICS_IPI)
+	if (hwirq == XICS_IPI)
 		return;
 
-	call_status = rtas_call(ibm_int_off, 1, 1, NULL, d->irq);
+	call_status = rtas_call(ibm_int_off, 1, 1, NULL, hwirq);
 	if (call_status != 0) {
 		printk(KERN_ERR "%s: ibm_int_off irq=%u returned %d\n",
-			__func__, d->irq, call_status);
+			__func__, hwirq, call_status);
 		return;
 	}
 
 	/* Have to set XIVE to 0xff to be able to remove a slot */
-	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, d->irq,
+	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, hwirq,
 				default_server, 0xff);
 	if (call_status != 0) {
 		printk(KERN_ERR "%s: ibm_set_xive(0xff) irq=%u returned %d\n",
-			__func__, d->irq, call_status);
+			__func__, hwirq, call_status);
 		return;
 	}
 }
@@ -283,13 +283,13 @@ static void xics_mask_irq(struct irq_data *d)
 	irq = (unsigned int)irq_map[d->irq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
-	xics_mask_real_irq(d);
+	xics_mask_real_irq(irq);
 }
 
 static void xics_mask_unknown_vec(unsigned int vec)
 {
 	printk(KERN_ERR "Interrupt %u (real) is invalid, disabling it.\n", vec);
-	xics_mask_real_irq(irq_get_irq_data(vec));
+	xics_mask_real_irq(vec);
 }
 
 static inline unsigned int xics_xirr_vector(unsigned int xirr)