Messages in this thread | | | From | Jesse Barnes <> | Subject | Re: [git pull] "big box" x86 changes, PCI | Date | Mon, 28 Apr 2008 13:34:15 -0700 |
| |
On Saturday, April 26, 2008 2:55 pm Ingo Molnar wrote: > @@ -184,51 +322,80 @@ static void __init pci_mmcfg_reject_broken(int type) > > cfg = &pci_mmcfg_config[0]; > > - /* > - * Handle more broken MCFG tables on Asus etc. > - * They only contain a single entry for bus 0-0. > - */ > - if (pci_mmcfg_config_num == 1 && > - cfg->pci_segment == 0 && > - (cfg->start_bus_number | cfg->end_bus_number) == 0) { > - printk(KERN_ERR "PCI: start and end of bus number is 0. " > - "Rejected as broken MCFG.\n"); > - goto reject; > + for (i = 0; i < pci_mmcfg_config_num; i++) { > + int valid = 0; > + u32 size = (cfg->end_bus_number + 1) << 20; > + cfg = &pci_mmcfg_config[i]; > + printk(KERN_NOTICE "PCI: MCFG configuration %d: base %lx " > + "segment %hu buses %u - %u\n", > + i, (unsigned long)cfg->address, cfg->pci_segment, > + (unsigned int)cfg->start_bus_number, > + (unsigned int)cfg->end_bus_number); > + > + if (!early && > + is_acpi_reserved(cfg->address, cfg->address + size - > 1)) { + printk(KERN_NOTICE "PCI: MCFG area at %Lx > reserved " + "in ACPI motherboard > resources\n", > + cfg->address); > + valid = 1; > + } > + > + if (valid) > + continue; > + > + if (!early) > + printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %Lx is > not" + " reserved in ACPI motherboard > resources\n", + cfg->address); > + /* Don't try to do this check unless configuration > + type 1 is available. how about type 2 ?*/ > + if (raw_pci_ops && e820_all_mapped(cfg->address, > + cfg->address + size - 1, > + E820_RESERVED)) { > + printk(KERN_NOTICE > + "PCI: MCFG area at %Lx reserved in E820\n", > + cfg->address); > + valid = 1; > + } > + > + if (!valid) > + goto reject; > }
This loop is a bit messy, is there some way of making it clearer? Maybe the early vs. late stuff should be split into separate routines entirely...
> @@ -842,11 +842,14 @@ static void set_pcie_port_type(struct pci_dev *pdev) > * reading the dword at 0x100 which must either be 0 or a valid extended > * capability header. > */ > -int pci_cfg_space_size(struct pci_dev *dev) > +int pci_cfg_space_size_ext(struct pci_dev *dev, unsigned check_exp_pcix) > { > int pos; > u32 status; > > + if (!check_exp_pcix) > + goto skip; > +
Rather than adding a flag to pci_cfg_space_size, you could either factor out the extended space probe into a separate routine and use it from both pci_cfg_space_size and the fixup code, or just make the fixup code do the probe & cfg_size setting by hand, moving the PCI_CFG_SPACE_SIZE and PCI_CFG_SPACE_EXP_SIZE to pci.h.
In both cases I'm just trying to avoid having a flag you pass to a routine that changes its behavior significantly enough that a new function would make things more readable.
Other than that, things look pretty good. And the resulting kernel boots fine on my test box, which is nice...
Thanks, Jesse
| |