On Wednesday 11 November 2009 02:07:13 pm Yinghai Lu wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 11 November 2009 12:48:08 pm Yinghai Lu wrote:
> >> Bjorn Helgaas wrote:
> >>> This patch encapsulate pci_mmcfg_config[] updates. All alloc/free is now
> >>> done in pci_mmconfig_add() and free_all_mcfg(), so all updates to
> >>> pci_mmcfg_config[] and pci_mmcfg_config_num are in those two functions.
> >>>
> >>> This replaces the previous sequence of extend_mmcfg(), fill_one_mmcfg()
> >>> with the single pci_mmconfig_add() interface. This interface is currently
> >>> static but will eventually be used in the host bridge hot-add path.
> >>>
> >>> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
> >>> ---
> >>> arch/x86/pci/mmconfig-shared.c | 85 ++++++++++++++++++----------------------
> >>> 1 files changed, 39 insertions(+), 46 deletions(-)
> >>>
> >>> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> >>> index 7a7b6ba..62a8ecd 100644
> >>> --- a/arch/x86/pci/mmconfig-shared.c
> >>> +++ b/arch/x86/pci/mmconfig-shared.c
> >>> @@ -26,14 +26,24 @@
> >>> /* Indicate if the mmcfg resources have been placed into the resource table. */
> >>> static int __initdata pci_mmcfg_resources_inserted;
> >>>
> >>> -static __init int extend_mmcfg(int num)
> >>> +static __init void free_all_mmcfg(void)
> >>> +{
> >>> + pci_mmcfg_arch_free();
> >>> + pci_mmcfg_config_num = 0;
> >>> + kfree(pci_mmcfg_config);
> >>> + pci_mmcfg_config = NULL;
> >>> +}
> >>> +
> >>> +static __init struct acpi_mcfg_allocation *pci_mmconfig_add(int segment,
> >>> + int start, int end, u64 addr)
> >>> {
> >>> struct acpi_mcfg_allocation *new;
> >>> - int new_num = pci_mmcfg_config_num + num;
> >>> + int new_num = pci_mmcfg_config_num + 1;
> >>> + int i = pci_mmcfg_config_num;
> >>>
> >>> new = kzalloc(sizeof(pci_mmcfg_config[0]) * new_num, GFP_KERNEL);
> >>> if (!new)
> >>> - return -1;
> >>> + return NULL;
> >>>
> >>> if (pci_mmcfg_config) {
> >>> memcpy(new, pci_mmcfg_config,
> >>> @@ -42,18 +52,13 @@ static __init int extend_mmcfg(int num)
> >>> }
> >>> pci_mmcfg_config = new;
> >>>
> >>> - return 0;
> >>> -}
> >>> -
> >>> -static __init void fill_one_mmcfg(u64 addr, int segment, int start, int end)
> >>> -{
> >>> - int i = pci_mmcfg_config_num;
> >>> -
> >>> pci_mmcfg_config_num++;
> >>> pci_mmcfg_config[i].address = addr;
> >>> pci_mmcfg_config[i].pci_segment = segment;
> >>> pci_mmcfg_config[i].start_bus_number = start;
> >>> pci_mmcfg_config[i].end_bus_number = end;
> >>> +
> >>> + return &pci_mmcfg_config[i];
> >>> }
> >>>
> >>> static const char __init *pci_mmcfg_e7520(void)
> >>> @@ -65,11 +70,9 @@ static const char __init *pci_mmcfg_e7520(void)
> >>> if (win == 0x0000 || win == 0xf000)
> >>> return NULL;
> >>>
> >>> - if (extend_mmcfg(1) == -1)
> >>> + if (pci_mmconfig_add(0, 0, 255, win << 16) == NULL)
> >>> return NULL;
> >>>
> >>> - fill_one_mmcfg(win << 16, 0, 0, 255);
> >>> -
> >>> return "Intel Corporation E7520 Memory Controller Hub";
> >>> }
> >>>
> >>> @@ -111,11 +114,9 @@ static const char __init *pci_mmcfg_intel_945(void)
> >>> if ((pciexbar & mask) >= 0xf0000000U)
> >>> return NULL;
> >>>
> >>> - if (extend_mmcfg(1) == -1)
> >>> + if (pci_mmconfig_add(0, 0, (len >> 20) - 1, pciexbar & mask) == NULL)
> >>> return NULL;
> >>>
> >>> - fill_one_mmcfg(pciexbar & mask, 0, 0, (len >> 20) - 1);
> >>> -
> >>> return "Intel Corporation 945G/GZ/P/PL Express Memory Controller Hub";
> >>> }
> >>>
> >>> @@ -124,7 +125,7 @@ static const char __init *pci_mmcfg_amd_fam10h(void)
> >>> u32 low, high, address;
> >>> u64 base, msr;
> >>> int i;
> >>> - unsigned segnbits = 0, busnbits;
> >>> + unsigned segnbits = 0, busnbits, end_bus;
> >>>
> >>> if (!(pci_probe & PCI_CHECK_ENABLE_AMD_MMCONF))
> >>> return NULL;
> >>> @@ -158,11 +159,13 @@ static const char __init *pci_mmcfg_amd_fam10h(void)
> >>> busnbits = 8;
> >>> }
> >>>
> >>> - if (extend_mmcfg(1 << segnbits) == -1)
> >>> - return NULL;
> >>> -
> >>> + end_bus = (1 << busnbits) - 1;
> >>> for (i = 0; i < (1 << segnbits); i++)
> >>> - fill_one_mmcfg(base + (1<<28) * i, i, 0, (1 << busnbits) - 1);
> >>> + if (pci_mmconfig_add(i, 0, end_bus,
> >>> + base + (1<<28) * i) == NULL) {
> >>> + free_all_mmcfg();
> >>> + return NULL;
> >>> + }
> >> here it is not right. will have extra alloc/and free.
> >>
> >> you could use extend/fill to do pci_mmconfig_add
> >
> > I don't see the change in behavior. Can you clarify and give an
> > example, please?
> >
> > Previously, we would extend the table by N entries, then fill them
> > all. If the table couldn't be extended, we wouldn't add any entries
> > at all. The fill never fails. We return with either all the entries
> > added, or none of them.
>
> if you got 16 entries:
> pci_mmconfig_add every time will allocate and copy the old.
> you will allocate 16 times and copy 15 times.
In my new code, we'll call pci_mmconfig_add() 16 times. It will
allocate new table space 16 times. It will copy the old table
15 times (the first time we don't copy, because there was no old
table). It will free the old table 15 times, leaving us with the
single table with 16 entries. This still seems correct to me.
> orginal is extend 16, and fill one by one.
It's true that my new code calls kzalloc() 16 times, where the
old code called it once. I don't think this is a problem.
> your following patches change that to list. so it is ok at last
If there's a problem here, I'd like to fix it. I want the series
to be fully bisectable, with no problems at any intermediate point.
Bjorn
--
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
