[RFCv1,08/11] PCI: Introduce new MSI chip infrastructure

Commit Message

Thomas Petazzoni March 26, 2013, 4:52 p.m. UTC

From: Thierry Reding <[e-mail]>

The new struct msi_chip is used to associated an MSI controller with a
PCI bus. It is automatically handed down from the root to its children
during bus enumeration.

This patch provides default (weak) implementations for the architecture-
specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
and arch_msi_check_device()) which check if a PCI device's bus has an
attached MSI chip and forward the call appropriately.

Signed-off-by: Thierry Reding <[e-mail]>
Signed-off-by: Thomas Petazzoni <[e-mail]>
---
 drivers/pci/msi.c   |   35 +++++++++++++++++++++++++++++++----
 drivers/pci/probe.c |    1 +
 include/linux/msi.h |   10 ++++++++++
 include/linux/pci.h |    1 +
 4 files changed, 43 insertions(+), 4 deletions(-)

Comments

Bjorn Helgaas April 8, 2013, 10:28 p.m. UTC | #1

On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
<[e-mail]> wrote:
> From: Thierry Reding <[e-mail]>
>
> The new struct msi_chip is used to associated an MSI controller with a
> PCI bus. It is automatically handed down from the root to its children
> during bus enumeration.
>
> This patch provides default (weak) implementations for the architecture-
> specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> and arch_msi_check_device()) which check if a PCI device's bus has an
> attached MSI chip and forward the call appropriately.
>
> Signed-off-by: Thierry Reding <[e-mail]>
> Signed-off-by: Thomas Petazzoni <[e-mail]>
> ---
>  drivers/pci/msi.c   |   35 +++++++++++++++++++++++++++++++----
>  drivers/pci/probe.c |    1 +
>  include/linux/msi.h |   10 ++++++++++
>  include/linux/pci.h |    1 +
>  4 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 00cc78c..fce3549 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -26,14 +26,41 @@
>
>  static int pci_msi_enable = 1;
>
> -/* Arch hooks */
> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> +{
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->setup_irq) {
> +               int err;
> +
> +               err = chip->setup_irq(chip, dev, desc);
> +               if (err < 0)
> +                       return err;
> +
> +               irq_set_chip_data(desc->irq, chip);
> +               return err;
> +       }
> +
> +       return -EINVAL;
> +}
>
> -#ifndef arch_msi_check_device
> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +void __weak arch_teardown_msi_irq(unsigned int irq)
>  {
> +       struct msi_chip *chip = irq_get_chip_data(irq);
> +
> +       if (chip && chip->teardown_irq)
> +               chip->teardown_irq(chip, irq);
> +}
> +
> +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)

I like the replacement of "#ifndef arch_msi_check_device()" with a
weak implementation here, but this only does half the job -- shouldn't
we remove the powerpc "#define arch_msi_check_device
arch_msi_check_device" at the same time?

And since we're touching all the check_device() implementations, maybe
we could come up with a better name.  "check_device()" conveys
absolutely no information about what we're checking or what the sense
of the result is.  arch_msi_supported()?  pcibios_msi_supported()?  I
guess it should be consistent with the other arch interfaces, so
arch_*() is probably better.

Maybe the ugly #ifdef-ery around arch_setup_msi_irqs,
arch_teardown_msi_irqs, and arch_restore_msi_irqs could be cleaned up
similarly?  Somebody worked pretty hard to obfuscate all that,
probably before weak functions were available.

> +{
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->check_device)
> +               return chip->check_device(chip, dev, nvec, type);
> +
>         return 0;
>  }
> -#endif
>
>  #ifndef arch_setup_msi_irqs
>  # define arch_setup_msi_irqs default_setup_msi_irqs
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b494066..9307550 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>
>         child->parent = parent;
>         child->ops = parent->ops;
> +       child->msi = parent->msi;
>         child->sysdata = parent->sysdata;
>         child->bus_flags = parent->bus_flags;
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index ce93a34..ea4a5be 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
>  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
>  extern int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>
> +struct msi_chip {
> +       struct module *owner;
> +       struct device *dev;
> +
> +       int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> +                        struct msi_desc *desc);
> +       void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> +       int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> +                           int nvec, int type);

If we do end up adding interfaces like this (I'm not sure it will work
-- see below), I think it would be better to pass just the pci_dev,
not the "msi_chip, pci_dev" pair.  Passing both pointers avoids a
cheap lookup in the caller, but it adds a way that two inseparable
things can get out of sync.

