Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757932Ab3FTQSx (ORCPT ); Thu, 20 Jun 2013 12:18:53 -0400 Received: from mail-pb0-f50.google.com ([209.85.160.50]:48626 "EHLO mail-pb0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965122Ab3FTQSv (ORCPT ); Thu, 20 Jun 2013 12:18:51 -0400 Message-ID: <51C32B64.1010608@gmail.com> Date: Fri, 21 Jun 2013 00:18:44 +0800 From: Jiang Liu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 MIME-Version: 1.0 To: Bjorn Helgaas , Yinghai Lu CC: Jiang Liu , "Rafael J . Wysocki" , Greg Kroah-Hartman , Gu Zheng , Toshi Kani , Myron Stowe , Yijing Wang , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3, part2 01/20] PCI: introduce hotplug-safe PCI bus iterators References: <1369583597-3801-1-git-send-email-jiang.liu@huawei.com> <1369583597-3801-2-git-send-email-jiang.liu@huawei.com> <20130617200639.GA7877@google.com> In-Reply-To: <20130617200639.GA7877@google.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13120 Lines: 352 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 >> Acked-by: Yinghai Lu >> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/