Re: [PATCH 2/7] Powerpc MSI infrastructure

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]

On Sat, 2007-04-21 at 18:15 -0500, Milton Miller wrote:
> On Apr 19, 2007, Michael Ellerman wrote:
> > This patch provides the architecture specific hooks to support MSI on
> > powerpc. We implement the newly added arch_setup_msi_irqs() and
> > arch_teardown_msi_irqs(), and then delegate to ppc_md routines.
> >
> 
> > Index: msi-new/arch/powerpc/kernel/msi.c
> > ===================================================================
> > --- /dev/null
> > +++ msi-new/arch/powerpc/kernel/msi.c
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Copyright 2006-2007, Michael Ellerman, IBM Corporation.
> > + *
> > + * 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/kernel.h>
> > +#include <linux/msi.h>
> > +
> > +#include <asm/machdep.h>
> > +
> > +int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
> > +{
> > +	if (ppc_md.msi_check_device) {
> > +		pr_debug("msi: Using platform check routine.\n");
> > +		return ppc_md.msi_check_device(dev, nvec, type);
> > +	}
> > +
> > +	if (!ppc_md.setup_msi_irqs || !ppc_md.teardown_msi_irqs) {
> > +		pr_debug("msi: Platform doesn't provide MSI callbacks.\n");
> > +		return -ENOSYS;
> > +	}
> 
> Should we not do this check first?  Or do you expect some platform
> to fill out these hooks in the check call above?  Othewise we will
> branch to function pointer NULL.

Not really. More a case of if you provide check then you'd better get
your act together and provide the others. I'll rearrange it to be safer.

> 
> > +
> > +        return 0;
> > +}
> > +
> > +int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> > +{
> > +	return ppc_md.setup_msi_irqs(dev, nvec, type);
> > +}
> > +
> > +void arch_teardown_msi_irqs(struct pci_dev *dev)
> > +{
> > +	return ppc_md.teardown_msi_irqs(dev);
> > +}
> 
> 
> > Index: msi-new/arch/powerpc/kernel/Makefile
> > ===================================================================
> > --- msi-new.orig/arch/powerpc/kernel/Makefile
> > +++ msi-new/arch/powerpc/kernel/Makefile
> 
> > +obj-$(CONFIG_PCI_MSI)		+= msi.o
> 
> 
> Do we really need a new file for these simple hooks?  It doesn't look
> like anthing else is going to be added to it either.   I'd prefer to
> just do an ifdef in pci.c.   Hmmm, we don't seem to have that, but
> these are soo small I'd include machdep.h and put them inline in
> asm-powerpc/pci.h.

I'd rather leave it as is. We can't have them inline, because they're
already defined in drivers/pci/msi.c. And having the CONFIG logic in the
Makefile is cleaner than #ifdef goo. Also it might grow one day, if we
make the hooks per pci_dev or per bus rather than in ppc_md.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]