Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752331AbcD2RgS (ORCPT ); Fri, 29 Apr 2016 13:36:18 -0400 Received: from mail-yw0-f169.google.com ([209.85.161.169]:35538 "EHLO mail-yw0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751153AbcD2RgP (ORCPT ); Fri, 29 Apr 2016 13:36:15 -0400 MIME-Version: 1.0 In-Reply-To: <20160429083709.GA3249@red-moon> References: <1460740008-19489-1-git-send-email-tn@semihalf.com> <1460740008-19489-10-git-send-email-tn@semihalf.com> <20160428214800.GG25125@localhost> <20160429083709.GA3249@red-moon> From: Jayachandran C Date: Fri, 29 Apr 2016 23:05:34 +0530 Message-ID: Subject: Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller To: Lorenzo Pieralisi Cc: Bjorn Helgaas , Tomasz Nowicki , Arnd Bergmann , Will Deacon , Catalin Marinas , rafael@kernel.org, Hanjun Guo , Sinan Kaya , jiang.liu@linux.intel.com, robert.richter@caviumnetworks.com, Marcin Wojtas , Liviu.Dudau@arm.com, David Daney , Wangyijing , Suravee.Suthikulpanit@amd.com, msalter@redhat.com, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linaro-acpi@lists.linaro.org, Jon Masters Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3747 Lines: 93 On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi 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.