Re: [PATCH v7 08/10] PCI, x86: add MMCFG information on demand

[Jiang Liu <jiang.liu@huawei.com>]

Hi Yinghai,
	Thanks for your review and comments!
On 2012-6-15 15:15, Yinghai Lu wrote:
>> @@ -625,6 +636,13 @@ static int __init pci_parse_mcfg(struct acpi_table_header *header)
>>                }
>>        }
>>
>> +       i = entries * sizeof(*cfg_table);
>> +       pci_acpi_mcfg_array = kmalloc(i, GFP_KERNEL);
>> +       if (pci_acpi_mcfg_array) {
>> +               memcpy(pci_acpi_mcfg_array, cfg_table, i);
>> +               pci_acpi_mcfg_entries = entries;
>> +       }
>> +
> 
> here cache cfg too early.  should do that after
> 
> pci_mmcfg_reject_broken().
> 
> otherwise will use mcfg even try to reject that before.
Good cache, how about the following patch to fix this?
I'm also preparing an V8 to fix build failure issues reported by Fengguang,
and also simplify the __pci_mmcfg_init() implementation.

---
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 77f0db2..bcd0928 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -623,11 +623,27 @@ static int __init pci_parse_mcfg(struct acpi_table_header *header)
                }
        }

-       i = entries * sizeof(*cfg_table);
-       pci_acpi_mcfg_array = kmalloc(i, GFP_KERNEL);
-       if (pci_acpi_mcfg_array) {
-               memcpy(pci_acpi_mcfg_array, cfg_table, i);
-               pci_acpi_mcfg_entries = entries;
+       return 0;
+}
+
+static int __init pci_cache_mcfg(struct acpi_table_header *header)
+{
+       unsigned long i;
+       struct acpi_table_mcfg *mcfg;
+       struct acpi_mcfg_allocation *cfg_table;
+
+       if (!header)
+               return -EINVAL;
+
+       i = (header->length - sizeof(struct acpi_table_mcfg));
+       if (i) {
+               pci_acpi_mcfg_array = kmalloc(i, GFP_KERNEL);
+               if (pci_acpi_mcfg_array) {
+                       mcfg = (struct acpi_table_mcfg *)header;
+                       cfg_table = (struct acpi_mcfg_allocation *) &mcfg[1];
+                       memcpy(pci_acpi_mcfg_array, cfg_table, i);
+                       pci_acpi_mcfg_entries = i / sizeof (*cfg_table);
+               }
        }

        return 0;
@@ -682,8 +698,11 @@ out:
         * Free all MCFG entries if ACPI is enabled. MCFG information will
         * be added back on demand by the pci_root driver later.
         */
-       if (!early && !acpi_disabled && !known_bridge && pci_acpi_mcfg_array)
-               free_all_mmcfg();
+       if (!early && !acpi_disabled && !known_bridge) {
+               acpi_table_parse(ACPI_SIG_MCFG, pci_cache_mcfg);
+               if (pci_acpi_mcfg_array)
+                       free_all_mmcfg();
+       }
 }

 void __init pci_mmcfg_early_init(void)
>> @@ -675,6 +693,14 @@ static void __init __pci_mmcfg_init(int early)
>>                pci_mmcfg_resources_inserted = 1;
>>                pci_mmcfg_arch_init_failed = true;
>>        }
>> +
>> +out:
>> +       /*
>> +        * Free all MCFG entries if ACPI is enabled. MCFG information will
>> +        * be added back on demand by the pci_root driver later.
>> +        */
>> +       if (!early && !acpi_disabled && !known_bridge && pci_acpi_mcfg_array)
>> +               free_all_mmcfg();
> 
> that really change the logic.
> 
> looks like it will break mrst/sfi path.
> 
> the scan from pci_legacy_init() for mrst/sfi will not have ext_pci_ops
> set for bus 0.
> 
> | int __init pci_subsys_init(void)
> | {
> |        /*
> |         * The init function returns an non zero value when
> |         * pci_legacy_init should be invoked.
> |         */
> |        if (x86_init.pci.init())
> |                pci_legacy_init();
> |
> |        pcibios_fixup_peer_bridges();

On platform with SFI implementation such as Moorsetown, acpi_disabled will
be set to 1 because there's no ACPI root pointer and acpi initialize will fail.
And sfi_init() implementation confirms that if acpi is enabled, then sfi will
be disabled. So I will it won't break SFI/MRST.

void __init sfi_init(void)
{
        if (!acpi_disabled)
                disable_sfi();

        if (sfi_disabled)
                return;
}

thanks!
Gerry