Hi Shawn,
On Wed, Mar 22, 2017 at 05:42:24PM +0800, Shawn Lin wrote:
> If not getting domain number from DT, the domain number will
> keep increasing once doing unbind/bind RC drivers. This could
> introduce pointless tree view of lspci as shows below:
>
> -+-[0001:00]---00.0-[01]----00.0
> \-[0000:00]-
>
> The more test we do, the lengthier it would be. The more serious
> issue is that if attaching two hierarchies for two different domains
> belonging to two root bridges, so when doing unbind/bind test for one
> of them and keep the other, then the domain number would finally
> overflow and make the two hierarchies of devices share the some domain
> number but actually they shouldn't. So it looks like we need to invent
> a new indexing ID mechanism to manage domain number. This patch
> introduces idr to achieve our purpose.
I like the idea of this patch, but I have some comments/suggestions
about the implementation.
> Cc: Brian Norris <briannorris@xxxxxxxxxxxx>
> Cc: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> ---
>
> drivers/pci/pci.c | 11 +++++++----
> drivers/pci/remove.c | 5 +++++
> include/linux/pci.h | 2 ++
> 3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a881c0d..1752ccc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -11,6 +11,7 @@
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/dmi.h>
> +#include <linux/idr.h>
> #include <linux/init.h>
> #include <linux/of.h>
> #include <linux/of_pci.h>
> @@ -5172,15 +5173,16 @@ static void pci_no_domains(void)
> }
>
> #ifdef CONFIG_PCI_DOMAINS
> -static atomic_t __domain_nr = ATOMIC_INIT(-1);
> +DEFINE_IDA(__domain_nr);
>
> int pci_get_new_domain_nr(void)
> {
> - return atomic_inc_return(&__domain_nr);
> + return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
> }
>
> #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static int of_pci_bus_find_domain_nr(struct device *parent)
> +static int of_pci_bus_find_domain_nr(struct pci_bus *bus,
> + struct device *parent)
> {
> static int use_dt_domains = -1;
> int domain = -1;
> @@ -5224,12 +5226,13 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> domain = -1;
> }
>
> + bus->use_dt_domains = use_dt_domains;
> return domain;
> }
>
> int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
> + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
> acpi_pci_bus_find_domain_nr(bus);
> }
> #endif
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 73a03d3..ad93378 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -157,6 +157,11 @@ void pci_remove_root_bus(struct pci_bus *bus)
> list_for_each_entry_safe(child, tmp,
> &bus->devices, bus_list)
> pci_remove_bus_device(child);
> + #ifdef CONFIG_PCI_DOMAINS_GENERIC
> + if (!bus->use_dt_domains)
> + ida_simple_remove(&__domain_nr, bus->domain_nr);
> + #endif
1) I think there should be a wrapper alongside pci_get_new_domain_nr()
that does the remove. That would remove the #ifdef here and help make
the remove symmetric with the get.
2) The "use_dt_domains" name is too specific and makes too many
assumptions about CONFIG_PCI_DOMAINS_GENERIC, e.g., it assumes that we
don't use CONFIG_PCI_DOMAINS_GENERIC with ACPI.
At a high level, if DT or ACPI supplies a domain number, we use that.
We only use the IDA if neither supplies a domain number. I think it
would be clearer to set a bit in pci_get_new_domain_nr() that means
"we got this domain from IDA", and then test that bit in the
corresponding remove wrapper.
> pci_remove_bus(bus);
> host_bridge->bus = NULL;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6732d32..235b336 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -505,6 +505,7 @@ struct pci_bus {
> unsigned char cur_bus_speed; /* enum pci_bus_speed */
> #ifdef CONFIG_PCI_DOMAINS_GENERIC
> int domain_nr;
> + int use_dt_domains;
> #endif
>
> char name[48];
> @@ -1449,6 +1450,7 @@ static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
> * configuration space.
> */
> #ifdef CONFIG_PCI_DOMAINS
> +extern struct ida __domain_nr;
> extern int pci_domains_supported;
> int pci_get_new_domain_nr(void);
> #else
> --
> 1.9.1
>
>
