On 04/10/2012 06:32 PM, Kenji Kaneshige wrote: > (2012/04/10 1:02), Jiang Liu wrote: >> Hi Kenji, >> Thanks for your careful review and comments. >> >> On 04/09/2012 07:43 PM, Kenji Kaneshige wrote: >>> Your patch looks good to me. >>> >>> I have some comments. >>> >>> (2012/04/09 2:12), Jiang Liu wrote: >>>> This patch enhances pci_root driver to update MMCFG information when >>>> hot-plugging PCI root bridges on x86 platforms. >>>> >>> >>> Do you have the patch that can be applied to Bjorn's pci tree? >>> >>> <snip.> >> Will try to generate a version against Bjorn's version. Could you please tell >> me the exact git link for that? I haven't pull from Bjorn's tree yet. > > As you may know, it was announced _today_ (sorry:). > > http://www.spinics.net/lists/linux-pci/msg14626.html > >> >>> >>>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root) >>>> +{ >>>> + int result = 0; >>>> + acpi_status status; >>>> + unsigned long long base_addr; >>>> + struct pci_mmcfg_region *cfg; >>>> + >>>> + /* >>>> + * Try to insert MMCFG information for host bridges with _CBA method >>>> + */ >>>> + status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA, >>>> + NULL,&base_addr); >>>> + if (ACPI_SUCCESS(status)) { >>>> + result = pci_mmconfig_insert(root->segment, >>>> + root->secondary.start, >>>> + root->secondary.end, >>>> + base_addr); >>>> + /* >>>> + * MMCFG information for hot-pluggable host bridges may have >>>> + * already been added by __pci_mmcfg_init(); >>>> + */ >>>> + if (result == -EEXIST) >>>> + result = 0; >>> >>> Just for confirmation. >>> From my interpretation of PCI firmware spec, MCFG doesn't have any entry >>> for hot-pluggable hostbridge. So I assume this is for the machine that >>> is not compliant to the spec. Is my understanding same as yours? >>> >>> <snip.> >> You are right, it's defined to that way in PCI FW Spec 3.1. >> Here I have some concerns about the PCI buses to host all Ubox components >> on Intel NHM/WSM/SNB/IVB processors. BIOS people are prone to declare >> MMCFG information for those host bridges by MCFG table instead of _CBA method, >> though those host bridge will disappear after hot-removing a physical processor. > > Ok, thank you for clarification. > >> >>> >>>> static int __devinit acpi_pci_root_add(struct acpi_device *device) >>>> { >>>> unsigned long long segment, bus; >>>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device) >>>> strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); >>>> device->driver_data = root; >>>> >>>> + if (arch_acpi_pci_root_add(root)) { >>>> + printk(KERN_ERR PREFIX >>>> + "can't add MMCFG information for Bus %04x:%02x\n", >>>> + root->segment, (unsigned int)root->secondary.start); > > Additional comment. > > This printk message looks strange because arch_acpi_pci_root_add() > is not a mmconfig specific function. So I think this message > should be moved to arch specific code (arch_acpi_pci_root_add()). > >>>> + result = -ENODEV; >>>> + goto out_free; >>>> + } >>> >>> Desn't this break the system that doesn't support MMCONFIG? >>> >>> In my understanding, arch_acpi_pci_root_add() returns -ENODEV if >>> mmconfig information is found neither in MCFG table nor _CBA. And >>> pci root bridge initialization seems to fail arch_acpi_pci_root_add() >>> returns non-zero value. >> Good catch, will add following code into arch_acpi_pci_root_add() and >> arch_acpi_pci_root_remove() to solve this issue. >> --- >> /* MMCONFIG disabled */ >> if ((pci_probe& PCI_PROBE_MMCONF) == 0) >> return 0; >> --- > > My understanding is that PCI_PROBE_MMCONF is set even if the system > doesn't have MCFG table. So I don't think this solves the issue. I > guess this is what Yinghai pointed out on your V2 patch [6/6]. > > Additionally, I think there is a remaining issue even if we change > this check like below. > > if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF)) > return 0; > > I think this check has an assumption that system has at least one > MCFG table entry and it has been initialized before > acpi_pci_root_add() is called. I think this doesn't work on the > system that doesn't have MCFG and all the pci root bridge have > _CBA (that is, all host bridges are hot-pluggable and BIOS is > implemented in the way PCI FW spec defines). As a result, MMCONFIG > would never be enabled on such systems. Could you double check this? Good points. We underestimated the complexity here. I'm working on a new patch based on Yinghai's v3 patchset and will send it out soon. Thanks! > > Regards, > Kenji Kaneshige -- 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
