On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> 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?
JC.
--
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
