powerpc/powernv/opal: Use standard interrupts property when available

Message ID 1523344570.11062.65.camel@kernel.crashing.org
State Accepted
Commit 77b5f703dcc859915f0f20d92bc538e4a99ef149
Headers show
Series
  • powerpc/powernv/opal: Use standard interrupts property when available
Related show

Commit Message

Benjamin Herrenschmidt April 10, 2018, 7:16 a.m.
For (bad) historical reasons, OPAL used to create a non-standard pair of
properties "opal-interrupts" and "opal-interrupts-names" for representing
the list of interrupts it wants Linux to request on its behalf.

Among other issues, the opal-interrupts doesn't have a way to carry the
type of interrupts, and they were assumed to be all level sensitive.

This is wrong on some recent systems where some of them are edge sensitive
causing warnings in the XIVE code and possible misbehaviours if they need
to be retriggered (typically the NPU2 TCE error interrupts).

This makes Linux switch to using the standard "interrupts" and
"interrupt-names" properties instead when they are available, using standard
of_irq helpers, which can carry all the desired type information.

Newer versions of OPAL will generate those properties in addition to the
legacy ones.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Comments

Michael Ellerman Aug. 1, 2018, 11:37 a.m. | #1
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> diff --git a/arch/powerpc/platforms/powernv/opal-irqchip.c b/arch/powerpc/platforms/powernv/opal-irqchip.c
> index 9d1b8c0aaf93..46785eaf625d 100644
> --- a/arch/powerpc/platforms/powernv/opal-irqchip.c
> +++ b/arch/powerpc/platforms/powernv/opal-irqchip.c
> @@ -216,67 +214,91 @@ int __init opal_event_init(void)
...
>  
>  	/* Install interrupt handlers */
>  	for (i = 0; i < opal_irq_count; i++) {
> -		unsigned int virq;
> -		char *name;
> +		struct resource *r = &opal_irqs[i];
> +		const char *name;
>  
> -		/* Get hardware and virtual IRQ */
> -		virq = irq_create_mapping(NULL, irqs[i]);
> -		if (!virq) {
> -			pr_warn("Failed to map irq 0x%x\n", irqs[i]);
> -			continue;
> -		}
> -
> -		if (names[i] && strlen(names[i]))
> -			name = kasprintf(GFP_KERNEL, "opal-%s", names[i]);
> +		/* Prefix name */
> +		if (r->name)
> +			name = kasprintf(GFP_KERNEL, "opal-%s", r->name);
>  		else
>  			name = kasprintf(GFP_KERNEL, "opal");

I'm getting:

root@p85:/proc/irq# find . -name '*opal*'
...
./63/opal-ipmi
./227/opal-
./228/opal-
./231/opal-
./232/opal-
./247/opal-
./248/opal-
./483/opal-
./484/opal-
./487/opal-
./488/opal-
./500/opal-
./510/opal-
./511/opal-
./512/opal-


I fixed it with:

diff --git a/arch/powerpc/platforms/powernv/opal-irqchip.c b/arch/powerpc/platforms/powernv/opal-irqchip.c
index 41c3303157f7..a2d067925140 100644
--- a/arch/powerpc/platforms/powernv/opal-irqchip.c
+++ b/arch/powerpc/platforms/powernv/opal-irqchip.c
@@ -275,7 +275,7 @@ int __init opal_event_init(void)
 		const char *name;
 
 		/* Prefix name */
-		if (r->name)
+		if (r->name && strlen(r->name))
 			name = kasprintf(GFP_KERNEL, "opal-%s", r->name);
 		else
 			name = kasprintf(GFP_KERNEL, "opal");


cheers
Michael Ellerman Aug. 4, 2018, 2:39 a.m. | #2
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
...
> diff --git a/arch/powerpc/platforms/powernv/opal-irqchip.c b/arch/powerpc/platforms/powernv/opal-irqchip.c
> index 9d1b8c0aaf93..46785eaf625d 100644
> --- a/arch/powerpc/platforms/powernv/opal-irqchip.c
> +++ b/arch/powerpc/platforms/powernv/opal-irqchip.c
> @@ -174,24 +175,21 @@ void opal_event_shutdown(void)
>  
>  	/* First free interrupts, which will also mask them */
>  	for (i = 0; i < opal_irq_count; i++) {
> -		if (!opal_irqs[i])
> +		if (!opal_irqs || !opal_irqs[i].start)
>  			continue;
>  
>  		if (in_interrupt())
> -			disable_irq_nosync(opal_irqs[i]);
> +			disable_irq_nosync(opal_irqs[i].start);
>  		else
> -			free_irq(opal_irqs[i], NULL);
> -
> -		opal_irqs[i] = 0;

This             ^^^^^^^^^^^^^^

> +			free_irq(opal_irqs[i].start, NULL);
>  	}

causes:

  ------------[ cut here ]------------
  Trying to free already-free IRQ 22
  WARNING: CPU: 0 PID: 1295 at ../kernel/irq/manage.c:1583 __free_irq+0xe0/0x420
  Modules linked in:
  CPU: 0 PID: 1295 Comm: init Tainted: G        W         4.18.0-rc3-gcc-7.3.1-00187-g46a3659b3791-dirty #80
  NIP:  c00000000017ca90 LR: c00000000017ca8c CTR: c000000000771a90
  REGS: c00000007552b810 TRAP: 0700   Tainted: G        W          (4.18.0-rc3-gcc-7.3.1-00187-g46a3659b3791-dirty)
  MSR:  9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 28000222  XER: 20000000
  CFAR: c000000000106f10 IRQMASK: 1 
  GPR00: c00000000017ca8c c00000007552ba90 c000000000fd1600 0000000000000022 
  GPR04: 0000000000000001 000000000000015e 9000000000009033 0000000000000000 
  GPR08: 000000007ef50000 c000000000ef0bf0 c000000000ef0bf0 9000000000001003 
  GPR12: 0000000000000000 c000000001160000 0000000000000000 0000000000000000 
  GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
  GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
  GPR24: 00000000100c0828 0000000000000000 0000000000000016 c000000077118094 
  GPR28: c000000077118148 0000000000000000 c000000077118000 0000000000000000 
  NIP [c00000000017ca90] __free_irq+0xe0/0x420
  LR [c00000000017ca8c] __free_irq+0xdc/0x420
  Call Trace:
  [c00000007552ba90] [c00000000017ca8c] __free_irq+0xdc/0x420 (unreliable)
  [c00000007552bb30] [c00000000017ced8] free_irq+0x78/0xe0
  [c00000007552bb60] [c0000000000a7c48] opal_event_shutdown+0x158/0x160
  [c00000007552bbf0] [c00000000009d150] pnv_prepare_going_down+0x20/0x80
  [c00000007552bc10] [c00000000009d1d4] pnv_power_off+0x24/0x70
  [c00000007552bc40] [c00000000009d240] pnv_restart+0x0/0x70
  [c00000007552bc60] [c00000000002c2a0] machine_halt+0x60/0x70
  [c00000007552bc80] [c000000000139f74] kernel_halt+0x84/0xa0
  [c00000007552bce0] [c00000000013a36c] sys_reboot+0x28c/0x2c0
  [c00000007552be30] [c00000000000b9e4] system_call+0x5c/0x70
  Instruction dump:
  e93f0008 7fa9e840 419e0098 7feafb78 ebea0018 2fbf0000 409effe8 3c62ffd0 
  7f44d378 386326c8 4bf8a421 60000000 <0fe00000> 7f24cb78 7f63db78 48952bdd 
  ---[ end trace 926f8007a9952304 ]---


Because we're calling opal_event_shutdown() twice, via opal_shutdown()
and then pnv_prepare_going_down().

Did you drop that line on purpose, or was it just collateral damage?

The obvious fix does work:

+	opal_irqs[i].start = 0;


cheers
Michael Ellerman Aug. 8, 2018, 2:25 p.m. | #3
On Tue, 2018-04-10 at 07:16:10 UTC, Benjamin Herrenschmidt wrote:
> For (bad) historical reasons, OPAL used to create a non-standard pair of
> properties "opal-interrupts" and "opal-interrupts-names" for representing
> the list of interrupts it wants Linux to request on its behalf.
> 
> Among other issues, the opal-interrupts doesn't have a way to carry the
> type of interrupts, and they were assumed to be all level sensitive.
> 
> This is wrong on some recent systems where some of them are edge sensitive
> causing warnings in the XIVE code and possible misbehaviours if they need
> to be retriggered (typically the NPU2 TCE error interrupts).
> 
> This makes Linux switch to using the standard "interrupts" and
> "interrupt-names" properties instead when they are available, using standard
> of_irq helpers, which can carry all the desired type information.
> 
> Newer versions of OPAL will generate those properties in addition to the
> legacy ones.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/77b5f703dcc859915f0f20d92bc538

cheers

Patch

diff --git a/arch/powerpc/platforms/powernv/opal-irqchip.c b/arch/powerpc/platforms/powernv/opal-irqchip.c
index 9d1b8c0aaf93..46785eaf625d 100644
--- a/arch/powerpc/platforms/powernv/opal-irqchip.c
+++ b/arch/powerpc/platforms/powernv/opal-irqchip.c
@@ -23,6 +23,7 @@ 
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/irq_work.h>
+#include <linux/of_irq.h>
 
 #include <asm/machdep.h>
 #include <asm/opal.h>
@@ -39,8 +40,8 @@  struct opal_event_irqchip {
 };
 static struct opal_event_irqchip opal_event_irqchip;
 
-static unsigned int opal_irq_count;
-static unsigned int *opal_irqs;
+static int opal_irq_count;
+static struct resource *opal_irqs;
 
 static void opal_handle_irq_work(struct irq_work *work);
 static u64 last_outstanding_events;
@@ -174,24 +175,21 @@  void opal_event_shutdown(void)
 
 	/* First free interrupts, which will also mask them */
 	for (i = 0; i < opal_irq_count; i++) {
-		if (!opal_irqs[i])
+		if (!opal_irqs || !opal_irqs[i].start)
 			continue;
 
 		if (in_interrupt())
-			disable_irq_nosync(opal_irqs[i]);
+			disable_irq_nosync(opal_irqs[i].start);
 		else
-			free_irq(opal_irqs[i], NULL);
-
-		opal_irqs[i] = 0;
+			free_irq(opal_irqs[i].start, NULL);
 	}
 }
 
 int __init opal_event_init(void)
 {
 	struct device_node *dn, *opal_node;
-	const char **names;
-	u32 *irqs;
-	int i, rc;
+	bool old_style = false;
+	int i, rc = 0;
 
 	opal_node = of_find_node_by_path("/ibm,opal");
 	if (!opal_node) {
@@ -216,67 +214,91 @@  int __init opal_event_init(void)
 		goto out;
 	}
 
-	/* Get opal-interrupts property and names if present */
-	rc = of_property_count_u32_elems(opal_node, "opal-interrupts");
-	if (rc < 0)
-		goto out;
+	/* Look for new-style (standard) "interrupts" property */
+	opal_irq_count = of_irq_count(opal_node);
 
-	opal_irq_count = rc;
-	pr_debug("Found %d interrupts reserved for OPAL\n", opal_irq_count);
+	/* Absent ? Look for the old one */
+	if (opal_irq_count < 1) {
+		/* Get opal-interrupts property and names if present */
+		rc = of_property_count_u32_elems(opal_node, "opal-interrupts");
+		if (rc > 0)
+			opal_irq_count = rc;
+		old_style = true;
+	}
 
-	irqs = kcalloc(opal_irq_count, sizeof(*irqs), GFP_KERNEL);
-	names = kcalloc(opal_irq_count, sizeof(*names), GFP_KERNEL);
-	opal_irqs = kcalloc(opal_irq_count, sizeof(*opal_irqs), GFP_KERNEL);
+	/* No interrupts ? Bail out */
+	if (!opal_irq_count)
+		goto out;
 
-	if (WARN_ON(!irqs || !names || !opal_irqs))
-		goto out_free;
+	pr_debug("OPAL: Found %d interrupts reserved for OPAL using %s scheme\n",
+		 opal_irq_count, old_style ? "old" : "new");
 
-	rc = of_property_read_u32_array(opal_node, "opal-interrupts",
-					irqs, opal_irq_count);
-	if (rc < 0) {
-		pr_err("Error %d reading opal-interrupts array\n", rc);
-		goto out_free;
+	/* Allocate an IRQ resources array */
+	opal_irqs = kcalloc(opal_irq_count, sizeof(struct resource), GFP_KERNEL);
+	if (WARN_ON(!opal_irqs)) {
+		rc = -ENOMEM;
+		goto out;
 	}
 
-	/* It's not an error for the names to be missing */
-	of_property_read_string_array(opal_node, "opal-interrupts-names",
-				      names, opal_irq_count);
+	/* Build the resources array */
+	if (old_style) {
+		/* Old style "opal-interrupts" property */
+		for (i = 0; i < opal_irq_count; i++) {
+			struct resource *r = &opal_irqs[i];
+			const char *name = NULL;
+			u32 hw_irq;
+			int virq;
+
+			rc = of_property_read_u32_index(opal_node, "opal-interrupts",
+							i, &hw_irq);
+			if (WARN_ON(rc < 0)) {
+				opal_irq_count = i;
+				break;
+			}
+			of_property_read_string_index(opal_node, "opal-interrupts-names",
+						      i, &name);
+			virq = irq_create_mapping(NULL, hw_irq);
+			if (!virq) {
+				pr_warn("Failed to map OPAL irq 0x%x\n", hw_irq);
+				continue;
+			}
+			r->start = r->end = virq;
+			r->flags = IORESOURCE_IRQ | IRQ_TYPE_LEVEL_LOW;
+			r->name = name;
+		}
+	} else {
+		/* new style standard "interrupts" property */
+		rc = of_irq_to_resource_table(opal_node, opal_irqs, opal_irq_count);
+		if (WARN_ON(rc < 0)) {
+			opal_irq_count = 0;
+			kfree(opal_irqs);
+			goto out;
+		}
+		if (WARN_ON(rc < opal_irq_count))
+			opal_irq_count = rc;
+	}
 
 	/* Install interrupt handlers */
 	for (i = 0; i < opal_irq_count; i++) {
-		unsigned int virq;
-		char *name;
+		struct resource *r = &opal_irqs[i];
+		const char *name;
 
-		/* Get hardware and virtual IRQ */
-		virq = irq_create_mapping(NULL, irqs[i]);
-		if (!virq) {
-			pr_warn("Failed to map irq 0x%x\n", irqs[i]);
-			continue;
-		}
-
-		if (names[i] && strlen(names[i]))
-			name = kasprintf(GFP_KERNEL, "opal-%s", names[i]);
+		/* Prefix name */
+		if (r->name)
+			name = kasprintf(GFP_KERNEL, "opal-%s", r->name);
 		else
 			name = kasprintf(GFP_KERNEL, "opal");
 
 		/* Install interrupt handler */
-		rc = request_irq(virq, opal_interrupt, IRQF_TRIGGER_LOW,
+		rc = request_irq(r->start, opal_interrupt, r->flags & IRQD_TRIGGER_MASK,
 				 name, NULL);
 		if (rc) {
-			irq_dispose_mapping(virq);
-			pr_warn("Error %d requesting irq %d (0x%x)\n",
-				 rc, virq, irqs[i]);
+			pr_warn("Error %d requesting OPAL irq %d\n", rc, (int)r->start);
 			continue;
 		}
-
-		/* Cache IRQ */
-		opal_irqs[i] = virq;
 	}
-
-out_free:
-	kfree(irqs);
-	kfree(names);
-out:
+	rc = 0;
+ out:
 	of_node_put(opal_node);
 	return rc;
 }