On 2014/1/8 11:06, Kai Huang wrote: > > > > On Tue, Jan 7, 2014 at 5:00 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx > <mailto:jiang.liu@xxxxxxxxxxxxxxx>> wrote: > > Function get_domain_for_dev() is a little complex, simplify it > by factoring out dmar_search_domain_by_dev_info() and > dmar_insert_dev_info(). > > Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx > <mailto:jiang.liu@xxxxxxxxxxxxxxx>> > --- > drivers/iommu/intel-iommu.c | 161 > ++++++++++++++++++++----------------------- > 1 file changed, 75 insertions(+), 86 deletions(-) > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c > index 1704e97..da65884 100644 > --- a/drivers/iommu/intel-iommu.c > +++ b/drivers/iommu/intel-iommu.c > @@ -1957,18 +1957,70 @@ find_domain(struct pci_dev *pdev) > return NULL; > } > > +static inline struct dmar_domain * > +dmar_search_domain_by_dev_info(int segment, int bus, int devfn) > +{ > + struct device_domain_info *info; > + > + list_for_each_entry(info, &device_domain_list, global) > + if (info->segment == segment && info->bus == bus && > + info->devfn == devfn) > + return info->domain; > + > + return NULL; > +} > + > +static int dmar_insert_dev_info(int segment, int bus, int devfn, > + struct pci_dev *dev, struct > dmar_domain **domp) > +{ > + struct dmar_domain *found, *domain = *domp; > + struct device_domain_info *info; > + unsigned long flags; > + > + info = alloc_devinfo_mem(); > + if (!info) > + return -ENOMEM; > + > + info->segment = segment; > + info->bus = bus; > + info->devfn = devfn; > + info->dev = dev; > + info->domain = domain; > + if (!dev) > + domain->flags |= DOMAIN_FLAG_P2P_MULTIPLE_DEVICES; > + > + spin_lock_irqsave(&device_domain_lock, flags); > + if (dev) > + found = find_domain(dev); > + else > + found = dmar_search_domain_by_dev_info(segment, bus, > devfn); > + if (found) { > + spin_unlock_irqrestore(&device_domain_lock, flags); > + free_devinfo_mem(info); > + if (found != domain) { > + domain_exit(domain); > + *domp = found; > + } > + } else { > + list_add(&info->link, &domain->devices); > + list_add(&info->global, &device_domain_list); > + if (dev) > + dev->dev.archdata.iommu = info; > + spin_unlock_irqrestore(&device_domain_lock, flags); > + } > + > + return 0; > +} > > > I am a little bit confused about the "alloc before check" sequence in > above function. I believe the purpose of allocating the > device_domain_info before searching the domain with spin_lock hold is to > avoid the memory allocation in the spin_lock? In this case, why can't we > use mutex instead of spin_lock? In my understanding, if we use mutex, we > can first check if the domain exists or not before we really allocate > device_domain_info, right? Hi Kai, Thanks for review! The domain->devices list may be access in interrupt context, so we need to protect it with spin_lock_irqsave(). Otherwise it may case deadlock. > > Another question is when is info->iommu field initialized? Looks it is > not initialized here? It's set in function iommu_support_dev_iotlb() for devices supporting device IOTLB. > > -Kai > > + > /* domain is initialized */ > static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, > int gaw) > { > - struct dmar_domain *domain, *found = NULL; > + struct dmar_domain *domain; > struct intel_iommu *iommu; > struct dmar_drhd_unit *drhd; > - struct device_domain_info *info, *tmp; > - struct pci_dev *dev_tmp; > + struct pci_dev *bridge; > unsigned long flags; > - int bus = 0, devfn = 0; > - int segment; > - int ret; > + int segment, bus, devfn; > > domain = find_domain(pdev); > if (domain) > @@ -1976,119 +2028,56 @@ static struct dmar_domain > *get_domain_for_dev(struct pci_dev *pdev, int gaw) > > segment = pci_domain_nr(pdev->bus); > > - dev_tmp = pci_find_upstream_pcie_bridge(pdev); > - if (dev_tmp) { > - if (pci_is_pcie(dev_tmp)) { > - bus = dev_tmp->subordinate->number; > + bridge = pci_find_upstream_pcie_bridge(pdev); > + if (bridge) { > + if (pci_is_pcie(bridge)) { > + bus = bridge->subordinate->number; > devfn = 0; > } else { > - bus = dev_tmp->bus->number; > - devfn = dev_tmp->devfn; > + bus = bridge->bus->number; > + devfn = bridge->devfn; > } > spin_lock_irqsave(&device_domain_lock, flags); > - list_for_each_entry(info, &device_domain_list, global) { > - if (info->segment == segment && > - info->bus == bus && info->devfn == devfn) { > - found = info->domain; > - break; > - } > - } > + domain = dmar_search_domain_by_dev_info(segment, > bus, devfn); > spin_unlock_irqrestore(&device_domain_lock, flags); > /* pcie-pci bridge already has a domain, uses it */ > - if (found) { > - domain = found; > + if (domain) > goto found_domain; > - } > } > > - domain = alloc_domain(); > - if (!domain) > - goto error; > - > - /* Allocate new domain for the device */ > drhd = dmar_find_matched_drhd_unit(pdev); > if (!drhd) { > printk(KERN_ERR "IOMMU: can't find DMAR for device > %s\n", > pci_name(pdev)); > - free_domain_mem(domain); > return NULL; > } > iommu = drhd->iommu; > > - ret = iommu_attach_domain(domain, iommu); > - if (ret) { > + /* Allocate new domain for the device */ > + domain = alloc_domain(); > + if (!domain) > + goto error; > + if (iommu_attach_domain(domain, iommu)) { > free_domain_mem(domain); > goto error; > } > - > if (domain_init(domain, gaw)) { > domain_exit(domain); > goto error; > } > > /* register pcie-to-pci device */ > - if (dev_tmp) { > - info = alloc_devinfo_mem(); > - if (!info) { > + if (bridge) { > + if (dmar_insert_dev_info(segment, bus, devfn, NULL, > &domain)) { > domain_exit(domain); > goto error; > } > - info->segment = segment; > - info->bus = bus; > - info->devfn = devfn; > - info->dev = NULL; > - info->domain = domain; > - /* This domain is shared by devices under p2p bridge */ > - domain->flags |= DOMAIN_FLAG_P2P_MULTIPLE_DEVICES; > - > - /* pcie-to-pci bridge already has a domain, uses it */ > - found = NULL; > - spin_lock_irqsave(&device_domain_lock, flags); > - list_for_each_entry(tmp, &device_domain_list, global) { > - if (tmp->segment == segment && > - tmp->bus == bus && tmp->devfn == devfn) { > - found = tmp->domain; > - break; > - } > - } > - if (found) { > - spin_unlock_irqrestore(&device_domain_lock, > flags); > - free_devinfo_mem(info); > - domain_exit(domain); > - domain = found; > - } else { > - list_add(&info->link, &domain->devices); > - list_add(&info->global, &device_domain_list); > - spin_unlock_irqrestore(&device_domain_lock, > flags); > - } > } > > found_domain: > - info = alloc_devinfo_mem(); > - if (!info) > - goto error; > - info->segment = segment; > - info->bus = pdev->bus->number; > - info->devfn = pdev->devfn; > - info->dev = pdev; > - info->domain = domain; > - spin_lock_irqsave(&device_domain_lock, flags); > - /* somebody is fast */ > - found = find_domain(pdev); > - if (found != NULL) { > - spin_unlock_irqrestore(&device_domain_lock, flags); > - if (found != domain) { > - domain_exit(domain); > - domain = found; > - } > - free_devinfo_mem(info); > + if (dmar_insert_dev_info(segment, pdev->bus->number, > pdev->devfn, > + pdev, &domain) == 0) > return domain; > - } > - list_add(&info->link, &domain->devices); > - list_add(&info->global, &device_domain_list); > - pdev->dev.archdata.iommu = info; > - spin_unlock_irqrestore(&device_domain_lock, flags); > - return domain; > error: > /* recheck it here, maybe others set it */ > return find_domain(pdev); > -- > 1.7.10.4 > > _______________________________________________ > iommu mailing list > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx > <mailto:iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx> > https://lists.linuxfoundation.org/mailman/listinfo/iommu > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
