public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [patch] PCI Cleanup
@ 2002-08-13  0:08 Matthew Dobson
  2002-08-13 11:45 ` Alan Cox
  0 siblings, 1 reply; 26+ messages in thread
From: Matthew Dobson @ 2002-08-13  0:08 UTC (permalink / raw)
  To: Linus Torvalds, linux-kernel, Alan Cox
  Cc: Martin Bligh, Michael Hohnbaum, Greg KH

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

Linus, Alan, et al.

	Here's a patch I'd like to see included in the 2.5 mainline.  Some of the code 
in arch/i386/pci/direct.c is a bit ugly and definitely redundant.  This patch 
cleans that up.  I've tested in on PCI configuration 1 systems, but I haven't 
had a chance to test this on any systems that use PCI configuration 2.  If 
anyone can do that, that would be awesome.

[mcd@arrakis src]$ diffstat pci_fix-2531.patch
  direct.c | 158 
+++++++++++++++++++-------------------------------------------- 1 files 
changed, 48 insertions(+), 110 deletions(-)

	The gist of the patch is that it move the indirection one layer down and saves 
about 60+ lines of code.  Rather than having 
pci_conf{1|2}_{read|write}_config_{byte|word|dword}, this patch removes the 1/2 
distinction by pushing that down a layer, and calling a generic pointer 
instead.  That pointer is set at init time by the PCI init code.  This pulls 
out 6 functions (pci_conf2_*) that were exact duplicates of the others 
(pci_conf1_*), but differed by 1 character each (s/conf1/conf2/).

Linus, please apply...

Cheers!

-Matt

[-- Attachment #2: pci_fix-2531.patch --]
[-- Type: text/plain, Size: 6852 bytes --]

diff -Nur linux-2.5.30-vanilla/arch/i386/pci/direct.c linux-2.5.30-patched/arch/i386/pci/direct.c
--- linux-2.5.30-vanilla/arch/i386/pci/direct.c	Thu Aug  1 14:16:14 2002
+++ linux-2.5.30-patched/arch/i386/pci/direct.c	Mon Aug 12 10:41:32 2002
@@ -69,76 +69,8 @@
 	return 0;
 }
 
-
 #undef PCI_CONF1_ADDRESS
 
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
-	int result; 
-	u32 data;
-
-	if (!value) 
-		return -EINVAL;
-
-	result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, &data);
-
-	*value = (u8)data;
-
-	return result;
-}
-
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
-	int result; 
-	u32 data;
-
-	if (!value) 
-		return -EINVAL;
-
-	result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 2, &data);
-
-	*value = (u16)data;
-
-	return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
-	if (!value) 
-		return -EINVAL;
-
-	return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
-	return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
-	return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
-	return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static struct pci_ops pci_direct_conf1 = {
-	pci_conf1_read_config_byte,
-	pci_conf1_read_config_word,
-	pci_conf1_read_config_dword,
-	pci_conf1_write_config_byte,
-	pci_conf1_write_config_word,
-	pci_conf1_write_config_dword
-};
 
 
 /*
@@ -217,57 +149,70 @@
 
 #undef PCI_CONF2_ADDRESS
 
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+
+
+static int pci_config_read_byte(struct pci_dev *dev, int where, u8 *value)
 {
 	int result; 
 	u32 data;
-	result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+
+	if (!value) 
+		return -EINVAL;
+
+	result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 1, &data);
 	*value = (u8)data;
 	return result;
 }
 
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_config_read_word(struct pci_dev *dev, int where, u16 *value)
 {
 	int result; 
 	u32 data;
-	result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+
+	if (!value) 
+		return -EINVAL;
+
+	result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 2, &data);
 	*value = (u16)data;
 	return result;
 }
 
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
+static int pci_config_read_dword(struct pci_dev *dev, int where, u32 *value)
 {
-	return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+	if (!value) 
+		return -EINVAL;
+
+	return pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 4, value);
 }
 
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_config_write_byte(struct pci_dev *dev, int where, u8 value)
 {
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+	return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 1, value);
 }
 
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_config_write_word(struct pci_dev *dev, int where, u16 value)
 {
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+	return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 2, value);
 }
 
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
+static int pci_config_write_dword(struct pci_dev *dev, int where, u32 value)
 {
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+	return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
 		PCI_FUNC(dev->devfn), where, 4, value);
 }
 
-static struct pci_ops pci_direct_conf2 = {
-	pci_conf2_read_config_byte,
-	pci_conf2_read_config_word,
-	pci_conf2_read_config_dword,
-	pci_conf2_write_config_byte,
-	pci_conf2_write_config_word,
-	pci_conf2_write_config_dword
+static struct pci_ops pci_direct = {
+	pci_config_read_byte,
+	pci_config_read_word,
+	pci_config_read_dword,
+	pci_config_write_byte,
+	pci_config_write_word,
+	pci_config_write_dword
 };
 
 
@@ -301,7 +246,7 @@
 	return 0;
 }
 
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
 {
 	unsigned int tmp;
 	unsigned long flags;
@@ -312,17 +257,21 @@
 	 * Check if configuration type 1 works.
 	 */
 	if (pci_probe & PCI_PROBE_CONF1) {
+		pci_config_read = pci_conf1_read;
+		pci_config_write = pci_conf1_write;
 		outb (0x01, 0xCFB);
 		tmp = inl (0xCF8);
 		outl (0x80000000, 0xCF8);
 		if (inl (0xCF8) == 0x80000000 &&
-		    pci_sanity_check(&pci_direct_conf1)) {
+		    pci_sanity_check(&pci_direct)) {
 			outl (tmp, 0xCF8);
 			local_irq_restore(flags);
 			printk(KERN_INFO "PCI: Using configuration type 1\n");
 			if (!request_region(0xCF8, 8, "PCI conf1"))
-				return NULL;
-			return &pci_direct_conf1;
+				pci_root_ops = NULL;
+			else
+				pci_root_ops = &pci_direct;
+			return 0;
 		}
 		outl (tmp, 0xCF8);
 	}
@@ -331,36 +280,25 @@
 	 * Check if configuration type 2 works.
 	 */
 	if (pci_probe & PCI_PROBE_CONF2) {
+		pci_config_read = pci_conf2_read;
+		pci_config_write = pci_conf2_write;
 		outb (0x00, 0xCFB);
 		outb (0x00, 0xCF8);
 		outb (0x00, 0xCFA);
 		if (inb (0xCF8) == 0x00 && inb (0xCFA) == 0x00 &&
-		    pci_sanity_check(&pci_direct_conf2)) {
+		    pci_sanity_check(&pci_direct)) {
 			local_irq_restore(flags);
 			printk(KERN_INFO "PCI: Using configuration type 2\n");
 			if (!request_region(0xCF8, 4, "PCI conf2"))
-				return NULL;
-			return &pci_direct_conf2;
+				pci_root_ops = NULL;
+			else
+				pci_root_ops = &pci_direct;
+			return 0;
 		}
 	}
 
 	local_irq_restore(flags);
-	return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
-	if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2)) 
-		&& (pci_root_ops = pci_check_direct())) {
-		if (pci_root_ops == &pci_direct_conf1) {
-			pci_config_read = pci_conf1_read;
-			pci_config_write = pci_conf1_write;
-		}
-		else {
-			pci_config_read = pci_conf2_read;
-			pci_config_write = pci_conf2_write;
-		}
-	}
+	pci_root_ops = NULL;
 	return 0;
 }
 

^ permalink raw reply	[flat|nested] 26+ messages in thread
* RE: [patch] PCI Cleanup
@ 2002-08-15  2:24 Grover, Andrew
  2002-08-15  7:49 ` Martin Mares
                   ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Grover, Andrew @ 2002-08-15  2:24 UTC (permalink / raw)
  To: 'colpatch@us.ibm.com', Linus Torvalds
  Cc: Alan Cox, Martin J. Bligh, linux-kernel, Michael Hohnbaum,
	Greg KH, jgarzik, Grover, Andrew, Diefenbaugh, Paul S

