linux-kernel - Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160922230806.GB1514@localhost>
Date:   Thu, 22 Sep 2016 18:08:07 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Christopher Covington <cov@...eaurora.org>
Cc:     Tomasz Nowicki <tn@...ihalf.com>, will.deacon@....com,
        catalin.marinas@....com, rafael@...nel.org,
        Lorenzo.Pieralisi@....com, arnd@...db.de, hanjun.guo@...aro.org,
        okaya@...eaurora.org, jchandra@...adcom.com, dhdang@....com,
        ard.biesheuvel@...aro.org, robert.richter@...iumnetworks.com,
        mw@...ihalf.com, Liviu.Dudau@....com, ddaney@...iumnetworks.com,
        wangyijing@...wei.com, msalter@...hat.com,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linaro-acpi@...ts.linaro.org, jcm@...hat.com,
        andrea.gallo@...aro.org, jeremy.linton@....com,
        liudongdong3@...wei.com, gabriele.paoloni@...wei.com,
        jhugo@...eaurora.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 0/5] ECAM quirks handling for ARM64 platforms

On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote:
> Hi Bjorn,
> 
> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
> > On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@...eaurora.org wrote:
> 
> >>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >>> index eb14f74..bb3b8ad 100644
> >>> --- a/drivers/acpi/pci_mcfg.c
> >>> +++ b/drivers/acpi/pci_mcfg.c
> >>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
> >>> 	struct resource cfgres;
> >>> };
> >>>
> >>> -#define MCFG_DOM_ANY			(-1)
> >>
> >> Did you delete this because there were no current users, because you'd
> >> prefer users just use "-1", or for some other reason?
> > 
> > I removed it because there were no users of it and, more importantly,
> > the code doesn't implement support for it.
> 
> It looks like a stale "First match against PCI topology <domain:bus>..."
> comment remains.

Yep.  I removed the comment since it's sort of obvious from the code.
I also renamed a few things and pulled the match out into a helper
function.

I also changed the dmesg note: I think the actual resource and the
name of the pci_ecam_ops is more interesting than the table IDs (which
I think are already elsewhere in the dmesg log).

Here's the incremental diff, which I can't really test:


diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 245b79f..0b36bc5 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -36,7 +36,7 @@ struct mcfg_fixup {
 	char oem_id[ACPI_OEM_ID_SIZE + 1];
 	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
 	u32 oem_revision;
-	u16 seg;
+	u16 segment;
 	struct resource bus_range;
 	struct pci_ecam_ops *ops;
 	struct resource cfgres;
@@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
 static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
 static u32 mcfg_oem_revision;
 
-static void pci_mcfg_match_quirks(struct acpi_pci_root *root,
+static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment,
+				  struct resource *bus_range)
+{
+	if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
+	    !memcmp(f->oem_table_id, mcfg_oem_table_id,
+	            ACPI_OEM_TABLE_ID_SIZE) &&
+	    f->oem_revision == mcfg_oem_revision &&
+	    f->segment == segment &&
+	    resource_contains(&f->bus_range, bus_range))
+		return 1;
+
+	return 0;
+}
+
+static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
 				  struct resource *cfgres,
 				  struct pci_ecam_ops **ecam_ops)
 {
+	u16 segment = root->segment;
+	struct resource *bus_range = &root->secondary;
 	struct mcfg_fixup *f;
 	int i;
 
-	/*
-	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
-	 * table ID, and OEM revision from MCFG table standard header.
-	 */
 	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
-		if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
-		    !memcmp(f->oem_table_id, mcfg_oem_table_id,
-		            ACPI_OEM_TABLE_ID_SIZE) &&
-		    f->oem_revision == mcfg_oem_revision &&
-		    f->seg == root->segment &&
-		    resource_contains(&f->bus_range, &root->secondary)) {
+		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
 			if (f->cfgres.start)
 				*cfgres = f->cfgres;
 			if (f->ops)
 				*ecam_ops =  f->ops;
-			dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n",
-				 f->oem_id, f->oem_table_id, f->oem_revision);
+			dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n",
+				 bus_range, cfgres, *ecam_ops);
 			return;
 		}
 	}
@@ -173,7 +180,7 @@ skip_lookup:
 	 * MCFG does not have it.  Invalid CFG start address means MCFG
 	 * firmware bug or we need another quirk in array.
 	 */
-	pci_mcfg_match_quirks(root, &res, &ops);
+	pci_mcfg_apply_quirks(root, &res, &ops);
 	if (!res.start)
 		return -ENXIO;