Re: raw_pci_read in quirk_intel_irqbalance

[Matthew Wilcox <matthew@wil.cx>]

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@wil.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@linux.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."