Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
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 ..
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.
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 ma...@au1.ibm.com
---
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 :)
+ * 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 identifierx00-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 Typex08-x08
+ u8 rpc_field_len; // PRC Field Length x09-x09
+ u8 scope; // Error/Event Scopex0A-x0A
+ u8 event_subtype; // I/O-Event Sub-Type x0B-x0B
+ u32 drc_index; // DRC Indexx0C-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...
+#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_PROC0x51
+#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
Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Wednesday 19 May 2010 14:27:44 Sonny Rao wrote:
On Tue, 18 May 2010 23:37:31 +1000, Michael Ellerman wrote:
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
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 ..
Firmware has decided to multiplex all i/o error reporting through a single
interrupt for reasons unknown, that is the primary reason for this patch.
One question is, we already register a few RAS interrupts which call
RTAS using check-exception for getting error information. Those live
in platforms/pseries/ras.c -- should we combine the two into a common
implementation somehow?
I developed in ras.c but then moved it out just for functional
separation but you could be right in that they should live together.
I'll have another look at it when I've fixed it up.
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?
Nope it shouldn't be performance critical, but it does raise the point
that the current RTAS implementation in Linux *always* uses the global
lock. There is a set of calls which are not required to be serialized
against each other. This would be a totally different patchset to fix that
problem. The check-exception call is one that doesn't require the global
RTAS lock. I might work on that someday :-)
snip
+ /* 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.
Well, he could use GFP_ATOMIC but that's the wrong approach. You should
allocate the buffer (using GFP_KERNEL) before taking the spin lock.
And then free it before returning in the case that we've got a
duplicate, right?
snip
+#define EXT_INT_VECTOR_OFFSET0x500
+#define RTAS_TYPE_IO_EVENT 0xE1
These defines should probably go in asm/rtas.h
I noticed the code in ras.c has it's own define too RAS_VECTOR_OFFSET
for 0x500 and asm/rtas.h actually has RTAS_TYPE_IO for 0xE1
I'm not sure how I missed RTAS_TYPE_IO in asm/rtas.h but I'll use
that and then add EXT_INT_VECTOR_OFFSET and update the RAS code to use
it too.
+
+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().
+RTAS_IO_EVENTS, 1 /*Time Critical */,
Missing space before the T ^
+__pa(ioei_log_buf),
Does the buffer need to be aligned, and/or inside the RMO? I'd guess
yes.
The docs for check-exception don't particularly specify alignment
requirements, but RTAS in generally going to want it in the RMO
Also, if we're going to go ahead and use rtas_call() which locks
it's own buffer which meets the requirements, why do we even need
a
Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Wed, 2010-05-19 at 20:24 +1000, Mark Nelson wrote:
On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
+ /* 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?
Yeah I think so, optimise for the normal case where it doesn't already
exist. The other option would be to take the lock, check, do the alloc,
retake the lock and recheck - but that's a pain and not really worth the
trouble.
+ 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.
OK good to check. Could be worth checking in the code, unless it's going
to be really expensive.
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.
Sounds like we should. It's not such a concern to call the handlers with
the lock held IMHO (Sonny raised that), as long as the handlers don't
try and register/unregister themselves. But that will be pretty obvious
if it happens.
+ 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?
Yep.
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?
Select is not great because it disregards dependencies, and BML is
PSERIES. Probably just have an option that depends on PSERIES and is
default y.
cheers
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Tue, 2010-05-18 at 23:27 -0500, Sonny Rao wrote:
On Tue, 18 May 2010 23:37:31 +1000, Michael Ellerman wrote:
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
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 ..
Firmware has decided to multiplex all i/o error reporting through a single
interrupt for reasons unknown, that is the primary reason for this patch.
Yeah, I just don't get why we would do that, but hey whatever.
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?
Nope it shouldn't be performance critical, but it does raise the point
that the current RTAS implementation in Linux *always* uses the global
lock. There is a set of calls which are not required to be serialized
against each other. This would be a totally different patchset to fix that
problem. The check-exception call is one that doesn't require the global
RTAS lock. I might work on that someday :-)
Aha, that's kind of what I was wondering. I take it PAPR documents which
calls need to be serialised and which don't?
+ cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
But you don't want to kmalloc while holding the lock and with interrupts
off.
Well, he could use GFP_ATOMIC but that's the wrong approach. You should
allocate the buffer (using GFP_KERNEL) before taking the spin lock.
True, this is not a good example of when to use GFP_ATOMIC though :)
Also, if we're going to go ahead and use rtas_call() which locks
it's own buffer which meets the requirements, why do we even need
a separate buffer? Really, we should make this call, then copy
the content of the buffer before handing it over to the drivers.
But another CPU could rtas_call() and blow away our buffer after we've
dropped the RTAS lock but before we've used the content of the buffer.
+ 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;
+ }
Should we try to process this instead of just warning?
The type we get might be one of the the ones we recognize in
ras.c; so this is an argument for combining ras.c with this code
or at least report this in the same manner we report any other
RTAS error log.
We could, but that would be a massive firmware bug - not that it
wouldn't happen, but it would be Not Our Problem TM.
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.
I think we *have* to copy it because we don't want our lock held when we
call random handlers.
They're not really random, and as long as they don't call the
register/unregister routines it should be /OK/. But copying is probably
good so we don't hold the lock for too long.
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?
Or, we might subvert it for our own evil purposes ;-)
I can only imagine :)
cheers
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Wed, May 19, 2010 at 10:10:58PM +1000, Michael Ellerman wrote:
snip
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?
Nope it shouldn't be performance critical, but it does raise the point
that the current RTAS implementation in Linux *always* uses the global
lock. There is a set of calls which are not required to be serialized
against each other. This would be a totally different patchset to fix that
problem. The check-exception call is one that doesn't require the global
RTAS lock. I might work on that someday :-)
Aha, that's kind of what I was wondering. I take it PAPR documents which
calls need to be serialised and which don't?
Yeah, here's my workin list of what calls can avoid the global lock:
List of re-entrant to the number of processors in the system RTAS Calls
--
ibm,get-xive
ibm,set-xive
ibm,int-off
ibm,int-on
OS machine check and soft-reset handlerse must be able to call rtas
(I'm saying these are therefore re-entrant because we could deadlock
if we took a machine check or reset with the global lock held)
--
nvram-fetch
nvram-store
check-exception (includes our io-events)
display-character
system-reboot
set-power-level(0,0)
power-off
ibm,set-eeh-option
ibm,set-slot-reset
ibm,read-slot-reset-state2
additional serialiaztion group by itself
--
stop-self
start-cpu
set-power-level
snip
Also, if we're going to go ahead and use rtas_call() which locks
it's own buffer which meets the requirements, why do we even need
a separate buffer? Really, we should make this call, then copy
the content of the buffer before handing it over to the drivers.
But another CPU could rtas_call() and blow away our buffer after we've
dropped the RTAS lock but before we've used the content of the buffer.
Yeah, maybe I'm getting ahead of myself here -- to me this just highlights
the whole locking problem with the API as written, since the locking is
all done inside that call. The API needs to be extended such that
we have the option of doing our own locking of RTAS calls.
+ 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;
+ }
Should we try to process this instead of just warning?
The type we get might be one of the the ones we recognize in
ras.c; so this is an argument for combining ras.c with this code
or at least report this in the same manner we report any other
RTAS error log.
We could, but that would be a massive firmware bug - not that it
wouldn't happen, but it would be Not Our Problem TM.
Yeah, this is paranoia (*cough* Milton's suggestion)
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.
I think we *have* to copy it because we don't want our lock held when we
call random handlers.
They're not really random, and as long as they don't call the
register/unregister routines it should be /OK/. But copying is probably
good so we don't hold the lock for too long.
Yeah, this is probably ok since it's all happening in interrupt
context anyway the handlers have to be running in an atomic context
anyway.
--
Sonny Rao, LTC OzLabs, BML team
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
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 ..
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?
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 ma...@au1.ibm.com
---
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.
+ * 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 identifierx00-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 Typex08-x08
+ u8 rpc_field_len; // PRC Field Length x09-x09
+ u8 scope; // Error/Event Scopex0A-x0A
+ u8 event_subtype; // I/O-Event Sub-Type x0B-x0B
+ u32 drc_index; // DRC Indexx0C-x0F
+ u32 rpc_data[]; // RPC Data (optional) x10-...
+};
I know it's idiomatic in that sort of code, but C++ comments?
+#define IOEI_SCOPE_NOT_APP 0x00
+#define IOEI_SCOPE_RIO_HUB 0x36
+#define IOEI_SCOPE_RIO_BRIDGE0x37
+#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.
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
[PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
On Tue, 18 May 2010 23:37:31 +1000, Michael Ellerman wrote:
On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
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 ..
Firmware has decided to multiplex all i/o error reporting through a single
interrupt for reasons unknown, that is the primary reason for this patch.
One question is, we already register a few RAS interrupts which call
RTAS using check-exception for getting error information. Those live
in platforms/pseries/ras.c -- should we combine the two into a common
implementation somehow?
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?
Nope it shouldn't be performance critical, but it does raise the point
that the current RTAS implementation in Linux *always* uses the global
lock. There is a set of calls which are not required to be serialized
against each other. This would be a totally different patchset to fix that
problem. The check-exception call is one that doesn't require the global
RTAS lock. I might work on that someday :-)
snip
+ /* 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.
Well, he could use GFP_ATOMIC but that's the wrong approach. You should
allocate the buffer (using GFP_KERNEL) before taking the spin lock.
snip
+#define EXT_INT_VECTOR_OFFSET 0x500
+#define RTAS_TYPE_IO_EVENT 0xE1
These defines should probably go in asm/rtas.h
I noticed the code in ras.c has it's own define too RAS_VECTOR_OFFSET
for 0x500 and asm/rtas.h actually has RTAS_TYPE_IO for 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().
+ RTAS_IO_EVENTS, 1 /*Time Critical */,
Missing space before the T ^
+ __pa(ioei_log_buf),
Does the buffer need to be aligned, and/or inside the RMO? I'd guess
yes.
The docs for check-exception don't particularly specify alignment
requirements, but RTAS in generally going to want it in the RMO
Also, if we're going to go ahead and use rtas_call() which locks
it's own buffer which meets the requirements, why do we even need
a separate buffer? Really, we should make this call, then copy
the content of the buffer before handing it over to the drivers.
+ rtas_get_error_log_max());
Here, we're passing back what RTAS told us what it's max is
which doesn't necessarily equal the static buffer size we
allocated which can cause a buffer overflow. So this
argument should be the static size of the buffer.
+
+ rtas_elog = (struct rtas_error_log *)ioei_log_buf;
+
+ if (status != 0)
+ goto out;
+
+ /* We
[PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
This patch adds support for handling IO Event interrupts which come
through at the /event-sources/ibm,io-events device tree node.
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.
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 ma...@au1.ibm.com
---
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
+ *
+ * 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 identifierx00-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 Typex08-x08
+ u8 rpc_field_len; // PRC Field Length x09-x09
+ u8 scope; // Error/Event Scopex0A-x0A
+ u8 event_subtype; // I/O-Event Sub-Type x0B-x0B
+ u32 drc_index; // DRC Indexx0C-x0F
+ u32 rpc_data[]; // RPC Data (optional) x10-...
+};
+
+#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_PROC0x51
+#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);
+
+#endif /* _ASM_POWERPC_IOEVENTS_H */
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 *);
+ int scope;
+ struct ioei_consumer
