linux-kernel - Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk

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: <5c39cf29-a08f-48d1-b873-ce0fda763d66@arm.com>
Date:   Mon, 30 Aug 2021 11:23:27 -0500
From:   Jeremy Linton <jeremy.linton@....com>
To:     nicolas saenz julienne <nsaenz@...nel.org>,
        linux-pci@...r.kernel.org
Cc:     lorenzo.pieralisi@....com, bhelgaas@...gle.com, rjw@...ysocki.net,
        lenb@...nel.org, robh@...nel.org, kw@...ux.com,
        f.fainelli@...il.com, bcm-kernel-feedback-list@...adcom.com,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rpi-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk

Hi,

On 8/30/21 3:36 AM, nicolas saenz julienne wrote:
> Hi Jeremy,
> sorry for the late reply, I've been on vacation.
> 
> On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote:
> 
> [...]
> 
>> +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus,
>> +					unsigned int devfn, int where)
>> +{
>> +	struct pci_config_window *cfg = bus->sysdata;
>> +	void __iomem *base = cfg->win;
>> +	int idx;
>> +	u32 up;
>> +
>> +	/* Accesses to the RC go right to the RC registers if slot==0 */
>> +	if (pci_is_root_bus(bus))
>> +		return PCI_SLOT(devfn) ? NULL : base + where;
>> +
>> +	/*
>> +	 * Assure the link is up before sending requests downstream. This is done
>> +	 * to avoid sending transactions to EPs that don't exist. Link flap
>> +	 * conditions/etc make this race more probable. The resulting unrecoverable
>> +	 * SERRORs will result in the machine crashing.
>> +	 */
>> +	up = readl(base + PCIE_MISC_PCIE_STATUS);
>> +	if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK))
>> +		return NULL;
>> +
>> +	if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK))
>> +		return NULL;
> 
> Couldn't this be integrated in the original brcm_pcie_map_conf()? IIUC there is
> nothing ACPI specific about it. It'd also make for less code duplication.

That is where I started with this, but it wasn't the linkup check/etc 
which caused me to hoist it but the fact that if ACPI quirks are enabled 
they end up statically built into the kernel. While if this host bridge 
is enabled, it can end up being a module, and the resulting mess I 
created trying to satisfy the CONFIG variations. I'm not much of a fan 
of copy/paste programming, but that IMHO ended up being the cleanest here.