> +};
>
>  #endif /* LINUX_MSI_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..6aca43ea 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -416,6 +416,7 @@ struct pci_bus {
>         struct resource busn_res;       /* bus numbers routed to this bus */
>
>         struct pci_ops  *ops;           /* configuration access functions */
> +       struct msi_chip *msi;           /* MSI controller */

"msi" seems like a too-generic name here; it suggests an interrupt or
IRQ, not a controller.

I'm not sure this is the correct place for it.  Having it in the
struct pci_bus means you need arch code to fill it in, e.g., you added
it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
that for arches that use pci_scan_root_bus(), which is the direction
I'd like to go.

I think it probably should go in sysdata instead.  That would mean you
can't really do generic weak setup/tear-down functions, because they
wouldn't know how to pull the MSI controller info out of the
arch-specific sysdata.  But there are so many levels of weak-ness
going on there, maybe it would be a good thing to get rid of one :)

Bjorn

>         void            *sysdata;       /* hook for sys-specific extension */
>         struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */
>
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Murray April 9, 2013, 8:11 a.m. UTC | #2

On Mon, Apr 08, 2013 at 11:28:58PM +0100, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
> <[e-mail]> wrote:
> > From: Thierry Reding <[e-mail]>
> >
> >  #endif /* LINUX_MSI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2461033a..6aca43ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -416,6 +416,7 @@ struct pci_bus {
> >         struct resource busn_res;       /* bus numbers routed to this bus */
> >
> >         struct pci_ops  *ops;           /* configuration access functions */
> > +       struct msi_chip *msi;           /* MSI controller */
> 
> "msi" seems like a too-generic name here; it suggests an interrupt or
> IRQ, not a controller.
> 
> I'm not sure this is the correct place for it.  Having it in the
> struct pci_bus means you need arch code to fill it in, e.g., you added
> it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
> that for arches that use pci_scan_root_bus(), which is the direction
> I'd like to go.
> 
> I think it probably should go in sysdata instead.  That would mean you
> can't really do generic weak setup/tear-down functions, because they
> wouldn't know how to pull the MSI controller info out of the
> arch-specific sysdata.  But there are so many levels of weak-ness
> going on there, maybe it would be a good thing to get rid of one :)

But generic setup/tear-down functions would allow for architecture
independent MSI controllers. This would be useful for MSI controllers that
are unrelated to PCI host controllers or a specific architecture. It would
make drivers sit more comfortably in drivers/irqchip or drivers/pci/host. Also
having the msi_chip in struct pci_bus could allow there to exist multiple
MSI controllers on a PCI fabric and is consistent with pci_ops.

Assuming the MSI controller is represented in the device tree and there is a
relationship between the controller and the host bridge
(phandle/interrupt-parent) then as Thierry suggested[1] previously you could call
something like of_find_msi_chip_by_node(node) to locate an msi_chip from a
device node. Could this look up exist in pci_scan_root_bus via its struct
device.of_node? Perhaps pci_create_root_bus can be changed to take a parameter
for msi_chip alongside the ops parameter? The of_find_msi_chip_by_node could
walk up the device tree until it finds an MSI controller.

In the case where device tree isn't used - then I guess the weakly defined
arch_ callbacks would be replaced with the architectures existing
implementation. Or perhaps if MSI controllers are registered (msi_chip_add)
then pci_scan_root_bus could use the first controller it finds.

Also I believe pci_alloc_child_bus function would need to be changed to add
"b->msi = msi" to inherit msi_chip for child buses in the above patch?

Andrew Murray

[1] http://lkml.org/lkml/2013/3/25/67
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding April 9, 2013, 8:18 a.m. UTC | #3

