On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:

On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:

quoted

This patch adds support for handling IO Event interrupts which come
through at the /event-sources/ibm,io-events device tree node.

Hi Mark,

You'll have to explain to me offline sometime how it is we ran out of
interrupts and started needing to multiplex them ..

quoted

There is one ibm,io-events interrupt, but this interrupt might be used
for multiple I/O devices, each with their own separate driver. So, we
create a platform interrupt handler that will do the RTAS check-exception
call and then call the appropriate driver's interrupt handler (the one(s)
that registered with a scope that matches the scope of the incoming
interrupt).

So, a driver for a device that uses IO Event interrupts will register
it's interrupt service routine (or interrupt handler) with the platform
code using ioei_register_isr(). This register function takes a function
pointer to the driver's handler and the scope that the driver is
interested in (scopes defined in arch/powerpc/include/asm/io_events.h).
The driver's handler must take a pointer to a struct io_events_section
and must not return anything.

The platform code registers io_event_interrupt() as the interrupt handler
for the ibm,io-events interrupt. Upon receiving an IO Event interrupt, it
checks the scope of the incoming interrupt and only calls those drivers'
handlers that have registered as being interested in that scope.

The "checks the scope" requires an RTAS call, which takes a global lock
(and you add another) - these aren't going to be used for anything
performance critical I hope?

I've been told they're not performance critical but I'll have to go
back and have a closer look at the locking again - I probably am being
a bit overzealous.

quoted

It is possible for a single driver to register the same function pointer
more than once with different scopes if it is interested in more than one
type of IO Event interrupt. If a handler wants to be notified of all
incoming IO Event interrupts it can register with IOEI_SCOPE_ANY.

A driver can unregister to stop receiving the IO Event interrupts using
ioei_unregister_isr(), passing it the same function pointer to the
driver's handler and the scope the driver was registered with.

Signed-off-by: Mark Nelson <redacted>
---
 arch/powerpc/include/asm/io_events.h       |   40 +++++
 arch/powerpc/platforms/pseries/Makefile    |    2 
 arch/powerpc/platforms/pseries/io_events.c |  205 +++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+), 1 deletion(-)

Index: upstream/arch/powerpc/include/asm/io_events.h
===================================================================

--- /dev/null
+++ upstream/arch/powerpc/include/asm/io_events.h

