| Message ID | 1495611455-41952-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 24, 2017 at 03:37:35PM +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. > > Cc: Brian Norris <briannorris@chromium.org> > Cc: Jeffy Chen <jeffy.chen@rock-chips.com> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > Changes in v4: > - make domain_nr depends on CONFIG_PCI_DOMAINS instead of > CONFIG_PCI_DOMAINS_GENERIC.(reported by Kbuild Robot) I'm confused about why you posted this v4. It addresses a kbuild issue, but not any of the questions from my review of v3, so reviewing v4 would be a waste of my time. But since we're here, my naive suggestion: >> 1) If we're using ACPI, every host bridge must have a _SEG method, >> and it supplies the domain. We ignore any bridge without _SEG. >> >> 2) If we're using DT, every host bridge must supply >> "linux,pci-domain", and it supplies the domain. We ignore any >> bridge without "linux,pci-domain". >> >> 3) Otherwise, we always use IDA. was *too* simplistic. _SEG is optional. If it's missing we default to domain 0. The point is that we can't mix the IDA with either the ACPI or DT info. I think for ACPI it should be easy: if _SEG exists, we use that. If _SEG doesn't exist, the spec says we that bridge is in domain 0. So I think we should never use IDA if we're using ACPI. For DT, I think we can't use IDA if *any* bridge uses "linux,pci-domain", because there's no way to allocate a specified domain from the IDA. Of course, we see the bridges one at a time, so we don't know ahead of time whether any uses "linux,pci-domain". I think that means we have to decide when we see the very first host bridge which strategy to use. If the first host bridge has "linux,pci-domain", we use that, and if future host bridges don't supply "linux,pci-domain", we probably have to ignore the whole bridge. If the first host bridge doesn't have "linux,pci-domain", we use IDA, and we probably have to ignore any future bridges that *do* have "linux,pci-domain". So it seems like "linux,pci-domain" is basically an all-or-none proposition. Bjorn
Hi Bjorn, On 2017/5/26 3:43, Bjorn Helgaas wrote: > On Wed, May 24, 2017 at 03:37:35PM +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. >> >> Cc: Brian Norris <briannorris@chromium.org> >> Cc: Jeffy Chen <jeffy.chen@rock-chips.com> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> >> --- >> >> Changes in v4: >> - make domain_nr depends on CONFIG_PCI_DOMAINS instead of >> CONFIG_PCI_DOMAINS_GENERIC.(reported by Kbuild Robot) > > I'm confused about why you posted this v4. It addresses a kbuild > issue, but not any of the questions from my review of v3, so reviewing > v4 would be a waste of my time. > I feel very sorry about that if you were confused by my v4 which didn't address your comment in v3. But the fact is that it's unfortunate your comment for v3 arrived just right after I posted my v4, if you checked the mail's timestamp. :( > But since we're here, my naive suggestion: > >>> 1) If we're using ACPI, every host bridge must have a _SEG method, >>> and it supplies the domain. We ignore any bridge without _SEG. >>> >>> 2) If we're using DT, every host bridge must supply >>> "linux,pci-domain", and it supplies the domain. We ignore any >>> bridge without "linux,pci-domain". >>> >>> 3) Otherwise, we always use IDA. > > was *too* simplistic. _SEG is optional. If it's missing we default > to domain 0. > > The point is that we can't mix the IDA with either the ACPI or DT > info. I think for ACPI it should be easy: if _SEG exists, we use > that. If _SEG doesn't exist, the spec says we that bridge is in > domain 0. So I think we should never use IDA if we're using ACPI. got it. > > For DT, I think we can't use IDA if *any* bridge uses > "linux,pci-domain", because there's no way to allocate a specified > domain from the IDA. > > Of course, we see the bridges one at a time, so we don't know ahead of > time whether any uses "linux,pci-domain". I think that means we have > to decide when we see the very first host bridge which strategy to > use. If the first host bridge has "linux,pci-domain", we use that, > and if future host bridges don't supply "linux,pci-domain", we > probably have to ignore the whole bridge. > > If the first host bridge doesn't have "linux,pci-domain", we use IDA, > and we probably have to ignore any future bridges that *do* have > "linux,pci-domain". > > So it seems like "linux,pci-domain" is basically an all-or-none > proposition. That makes sense to me. > > Bjorn > > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b01bd5b..e5f5db0 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> @@ -5340,15 +5341,25 @@ 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) +/* get domain number from IDA */ +static bool ida_domain = true; + +int pci_get_new_domain_nr(struct pci_bus *bus) +{ + return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL); +} + +void pci_put_old_domain_nr(struct pci_bus *bus) { - return atomic_inc_return(&__domain_nr); + if (ida_domain) + ida_simple_remove(&__domain_nr, bus->domain_nr); } #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; @@ -5361,19 +5372,21 @@ static int of_pci_bus_find_domain_nr(struct device *parent) * If DT domain property is valid (domain >= 0) and * use_dt_domains != 0, the DT assignment is valid since this means * we have not previously allocated a domain number by using - * pci_get_new_domain_nr(); we should also update use_dt_domains to - * 1, to indicate that we have just assigned a domain number from - * DT. + * pci_get_new_domain_nr(), so we should set ida_domain to false to + * indicate that we don't allocate domain from idr; we should also + * update use_dt_domains to 1, to indicate that we have just assigned + * a domain number from DT. * * If DT domain property value is not valid (ie domain < 0), and we * have not previously assigned a domain number from DT - * (use_dt_domains != 1) we should assign a domain number by - * using the: + * (use_dt_domains != 1 && ida_domain != false) we should assign a + * domain number by using the: * * pci_get_new_domain_nr() * - * API and update the use_dt_domains value to keep track of method we - * are using to assign domain numbers (use_dt_domains = 0). + * API and update the use_dt_domains and ida_domain value to keep track + * of method we are using to assign domain numbers (use_dt_domains = 0 + * and ida_domain = true). * * All other combinations imply we have a platform that is trying * to mix domain numbers obtained from DT and pci_get_new_domain_nr(), @@ -5383,9 +5396,10 @@ static int of_pci_bus_find_domain_nr(struct device *parent) */ if (domain >= 0 && use_dt_domains) { use_dt_domains = 1; - } else if (domain < 0 && use_dt_domains != 1) { + ida_domain = false; + } else if (domain < 0 && use_dt_domains != 1 && ida_domain != false) { use_dt_domains = 0; - domain = pci_get_new_domain_nr(); + domain = pci_get_new_domain_nr(bus); } else { dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n", parent->of_node->full_name); @@ -5397,7 +5411,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent) 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..1bbbb37 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -157,6 +157,8 @@ 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); + + pci_put_old_domain_nr(bus); pci_remove_bus(bus); host_bridge->bus = NULL; diff --git a/include/linux/pci.h b/include/linux/pci.h index 33c2b0b..af81e76 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -511,7 +511,7 @@ struct pci_bus { unsigned char primary; /* number of primary bridge */ unsigned char max_bus_speed; /* enum pci_bus_speed */ unsigned char cur_bus_speed; /* enum pci_bus_speed */ -#ifdef CONFIG_PCI_DOMAINS_GENERIC +#ifdef CONFIG_PCI_DOMAINS int domain_nr; #endif @@ -1445,13 +1445,17 @@ 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); +int pci_get_new_domain_nr(struct pci_bus *bus); +void pci_put_old_domain_nr(struct pci_bus *bus); #else enum { pci_domains_supported = 0 }; static inline int pci_domain_nr(struct pci_bus *bus) { return 0; } static inline int pci_proc_domain(struct pci_bus *bus) { return 0; } -static inline int pci_get_new_domain_nr(void) { return -ENOSYS; } +static inline int pci_get_new_domain_nr(struct pci_bus *bus) +{ return -ENOSYS; } +static inline void pci_put_old_domain_nr(struct pci_bus *bus) { } #endif /* CONFIG_PCI_DOMAINS */ /*
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. Cc: Brian Norris <briannorris@chromium.org> Cc: Jeffy Chen <jeffy.chen@rock-chips.com> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Changes in v4: - make domain_nr depends on CONFIG_PCI_DOMAINS instead of CONFIG_PCI_DOMAINS_GENERIC.(reported by Kbuild Robot) Changes in v3: - make ida_domain a system-wide property and check it in the code to combine with use_dt_domains. Also update the comment there. Changes in v2: - add a remove wrapper - rename use_dt_domains to ida_domain and set this bit in pci_get_new_domain_nr and test it in the remove wrapper. drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++-------------- drivers/pci/remove.c | 2 ++ include/linux/pci.h | 10 +++++++--- 3 files changed, 37 insertions(+), 17 deletions(-)