Re: [PATCH 6/6] PCI, x86: change the way to add MMCFG region on x86

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jian, Yinghai,

 Thank you your comment and review.

On Thu, 26 Apr 2012 23:04:33 +0800
Jiang Liu <liuj97@xxxxxxxxx> wrote:

> Hi Taku,
> 	Thanks for following this up. Please refer to inline comments below.
> I feel there are still some concerns from Don about the change proposed by
> Bjorn. And I haven't found a solution to address Don's concerns yet. So it
> would be better for us to hold on this for a while.
> 
> 	BTW, is it OK to split the task into two steps? 

   Sure.

> 1) enhance mmcfg to support host bridge hotplug.
> 2) refine the mmcfg setup logic.
> Seems it's more realistic for us to reach an agreement for task 1 in a short time,
> but there is still much work ahead for task 2.

  I agree with you.
  To achive task 1, your V4 should is good enough, but
  I found a problem in your patchset.

  When a hotpluggable hostbridge is found at boot time, its MCFG information should be 
  provided via _CBA method, and will added at the following timing:

         arch_acpi_pci_root_add()
            + pci_mmconfig_insert()
              + insert_resource()

  But at boot time, all entries in pci_mmcfg_list collection will added at the following timing:

        late_initcall
           + pci_mmcfg_late_insert_resources()
             + pci_mmcfg_insert_resources()
               + insert_resource()

  So, MCFG information of hotpluggable hostbridges which exist at boot time will fail to 
  add at this timing.

+int __devinit pci_mmconfig_insert(int segment, int start, int end, u64 addr)
+{
+	int rc = -EINVAL;
+	struct pci_mmcfg_region *cfg = NULL;
+
+	if (segment < 0 || segment > USHRT_MAX ||
+	    start < 0 || start > 255 || end < start || end > 255)
+		return rc;
+
+	mutex_lock(&pci_mmcfg_lock);
+	cfg = pci_mmconfig_lookup(segment, start);
+	if (cfg) {
+		if (cfg->start_bus <= start && cfg->end_bus >= end) {
+			rc = -EEXIST;
+		} else {
+			printk(KERN_WARNING PREFIX
+			       "MMCONFIG for domain %04x [bus %02x-%02x] "
+			       "conflicts with domain %04x [bus %02x-%02x]\n",
+			       segment, start, end,
+			       cfg->segment, cfg->start_bus, cfg->end_bus);
+		}
+		goto out;
+	}
+	if (!addr)
+		goto out;
+
+	cfg = pci_mmconfig_alloc(segment, start, end, addr);
+	if (cfg == NULL) {
+		rc = -ENOMEM;
+	} else if (!pci_mmcfg_check_reserved(cfg, 0)) {
+		printk(KERN_WARNING PREFIX
+		       "MMCONFIG for domain %04x [bus %02x-%02x] "
+		       "isn't reserved\n", segment, start, end);
+	} else if (insert_resource(&iomem_resource, &cfg->res)) {

          I think the following is better.

          else if (pci_mmcfg_resources_inserted &&
                   insert_resource(&iomem_resource, &cfg->res)) {


+		rc = -EBUSY;
+		printk(KERN_WARNING PREFIX
+		       "failed to insert resource for domain "
+		       "%04x [bus %02x-%02x]\n", segment, start, end);
+	} else if (pci_mmcfg_arch_map(cfg)) {
+		rc = -EBUSY;


  Could you please fix this?


Best regards,
Taku Izumi

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


[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux