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.
The new code adds the same N entries, and if any add fails, we
free them all. We return with either all entries added, or the table
being empty.
The new add only fails because of kzalloc failure, just like the
previous extend.
In the previous code, the table is cleared by pci_mmcfg_check_hostbridge()
before calling this function, so the table is empty if the extend fails,
just like it is with the new code.
I tried hard to preserve the previous behavior (and I think I did), but
I actually think it would be simpler and better to do this instead:
for (i = 0; i < (1 << segnbits); i++)
pci_mmconfig_add(i, 0, end_bus, base + (1<<28) * i);
I don't see any reason to force *all* adds to fail just because one of
them did.
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
