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
