Path: csiph.com!news.mixmin.net!aioe.org!bofh.it!news.nic.it!robomod From: Jon Masters Newsgroups: linux.kernel Subject: Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller Date: Thu, 01 Dec 2016 20:30:02 +0100 Message-ID: References: User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 01 Dec 2016 19:26:35 +0000 (UTC) Sender: robomod@news.nic.it List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Approved: robomod@news.nic.it Lines: 54 Organization: linux.* mail to news gateway X-Original-Cc: Rafael Wysocki , Lorenzo Pieralisi , Arnd Bergmann , linux-pci@vger.kernel.org, linux-arm , Linux Kernel Mailing List , Tomasz Nowicki , patches X-Original-Date: Thu, 1 Dec 2016 14:26:29 -0500 X-Original-Message-ID: <0e7e7181-94f5-0d09-b571-61fb71537f02@redhat.com> X-Original-References: <1480549373-2123-1-git-send-email-dhdang@apm.com> <20161201183350.GF30746@bhelgaas-glaptop.roam.corp.google.com> <1480620053.4751.30.camel@redhat.com> X-Original-Sender: linux-kernel-owner@vger.kernel.org Xref: csiph.com linux.kernel:1534400 On 12/01/2016 02:20 PM, Mark Salter wrote: > On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote: >>> + csr = &xgene_v1_csr_res[root->segment]; >> This makes me nervous because root->segment comes from the ACPI _SEG, >> and if firmware gives us junk in _SEG, we will reference something in >> the weeds. > > The SoC provide some number of RC bridges, each with a different base > for some mmio registers. Even if segment is legitimate in MCFG, there > is still a problem if a platform doesn't use the segment ordering > implied by the code. But the PNP0A03 _CRS does have this base address > as the first memory resource, so we could get it from there and not > have hard-coded addresses and implied ording in the quirk code. > > I have tested a modified version of these quirks using this to > get the CSR base and it works on the 3 different platforms I have > access to. > > static int xgene_pcie_get_csr(struct device *dev, struct resource *r) > { > struct acpi_device *adev = to_acpi_device(dev); > unsigned long flags; > struct list_head list; > struct resource_entry *entry; > int ret; > > INIT_LIST_HEAD(&list); > flags = IORESOURCE_MEM; > ret = acpi_dev_get_resources(adev, &list, > acpi_dev_filter_resource_type_cb, > (void *)flags); > if (ret < 0) { > dev_err(dev, "failed to parse _CRS, error: %d\n", ret); > return ret; > } else if (ret == 0) { > dev_err(dev, "no memory resources present in _CRS\n"); > return -EINVAL; > } > > entry = list_first_entry(&list, struct resource_entry, node); > *r = *entry->res; > acpi_dev_free_resource_list(&list); > return 0; > } This seems a lot safer. At some point trusting firmware to provide the correct _CRS for the RC in use is better than hard coding for every possible implementation configuration of an X-Gene SoC. Jon. -- Computer Architect | Sent from my Fedora powered laptop