| Message ID | 1372686136-1370-6-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Jul 1, 2013 at 7:42 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > This commit adds a very basic registry of msi_chip structures, so that > an IRQ controller driver can register an msi_chip, and a PCIe host > controller can find it, based on a 'struct device_node'. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/of/of_pci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/msi.h | 2 ++ > include/linux/of_pci.h | 12 ++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 42c687a..f516632 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -89,3 +89,43 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res) > return 0; > } > EXPORT_SYMBOL_GPL(of_pci_parse_bus_range); > + > +#ifdef CONFIG_PCI_MSI > + > +static LIST_HEAD(msi_chip_list); > +static DEFINE_MUTEX(msi_chip_mutex); > + > +int of_msi_chip_add(struct msi_chip *chip) > +{ > + if (! of_property_read_bool(chip->of_node, "msi-controller")) The space between "! of_property..." is atypical. > + return -EINVAL; > + > + mutex_lock(&msi_chip_mutex); > + list_add(&chip->list, &msi_chip_list); > + mutex_unlock(&msi_chip_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_add); > + > +void of_msi_chip_remove(struct msi_chip *chip) > +{ > + mutex_lock(&msi_chip_mutex); > + list_del(&chip->list); > + mutex_unlock(&msi_chip_mutex); > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_remove); > + > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node) > +{ > + struct msi_chip *c; Normally there's a blank line here. The list traversal below isn't safe, is it? A simultaneous remove, e.g., of an MSI chip unrelated to the one we're looking up, might change the list while we're traversing it. > + list_for_each_entry(c, &msi_chip_list, list) { > + if (c->of_node == of_node) > + return c; > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(of_find_msi_chip_by_node); > + > +#endif /* CONFIG_PCI_MSI */ > diff --git a/include/linux/msi.h b/include/linux/msi.h > index 5b357d92..9e1a44b 100644 > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -66,6 +66,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev); > struct msi_chip { > struct module *owner; > struct device *dev; > + struct device_node *of_node; > + struct list_head list; > > int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev, > struct msi_desc *desc); > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h > index 7a04826..99e4361 100644 > --- a/include/linux/of_pci.h > +++ b/include/linux/of_pci.h > @@ -2,6 +2,7 @@ > #define __OF_PCI_H > > #include <linux/pci.h> > +#include <linux/msi.h> > > struct pci_dev; > struct of_irq; > @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, > int of_pci_get_devfn(struct device_node *np); > int of_pci_parse_bus_range(struct device_node *node, struct resource *res); > > +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI) > +int of_msi_chip_add(struct msi_chip *chip); > +void of_msi_chip_remove(struct msi_chip *chip); > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node); > +#else > +static inline int of_msi_chip_add(struct msi_chip *chip) { return -EINVAL; } > +static inline void of_msi_chip_remove(struct msi_chip *chip) { } > +static inline struct msi_chip * > +of_find_msi_chip_by_node(struct device_node *of_node) { return NULL }; > +#endif > + > #endif > -- > 1.8.1.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Bjorn Helgaas, On Fri, 5 Jul 2013 15:56:55 -0600, Bjorn Helgaas wrote: > > +int of_msi_chip_add(struct msi_chip *chip) > > +{ > > + if (! of_property_read_bool(chip->of_node, "msi-controller")) > > The space between "! of_property..." is atypical. Indeed. > > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node) > > +{ > > + struct msi_chip *c; > > Normally there's a blank line here. > > The list traversal below isn't safe, is it? A simultaneous remove, > e.g., of an MSI chip unrelated to the one we're looking up, might > change the list while we're traversing it. True, will fix! Thomas
Grant, Rob, Would it be possible to get your feeling about the below patch? Are you ok with the idea of this msi_chip registry, based on device_node pointer? This is the last piece of this patch set that needs an approval in terms of general approach. I am at Linaro Connect this week, so if you want to discuss this live, do not hesitate to ping me. Thanks a lot, Thomas On Mon, 1 Jul 2013 15:42:10 +0200, Thomas Petazzoni wrote: > This commit adds a very basic registry of msi_chip structures, so that > an IRQ controller driver can register an msi_chip, and a PCIe host > controller can find it, based on a 'struct device_node'. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/of/of_pci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/msi.h | 2 ++ > include/linux/of_pci.h | 12 ++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 42c687a..f516632 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -89,3 +89,43 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res) > return 0; > } > EXPORT_SYMBOL_GPL(of_pci_parse_bus_range); > + > +#ifdef CONFIG_PCI_MSI > + > +static LIST_HEAD(msi_chip_list); > +static DEFINE_MUTEX(msi_chip_mutex); > + > +int of_msi_chip_add(struct msi_chip *chip) > +{ > + if (! of_property_read_bool(chip->of_node, "msi-controller")) > + return -EINVAL; > + > + mutex_lock(&msi_chip_mutex); > + list_add(&chip->list, &msi_chip_list); > + mutex_unlock(&msi_chip_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_add); > + > +void of_msi_chip_remove(struct msi_chip *chip) > +{ > + mutex_lock(&msi_chip_mutex); > + list_del(&chip->list); > + mutex_unlock(&msi_chip_mutex); > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_remove); > + > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node) > +{ > + struct msi_chip *c; > + list_for_each_entry(c, &msi_chip_list, list) { > + if (c->of_node == of_node) > + return c; > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(of_find_msi_chip_by_node); > + > +#endif /* CONFIG_PCI_MSI */ > diff --git a/include/linux/msi.h b/include/linux/msi.h > index 5b357d92..9e1a44b 100644 > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -66,6 +66,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev); > struct msi_chip { > struct module *owner; > struct device *dev; > + struct device_node *of_node; > + struct list_head list; > > int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev, > struct msi_desc *desc); > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h > index 7a04826..99e4361 100644 > --- a/include/linux/of_pci.h > +++ b/include/linux/of_pci.h > @@ -2,6 +2,7 @@ > #define __OF_PCI_H > > #include <linux/pci.h> > +#include <linux/msi.h> > > struct pci_dev; > struct of_irq; > @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, > int of_pci_get_devfn(struct device_node *np); > int of_pci_parse_bus_range(struct device_node *node, struct resource *res); > > +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI) > +int of_msi_chip_add(struct msi_chip *chip); > +void of_msi_chip_remove(struct msi_chip *chip); > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node); > +#else > +static inline int of_msi_chip_add(struct msi_chip *chip) { return -EINVAL; } > +static inline void of_msi_chip_remove(struct msi_chip *chip) { } > +static inline struct msi_chip * > +of_find_msi_chip_by_node(struct device_node *of_node) { return NULL }; > +#endif > + > #endif
On Mon, Jul 1, 2013 at 8:42 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > This commit adds a very basic registry of msi_chip structures, so that > an IRQ controller driver can register an msi_chip, and a PCIe host > controller can find it, based on a 'struct device_node'. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/of/of_pci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/msi.h | 2 ++ > include/linux/of_pci.h | 12 ++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 42c687a..f516632 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -89,3 +89,43 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res) > return 0; > } > EXPORT_SYMBOL_GPL(of_pci_parse_bus_range); > + > +#ifdef CONFIG_PCI_MSI > + > +static LIST_HEAD(msi_chip_list); > +static DEFINE_MUTEX(msi_chip_mutex); > + > +int of_msi_chip_add(struct msi_chip *chip) Perhaps of_pci_msi_chip_add instead. > +{ > + if (! of_property_read_bool(chip->of_node, "msi-controller")) > + return -EINVAL; > + > + mutex_lock(&msi_chip_mutex); > + list_add(&chip->list, &msi_chip_list); > + mutex_unlock(&msi_chip_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_add); > + > +void of_msi_chip_remove(struct msi_chip *chip) ditto > +{ > + mutex_lock(&msi_chip_mutex); > + list_del(&chip->list); > + mutex_unlock(&msi_chip_mutex); > +} > +EXPORT_SYMBOL_GPL(of_msi_chip_remove); > + > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node) ditto. > +{ > + struct msi_chip *c; > + list_for_each_entry(c, &msi_chip_list, list) { Need the safe variant here? Rob > + if (c->of_node == of_node) > + return c; > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(of_find_msi_chip_by_node); > + > +#endif /* CONFIG_PCI_MSI */ > diff --git a/include/linux/msi.h b/include/linux/msi.h > index 5b357d92..9e1a44b 100644 > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -66,6 +66,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev); > struct msi_chip { > struct module *owner; > struct device *dev; > + struct device_node *of_node; > + struct list_head list; > > int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev, > struct msi_desc *desc); > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h > index 7a04826..99e4361 100644 > --- a/include/linux/of_pci.h > +++ b/include/linux/of_pci.h > @@ -2,6 +2,7 @@ > #define __OF_PCI_H > > #include <linux/pci.h> > +#include <linux/msi.h> > > struct pci_dev; > struct of_irq; > @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, > int of_pci_get_devfn(struct device_node *np); > int of_pci_parse_bus_range(struct device_node *node, struct resource *res); > > +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI) > +int of_msi_chip_add(struct msi_chip *chip); > +void of_msi_chip_remove(struct msi_chip *chip); > +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node); > +#else > +static inline int of_msi_chip_add(struct msi_chip *chip) { return -EINVAL; } > +static inline void of_msi_chip_remove(struct msi_chip *chip) { } > +static inline struct msi_chip * > +of_find_msi_chip_by_node(struct device_node *of_node) { return NULL }; > +#endif > + > #endif > -- > 1.8.1.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob, On Tue, 9 Jul 2013 08:43:36 -0500, Rob Herring wrote: > > +int of_msi_chip_add(struct msi_chip *chip) > > Perhaps of_pci_msi_chip_add instead. Sure, makes sense. > > +{ > > + struct msi_chip *c; > > + list_for_each_entry(c, &msi_chip_list, list) { > > Need the safe variant here? As suggested by Bjorn, I've changed this function to grab the msi_chip_mutex while traversing the list. Does your e-mail implies that the general approach seems ok? If so, I'll resend an updated v5 version taking into account the comments that I have received on this v4. Thanks! Thomas
On Tue, Jul 9, 2013 at 8:01 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Tue, 9 Jul 2013 08:43:36 -0500, Rob Herring wrote: >> > +{ >> > + struct msi_chip *c; >> > + list_for_each_entry(c, &msi_chip_list, list) { >> >> Need the safe variant here? > > As suggested by Bjorn, I've changed this function to grab the > msi_chip_mutex while traversing the list. The "safe" list functions don't do any mutual exclusion. The only safety they provide is that we won't go in the weeds if the body of the loop deletes the current list entry. This loop doesn't delete entries, so we don't need the safe variant. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2013 09:01 AM, Thomas Petazzoni wrote: > Rob, > > On Tue, 9 Jul 2013 08:43:36 -0500, Rob Herring wrote: > >>> +int of_msi_chip_add(struct msi_chip *chip) >> >> Perhaps of_pci_msi_chip_add instead. > > Sure, makes sense. > >>> +{ >>> + struct msi_chip *c; >>> + list_for_each_entry(c, &msi_chip_list, list) { >> >> Need the safe variant here? > > As suggested by Bjorn, I've changed this function to grab the > msi_chip_mutex while traversing the list. > > Does your e-mail implies that the general approach seems ok? If so, > I'll resend an updated v5 version taking into account the comments > that I have received on this v4. Yes. Since it is only trivial comments, you can add my ack. Acked-by: Rob Herring <rob.herring@calxeda.com> Rob > > Thanks! > > Thomas > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 42c687a..f516632 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -89,3 +89,43 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res) return 0; } EXPORT_SYMBOL_GPL(of_pci_parse_bus_range); + +#ifdef CONFIG_PCI_MSI + +static LIST_HEAD(msi_chip_list); +static DEFINE_MUTEX(msi_chip_mutex); + +int of_msi_chip_add(struct msi_chip *chip) +{ + if (! of_property_read_bool(chip->of_node, "msi-controller")) + return -EINVAL; + + mutex_lock(&msi_chip_mutex); + list_add(&chip->list, &msi_chip_list); + mutex_unlock(&msi_chip_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(of_msi_chip_add); + +void of_msi_chip_remove(struct msi_chip *chip) +{ + mutex_lock(&msi_chip_mutex); + list_del(&chip->list); + mutex_unlock(&msi_chip_mutex); +} +EXPORT_SYMBOL_GPL(of_msi_chip_remove); + +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node) +{ + struct msi_chip *c; + list_for_each_entry(c, &msi_chip_list, list) { + if (c->of_node == of_node) + return c; + } + + return NULL; +} +EXPORT_SYMBOL_GPL(of_find_msi_chip_by_node); + +#endif /* CONFIG_PCI_MSI */ diff --git a/include/linux/msi.h b/include/linux/msi.h index 5b357d92..9e1a44b 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -66,6 +66,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev); struct msi_chip { struct module *owner; struct device *dev; + struct device_node *of_node; + struct list_head list; int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev, struct msi_desc *desc); diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 7a04826..99e4361 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -2,6 +2,7 @@ #define __OF_PCI_H #include <linux/pci.h> +#include <linux/msi.h> struct pci_dev; struct of_irq; @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, int of_pci_get_devfn(struct device_node *np); int of_pci_parse_bus_range(struct device_node *node, struct resource *res); +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI) +int of_msi_chip_add(struct msi_chip *chip); +void of_msi_chip_remove(struct msi_chip *chip); +struct msi_chip *of_find_msi_chip_by_node(struct device_node *of_node); +#else +static inline int of_msi_chip_add(struct msi_chip *chip) { return -EINVAL; } +static inline void of_msi_chip_remove(struct msi_chip *chip) { } +static inline struct msi_chip * +of_find_msi_chip_by_node(struct device_node *of_node) { return NULL }; +#endif + #endif
This commit adds a very basic registry of msi_chip structures, so that an IRQ controller driver can register an msi_chip, and a PCIe host controller can find it, based on a 'struct device_node'. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/of/of_pci.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/msi.h | 2 ++ include/linux/of_pci.h | 12 ++++++++++++ 3 files changed, 54 insertions(+)