@@ -0,0 +1,40 @@
+/*
+ * Copyright 2010 IBM Corporation, Mark Nelson

I usually have name, corp, but I'm not sure if it matters.

I'll find out what the officially sanctioned way is :)

quoted

+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ASM_POWERPC_IOEVENTS_H
+#define _ASM_POWERPC_IOEVENTS_H
+
+struct io_events_section {
+	u16	id;			// Unique section identifier	x00-x01
+	u16	length;			// Section length (bytes)	x02-x03
+	u8	version;		// Section Version		x04-x04
+	u8	sec_subtype;		// Section subtype		x05-x05
+	u16	creator_id;		// Creator Component ID		x06-x07
+	u8	event_type;		// IO-Event Type		x08-x08
+	u8	rpc_field_len;		// PRC Field Length		x09-x09
+	u8	scope;			// Error/Event Scope		x0A-x0A
+	u8	event_subtype;		// I/O-Event Sub-Type		x0B-x0B
+	u32	drc_index;		// DRC Index			x0C-x0F
+	u32	rpc_data[];		// RPC Data (optional)		x10-...
+};

I know it's idiomatic in that sort of code, but C++ comments?

Yeah, I'm not too phased - I was just copying what lppaca.h did...

quoted

+#define IOEI_SCOPE_NOT_APP	0x00
+#define IOEI_SCOPE_RIO_HUB	0x36
+#define IOEI_SCOPE_RIO_BRIDGE	0x37
+#define IOEI_SCOPE_PHB		0x38
+#define IOEI_SCOPE_EADS_GLOBAL	0x39
+#define IOEI_SCOPE_EADS_SLOT	0x3A
+#define IOEI_SCOPE_TORRENT_HUB	0x3B
+#define IOEI_SCOPE_SERVICE_PROC	0x51
+#define IOEI_SCOPE_ANY		-1
+
+int ioei_register_isr(void (*isr)(struct io_events_section *), int scope);
+int ioei_unregister_isr(void (*isr)(struct io_events_section *), int scope);

Given these are exported to the whole kernel I'd vote for
pseries_ioei_register_isr(), if I get to vote that is.

That's a very good point - I'll change that.

quoted

Index: upstream/arch/powerpc/platforms/pseries/io_events.c
===================================================================

--- /dev/null
+++ upstream/arch/powerpc/platforms/pseries/io_events.c

@@ -0,0 +1,205 @@
+/*
+ * Copyright 2010 IBM Corporation, Mark Nelson
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/errno.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+
+#include <asm/io_events.h>
+#include <asm/rtas.h>
+#include <asm/irq.h>
+
+#include "event_sources.h"
+
+struct ioei_consumer {
+	void (*ioei_isr)(struct io_events_section *);

Function pointers is one case where a typedef can make the code easier
to read, if you like.

quoted

+	int scope;
+	struct ioei_consumer *next;
+};

You forgot the fourth commandment:
        Thou shalt not write thine own linked list implementation when
        there already exist several perfectly good ones in the kernel!
        
Or is there some good reason for it I'm missing? :)

No, there was no good reason at all here and I do feel stupid as in a
previous patch I fought to use the in kernel implementation. I'll use
a struct list_head for the next version.

You could even use a hlist to save a void * in your list head.

quoted

+static int ioei_check_exception_token;
+
+static unsigned char ioei_log_buf[RTAS_ERROR_LOG_MAX];
+static DEFINE_SPINLOCK(ioei_log_buf_lock);
+
+static struct ioei_consumer *ioei_isr_list;
+static DEFINE_SPINLOCK(ioei_isr_list_lock);
+
+int ioei_register_isr(void (*isr)(struct io_events_section *), int scope)
+{
+	struct ioei_consumer *iter;
+	struct ioei_consumer *cons;
+	int ret = 0;
+
+	spin_lock(&ioei_isr_list_lock);

Here and unregister you should be using spin_lock_irq(), because you
need to protect against an interrupt and you know you're not being
called from an irq handler (so you don't need save/restore - though you
could use it anyway to be lazy :D).

I actually had that right in an earlier version but I can't remember
what possessed me to change it... I'll fix it.

quoted

+	/* check to see if we've already registered this function with
+	 * this scope. If we have, don't register it again
+	 */
+	iter = ioei_isr_list;
+	while (iter) {
+		if (iter->ioei_isr == isr && iter->scope == scope)
+			break;
+		iter = iter->next;
+	}
+
+	if (iter) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);

But you don't want to kmalloc while holding the lock and with interrupts
off.

I could allocate above, before taking the lock, and then if we get the
case where it already exists in the list I could just free it before
returning. Would that work?

quoted

+	if (!cons) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	cons->ioei_isr = isr;
+	cons->scope = scope;
+
+	cons->next = ioei_isr_list;
+	ioei_isr_list = cons;
+
+out:
+	spin_unlock(&ioei_isr_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ioei_register_isr);
+
+int ioei_unregister_isr(void (*isr)(struct io_events_section *), int scope)
+{
+	struct ioei_consumer *iter;
+	struct ioei_consumer *prev = NULL;
+	int ret = 0;
+
+	spin_lock(&ioei_isr_list_lock);
+	iter = ioei_isr_list;
+	while (iter) {
+		if (iter->ioei_isr == isr && iter->scope == scope)
+			break;
+		prev = iter;
+		iter = iter->next;
+	}
+
+	if (!iter) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (prev)
+		prev->next = iter->next;
+	else
+		ioei_isr_list = iter->next;
+
+	kfree(iter);
+
+out:
+	spin_unlock(&ioei_isr_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ioei_unregister_isr);
+
+static void ioei_call_consumers(int scope, struct io_events_section *sec)
+{
+	struct ioei_consumer *iter;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ioei_isr_list_lock, flags);

You don't need to disable interrupts, because you're being called from
the interrupt handler, and it won't be called again until you're
finished.

You're right. Sorry, I think I mixed up the irq saving between the
register/unregister functions and this function. I'll fix it.

quoted

+	iter = ioei_isr_list;
+	while (iter) {
+		if (iter->scope == scope || iter->scope == IOEI_SCOPE_ANY)
+			iter->ioei_isr(sec);
+		iter = iter->next;
+	}
+
+	spin_unlock_irqrestore(&ioei_isr_list_lock, flags);
+}
+
+#define EXT_INT_VECTOR_OFFSET	0x500
+#define RTAS_TYPE_IO_EVENT	0xE1
+
+static irqreturn_t io_event_interrupt(int irq, void *dev_id)
+{
+	struct rtas_error_log *rtas_elog;
+	struct io_events_section *ioei_sec;
+	char *ch_ptr;
+	int status;
+	u16 *sec_len;
+
+	spin_lock(&ioei_log_buf_lock);
+
+	status = rtas_call(ioei_check_exception_token, 6, 1, NULL,
+			   EXT_INT_VECTOR_OFFSET,
+			   irq_map[irq].hwirq,

This is going to be  slow anyway, you may as well use virq_to_hw().

Oh yeah, good idea

quoted

+			   RTAS_IO_EVENTS, 1 /*Time Critical */,

Missing space before the T                      ^

Nice catch!

quoted

+			   __pa(&ioei_log_buf),

Does the buffer need to be aligned, and/or inside the RMO? I'd guess
yes.

Good question, I'll look into it

quoted

+				rtas_get_error_log_max());
+
+	rtas_elog = (struct rtas_error_log *)ioei_log_buf;
+
+	if (status != 0)
+		goto out;
+
+	/* We assume that we will only ever get called for io-event
+	 * interrupts. But if we get called with something else
+	 * make some noise about it.
+	 */

That would mean we'd requested the wrong interrupt wouldn't it? I'd
almost BUG, but benh always bitches that I do that too often so .. :)

