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

[ebiederm@xmission.com (Eric W. Biederman)]

Michael Ellerman <michael@ellerman.id.au> writes:

> OK, here's a first cut at moving ops based MSI into the generic code. I'm
> posting this now to make sure I'm not heading off into the weeds.

First thanks for copying me on this.  I really appreciate it.

> The fifth patch contain the guts of it, I've included the MPIC and
> RTAS backends as examples. In fact they actually work.
>
> In order to smoothly merge this with the old MSI code, the two will need to
> coexist in the tree for at least a few commits, so I've added (invisible)
> Kconfig symbols to allow that.
>
> I plan to merge the Intel code by:
>  * copying it into drivers/pci/msi/intel.c with zero changes.
>  * providing a minimal shim to connect the ops code to the intel code.
>  * at this point the code should be functional but ugly as hell.
>  * via a longish series of patches, adapt the intel code to better match
>    the new ops code.
>  * this should allow us to bisect through to find any mistakes.
>
> If people think that's crazy and or stupid please let me know :)
>
> TBD are:
>  * suspend / resume hooks in the ops - this shouldn't be too tricky with
>    the power management API cleaned up a touch.
>  * working out why the hell msi_remove_pci_irq_vectors() is a special case ?


I haven't done more than skim the patches yet but I am distressed.

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.

You don't have MSI-X support (which is the interesting case) and you don't have
suspend/resume support.

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.

So in short summary I cannot use your msi_ops they are inappropriate for
i386, x86_64 and ia64.

So at the moment I am opposed to this code because as it sits it appears to
be a serious regression.

The additional bits that feel like this code was primarily targeted at supporting
the RTAS with real hardware support thrown in as an after thought just seem
to add insult to injury.  To date I have no information that indicates to me
that the RTAS model is at all sane or makes any sense to duplicate elsewhere.
If supporting the RTAS is what is obscuring your vision of what is really
needed to support MSI I don't want to see RTAS support in a patch set
until we get a good multiple platform architecture, merged into the kernel.

Supporting the RTAS first and breaking everyone who actually has real
hardware seems like very much the wrong approach to get a good
multiple platform solution.

After I get some sleep I will see if I can up with some constructive
criticism on how we can make things work.

Eric