> From: Matthew Dobson [mailto:colpatch@us.ibm.com] 
> OK... Here's the latest version.  Sorry about that last 
> posting... Stupid line 
> wrapping broke the patch!  :(  This patch also removes the 
> pci_config_(read|write) function pointers.  People shouldn't 
> be using these (I 
> don't think) and should be using the pci_ops structure linked 
> through the 
> pci_dev structure.  These end up calling the same functions that the 
> pci_config_(read|write) pointers refer to anyway.  The only 
> places I can see 
> that these are being used in the kernel are in 
> drivers/acpi/osl.c...  Anyone 
> care to comment on the use there or if it can be changed?  
> I've cc'd the 
> authors of the file...

Hi Matthew,

ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
to pass to access functions. It doesn't look like your patch exposes an
interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
access method, does it?

Regards -- Andy

^ permalink raw reply	[flat|nested] 26+ messages in thread
* RE: [patch] PCI Cleanup
@ 2002-08-15 20:23 Grover, Andrew
  2002-08-15 20:54 ` Patrick Mochel
  0 siblings, 1 reply; 26+ messages in thread
From: Grover, Andrew @ 2002-08-15 20:23 UTC (permalink / raw)
  To: 'Patrick Mochel'
  Cc: 'colpatch@us.ibm.com', Linus Torvalds, Alan Cox,
	Martin J. Bligh, linux-kernel, Michael Hohnbaum, Greg KH, jgarzik

> From: Patrick Mochel [mailto:mochel@osdl.org] 
> > ACPI needs access to PCI config space, and it doesn't have 
> a struct pci_dev
> > to pass to access functions. It doesn't look like your 
> patch exposes an
> > interface that 1) doesn't require a pci_dev and 2) 
> abstracts the PCI config
> > access method, does it?
> 
> I think your dependencies are backwards. IIRC, and based on a recent 
> conversation, ACPI needs to access PCI config space when ACPI finds a 
> _INI method for a device in the ACPI namespace. That assumes 
> that it can 
> access the root bus that the device is on. 
> 
> You don't have a PCI device because you haven't implement lockstep 
> enumeration yet in ACPI. With lockstep enumeration, you would 
> add devices 
> to the device tree and let the bus drivers initialize them. 
> With a bit a 
> glue, you would have a pointer to the PCI device correlating 
> to the ACPI 
> namespace object, and a pointer to the PCI bus on which each PCI 
> device/namespace object resides. 
> 
> To spell it out a bit more explicitly, you would start to 
> parse the ACPI
> namespace and find a Host/PCI bridge. You would tell the PCI 
> subsystem to
> probe for a device at that address. It would come back 
> successful, and you
> would obtain a pointer to that bridge device (and bus 
> object). For all the
> subordinate devices to that bridge, you then have access to the config
> space via a real struct pci_bus.

Yes, except that to find the host/pci bridge for bus 0, for example, I need
to run _INI on the device before I run _HID (which is the method that
returns the PNPID). _INI can theoretically access a bus 0 pci config
operation region.

People have mentioned to me that this is unpleasant and I agree, but the
ACPI spec *specifically* says that bus 0 pci config access is always
available.

That said, maybe it is better to keep ugliness caused by ACPI in the ACPI
driver, so if you want to have interfaces that depend on struct pci_dev or
pci_bus, fine, and the ACPI driver can generate a temporary one in order to
call the function. This completely violates the abstraction you are creating
but sssh we won't tell anyone. ;)

BTW this is not just a matter of spec compliance. Some machines actually
didn't work until this was implemented originally.

> If you remember, I sent you a patch that did most of this 
> about 5 months 
> ago. It's a bit out of date, and I guarantee that it doesn't apply 
> anymore. But the concept is the same: we should fix the 
> drivers, not hack 
> them to support a broken interface.

Is this the one that was on bkbits.net for a while? I liked a lot of what
you did with that, but I was so busy with other stuff I didn't get a chance
to pull it in before it got too stale... :(

Regards -- Andy

^ permalink raw reply	[flat|nested] 26+ messages in thread
end of thread, other threads:[~2002-08-19 23:40 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2002-08-13  0:08 [patch] PCI Cleanup Matthew Dobson
2002-08-13 11:45 ` Alan Cox
2002-08-13 14:17   ` Martin J. Bligh
2002-08-13 14:57     ` Alan Cox
2002-08-13 15:15       ` Martin J. Bligh
2002-08-13 17:00       ` Matthew Dobson
2002-08-13 17:23         ` Linus Torvalds
2002-08-13 19:57           ` Martin J. Bligh
2002-08-13 20:13             ` Alan Cox
2002-08-13 20:26               ` Linus Torvalds
2002-08-13 22:29                 ` Matthew Dobson
2002-08-13 22:46                   ` Linus Torvalds
2002-08-14  0:57                     ` Matthew Dobson
2002-08-15  0:23                     ` Matthew Dobson
2002-08-14  7:08               ` Martin Mares
2002-08-13 14:55   ` Martin J. Bligh
2002-08-13 15:07     ` Alan Cox
  -- strict thread matches above, loose matches on Subject: below --
2002-08-15  2:24 Grover, Andrew
2002-08-15  7:49 ` Martin Mares
2002-08-15 15:58 ` Kai Germaschewski
2002-08-15 16:36   ` Greg KH
2002-08-16 22:34     ` Greg KH
2002-08-19 23:41       ` Matthew Dobson
2002-08-15 18:28 ` Patrick Mochel
2002-08-15 20:23 Grover, Andrew
2002-08-15 20:54 ` Patrick Mochel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox