| Message ID | 1369583597-3801-2-git-send-email-jiang.liu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, May 26, 2013 at 8:52 AM, Jiang Liu <liuj97@gmail.com> wrote: > Introduce hotplug-safe PCI bus iterators as below, which hold a > reference on the returned PCI bus object. > bool pci_bus_exists(int domain, int busnr); > struct pci_bus *pci_get_bus(int domain, int busnr); > struct pci_bus *pci_get_next_bus(struct pci_bus *from); > struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); > #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) > #define for_each_pci_root_bus(b) \ > for (b = NULL; (b = pci_get_next_root_bus(b)); ) > > The long-term goal is to remove hotplug-unsafe pci_find_bus(), > pci_find_next_bus() and the global pci_root_buses list. > > These new interfaces may be a littler slower than existing interfaces, > but it should be acceptable because they are not used on hot paths. looks like go over all buses to find out several root buses is not good. > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > Acked-by: Yinghai Lu <yinghai@kernel.org> I take that back. > Cc: linux-pci@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 2 +- > drivers/pci/search.c | 159 +++++++++++++++++++++++++++++++++++++++++---------- > include/linux/pci.h | 23 +++++++- > 4 files changed, 153 insertions(+), 32 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 68678ed..8fe15f6 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } > > /* Lock for read/write access to pci device and bus lists */ > extern struct rw_semaphore pci_bus_sem; > +extern struct class pcibus_class; > > extern raw_spinlock_t pci_lock; > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2830070..1004a05 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) > kfree(pci_bus); > } > > -static struct class pcibus_class = { > +struct class pcibus_class = { > .name = "pci_bus", > .dev_release = &release_pcibus_dev, > .dev_attrs = pcibus_dev_attrs, ... > +static int pci_match_next_root_bus(struct device *dev, const void *data) > +{ > + return pci_is_root_bus(to_pci_bus(dev)); > } ... > + > +/** > + * pci_find_next_root_bus - begin or continue searching for a PCI root bus > + * @from: Previous PCI bus found, or %NULL for new search. > + * > + * Iterates through the list of known PCI root busses. If a PCI bus is found, > + * the reference count to the root bus is incremented and a pointer to it is > + * returned. Otherwise, %NULL is returned. A new search is initiated by > + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, > + * searches continue from next root bus on the global list. The reference > + * count for @from is always decremented if it is not %NULL. > + */ > +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) > +{ > + struct device *dev = from ? &from->dev : NULL; > + > + WARN_ON(in_interrupt()); > + dev = class_find_device(&pcibus_class, dev, NULL, > + &pci_match_next_root_bus); > + pci_bus_put(from); > + if (dev) > + return to_pci_bus(dev); > + > + return NULL; > } > +EXPORT_SYMBOL(pci_get_next_root_bus); this patchset is most doing same thing like my for_each_host_bridge patchset. that patchset add bus_type for host_bridge and loop with each_...bus to find host_bridge and to its bus. looks like for each host bridge should be right direction. also late we could add per host bridge lock instead of whole pci bus sem etc. Thanks Yinghai -- 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 Tue 28 May 2013 12:22:29 PM CST, Yinghai Lu wrote: > On Sun, May 26, 2013 at 8:52 AM, Jiang Liu <liuj97@gmail.com> wrote: >> Introduce hotplug-safe PCI bus iterators as below, which hold a >> reference on the returned PCI bus object. >> bool pci_bus_exists(int domain, int busnr); >> struct pci_bus *pci_get_bus(int domain, int busnr); >> struct pci_bus *pci_get_next_bus(struct pci_bus *from); >> struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >> #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >> #define for_each_pci_root_bus(b) \ >> for (b = NULL; (b = pci_get_next_root_bus(b)); ) >> >> The long-term goal is to remove hotplug-unsafe pci_find_bus(), >> pci_find_next_bus() and the global pci_root_buses list. >> >> These new interfaces may be a littler slower than existing interfaces, >> but it should be acceptable because they are not used on hot paths. > > looks like go over all buses to find out several root buses is not good. > > >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> Acked-by: Yinghai Lu <yinghai@kernel.org> > > I take that back. > >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/pci/pci.h | 1 + >> drivers/pci/probe.c | 2 +- >> drivers/pci/search.c | 159 +++++++++++++++++++++++++++++++++++++++++---------- >> include/linux/pci.h | 23 +++++++- >> 4 files changed, 153 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index 68678ed..8fe15f6 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } >> >> /* Lock for read/write access to pci device and bus lists */ >> extern struct rw_semaphore pci_bus_sem; >> +extern struct class pcibus_class; >> >> extern raw_spinlock_t pci_lock; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 2830070..1004a05 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) >> kfree(pci_bus); >> } >> >> -static struct class pcibus_class = { >> +struct class pcibus_class = { >> .name = "pci_bus", >> .dev_release = &release_pcibus_dev, >> .dev_attrs = pcibus_dev_attrs, > ... >> +static int pci_match_next_root_bus(struct device *dev, const void *data) >> +{ >> + return pci_is_root_bus(to_pci_bus(dev)); >> } > ... >> + >> +/** >> + * pci_find_next_root_bus - begin or continue searching for a PCI root bus >> + * @from: Previous PCI bus found, or %NULL for new search. >> + * >> + * Iterates through the list of known PCI root busses. If a PCI bus is found, >> + * the reference count to the root bus is incremented and a pointer to it is >> + * returned. Otherwise, %NULL is returned. A new search is initiated by >> + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, >> + * searches continue from next root bus on the global list. The reference >> + * count for @from is always decremented if it is not %NULL. >> + */ >> +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) >> +{ >> + struct device *dev = from ? &from->dev : NULL; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, dev, NULL, >> + &pci_match_next_root_bus); >> + pci_bus_put(from); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> } >> +EXPORT_SYMBOL(pci_get_next_root_bus); > > this patchset is most doing same thing like my for_each_host_bridge patchset. > that patchset add bus_type for host_bridge and loop with each_...bus > to find host_bridge > and to its bus. > > looks like for each host bridge should be right direction. > > also late we could add per host bridge lock instead of whole pci bus sem etc. > > Thanks > > Yinghai Hi Yinghai, Thanks for review! Actually this patchset is inspired by your for_each_host_bridge() patchset but trying to close some race windows. Currently pci_root_bus holds a reference to pci_host_bridge, so it's always safe to reference pci_root_bus->bridge. And pci_host_bridge doesn't hold reference to pci_root_bus, so we can't safely reference pci_host_bridge->bus. And it's hard to make pci_host_bridge to hold a reference to pci_root_bus because that will cause loop. So we try to achieve the same goal with this patchset, but with some level of inefficiency to search all PCI buses for root buses. The third patchset of this series is to introduce a PCI bus lock mechanism to solve many risk conditions in PCI host bridge/device hotplug we are facing recently. We have done basic tests against the new lock mechanism and seems it has solved all those risk conditions found by us and Gu Zheng. But we are still working on improving the third patchset. To all, seems we are trying to achieve the same goal with different approaches. Regards! Gerry -- 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 Sun, May 26, 2013 at 11:52:58PM +0800, Jiang Liu wrote: > Introduce hotplug-safe PCI bus iterators as below, which hold a > reference on the returned PCI bus object. > bool pci_bus_exists(int domain, int busnr); > struct pci_bus *pci_get_bus(int domain, int busnr); > struct pci_bus *pci_get_next_bus(struct pci_bus *from); > struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); > #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) > #define for_each_pci_root_bus(b) \ > for (b = NULL; (b = pci_get_next_root_bus(b)); ) > > The long-term goal is to remove hotplug-unsafe pci_find_bus(), > pci_find_next_bus() and the global pci_root_buses list. I think you should mark the unsafe interfaces as __deprecated so users will get compile-time warnings. I don't think pci_bus_exists() is a safe interface, because the value it returns may be incorrect before the caller can look at it. The only safe thing would be to make it so we try to create the bus and return failure if it already exists. Then the mutex can be in the code that creates the bus. I don't see any uses of for_each_pci_bus(), so please remove that. It sounds like we don't have a consensus on how to iterate over PCI root buses. If you separate that from the pci_get_bus() changes, maybe we can at least move forward on the pci_get_bus() stuff. Bjorn > These new interfaces may be a littler slower than existing interfaces, > but it should be acceptable because they are not used on hot paths. > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > Acked-by: Yinghai Lu <yinghai@kernel.org> > Cc: linux-pci@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 2 +- > drivers/pci/search.c | 159 +++++++++++++++++++++++++++++++++++++++++---------- > include/linux/pci.h | 23 +++++++- > 4 files changed, 153 insertions(+), 32 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 68678ed..8fe15f6 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } > > /* Lock for read/write access to pci device and bus lists */ > extern struct rw_semaphore pci_bus_sem; > +extern struct class pcibus_class; > > extern raw_spinlock_t pci_lock; > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2830070..1004a05 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) > kfree(pci_bus); > } > > -static struct class pcibus_class = { > +struct class pcibus_class = { > .name = "pci_bus", > .dev_release = &release_pcibus_dev, > .dev_attrs = pcibus_dev_attrs, > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index d0627fa..16ccaf8 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -52,20 +52,27 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev) > return tmp; > } > > -static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) > +struct pci_bus_match_arg { > + int domain; > + int bus; > +}; > + > +static int pci_match_bus(struct device *dev, const void *data) > { > - struct pci_bus* child; > - struct list_head *tmp; > + struct pci_bus *bus = to_pci_bus(dev); > + const struct pci_bus_match_arg *arg = data; > > - if(bus->number == busnr) > - return bus; > + return (pci_domain_nr(bus) == arg->domain && bus->number == arg->bus); > +} > > - list_for_each(tmp, &bus->children) { > - child = pci_do_find_bus(pci_bus_b(tmp), busnr); > - if(child) > - return child; > - } > - return NULL; > +static int pci_match_next_bus(struct device *dev, const void *data) > +{ > + return 1; > +} > + > +static int pci_match_next_root_bus(struct device *dev, const void *data) > +{ > + return pci_is_root_bus(to_pci_bus(dev)); > } > > /** > @@ -76,20 +83,19 @@ static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) > * Given a PCI bus number and domain number, the desired PCI bus is located > * in the global list of PCI buses. If the bus is found, a pointer to its > * data structure is returned. If no bus is found, %NULL is returned. > + * > + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. > + * Please use pci_get_bus() instead which holds a reference on the returned > + * PCI bus. > */ > -struct pci_bus * pci_find_bus(int domain, int busnr) > +struct pci_bus *pci_find_bus(int domain, int busnr) > { > - struct pci_bus *bus = NULL; > - struct pci_bus *tmp_bus; > + struct pci_bus *bus; > > - while ((bus = pci_find_next_bus(bus)) != NULL) { > - if (pci_domain_nr(bus) != domain) > - continue; > - tmp_bus = pci_do_find_bus(bus, busnr); > - if (tmp_bus) > - return tmp_bus; > - } > - return NULL; > + bus = pci_get_bus(domain, busnr); > + pci_bus_put(bus); > + > + return bus; > } > > /** > @@ -100,21 +106,114 @@ struct pci_bus * pci_find_bus(int domain, int busnr) > * initiated by passing %NULL as the @from argument. Otherwise if > * @from is not %NULL, searches continue from next device on the > * global list. > + * > + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. > + * Please use pci_get_next_root_bus() instead which holds a reference > + * on the returned PCI root bus. > */ > struct pci_bus * > pci_find_next_bus(const struct pci_bus *from) > { > - struct list_head *n; > - struct pci_bus *b = NULL; > + struct device *dev = from ? (struct device *)&from->dev : NULL; > + > + dev = class_find_device(&pcibus_class, dev, NULL, > + &pci_match_next_root_bus); > + if (dev) { > + put_device(dev); > + return to_pci_bus(dev); > + } > + > + return NULL; > +} > + > +bool pci_bus_exists(int domain, int busnr) > +{ > + struct device *dev; > + struct pci_bus_match_arg arg = { domain, busnr }; > > WARN_ON(in_interrupt()); > - down_read(&pci_bus_sem); > - n = from ? from->node.next : pci_root_buses.next; > - if (n != &pci_root_buses) > - b = pci_bus_b(n); > - up_read(&pci_bus_sem); > - return b; > + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); > + if (dev) > + put_device(dev); > + > + return dev != NULL; > +} > +EXPORT_SYMBOL(pci_bus_exists); > + > +/** > + * pci_get_bus - locate PCI bus from a given domain and bus number > + * @domain: number of PCI domain to search > + * @busnr: number of desired PCI bus > + * > + * Given a PCI bus number and domain number, the desired PCI bus is located. > + * If the bus is found, a pointer to its data structure is returned. > + * If no bus is found, %NULL is returned. > + * Caller needs to release the returned bus by calling pci_bus_put(). > + */ > +struct pci_bus *pci_get_bus(int domain, int busnr) > +{ > + struct device *dev; > + struct pci_bus_match_arg arg = { domain, busnr }; > + > + WARN_ON(in_interrupt()); > + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); > + if (dev) > + return to_pci_bus(dev); > + > + return NULL; > +} > +EXPORT_SYMBOL(pci_get_bus); > + > +/** > + * pci_get_next_bus - begin or continue searching for a PCI bus > + * @from: Previous PCI bus found, or %NULL for new search. > + * > + * Iterates through the list of known PCI busses. If a PCI bus is found, > + * the reference count to the bus is incremented and a pointer to it is > + * returned. Otherwise, %NULL is returned. A new search is initiated by > + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, > + * searches continue from next bus on the global list. The reference count > + * for @from is always decremented if it is not %NULL. > + */ > +struct pci_bus *pci_get_next_bus(struct pci_bus *from) > +{ > + struct device *dev = from ? &from->dev : NULL; > + > + WARN_ON(in_interrupt()); > + dev = class_find_device(&pcibus_class, dev, NULL, &pci_match_next_bus); > + pci_bus_put(from); > + if (dev) > + return to_pci_bus(dev); > + > + return NULL; > +} > +EXPORT_SYMBOL(pci_get_next_bus); > + > +/** > + * pci_find_next_root_bus - begin or continue searching for a PCI root bus > + * @from: Previous PCI bus found, or %NULL for new search. > + * > + * Iterates through the list of known PCI root busses. If a PCI bus is found, > + * the reference count to the root bus is incremented and a pointer to it is > + * returned. Otherwise, %NULL is returned. A new search is initiated by > + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, > + * searches continue from next root bus on the global list. The reference > + * count for @from is always decremented if it is not %NULL. > + */ > +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) > +{ > + struct device *dev = from ? &from->dev : NULL; > + > + WARN_ON(in_interrupt()); > + dev = class_find_device(&pcibus_class, dev, NULL, > + &pci_match_next_root_bus); > + pci_bus_put(from); > + if (dev) > + return to_pci_bus(dev); > + > + return NULL; > } > +EXPORT_SYMBOL(pci_get_next_root_bus); > > /** > * pci_get_slot - locate PCI device for a given PCI slot > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 7b23fa0..1e43423 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -454,6 +454,9 @@ struct pci_bus { > > #define pci_bus_b(n) list_entry(n, struct pci_bus, node) > #define to_pci_bus(n) container_of(n, struct pci_bus, dev) > +#define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) > +#define for_each_pci_root_bus(b) \ > + for (b = NULL; (b = pci_get_next_root_bus(b)); ) > > /* > * Returns true if the pci bus is root (behind host-pci bridge), > @@ -718,7 +721,6 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region, > void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res, > struct pci_bus_region *region); > void pcibios_scan_specific_bus(int busn); > -struct pci_bus *pci_find_bus(int domain, int busnr); > void pci_bus_add_devices(const struct pci_bus *bus); > struct pci_bus * __deprecated pci_scan_bus_parented(struct device *parent, > int bus, struct pci_ops *ops, void *sysdata); > @@ -778,8 +780,15 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap); > int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap); > int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); > int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); > + > +struct pci_bus *pci_find_bus(int domain, int busnr); > struct pci_bus *pci_find_next_bus(const struct pci_bus *from); > > +bool pci_bus_exists(int domain, int busnr); > +struct pci_bus *pci_get_bus(int domain, int busnr); > +struct pci_bus *pci_get_next_bus(struct pci_bus *from); > +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); > + > struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, > struct pci_dev *from); > struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, > @@ -1430,6 +1439,18 @@ static inline void pci_unblock_cfg_access(struct pci_dev *dev) > static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from) > { return NULL; } > > +static inline bool pci_bus_exists(int domain, int busnr) > +{ return false; } > + > +static inline struct pci_bus *pci_get_bus(int domain, int busnr) > +{ return NULL; } > + > +static inline struct pci_bus *pci_get_next_bus(struct pci_bus *from) > +{ return NULL; } > + > +static inline struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) > +{ return NULL; } > + > static inline struct pci_dev *pci_get_slot(struct pci_bus *bus, > unsigned int devfn) > { return NULL; } > -- > 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
On 06/18/2013 04:06 AM, Bjorn Helgaas wrote: > On Sun, May 26, 2013 at 11:52:58PM +0800, Jiang Liu wrote: >> Introduce hotplug-safe PCI bus iterators as below, which hold a >> reference on the returned PCI bus object. >> bool pci_bus_exists(int domain, int busnr); >> struct pci_bus *pci_get_bus(int domain, int busnr); >> struct pci_bus *pci_get_next_bus(struct pci_bus *from); >> struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >> #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >> #define for_each_pci_root_bus(b) \ >> for (b = NULL; (b = pci_get_next_root_bus(b)); ) >> >> The long-term goal is to remove hotplug-unsafe pci_find_bus(), >> pci_find_next_bus() and the global pci_root_buses list. > > I think you should mark the unsafe interfaces as __deprecated so > users will get compile-time warnings. > > I don't think pci_bus_exists() is a safe interface, because the value > it returns may be incorrect before the caller can look at it. The > only safe thing would be to make it so we try to create the bus > and return failure if it already exists. Then the mutex can be in > the code that creates the bus. > > I don't see any uses of for_each_pci_bus(), so please remove that. > > It sounds like we don't have a consensus on how to iterate over > PCI root buses. If you separate that from the pci_get_bus() > changes, maybe we can at least move forward on the pci_get_bus() > stuff. Hi Bjorn, Thanks for review, will send out next version according to your suggestions. And need more investigation about pci_bus_exists() interface. Regards! Gerry > > Bjorn > >> These new interfaces may be a littler slower than existing interfaces, >> but it should be acceptable because they are not used on hot paths. >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> Acked-by: Yinghai Lu <yinghai@kernel.org> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/pci/pci.h | 1 + >> drivers/pci/probe.c | 2 +- >> drivers/pci/search.c | 159 +++++++++++++++++++++++++++++++++++++++++---------- >> include/linux/pci.h | 23 +++++++- >> 4 files changed, 153 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index 68678ed..8fe15f6 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } >> >> /* Lock for read/write access to pci device and bus lists */ >> extern struct rw_semaphore pci_bus_sem; >> +extern struct class pcibus_class; >> >> extern raw_spinlock_t pci_lock; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 2830070..1004a05 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) >> kfree(pci_bus); >> } >> >> -static struct class pcibus_class = { >> +struct class pcibus_class = { >> .name = "pci_bus", >> .dev_release = &release_pcibus_dev, >> .dev_attrs = pcibus_dev_attrs, >> diff --git a/drivers/pci/search.c b/drivers/pci/search.c >> index d0627fa..16ccaf8 100644 >> --- a/drivers/pci/search.c >> +++ b/drivers/pci/search.c >> @@ -52,20 +52,27 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev) >> return tmp; >> } >> >> -static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) >> +struct pci_bus_match_arg { >> + int domain; >> + int bus; >> +}; >> + >> +static int pci_match_bus(struct device *dev, const void *data) >> { >> - struct pci_bus* child; >> - struct list_head *tmp; >> + struct pci_bus *bus = to_pci_bus(dev); >> + const struct pci_bus_match_arg *arg = data; >> >> - if(bus->number == busnr) >> - return bus; >> + return (pci_domain_nr(bus) == arg->domain && bus->number == arg->bus); >> +} >> >> - list_for_each(tmp, &bus->children) { >> - child = pci_do_find_bus(pci_bus_b(tmp), busnr); >> - if(child) >> - return child; >> - } >> - return NULL; >> +static int pci_match_next_bus(struct device *dev, const void *data) >> +{ >> + return 1; >> +} >> + >> +static int pci_match_next_root_bus(struct device *dev, const void *data) >> +{ >> + return pci_is_root_bus(to_pci_bus(dev)); >> } >> >> /** >> @@ -76,20 +83,19 @@ static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) >> * Given a PCI bus number and domain number, the desired PCI bus is located >> * in the global list of PCI buses. If the bus is found, a pointer to its >> * data structure is returned. If no bus is found, %NULL is returned. >> + * >> + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. >> + * Please use pci_get_bus() instead which holds a reference on the returned >> + * PCI bus. >> */ >> -struct pci_bus * pci_find_bus(int domain, int busnr) >> +struct pci_bus *pci_find_bus(int domain, int busnr) >> { >> - struct pci_bus *bus = NULL; >> - struct pci_bus *tmp_bus; >> + struct pci_bus *bus; >> >> - while ((bus = pci_find_next_bus(bus)) != NULL) { >> - if (pci_domain_nr(bus) != domain) >> - continue; >> - tmp_bus = pci_do_find_bus(bus, busnr); >> - if (tmp_bus) >> - return tmp_bus; >> - } >> - return NULL; >> + bus = pci_get_bus(domain, busnr); >> + pci_bus_put(bus); >> + >> + return bus; >> } >> >> /** >> @@ -100,21 +106,114 @@ struct pci_bus * pci_find_bus(int domain, int busnr) >> * initiated by passing %NULL as the @from argument. Otherwise if >> * @from is not %NULL, searches continue from next device on the >> * global list. >> + * >> + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. >> + * Please use pci_get_next_root_bus() instead which holds a reference >> + * on the returned PCI root bus. >> */ >> struct pci_bus * >> pci_find_next_bus(const struct pci_bus *from) >> { >> - struct list_head *n; >> - struct pci_bus *b = NULL; >> + struct device *dev = from ? (struct device *)&from->dev : NULL; >> + >> + dev = class_find_device(&pcibus_class, dev, NULL, >> + &pci_match_next_root_bus); >> + if (dev) { >> + put_device(dev); >> + return to_pci_bus(dev); >> + } >> + >> + return NULL; >> +} >> + >> +bool pci_bus_exists(int domain, int busnr) >> +{ >> + struct device *dev; >> + struct pci_bus_match_arg arg = { domain, busnr }; >> >> WARN_ON(in_interrupt()); >> - down_read(&pci_bus_sem); >> - n = from ? from->node.next : pci_root_buses.next; >> - if (n != &pci_root_buses) >> - b = pci_bus_b(n); >> - up_read(&pci_bus_sem); >> - return b; >> + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); >> + if (dev) >> + put_device(dev); >> + >> + return dev != NULL; >> +} >> +EXPORT_SYMBOL(pci_bus_exists); >> + >> +/** >> + * pci_get_bus - locate PCI bus from a given domain and bus number >> + * @domain: number of PCI domain to search >> + * @busnr: number of desired PCI bus >> + * >> + * Given a PCI bus number and domain number, the desired PCI bus is located. >> + * If the bus is found, a pointer to its data structure is returned. >> + * If no bus is found, %NULL is returned. >> + * Caller needs to release the returned bus by calling pci_bus_put(). >> + */ >> +struct pci_bus *pci_get_bus(int domain, int busnr) >> +{ >> + struct device *dev; >> + struct pci_bus_match_arg arg = { domain, busnr }; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> +} >> +EXPORT_SYMBOL(pci_get_bus); >> + >> +/** >> + * pci_get_next_bus - begin or continue searching for a PCI bus >> + * @from: Previous PCI bus found, or %NULL for new search. >> + * >> + * Iterates through the list of known PCI busses. If a PCI bus is found, >> + * the reference count to the bus is incremented and a pointer to it is >> + * returned. Otherwise, %NULL is returned. A new search is initiated by >> + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, >> + * searches continue from next bus on the global list. The reference count >> + * for @from is always decremented if it is not %NULL. >> + */ >> +struct pci_bus *pci_get_next_bus(struct pci_bus *from) >> +{ >> + struct device *dev = from ? &from->dev : NULL; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, dev, NULL, &pci_match_next_bus); >> + pci_bus_put(from); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> +} >> +EXPORT_SYMBOL(pci_get_next_bus); >> + >> +/** >> + * pci_find_next_root_bus - begin or continue searching for a PCI root bus >> + * @from: Previous PCI bus found, or %NULL for new search. >> + * >> + * Iterates through the list of known PCI root busses. If a PCI bus is found, >> + * the reference count to the root bus is incremented and a pointer to it is >> + * returned. Otherwise, %NULL is returned. A new search is initiated by >> + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, >> + * searches continue from next root bus on the global list. The reference >> + * count for @from is always decremented if it is not %NULL. >> + */ >> +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) >> +{ >> + struct device *dev = from ? &from->dev : NULL; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, dev, NULL, >> + &pci_match_next_root_bus); >> + pci_bus_put(from); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> } >> +EXPORT_SYMBOL(pci_get_next_root_bus); >> >> /** >> * pci_get_slot - locate PCI device for a given PCI slot >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 7b23fa0..1e43423 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -454,6 +454,9 @@ struct pci_bus { >> >> #define pci_bus_b(n) list_entry(n, struct pci_bus, node) >> #define to_pci_bus(n) container_of(n, struct pci_bus, dev) >> +#define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >> +#define for_each_pci_root_bus(b) \ >> + for (b = NULL; (b = pci_get_next_root_bus(b)); ) >> >> /* >> * Returns true if the pci bus is root (behind host-pci bridge), >> @@ -718,7 +721,6 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region, >> void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res, >> struct pci_bus_region *region); >> void pcibios_scan_specific_bus(int busn); >> -struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> struct pci_bus * __deprecated pci_scan_bus_parented(struct device *parent, >> int bus, struct pci_ops *ops, void *sysdata); >> @@ -778,8 +780,15 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap); >> int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap); >> int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); >> int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); >> + >> +struct pci_bus *pci_find_bus(int domain, int busnr); >> struct pci_bus *pci_find_next_bus(const struct pci_bus *from); >> >> +bool pci_bus_exists(int domain, int busnr); >> +struct pci_bus *pci_get_bus(int domain, int busnr); >> +struct pci_bus *pci_get_next_bus(struct pci_bus *from); >> +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >> + >> struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, >> struct pci_dev *from); >> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, >> @@ -1430,6 +1439,18 @@ static inline void pci_unblock_cfg_access(struct pci_dev *dev) >> static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from) >> { return NULL; } >> >> +static inline bool pci_bus_exists(int domain, int busnr) >> +{ return false; } >> + >> +static inline struct pci_bus *pci_get_bus(int domain, int busnr) >> +{ return NULL; } >> + >> +static inline struct pci_bus *pci_get_next_bus(struct pci_bus *from) >> +{ return NULL; } >> + >> +static inline struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) >> +{ return NULL; } >> + >> static inline struct pci_dev *pci_get_slot(struct pci_bus *bus, >> unsigned int devfn) >> { return NULL; } >> -- >> 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
On 06/18/2013 04:06 AM, Bjorn Helgaas wrote: > On Sun, May 26, 2013 at 11:52:58PM +0800, Jiang Liu wrote: >> Introduce hotplug-safe PCI bus iterators as below, which hold a >> reference on the returned PCI bus object. >> bool pci_bus_exists(int domain, int busnr); >> struct pci_bus *pci_get_bus(int domain, int busnr); >> struct pci_bus *pci_get_next_bus(struct pci_bus *from); >> struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >> #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >> #define for_each_pci_root_bus(b) \ >> for (b = NULL; (b = pci_get_next_root_bus(b)); ) >> >> The long-term goal is to remove hotplug-unsafe pci_find_bus(), >> pci_find_next_bus() and the global pci_root_buses list. > > I think you should mark the unsafe interfaces as __deprecated so > users will get compile-time warnings. > > I don't think pci_bus_exists() is a safe interface, because the value > it returns may be incorrect before the caller can look at it. The > only safe thing would be to make it so we try to create the bus > and return failure if it already exists. Then the mutex can be in > the code that creates the bus. > > I don't see any uses of for_each_pci_bus(), so please remove that. > > It sounds like we don't have a consensus on how to iterate over > PCI root buses. If you separate that from the pci_get_bus() > changes, maybe we can at least move forward on the pci_get_bus() > stuff. Hi Bjorn and Yinghai, I have thought about the way to implement pci_for_each_root_bus() again. And there are several possible ways here: 1) Yinghai has a patch set implementing an iterator for PCI host bridges, but we can't safely refer the PCI root bus associated with a host bridge device because the host bridge doesn't hold a reference to associated PCI root bus. So we need to find a safe way to refer the PCI root bus associated with a PCI host bridge. 2) Unexport pci_root_buses and convert it to klist, then we could walk all root buses effectively. This solution is straight-forward, but it may break out of tree drivers. 3) Keep current implementation, which does waste some computation cycles:( So what's your thoughts about above solutions? Or any other suggestions? Regards! Gerry > > Bjorn > >> These new interfaces may be a littler slower than existing interfaces, >> but it should be acceptable because they are not used on hot paths. >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> Acked-by: Yinghai Lu <yinghai@kernel.org> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/pci/pci.h | 1 + >> drivers/pci/probe.c | 2 +- >> drivers/pci/search.c | 159 +++++++++++++++++++++++++++++++++++++++++---------- >> include/linux/pci.h | 23 +++++++- >> 4 files changed, 153 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index 68678ed..8fe15f6 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } >> >> /* Lock for read/write access to pci device and bus lists */ >> extern struct rw_semaphore pci_bus_sem; >> +extern struct class pcibus_class; >> >> extern raw_spinlock_t pci_lock; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 2830070..1004a05 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) >> kfree(pci_bus); >> } >> >> -static struct class pcibus_class = { >> +struct class pcibus_class = { >> .name = "pci_bus", >> .dev_release = &release_pcibus_dev, >> .dev_attrs = pcibus_dev_attrs, >> diff --git a/drivers/pci/search.c b/drivers/pci/search.c >> index d0627fa..16ccaf8 100644 >> --- a/drivers/pci/search.c >> +++ b/drivers/pci/search.c >> @@ -52,20 +52,27 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev) >> return tmp; >> } >> >> -static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) >> +struct pci_bus_match_arg { >> + int domain; >> + int bus; >> +}; >> + >> +static int pci_match_bus(struct device *dev, const void *data) >> { >> - struct pci_bus* child; >> - struct list_head *tmp; >> + struct pci_bus *bus = to_pci_bus(dev); >> + const struct pci_bus_match_arg *arg = data; >> >> - if(bus->number == busnr) >> - return bus; >> + return (pci_domain_nr(bus) == arg->domain && bus->number == arg->bus); >> +} >> >> - list_for_each(tmp, &bus->children) { >> - child = pci_do_find_bus(pci_bus_b(tmp), busnr); >> - if(child) >> - return child; >> - } >> - return NULL; >> +static int pci_match_next_bus(struct device *dev, const void *data) >> +{ >> + return 1; >> +} >> + >> +static int pci_match_next_root_bus(struct device *dev, const void *data) >> +{ >> + return pci_is_root_bus(to_pci_bus(dev)); >> } >> >> /** >> @@ -76,20 +83,19 @@ static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) >> * Given a PCI bus number and domain number, the desired PCI bus is located >> * in the global list of PCI buses. If the bus is found, a pointer to its >> * data structure is returned. If no bus is found, %NULL is returned. >> + * >> + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. >> + * Please use pci_get_bus() instead which holds a reference on the returned >> + * PCI bus. >> */ >> -struct pci_bus * pci_find_bus(int domain, int busnr) >> +struct pci_bus *pci_find_bus(int domain, int busnr) >> { >> - struct pci_bus *bus = NULL; >> - struct pci_bus *tmp_bus; >> + struct pci_bus *bus; >> >> - while ((bus = pci_find_next_bus(bus)) != NULL) { >> - if (pci_domain_nr(bus) != domain) >> - continue; >> - tmp_bus = pci_do_find_bus(bus, busnr); >> - if (tmp_bus) >> - return tmp_bus; >> - } >> - return NULL; >> + bus = pci_get_bus(domain, busnr); >> + pci_bus_put(bus); >> + >> + return bus; >> } >> >> /** >> @@ -100,21 +106,114 @@ struct pci_bus * pci_find_bus(int domain, int busnr) >> * initiated by passing %NULL as the @from argument. Otherwise if >> * @from is not %NULL, searches continue from next device on the >> * global list. >> + * >> + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. >> + * Please use pci_get_next_root_bus() instead which holds a reference >> + * on the returned PCI root bus. >> */ >> struct pci_bus * >> pci_find_next_bus(const struct pci_bus *from) >> { >> - struct list_head *n; >> - struct pci_bus *b = NULL; >> + struct device *dev = from ? (struct device *)&from->dev : NULL; >> + >> + dev = class_find_device(&pcibus_class, dev, NULL, >> + &pci_match_next_root_bus); >> + if (dev) { >> + put_device(dev); >> + return to_pci_bus(dev); >> + } >> + >> + return NULL; >> +} >> + >> +bool pci_bus_exists(int domain, int busnr) >> +{ >> + struct device *dev; >> + struct pci_bus_match_arg arg = { domain, busnr }; >> >> WARN_ON(in_interrupt()); >> - down_read(&pci_bus_sem); >> - n = from ? from->node.next : pci_root_buses.next; >> - if (n != &pci_root_buses) >> - b = pci_bus_b(n); >> - up_read(&pci_bus_sem); >> - return b; >> + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); >> + if (dev) >> + put_device(dev); >> + >> + return dev != NULL; >> +} >> +EXPORT_SYMBOL(pci_bus_exists); >> + >> +/** >> + * pci_get_bus - locate PCI bus from a given domain and bus number >> + * @domain: number of PCI domain to search >> + * @busnr: number of desired PCI bus >> + * >> + * Given a PCI bus number and domain number, the desired PCI bus is located. >> + * If the bus is found, a pointer to its data structure is returned. >> + * If no bus is found, %NULL is returned. >> + * Caller needs to release the returned bus by calling pci_bus_put(). >> + */ >> +struct pci_bus *pci_get_bus(int domain, int busnr) >> +{ >> + struct device *dev; >> + struct pci_bus_match_arg arg = { domain, busnr }; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> +} >> +EXPORT_SYMBOL(pci_get_bus); >> + >> +/** >> + * pci_get_next_bus - begin or continue searching for a PCI bus >> + * @from: Previous PCI bus found, or %NULL for new search. >> + * >> + * Iterates through the list of known PCI busses. If a PCI bus is found, >> + * the reference count to the bus is incremented and a pointer to it is >> + * returned. Otherwise, %NULL is returned. A new search is initiated by >> + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, >> + * searches continue from next bus on the global list. The reference count >> + * for @from is always decremented if it is not %NULL. >> + */ >> +struct pci_bus *pci_get_next_bus(struct pci_bus *from) >> +{ >> + struct device *dev = from ? &from->dev : NULL; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, dev, NULL, &pci_match_next_bus); >> + pci_bus_put(from); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> +} >> +EXPORT_SYMBOL(pci_get_next_bus); >> + >> +/** >> + * pci_find_next_root_bus - begin or continue searching for a PCI root bus >> + * @from: Previous PCI bus found, or %NULL for new search. >> + * >> + * Iterates through the list of known PCI root busses. If a PCI bus is found, >> + * the reference count to the root bus is incremented and a pointer to it is >> + * returned. Otherwise, %NULL is returned. A new search is initiated by >> + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, >> + * searches continue from next root bus on the global list. The reference >> + * count for @from is always decremented if it is not %NULL. >> + */ >> +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) >> +{ >> + struct device *dev = from ? &from->dev : NULL; >> + >> + WARN_ON(in_interrupt()); >> + dev = class_find_device(&pcibus_class, dev, NULL, >> + &pci_match_next_root_bus); >> + pci_bus_put(from); >> + if (dev) >> + return to_pci_bus(dev); >> + >> + return NULL; >> } >> +EXPORT_SYMBOL(pci_get_next_root_bus); >> >> /** >> * pci_get_slot - locate PCI device for a given PCI slot >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 7b23fa0..1e43423 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -454,6 +454,9 @@ struct pci_bus { >> >> #define pci_bus_b(n) list_entry(n, struct pci_bus, node) >> #define to_pci_bus(n) container_of(n, struct pci_bus, dev) >> +#define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >> +#define for_each_pci_root_bus(b) \ >> + for (b = NULL; (b = pci_get_next_root_bus(b)); ) >> >> /* >> * Returns true if the pci bus is root (behind host-pci bridge), >> @@ -718,7 +721,6 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region, >> void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res, >> struct pci_bus_region *region); >> void pcibios_scan_specific_bus(int busn); >> -struct pci_bus *pci_find_bus(int domain, int busnr); >> void pci_bus_add_devices(const struct pci_bus *bus); >> struct pci_bus * __deprecated pci_scan_bus_parented(struct device *parent, >> int bus, struct pci_ops *ops, void *sysdata); >> @@ -778,8 +780,15 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap); >> int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap); >> int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); >> int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); >> + >> +struct pci_bus *pci_find_bus(int domain, int busnr); >> struct pci_bus *pci_find_next_bus(const struct pci_bus *from); >> >> +bool pci_bus_exists(int domain, int busnr); >> +struct pci_bus *pci_get_bus(int domain, int busnr); >> +struct pci_bus *pci_get_next_bus(struct pci_bus *from); >> +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >> + >> struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, >> struct pci_dev *from); >> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, >> @@ -1430,6 +1439,18 @@ static inline void pci_unblock_cfg_access(struct pci_dev *dev) >> static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from) >> { return NULL; } >> >> +static inline bool pci_bus_exists(int domain, int busnr) >> +{ return false; } >> + >> +static inline struct pci_bus *pci_get_bus(int domain, int busnr) >> +{ return NULL; } >> + >> +static inline struct pci_bus *pci_get_next_bus(struct pci_bus *from) >> +{ return NULL; } >> + >> +static inline struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) >> +{ return NULL; } >> + >> static inline struct pci_dev *pci_get_slot(struct pci_bus *bus, >> unsigned int devfn) >> { return NULL; } >> -- >> 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
On Thu, Jun 20, 2013 at 10:18 AM, Jiang Liu <liuj97@gmail.com> wrote: > On 06/18/2013 04:06 AM, Bjorn Helgaas wrote: >> On Sun, May 26, 2013 at 11:52:58PM +0800, Jiang Liu wrote: >>> Introduce hotplug-safe PCI bus iterators as below, which hold a >>> reference on the returned PCI bus object. >>> bool pci_bus_exists(int domain, int busnr); >>> struct pci_bus *pci_get_bus(int domain, int busnr); >>> struct pci_bus *pci_get_next_bus(struct pci_bus *from); >>> struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); >>> #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) >>> #define for_each_pci_root_bus(b) \ >>> for (b = NULL; (b = pci_get_next_root_bus(b)); ) >>> >>> The long-term goal is to remove hotplug-unsafe pci_find_bus(), >>> pci_find_next_bus() and the global pci_root_buses list. >> >> I think you should mark the unsafe interfaces as __deprecated so >> users will get compile-time warnings. >> >> I don't think pci_bus_exists() is a safe interface, because the value >> it returns may be incorrect before the caller can look at it. The >> only safe thing would be to make it so we try to create the bus >> and return failure if it already exists. Then the mutex can be in >> the code that creates the bus. >> >> I don't see any uses of for_each_pci_bus(), so please remove that. >> >> It sounds like we don't have a consensus on how to iterate over >> PCI root buses. If you separate that from the pci_get_bus() >> changes, maybe we can at least move forward on the pci_get_bus() >> stuff. > Hi Bjorn and Yinghai, > I have thought about the way to implement pci_for_each_root_bus() > again. And there are several possible ways here: > 1) Yinghai has a patch set implementing an iterator for PCI host > bridges, but we can't safely refer the PCI root bus associated with a > host bridge device because the host bridge doesn't hold a reference to > associated PCI root bus. So we need to find a safe way to refer the PCI > root bus associated with a PCI host bridge. > 2) Unexport pci_root_buses and convert it to klist, then we could walk > all root buses effectively. This solution is straight-forward, but it > may break out of tree drivers. > 3) Keep current implementation, which does waste some computation cycles:( > > So what's your thoughts about above solutions? Or any other suggestions? > Regards! > Gerry Iteration is generally the wrong approach because it doesn't fit well with hot plug. I recognize that it may be impossible to fix all the places that currently iterate over root buses, so we may have to accept iteration at least in the short term. Sometimes when we fix the things we *know* how to fix, it makes it easier to see how to fix the hard problem. Your pci_bus ref counting fixes seem like a clear step forward, and I'd like to get them in ASAP, even if the root bus iteration isn't figured out yet. So my advice is, let's do the simple stuff we know how to do now, then worry about the harder stuff later. 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
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 68678ed..8fe15f6 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -126,6 +126,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; } /* Lock for read/write access to pci device and bus lists */ extern struct rw_semaphore pci_bus_sem; +extern struct class pcibus_class; extern raw_spinlock_t pci_lock; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2830070..1004a05 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -93,7 +93,7 @@ static void release_pcibus_dev(struct device *dev) kfree(pci_bus); } -static struct class pcibus_class = { +struct class pcibus_class = { .name = "pci_bus", .dev_release = &release_pcibus_dev, .dev_attrs = pcibus_dev_attrs, diff --git a/drivers/pci/search.c b/drivers/pci/search.c index d0627fa..16ccaf8 100644 --- a/drivers/pci/search.c +++ b/drivers/pci/search.c @@ -52,20 +52,27 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev) return tmp; } -static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) +struct pci_bus_match_arg { + int domain; + int bus; +}; + +static int pci_match_bus(struct device *dev, const void *data) { - struct pci_bus* child; - struct list_head *tmp; + struct pci_bus *bus = to_pci_bus(dev); + const struct pci_bus_match_arg *arg = data; - if(bus->number == busnr) - return bus; + return (pci_domain_nr(bus) == arg->domain && bus->number == arg->bus); +} - list_for_each(tmp, &bus->children) { - child = pci_do_find_bus(pci_bus_b(tmp), busnr); - if(child) - return child; - } - return NULL; +static int pci_match_next_bus(struct device *dev, const void *data) +{ + return 1; +} + +static int pci_match_next_root_bus(struct device *dev, const void *data) +{ + return pci_is_root_bus(to_pci_bus(dev)); } /** @@ -76,20 +83,19 @@ static struct pci_bus *pci_do_find_bus(struct pci_bus *bus, unsigned char busnr) * Given a PCI bus number and domain number, the desired PCI bus is located * in the global list of PCI buses. If the bus is found, a pointer to its * data structure is returned. If no bus is found, %NULL is returned. + * + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. + * Please use pci_get_bus() instead which holds a reference on the returned + * PCI bus. */ -struct pci_bus * pci_find_bus(int domain, int busnr) +struct pci_bus *pci_find_bus(int domain, int busnr) { - struct pci_bus *bus = NULL; - struct pci_bus *tmp_bus; + struct pci_bus *bus; - while ((bus = pci_find_next_bus(bus)) != NULL) { - if (pci_domain_nr(bus) != domain) - continue; - tmp_bus = pci_do_find_bus(bus, busnr); - if (tmp_bus) - return tmp_bus; - } - return NULL; + bus = pci_get_bus(domain, busnr); + pci_bus_put(bus); + + return bus; } /** @@ -100,21 +106,114 @@ struct pci_bus * pci_find_bus(int domain, int busnr) * initiated by passing %NULL as the @from argument. Otherwise if * @from is not %NULL, searches continue from next device on the * global list. + * + * Note: it's not hotplug safe, the returned bus may be destroyed at any time. + * Please use pci_get_next_root_bus() instead which holds a reference + * on the returned PCI root bus. */ struct pci_bus * pci_find_next_bus(const struct pci_bus *from) { - struct list_head *n; - struct pci_bus *b = NULL; + struct device *dev = from ? (struct device *)&from->dev : NULL; + + dev = class_find_device(&pcibus_class, dev, NULL, + &pci_match_next_root_bus); + if (dev) { + put_device(dev); + return to_pci_bus(dev); + } + + return NULL; +} + +bool pci_bus_exists(int domain, int busnr) +{ + struct device *dev; + struct pci_bus_match_arg arg = { domain, busnr }; WARN_ON(in_interrupt()); - down_read(&pci_bus_sem); - n = from ? from->node.next : pci_root_buses.next; - if (n != &pci_root_buses) - b = pci_bus_b(n); - up_read(&pci_bus_sem); - return b; + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); + if (dev) + put_device(dev); + + return dev != NULL; +} +EXPORT_SYMBOL(pci_bus_exists); + +/** + * pci_get_bus - locate PCI bus from a given domain and bus number + * @domain: number of PCI domain to search + * @busnr: number of desired PCI bus + * + * Given a PCI bus number and domain number, the desired PCI bus is located. + * If the bus is found, a pointer to its data structure is returned. + * If no bus is found, %NULL is returned. + * Caller needs to release the returned bus by calling pci_bus_put(). + */ +struct pci_bus *pci_get_bus(int domain, int busnr) +{ + struct device *dev; + struct pci_bus_match_arg arg = { domain, busnr }; + + WARN_ON(in_interrupt()); + dev = class_find_device(&pcibus_class, NULL, &arg, &pci_match_bus); + if (dev) + return to_pci_bus(dev); + + return NULL; +} +EXPORT_SYMBOL(pci_get_bus); + +/** + * pci_get_next_bus - begin or continue searching for a PCI bus + * @from: Previous PCI bus found, or %NULL for new search. + * + * Iterates through the list of known PCI busses. If a PCI bus is found, + * the reference count to the bus is incremented and a pointer to it is + * returned. Otherwise, %NULL is returned. A new search is initiated by + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, + * searches continue from next bus on the global list. The reference count + * for @from is always decremented if it is not %NULL. + */ +struct pci_bus *pci_get_next_bus(struct pci_bus *from) +{ + struct device *dev = from ? &from->dev : NULL; + + WARN_ON(in_interrupt()); + dev = class_find_device(&pcibus_class, dev, NULL, &pci_match_next_bus); + pci_bus_put(from); + if (dev) + return to_pci_bus(dev); + + return NULL; +} +EXPORT_SYMBOL(pci_get_next_bus); + +/** + * pci_find_next_root_bus - begin or continue searching for a PCI root bus + * @from: Previous PCI bus found, or %NULL for new search. + * + * Iterates through the list of known PCI root busses. If a PCI bus is found, + * the reference count to the root bus is incremented and a pointer to it is + * returned. Otherwise, %NULL is returned. A new search is initiated by + * passing %NULL as the @from argument. Otherwise if @from is not %NULL, + * searches continue from next root bus on the global list. The reference + * count for @from is always decremented if it is not %NULL. + */ +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) +{ + struct device *dev = from ? &from->dev : NULL; + + WARN_ON(in_interrupt()); + dev = class_find_device(&pcibus_class, dev, NULL, + &pci_match_next_root_bus); + pci_bus_put(from); + if (dev) + return to_pci_bus(dev); + + return NULL; } +EXPORT_SYMBOL(pci_get_next_root_bus); /** * pci_get_slot - locate PCI device for a given PCI slot diff --git a/include/linux/pci.h b/include/linux/pci.h index 7b23fa0..1e43423 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -454,6 +454,9 @@ struct pci_bus { #define pci_bus_b(n) list_entry(n, struct pci_bus, node) #define to_pci_bus(n) container_of(n, struct pci_bus, dev) +#define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); ) +#define for_each_pci_root_bus(b) \ + for (b = NULL; (b = pci_get_next_root_bus(b)); ) /* * Returns true if the pci bus is root (behind host-pci bridge), @@ -718,7 +721,6 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region, void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res, struct pci_bus_region *region); void pcibios_scan_specific_bus(int busn); -struct pci_bus *pci_find_bus(int domain, int busnr); void pci_bus_add_devices(const struct pci_bus *bus); struct pci_bus * __deprecated pci_scan_bus_parented(struct device *parent, int bus, struct pci_ops *ops, void *sysdata); @@ -778,8 +780,15 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap); int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap); int pci_find_ht_capability(struct pci_dev *dev, int ht_cap); int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap); + +struct pci_bus *pci_find_bus(int domain, int busnr); struct pci_bus *pci_find_next_bus(const struct pci_bus *from); +bool pci_bus_exists(int domain, int busnr); +struct pci_bus *pci_get_bus(int domain, int busnr); +struct pci_bus *pci_get_next_bus(struct pci_bus *from); +struct pci_bus *pci_get_next_root_bus(struct pci_bus *from); + struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, struct pci_dev *from); struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, @@ -1430,6 +1439,18 @@ static inline void pci_unblock_cfg_access(struct pci_dev *dev) static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from) { return NULL; } +static inline bool pci_bus_exists(int domain, int busnr) +{ return false; } + +static inline struct pci_bus *pci_get_bus(int domain, int busnr) +{ return NULL; } + +static inline struct pci_bus *pci_get_next_bus(struct pci_bus *from) +{ return NULL; } + +static inline struct pci_bus *pci_get_next_root_bus(struct pci_bus *from) +{ return NULL; } + static inline struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn) { return NULL; }