On Mon, Apr 08, 2013 at 04:28:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
> <[e-mail]> wrote:
> > From: Thierry Reding <[e-mail]>
> >
> > The new struct msi_chip is used to associated an MSI controller with a
> > PCI bus. It is automatically handed down from the root to its children
> > during bus enumeration.
> >
> > This patch provides default (weak) implementations for the architecture-
> > specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> > and arch_msi_check_device()) which check if a PCI device's bus has an
> > attached MSI chip and forward the call appropriately.
> >
> > Signed-off-by: Thierry Reding <[e-mail]>
> > Signed-off-by: Thomas Petazzoni <[e-mail]>
> > ---
> >  drivers/pci/msi.c   |   35 +++++++++++++++++++++++++++++++----
> >  drivers/pci/probe.c |    1 +
> >  include/linux/msi.h |   10 ++++++++++
> >  include/linux/pci.h |    1 +
> >  4 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 00cc78c..fce3549 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -26,14 +26,41 @@
> >
> >  static int pci_msi_enable = 1;
> >
> > -/* Arch hooks */
> > +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > +{
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->setup_irq) {
> > +               int err;
> > +
> > +               err = chip->setup_irq(chip, dev, desc);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               irq_set_chip_data(desc->irq, chip);
> > +               return err;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> >
> > -#ifndef arch_msi_check_device
> > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +void __weak arch_teardown_msi_irq(unsigned int irq)
> >  {
> > +       struct msi_chip *chip = irq_get_chip_data(irq);
> > +
> > +       if (chip && chip->teardown_irq)
> > +               chip->teardown_irq(chip, irq);
> > +}
> > +
> > +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> 
> I like the replacement of "#ifndef arch_msi_check_device()" with a
> weak implementation here, but this only does half the job -- shouldn't
> we remove the powerpc "#define arch_msi_check_device
> arch_msi_check_device" at the same time?

Yes, there are a few things that can be done to cleanup the "mess"
around this subsequently. For instance I have a local patch which
completely removes the ARCH_SUPPORTS_MSI symbol because it isn't
useful after making the symbols weak. One other obvious item is to
convert more architectures to implement an MSI chip.

> And since we're touching all the check_device() implementations, maybe
> we could come up with a better name.  "check_device()" conveys
> absolutely no information about what we're checking or what the sense
> of the result is.  arch_msi_supported()?  pcibios_msi_supported()?  I
> guess it should be consistent with the other arch interfaces, so
> arch_*() is probably better.

At least on PowerPC the arch_msi_check_device() hook also refuses to
setup multiple MSI per device because it isn't supported. I can't think
of a better alternative than arch_msi_supported(), though so that's fine
with me.

Perhaps pcibios_msi_supported() wouldn't be so bad either. Other
architecture-specific functions already use that prefix (see the OF
support functions) and it might be good to settle on one convention for
consistency.

That said, the goal is to eventually get rid of all the arch_msi_*()
functions. The only reason they are still there is because I didn't want
to convert everything in one go. So I wanted to put the infrastructure
in place that we need to support multi-platform on ARM (which is given
by this generic MSI chip infrastructure). Later the remaining PCI
architectures can be converted to provide an msi_chip as well, at which
point the now weak implementations can become non-weak and be renamed to
something like pci_{setup,teardown,check}_msi() to make it more obvious
that they are generic.

> Maybe the ugly #ifdef-ery around arch_setup_msi_irqs,
> arch_teardown_msi_irqs, and arch_restore_msi_irqs could be cleaned up
> similarly?  Somebody worked pretty hard to obfuscate all that,
> probably before weak functions were available.

I think Andrew or Jason at one point commented whether they should be
allowed to be implemented by an MSI chip. If so we could use the same
approach as for the other functions.

> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b494066..9307550 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >
> >         child->parent = parent;
> >         child->ops = parent->ops;
> > +       child->msi = parent->msi;
> >         child->sysdata = parent->sysdata;
> >         child->bus_flags = parent->bus_flags;
> >
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index ce93a34..ea4a5be 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
> >  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
> >  extern int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
> >
> > +struct msi_chip {
> > +       struct module *owner;
> > +       struct device *dev;
> > +
> > +       int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> > +                        struct msi_desc *desc);
> > +       void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> > +       int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> > +                           int nvec, int type);
> 
> If we do end up adding interfaces like this (I'm not sure it will work
> -- see below), I think it would be better to pass just the pci_dev,
> not the "msi_chip, pci_dev" pair.  Passing both pointers avoids a
> cheap lookup in the caller, but it adds a way that two inseparable
> things can get out of sync.

Yes, that's a good idea and we can easily do that.

> > +};
> >
> >  #endif /* LINUX_MSI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2461033a..6aca43ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -416,6 +416,7 @@ struct pci_bus {
> >         struct resource busn_res;       /* bus numbers routed to this bus */
> >
> >         struct pci_ops  *ops;           /* configuration access functions */
> > +       struct msi_chip *msi;           /* MSI controller */
> 
> "msi" seems like a too-generic name here; it suggests an interrupt or
> IRQ, not a controller.

It's short and to the point. The bus abstraction doesn't really have any
interrupt functionality, has it? Even for PCI devices the MSI interrupt
number is actually stored in the .irq field, so I don't think it's too
generic. But if you insist I'll think of another name.

> I'm not sure this is the correct place for it.  Having it in the
> struct pci_bus means you need arch code to fill it in, e.g., you added
> it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
> that for arches that use pci_scan_root_bus(), which is the direction
> I'd like to go.

