Patchwork [v3,part2,01/20] PCI: introduce hotplug-safe PCI bus iterators

login
register
mail settings
Submitter Jiang Liu
Date May 26, 2013, 3:52 p.m.
Message ID <1369583597-3801-2-git-send-email-jiang.liu@huawei.com>
Download mbox | patch
Permalink /patch/246411/
State Rejected
Headers show

Comments

Jiang Liu - May 26, 2013, 3:52 p.m.
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.

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(-)
Yinghai Lu - May 28, 2013, 4:22 a.m.
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
Jiang Liu - May 28, 2013, 3:06 p.m.
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
Bjorn Helgaas - June 17, 2013, 8:06 p.m.
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
Jiang Liu - June 18, 2013, 4:23 p.m.
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
Jiang Liu - June 20, 2013, 4:18 p.m.
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
Bjorn Helgaas - June 26, 2013, 2:58 a.m.
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

Patch

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; }