Re: [RFC/PATCH 0/16] Ops based MSI Implementation

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2007-01-25 at 23:56 -0700, Grant Grundler wrote:
> On Thu, Jan 25, 2007 at 11:18:20PM -0700, Eric W. Biederman wrote:
> > You code appears to be nice simple clean and to not support MSI in
> > a useful way.  I may be reading too quickly but at the moment your
> > infrastructure appears useless if you are on a platform that doesn't
> > enforce MSI's get filtered with a legacy interrupt controller.
> 
> Hrm?
> Isn't the point of MSI to avoid any sort of interrupt controller?

Depends how it's implemented :-) In cases like powerpc where the
processor only has a single interrupt line, you need an interrupt
controller anyway.

> > You don't have MSI-X support (which is the interesting case) and you
> > don't have suspend/resume support.
> 
> I saw save/restore entry points.
> I expected suspend/resume code would use those.
> Do you agree (or not)?

MSI-X should be coming soon, I actually though Michael had that sorted
out already, I'll check with him.

Suspend resume should just hook to the backend.

> > You don't support the MSI mask bit.
> > 
> > Looking at your msi_ops it does not map to what I am doing on x86.  There
> > is the implicit assumption that the msi_message is fixed for the lifetime
> > of the msi.  Which is wrong.

The mask bit is not necessary when hooking onto an existing PIC,
however, we should provide a set of mask/unmask for use by the backend
using the mask bit, I agree there.

> Erm...wouldn't changing the message also effectively change which handler
> ends up catching the interrupt?
> I always understood the addr/msg were a pair that HW would map to a handler.
> Can you explain what you mean by "lifetime" and "fixed"?
> What event would change the message? system Suspend/resume?

Intel, in it's great wisdom, defined some bits of the message to have a
special meaning (in addition to the message address being used as a cpu
mask of destinations). I suppose there are cases where they want to
change things in there, not too sure though. I don't see anything that
can't be done by the backend though.

> ...
> > After I get some sleep I will see if I can up with some constructive
> > criticism on how we can make things work.
> 
> Well, I hope the questions I pose above help lead the discussion in
> that direction.

Well, our basic model of alloc/enable/disable/free should map pretty
much anything, and then we provide "raw" functions for the backend to
use rather than re-implement them.

If your backend needs something not provided by those (like mask/unmask
using the mask bit(s)), then either add it to the backend itself or add
new "generic" functions for optional use by the backend.

Ben.