We could add an argument to pci_scan_root_bus() to pass this information
in. It's a bit ugly but see below...

> I think it probably should go in sysdata instead.  That would mean you
> can't really do generic weak setup/tear-down functions, because they
> wouldn't know how to pull the MSI controller info out of the
> arch-specific sysdata.  But there are so many levels of weak-ness
> going on there, maybe it would be a good thing to get rid of one :)

Isn't putting more data into sysdata a step in the opposite direction of
where we want to go? I seem to remember some discussion about wanting to
consolidate the various architecture-specific implementations and move
more common code into the PCI core. If we handle the whole MSI business
in architecture-specific code we gain little to nothing compared to the
current situation.

Thierry

Thierry Reding April 9, 2013, 8:22 a.m. UTC | #4

On Tue, Apr 09, 2013 at 09:11:19AM +0100, Andrew Murray wrote:
[...]
> Also I believe pci_alloc_child_bus function would need to be changed to add
> "b->msi = msi" to inherit msi_chip for child buses in the above patch?

The patch already does:

	child->msi = parent->msi;

in pci_alloc_child_bus(), the same way that ops is inherited. I have
admittedly only tested this code on Tegra, but I don't see why that
wouldn't be enough. Or maybe I haven't understood your question?

Thierry

Andrew Murray April 9, 2013, 8:25 a.m. UTC | #5

On Tue, Apr 09, 2013 at 09:22:33AM +0100, Thierry Reding wrote:
> On Tue, Apr 09, 2013 at 09:11:19AM +0100, Andrew Murray wrote:
> [...]
> > Also I believe pci_alloc_child_bus function would need to be changed to add
> > "b->msi = msi" to inherit msi_chip for child buses in the above patch?
> 
> The patch already does:
> 
> 	child->msi = parent->msi;
> 
> in pci_alloc_child_bus(), the same way that ops is inherited. I have
> admittedly only tested this code on Tegra, but I don't see why that
> wouldn't be enough. Or maybe I haven't understood your question?

Sorry I missed that part of your patch - that was exacatly what I was
hoping to see.

Andrew Murray
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 00cc78c..fce3549 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -26,14 +26,41 @@ 
 
 static int pci_msi_enable = 1;
 
-/* Arch hooks */
+int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
+{
+	struct msi_chip *chip = dev->bus->msi;
+
+	if (chip && chip->setup_irq) {
+		int err;
+
+		err = chip->setup_irq(chip, dev, desc);
+		if (err < 0)
+			return err;
+
+		irq_set_chip_data(desc->irq, chip);
+		return err;
+	}
+
+	return -EINVAL;
+}
 
-#ifndef arch_msi_check_device
-int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
+void __weak arch_teardown_msi_irq(unsigned int irq)
 {
+	struct msi_chip *chip = irq_get_chip_data(irq);
+
+	if (chip && chip->teardown_irq)
+		chip->teardown_irq(chip, irq);
+}
+
+int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
+{
+	struct msi_chip *chip = dev->bus->msi;
+
+	if (chip && chip->check_device)
+		return chip->check_device(chip, dev, nvec, type);
+
 	return 0;
 }
-#endif
 
 #ifndef arch_setup_msi_irqs
 # define arch_setup_msi_irqs default_setup_msi_irqs
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b494066..9307550 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -634,6 +634,7 @@  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 
 	child->parent = parent;
 	child->ops = parent->ops;
+	child->msi = parent->msi;
 	child->sysdata = parent->sysdata;
 	child->bus_flags = parent->bus_flags;
 
diff --git a/include/linux/msi.h b/include/linux/msi.h
index ce93a34..ea4a5be 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -58,5 +58,15 @@  extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 extern void arch_teardown_msi_irqs(struct pci_dev *dev);
 extern int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
 
+struct msi_chip {
+	struct module *owner;
+	struct device *dev;
+
+	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
+			 struct msi_desc *desc);
+	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
+	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
+			    int nvec, int type);
+};
 
 #endif /* LINUX_MSI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2461033a..6aca43ea 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -416,6 +416,7 @@  struct pci_bus {
 	struct resource busn_res;	/* bus numbers routed to this bus */
 
 	struct pci_ops	*ops;		/* configuration access functions */
+	struct msi_chip	*msi;		/* MSI controller */
 	void		*sysdata;	/* hook for sys-specific extension */
 	struct proc_dir_entry *procdir;	/* directory entry in /proc/bus/pci */