Yeah, I don't think this would ever happen so I'm not sure exactly
what I'm protecting against here...

quoted

+	if (rtas_elog->type != RTAS_TYPE_IO_EVENT) {
+		pr_warning("IO Events: We got called with an event type of %d"
+			   " rather than %d!\n", rtas_elog->type,
+			   RTAS_TYPE_IO_EVENT);
+		WARN_ON(1);
+		goto out;
+	}
+
+	/* there are 24 bytes of event log data before the first section
+	 * (Main-A) begins
+	 */
+	ch_ptr = (char *)ioei_log_buf + 24;

Any reason you're casting from unsigned char to char?

None that I can fathom now... Good catch :)

quoted

+	/* loop through all the sections until we get to the IO Events
+	 * Section, with section ID "IE"
+	 */
+	while (*ch_ptr != 'I' && *(ch_ptr + 1) != 'E') {
+		sec_len = (u16 *)(ch_ptr + 2);
+		ch_ptr += *sec_len;
+	}

This would be neater if you cast to io_events_section and used the
fields I think.

That's a good idea and would be a lot nicer than the code above.

And even better if you know the length will be a multiple of the
section_header size, you can do the arithmetic in those terms.

I'll read the docs again and see if we can do that.

quoted

+	ioei_sec = (struct io_events_section *)ch_ptr;
+
+	ioei_call_consumers(ioei_sec->scope, ioei_sec);

Guaranteed to be only one section returned to us per call?

My understanding is that there's only ever one, but I'll double check.

We /could/ copy the ioei_sec and drop the buf lock, which would allow
another interrupt to come in and start doing the RTAS call (on another
cpu, and iff there are actually multiple interrupts). But we probably
don't care.

Good point - I'll update it so that we do the copy.

quoted

+out:
+	spin_unlock(&ioei_log_buf_lock);
+
+	return IRQ_HANDLED;
+}
+
+static int __init init_ioei_IRQ(void)

Never understood why IRQ always (sometimes) gets caps ..

Hmmm... Just following the pattern from the RAS code...

quoted

+{
+	struct device_node *np;
+
+	ioei_check_exception_token = rtas_token("check-exception");

Meep, need to check if it actually exists.

Good catch!

quoted

+	np = of_find_node_by_path("/event-sources/ibm,io-events");
+	if (np != NULL) {

if (np) would usually do it

Definitely. I'll update.

quoted

+		request_event_sources_irqs(np, io_event_interrupt, "IO_EVENT");
+		of_node_put(np);
+	}
+
+	return 0;
+}
+device_initcall(init_ioei_IRQ);

Should probably be a machine_initcall().

I'll change it to machine_device_initcall?

quoted

Index: upstream/arch/powerpc/platforms/pseries/Makefile
===================================================================

--- upstream.orig/arch/powerpc/platforms/pseries/Makefile
+++ upstream/arch/powerpc/platforms/pseries/Makefile

@@ -8,7 +8,7 @@ endif
 
 obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
 			   setup.o iommu.o event_sources.o ras.o \
-			   firmware.o power.o dlpar.o
+			   firmware.o power.o dlpar.o io_events.o

The BML guys might appreciate an option to turn it off?

I initially had an option that gets selected by PPC_PSERIES, how about
that?

Thanks for going through this!
Mark