linux-kernel - Re: raw_pci_read in quirk_intel

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: <20080211050415.GJ5299@parisc-linux.org>
Date:	Sun, 10 Feb 2008 22:04:16 -0700
From:	Matthew Wilcox <matthew@....cx>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Yinghai Lu <yhlu.kernel@...il.com>, Greg KH <greg@...ah.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>, Tony Camuso <tcamuso@...hat.com>,
	Grant Grundler <grundler@...isc-linux.org>,
	Loic Prylli <loic@...i.com>, Adrian Bunk <bunk@...nel.org>,
	Arjan van de Ven <arjan@...radead.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Ivan Kokshaysky <ink@...assic.park.msu.ru>,
	Greg KH <gregkh@...e.de>, linux-kernel@...r.kernel.org,
	Jeff Garzik <jeff@...zik.org>,
	linux-pci@...ey.karlin.mff.cuni.cz, Martin Mares <mj@....cz>
Subject: Re: raw_pci_read in quirk_intel_irqbalance

On Sun, Feb 10, 2008 at 04:02:04PM -0700, Matthew Wilcox wrote:
> The line in question reads:
> 
>         /* read xTPR register */
>         raw_pci_read(0, 0, 0x40, 0x4c, 2, &word);
> 
> That's domain 0, bus 0, device 8, function 0, address 0x4c, length 2.
> 
> I've checked the public E7525 and E7520 MCH datasheets, and they don't
> document the xTPR registers; nor do any of the devices in the datasheet
> have registers documented at 0x4c.
> 
> You can see from my lspci above that I don't _have_ a device 8 on bus 0.
> The aforementioned documentation says:
> 
> "A disabled or non-existent device's configuration register space is
> hidden. A disabled or non-existent device will return all ones for reads
> and will drop writes just as if the cycle terminated with a Master Abort
> on PCI."

I'd like to thank Grant for pointing out to me that this is exactly what
the write immediately above this is doing -- enabling device 8 to
respond to config space cycles.

> Now, my E7525 isn't affected by this quirk as it has a revision greater
> than 0x9.  So maybe it's expected that device 8 is hidden on my machine;
> that it's only present on revisions up to 0x9.  But maybe device 8 is
> always hidden, and that's why the author used raw_pci_ops?
> 
> We can still do better than this, though.  We can do:
> 
> -	raw_pci_read(0, 0, 0x40, 0x4c, 2, &word);
> +	pci_bus_read_config_word(dev->bus, PCI_DEVFN(8, 0), 0x4c, &word);
> 
> Using PCI_DEVFN tells people you really did mean device 8, and it's not
> a braino for device 4 or 2 (how many bits for slot and function again?)

Here's the patch to implement the above two suggestions:

----

>From f565b65591a3f90a272b1d511e4ab1728861fe77 Mon Sep 17 00:00:00 2001
From: Matthew Wilcox <matthew@....cx>
Date: Sun, 10 Feb 2008 23:18:15 -0500
Subject: [PATCH] Use proper abstractions in quirk_intel_irqbalance

Since we may not have a pci_dev for the device we need to access, we can't
use pci_read_config_word.  But raw_pci_read is an internal implementation
detail; it's better to use the architected pci_bus_read_config_word
interface.  Using PCI_DEVFN instead of a mysterious constant helps
reassure everyone that we really do intend to access device 8.

Signed-off-by: Matthew Wilcox <willy@...ux.intel.com>
---
 arch/x86/kernel/quirks.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index 1941482..c47208f 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -11,7 +11,7 @@
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)
 {
 	u8 config, rev;
-	u32 word;
+	u16 word;
 
 	/* BIOS may enable hardware IRQ balancing for
 	 * E7520/E7320/E7525(revision ID 0x9 and below)
@@ -26,8 +26,11 @@ static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)
 	pci_read_config_byte(dev, 0xf4, &config);
 	pci_write_config_byte(dev, 0xf4, config|0x2);
 
-	/* read xTPR register */
-	raw_pci_read(0, 0, 0x40, 0x4c, 2, &word);
+	/*
+	 * read xTPR register.  We may not have a pci_dev for device 8
+	 * because it might be hidden until the above write.
+	 */
+	pci_bus_read_config_word(dev->bus, PCI_DEVFN(8, 0), 0x4c, &word);
 
 	if (!(word & (1 << 13))) {
 		dev_info(&dev->dev, "Intel E7520/7320/7525 detected; "
-- 
1.5.2.5

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/