Messages in this thread | | | Subject | Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller | From | Tomasz Nowicki <> | Date | Mon, 2 May 2016 13:31:46 +0200 |
| |
On 04/29/2016 07:35 PM, Jayachandran C wrote: > On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote: >> >> [...] >> >>>> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root, >>>> + struct acpi_pci_generic_root_info *ri) >>>> +{ >>>> + u16 seg = root->segment; >>>> + u8 bus_start = root->secondary.start; >>>> + u8 bus_end = root->secondary.end; >>>> + struct pci_config_window *cfg; >>>> + struct mcfg_entry *e; >>>> + phys_addr_t addr; >>>> + int err = 0; >>>> + >>>> + mutex_lock(&pci_mcfg_lock); >>> What does this lock protect? The pci_mcfg_list should already be >>> initialized by the time we get there, and it should be immutable for >>> the life of the system. In fact, I would prefer if we could just >>> search the static table itself whenever we need it rather than caching >>> it in our own list. But I don't think we can easily do that because >>> acpi_table_parse() is __init. >>> >>>> + e = pci_mcfg_lookup(seg, bus_start); >>> I would argue that we should check for _CBA first, and fall back to >>> MCFG if _CBA doesn't exist. >>> >>>> + if (!e) { >>>> + addr = acpi_pci_root_get_mcfg_addr(root->device->handle); >>> IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be >>> acpi_pci_config_base_addr() or similar. It definitely is not related >>> to MCFG. Not your fault, obviously. >>> >>>> + if (addr == 0) { >>>> + pr_err(PREFIX"%04x:%02x-%02x bus range error\n", >>>> + seg, bus_start, bus_end); >>>> + err = -ENOENT; >>>> + goto err_out; >>>> + } >>>> + } else { >>>> + if (bus_start != e->bus_start) { >>>> + pr_err("%04x:%02x-%02x bus range mismatch %02x\n", >>>> + seg, bus_start, bus_end, e->bus_start); >>>> + err = -EINVAL; >>>> + goto err_out; >>>> + } else if (bus_end != e->bus_end) { >>>> + pr_warn("%04x:%02x-%02x bus end mismatch %02x\n", >>>> + seg, bus_start, bus_end, e->bus_end); >>>> + bus_end = min(bus_end, e->bus_end); >>>> + } >>>> + addr = e->addr; >>>> + } >>> I really don't think you need a lock around this, so you can factor >>> out the address lookup into something like: >>> >>> addr = acpi_pci_config_base_addr(...); >>> if (addr) >>> return addr; >>> >>> return acpi_pci_mcfg_lookup(seg, busn_res); >>> >>> You can check inside acpi_pci_mcfg_lookup() to make sure the entry you >>> find covers the entire [busn_res.start-busn_res.end] range and return >>> failure if it doesn't. At this point, I'm not sure it's worth it to >>> truncate the host bridge bus range to match something we find in MCFG. >>> >>> If the MCFG entry covers *more* than the host bridge range from _CRS, >>> that's fine. In any case, we have to be careful with the start address, >>> because the MCFG start address is always based on bus 0, but I think >>> pci_generic_ecam_create() expects the start address based on the >>> bus_start you pass to it. >> Yes, I spotted this too, it is unfortunate but DT and MCFG handle >> the ECAM regions differently. In DT the reg property is relative >> to bus_start - ie reg MMIO region maps config space starting at >> the first bus in bus-range: >> >> Documentation/devicetree/bindings/pci/host-generic-pci.txt >> >> in ACPI(MCFG) as you said it is always relative to bus 0, it is >> unfortunate but the address to be mapped should be computed >> differently in the ECAM layer. > Can't this be handled by fixing up the address before passing to > pci_generic_ecam_create? > I agree, this should work, IMO.
Tomasz
| |