RE: [patch] PCI Cleanup

RE: [patch] PCI Cleanup

[Grover, Andrew]

> 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

RE: [patch] PCI Cleanup

[Patrick Mochel]

[ cc list trimmed ]

> > 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.

Side question: if you can't run _HID, how do you know it's a PCI bridge? 
Or, if you can't run _HID, is there any way to determine that it's a 
Host/PCI bridge?

> 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.

I thought it was just the root bus that a device was on: either bus 0, or 
the bus specified by _BBN. For most systems it will be bus 0. 

> 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. ;)

Actually, there shouldn't be much magic involved, if any at all. 

The idea behind the patch I sent you before went something like this:

The ACPI namespace parser starts scans the namespace, before any PCI 
config access has been determined. This won't get you very many objects, 
because you won't be able to scan behind the bridges. 

You take that list and start adding devices to the device tree. This uses 
some new interfaces that allow you to tell the bus drivers that a bridge 
or device exists at a specific address. 

When you add a bridge, the bus driver will scan behind it. For each device 
it finds, it calls platform_notify(), which you will implement. This can 
then scan the namespace behind that object, and find more child objects, 
and add them into the device tree. 

There is some implicit recursion in that in the way I described it. It 
doesn't have to, though I don't remember exactly what I did. 

So, for bus 0, you don't have to wait and find it. You can hardcode it in 
there, and tell the system to look for it when you first start up. This is 
basically what the x86 arch code does: it assumes there is a bus 0 and 
starts scanning behind it. 

You'd do the same thing, which sounds like a step backward. But, with the 
lockstep process, you would get to do everything you need to do. Right?

> > 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... :(

Yes, that's the one. It's on my short list of things to update, which is 
long enough as it is. :/

	-pat

Re: [patch] PCI Cleanup

[Matthew Dobson]

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

Greg,
	Here is the numaq portion of the cleanup.  This is the part that I was 
originally intending to fix when this whole thing started.  This patch makes 
the PCI busses on the secondary quads for NUMA-Q working.

Please add this to the pile of PCI cleanups.

Thanks!

-Matt

Greg KH wrote:
> On Thu, Aug 15, 2002 at 09:36:45AM -0700, Greg KH wrote:
> 
>>If there are no complaints, I think I'll go implement this, and move the
>>functions into the main pci code so that other parts of the kernel (like
>>ACPI) can use them.
> 
> 
> Ok, here's the patch that goes on top of Matt's previous patch, moving
> the pci_ops functions to be pci_bus based instead of pci_dev, like they
> are today.  This enables us to get rid of all of the pci_*_nodev
> functions in the pci hotplug core (that's 245 lines removed :)  It's
> currently running on the machine I'm typing this from.
> 
> I used the devfn as a paramater instead of the function and slot, as
> some archs just take the devfn and move it around before using it.  If
> there were two paramaters, we would be splitting the values apart, and
> then putting them back together for every function.
> 
> This is only for i386, if there's no complaints I'll knock out the other
> archs before submitting it.
> 
> thanks,
> 
> greg k-h

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

diff -Nur linux-2.5.31-patched/arch/i386/pci/numa.c linux-2.5.31-numaq/arch/i386/pci/numa.c
--- linux-2.5.31-patched/arch/i386/pci/numa.c	Wed Aug 14 11:07:13 2002
+++ linux-2.5.31-numaq/arch/i386/pci/numa.c	Wed Aug 14 11:11:58 2002
@@ -1,19 +1,19 @@
 /*
  * numa.c - Low-level PCI access for NUMA-Q machines
  */
+
 #include <linux/pci.h>
 #include <linux/init.h>
-
 #include "pci.h"
 
 #define BUS2QUAD(global) (mp_bus_id_to_node[global])
 #define BUS2LOCAL(global) (mp_bus_id_to_local[global])
 #define QUADLOCAL2BUS(quad,local) (quad_local_to_mp_bus_id[quad][local])
 
-#define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
+#define PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg) \
 	(0x80000000 | (BUS2LOCAL(bus) << 16) | (dev << 11) | (fn << 8) | (reg & ~3))
 
-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf1_mq_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
 
@@ -22,7 +22,7 @@
 
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	outl_quad(PCI_CONF1_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
+	outl_quad(PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
 
 	switch (len) {
 	case 1:
@@ -41,7 +41,7 @@
 	return 0;
 }
 
-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf1_mq_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
 {
 	unsigned long flags;
 
@@ -50,7 +50,7 @@
 
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	outl_quad(PCI_CONF1_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
+	outl_quad(PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
 
 	switch (len) {
 	case 1:
@@ -69,6 +69,25 @@
 	return 0;
 }
 
+#undef PCI_CONF1_MQ_ADDRESS
+
+static int pci_conf1_mq_read(struct pci_dev *dev, int where, int size, u32 *value)
+{
+	return __pci_conf1_mq_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
+}
+
+static int pci_conf1_mq_write(struct pci_dev *dev, int where, int size, u32 value)
+{
+	return __pci_conf1_mq_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
+}
+
+static struct pci_ops pci_direct_conf1_mq = {
+	read:	pci_conf1_mq_read,
+	write:	pci_conf1_mq_write
+};
+
 
 static void __devinit pci_fixup_i450nx(struct pci_dev *d)
 {
@@ -102,8 +121,7 @@
 {
 	int quad;
 
-	pci_config_read = pci_conf1_read;
-	pci_config_write = pci_conf1_write;
+	pci_root_ops = &pci_direct_conf1_mq;
 
 	if (pcibios_scanned++)
 		return 0;

Re: [patch] PCI Cleanup

[Greg KH]

On Thu, Aug 15, 2002 at 09:36:45AM -0700, Greg KH wrote:
> 
> If there are no complaints, I think I'll go implement this, and move the
> functions into the main pci code so that other parts of the kernel (like
> ACPI) can use them.

Ok, here's the patch that goes on top of Matt's previous patch, moving
the pci_ops functions to be pci_bus based instead of pci_dev, like they
are today.  This enables us to get rid of all of the pci_*_nodev
functions in the pci hotplug core (that's 245 lines removed :)  It's
currently running on the machine I'm typing this from.

I used the devfn as a paramater instead of the function and slot, as
some archs just take the devfn and move it around before using it.  If
there were two paramaters, we would be splitting the values apart, and
then putting them back together for every function.

This is only for i386, if there's no complaints I'll knock out the other
archs before submitting it.

thanks,

greg k-h


diff -Nru a/arch/i386/pci/direct.c b/arch/i386/pci/direct.c
--- a/arch/i386/pci/direct.c	Fri Aug 16 15:31:06 2002
+++ b/arch/i386/pci/direct.c	Fri Aug 16 15:31:06 2002
@@ -71,16 +71,16 @@
 
 #undef PCI_CONF1_ADDRESS
 
-static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_conf1_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
 {
-	return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_conf1_read(0, bus->number, PCI_SLOT(devfn), 
+		PCI_FUNC(devfn), where, size, value);
 }
 
-static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_conf1_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
 {
-	return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_conf1_write(0, bus->number, PCI_SLOT(devfn), 
+		PCI_FUNC(devfn), where, size, value);
 }
 
 static struct pci_ops pci_direct_conf1 = {
@@ -165,16 +165,16 @@
 
 #undef PCI_CONF2_ADDRESS
 
-static int pci_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_conf2_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
 {
-	return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_conf2_read(0, bus->number, PCI_SLOT(devfn), 
+		PCI_FUNC(devfn), where, size, value);
 }
 
-static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_conf2_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
 {
-	return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_conf2_write(0, bus->number, PCI_SLOT(devfn), 
+		PCI_FUNC(devfn), where, size, value);
 }
 
 static struct pci_ops pci_direct_conf2 = {
@@ -204,9 +204,9 @@
 	bus.number = 0;
 	dev.bus = &bus;
 	for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
-		if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
+		if ((!o->read(&bus, dev.devfn, PCI_CLASS_DEVICE, 2, &x) &&
 		     (x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
-		    (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
+		    (!o->read(&bus, dev.devfn, PCI_VENDOR_ID, 2, &x) &&
 		     (x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
 			return 1;
 	DBG("PCI: Sanity check failed\n");
diff -Nru a/arch/i386/pci/pcbios.c b/arch/i386/pci/pcbios.c
--- a/arch/i386/pci/pcbios.c	Fri Aug 16 15:31:06 2002
+++ b/arch/i386/pci/pcbios.c	Fri Aug 16 15:31:06 2002
@@ -295,16 +295,16 @@
 	return (int)((result & 0xff00) >> 8);
 }
 
-static int pci_bios_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_bios_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
 {
-	return __pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_bios_read(0, bus->number, PCI_SLOT(devfn), 
+		PCI_FUNC(devfn), where, size, value);
 }
 
-static int pci_bios_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_bios_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
 {
-	return __pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-		PCI_FUNC(dev->devfn), where, size, value);
+	return __pci_bios_write(0, bus->number, PCI_SLOT(devfn),
+		PCI_FUNC(devfn), where, size, value);
 }
 
 
diff -Nru a/drivers/pci/access.c b/drivers/pci/access.c
--- a/drivers/pci/access.c	Fri Aug 16 15:31:06 2002
+++ b/drivers/pci/access.c	Fri Aug 16 15:31:06 2002
@@ -20,27 +20,29 @@
 #define PCI_dword_BAD (pos & 3)
 
 #define PCI_OP_READ(size,type,len) \
-int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
+int pci_bus_read_config_##size \
+	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
 {									\
 	int res;							\
 	unsigned long flags;						\
 	u32 data = 0;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	res = dev->bus->ops->read(dev, pos, len, &data);		\
+	res = bus->ops->read(bus, devfn, pos, len, &data);		\
 	*value = (type)data;						\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
 #define PCI_OP_WRITE(size,type,len) \
-int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+int pci_bus_write_config_##size \
+	(struct pci_bus *bus, unsigned int devfn, int pos, type value)	\
 {									\
 	int res;							\
 	unsigned long flags;						\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	res = dev->bus->ops->write(dev, pos, len, value);		\
+	res = bus->ops->write(bus, devfn, pos, len, value);		\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
@@ -52,10 +54,10 @@
 PCI_OP_WRITE(word, u16, 2)
 PCI_OP_WRITE(dword, u32, 4)
 
-EXPORT_SYMBOL(pci_read_config_byte);
-EXPORT_SYMBOL(pci_read_config_word);
-EXPORT_SYMBOL(pci_read_config_dword);
-EXPORT_SYMBOL(pci_write_config_byte);
-EXPORT_SYMBOL(pci_write_config_word);
-EXPORT_SYMBOL(pci_write_config_dword);
+EXPORT_SYMBOL(pci_bus_read_config_byte);
+EXPORT_SYMBOL(pci_bus_read_config_word);
+EXPORT_SYMBOL(pci_bus_read_config_dword);
+EXPORT_SYMBOL(pci_bus_write_config_byte);
+EXPORT_SYMBOL(pci_bus_write_config_word);
+EXPORT_SYMBOL(pci_bus_write_config_dword);
 EXPORT_SYMBOL(pci_lock);
diff -Nru a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h	Fri Aug 16 15:31:06 2002
+++ b/include/linux/pci.h	Fri Aug 16 15:31:06 2002
@@ -457,8 +457,8 @@
 /* Low-level architecture-dependent routines */
 
 struct pci_ops {
-	int (*read)(struct pci_dev *, int where, int size, u32 *val);
-	int (*write)(struct pci_dev *, int where, int size, u32 val);
+	int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
+	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
 };
 
 struct pbus_set_ranges_data
@@ -557,12 +557,37 @@
 struct pci_dev *pci_find_slot (unsigned int bus, unsigned int devfn);
 int pci_find_capability (struct pci_dev *dev, int cap);
 
-int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val);
-int pci_read_config_word(struct pci_dev *dev, int where, u16 *val);
-int pci_read_config_dword(struct pci_dev *dev, int where, u32 *val);
-int pci_write_config_byte(struct pci_dev *dev, int where, u8 val);
-int pci_write_config_word(struct pci_dev *dev, int where, u16 val);
-int pci_write_config_dword(struct pci_dev *dev, int where, u32 val);
+int pci_bus_read_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 *val);
+int pci_bus_read_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 *val);
+int pci_bus_read_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 *val);
+int pci_bus_write_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 val);
+int pci_bus_write_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 val);
+int pci_bus_write_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 val);
+
+static inline int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val)
+{
+	return pci_bus_read_config_byte (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
+{
+	return pci_bus_read_config_word (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_read_config_dword(struct pci_dev *dev, int where, u32 *val)
+{
+	return pci_bus_read_config_dword (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_byte(struct pci_dev *dev, int where, u8 val)
+{
+	return pci_bus_write_config_byte (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_word(struct pci_dev *dev, int where, u16 val)
+{
+	return pci_bus_write_config_word (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_dword(struct pci_dev *dev, int where, u32 val)
+{
+	return pci_bus_write_config_dword (dev->bus, dev->devfn, where, val);
+}
 
 extern spinlock_t pci_lock;
 

RE: [patch] PCI Cleanup

[Patrick Mochel]

> 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.

There is no need to modify the PCI interface to support a fake device. The 
PCI driver needs to be able to add and probe for devices as dictated by 
someone else, but it's not that difficult.

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.

	-pat

Re: [patch] PCI Cleanup

[Greg KH]

On Thu, Aug 15, 2002 at 10:58:17AM -0500, Kai Germaschewski wrote:
> On Wed, 14 Aug 2002, Grover, Andrew wrote:
> 
> > 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 drivers/hotplug/pci_hotplug_util.c implements something like you 
> need, pci_read_config_byte_nodev().
> 
> Of course that's currently only available for PCI hotplug, and for all I 
> can see the concept is somewhat messed up, but maybe that's an opportunity 
> to clean things up?

I would love to clean those functions up.

> Currently, pci_read_config_byte_nodev() will construct a fake struct 
> pci_dev, and then use the normal pci_read_config_byte(). I think it 
> makes more sense to actually do things the other way around.
> 
> For reading/writing config space, we need to know (dev, fn), and need the 
> access method (struct pci_ops), which is a property of the bridge plus 
> possibly some private data (the arch's sysdata). So the member
> 
> 	struct pci_ops *ops;
> 
> of struct pci_dev is actually not necessary, it will always be
> pdev->ops == pdev->bus->ops AFAICS.
> 
> So we could instead have
> 
> 	pci_bus_read_config_byte(struct pci_bus *bus, u8 dev, u8 fn, ...)
> 
> and for common use
> 
> 	static inline pci_read_config_byte(struct pci_dev, *pdev, ...)
> 	{
> 		return pci_bus_read_config_byte(pdev->bus,
> 						PCI_SLOT(pdev->devfn),
> 						PCI_FUNC(pdev->devfn));
> 	}
> 
> The PCI hotplug controllers / ACPI could then use the pci_bus_* variants, 
> when they don't have a struct pci_dev available. They would need at least 
> the root bridge's struct pci_bus, though.

Thats a good idea.  The hotplug controllers do have acess to the pci_bus
structure.  Andy, does ACPI have access to this when you are needing to
do these kinds of calls?

If there are no complaints, I think I'll go implement this, and move the
functions into the main pci code so that other parts of the kernel (like
ACPI) can use them.

Thanks for the idea!

greg k-h

RE: [patch] PCI Cleanup

[Kai Germaschewski]

On Wed, 14 Aug 2002, Grover, Andrew wrote:

> 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 drivers/hotplug/pci_hotplug_util.c implements something like you 
need, pci_read_config_byte_nodev().

Of course that's currently only available for PCI hotplug, and for all I 
can see the concept is somewhat messed up, but maybe that's an opportunity 
to clean things up?

Currently, pci_read_config_byte_nodev() will construct a fake struct 
pci_dev, and then use the normal pci_read_config_byte(). I think it 
makes more sense to actually do things the other way around.

For reading/writing config space, we need to know (dev, fn), and need the 
access method (struct pci_ops), which is a property of the bridge plus 
possibly some private data (the arch's sysdata). So the member

	struct pci_ops *ops;

of struct pci_dev is actually not necessary, it will always be
pdev->ops == pdev->bus->ops AFAICS.

So we could instead have

	pci_bus_read_config_byte(struct pci_bus *bus, u8 dev, u8 fn, ...)

and for common use

	static inline pci_read_config_byte(struct pci_dev, *pdev, ...)
	{
		return pci_bus_read_config_byte(pdev->bus,
						PCI_SLOT(pdev->devfn),
						PCI_FUNC(pdev->devfn));
	}

The PCI hotplug controllers / ACPI could then use the pci_bus_* variants, 
when they don't have a struct pci_dev available. They would need at least 
the root bridge's struct pci_bus, though.

--Kai

Re: [patch] PCI Cleanup

[Martin Mares]

Hi Andy,

> 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?

What interface would you like to see?

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
No fortune, no luck, no sig!

RE: [patch] PCI Cleanup

[Grover, Andrew]

> 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

Re: [patch] PCI Cleanup

[Matthew Dobson]

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

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...

A full version that fixes all arch's should be coming soon.

-Matt


Linus Torvalds wrote:
> On Tue, 13 Aug 2002, Matthew Dobson wrote:
> 
>>	I think I'm getting what you're aiming for here.  See if this patch comes close 
>>to what you're looking for.
> 
> 
> This seems to be more what I was thinking of, yes (although please drop 
> the "size_independent" part of the names, that just looks too horrible ;)
> 
> 		Linus
> 
> 


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

diff -Nur linux-2.5.31-vanilla/arch/i386/pci/common.c linux-2.5.31-patched/arch/i386/pci/common.c
--- linux-2.5.31-vanilla/arch/i386/pci/common.c	Sat Aug 10 18:41:27 2002
+++ linux-2.5.31-patched/arch/i386/pci/common.c	Wed Aug 14 10:57:40 2002
@@ -25,9 +25,6 @@
 struct pci_bus *pci_root_bus = NULL;
 struct pci_ops *pci_root_ops = NULL;
 
-int (*pci_config_read)(int seg, int bus, int dev, int fn, int reg, int len, u32 *value) = NULL;
-int (*pci_config_write)(int seg, int bus, int dev, int fn, int reg, int len, u32 value) = NULL;
-
 /*
  * legacy, numa, and acpi all want to call pcibios_scan_root
  * from their initcalls. This flag prevents that.
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c	Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c	Tue Aug 13 16:28:13 2002
@@ -13,7 +13,7 @@
 #define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
 	(0x80000000 | (bus << 16) | (dev << 11) | (fn << 8) | (reg & ~3))
 
-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
 
@@ -41,7 +41,7 @@
 	return 0;
 }
 
-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
 {
 	unsigned long flags;
 
@@ -69,75 +69,23 @@
 	return 0;
 }
 
-
 #undef PCI_CONF1_ADDRESS
 
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *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;
+	return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 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);
+	return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, 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
+	read:	pci_conf1_read,
+	write:	pci_conf1_write
 };
 
 
@@ -147,7 +95,7 @@
 
 #define PCI_CONF2_ADDRESS(dev, reg)	(u16)(0xC000 | (dev << 8) | reg)
 
-static int pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
 
@@ -181,7 +129,7 @@
 	return 0;
 }
 
-static int pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
 {
 	unsigned long flags;
 
@@ -217,57 +165,21 @@
 
 #undef PCI_CONF2_ADDRESS
 
-static int pci_conf2_read_config_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), 
-		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)
-{
-	int result; 
-	u32 data;
-	result = pci_conf2_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)
-{
-	return pci_conf2_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_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
 {
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, value);
+	return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
 {
-	return pci_conf2_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)
-{
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
+	return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, 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
+	read:	pci_conf2_read,
+	write:	pci_conf2_write
 };
 
 
@@ -283,7 +195,7 @@
  */
 static int __devinit pci_sanity_check(struct pci_ops *o)
 {
-	u16 x;
+	u32 x;
 	struct pci_bus bus;		/* Fake bus and device */
 	struct pci_dev dev;
 
@@ -292,16 +204,16 @@
 	bus.number = 0;
 	dev.bus = &bus;
 	for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
-		if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+		if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
 		     (x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
-		    (!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+		    (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
 		     (x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
 			return 1;
 	DBG("PCI: Sanity check failed\n");
 	return 0;
 }
 
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
 {
 	unsigned int tmp;
 	unsigned long flags;
@@ -321,8 +233,10 @@
 			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_conf1;
+			return 0;
 		}
 		outl (tmp, 0xCF8);
 	}
@@ -339,28 +253,15 @@
 			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_conf2;
+			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;
 }
 
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/pcbios.c linux-2.5.31-patched/arch/i386/pci/pcbios.c
--- linux-2.5.31-vanilla/arch/i386/pci/pcbios.c	Sat Aug 10 18:41:19 2002
+++ linux-2.5.31-patched/arch/i386/pci/pcbios.c	Wed Aug 14 10:55:20 2002
@@ -185,7 +185,7 @@
 	return (int) (ret & 0xff00) >> 8;
 }
 
-static int pci_bios_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_bios_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
 {
 	unsigned long result = 0;
 	unsigned long flags;
@@ -240,7 +240,7 @@
 	return (int)((result & 0xff00) >> 8);
 }
 
-static int pci_bios_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_bios_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
 {
 	unsigned long result = 0;
 	unsigned long flags;
@@ -295,63 +295,16 @@
 	return (int)((result & 0xff00) >> 8);
 }
 
-static int pci_bios_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_bios_read(struct pci_dev *dev, int where, int size, u32 *value)
 {
-	int result; 
-	u32 data;
-
-	if (!value) 
-		return -EINVAL;
-
-	result = pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, &data);
-
-	*value = (u8)data;
-
-	return result;
-}
-
-static int pci_bios_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
-	int result; 
-	u32 data;
-
-	if (!value) 
-		return -EINVAL;
-
-	result = pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 2, &data);
-
-	*value = (u16)data;
-
-	return result;
-}
-
-static int pci_bios_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
-	if (!value) 
-		return -EINVAL;
-	
-	return pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_bios_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
-	return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_bios_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
-	return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 2, value);
+	return __pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
-static int pci_bios_write_config_dword(struct pci_dev *dev, int where, u32 value)
+static int pci_bios_write(struct pci_dev *dev, int where, int size, u32 value)
 {
-	return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
+	return __pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
 
@@ -360,12 +313,8 @@
  */
 
 static struct pci_ops pci_bios_access = {
-      pci_bios_read_config_byte,
-      pci_bios_read_config_word,
-      pci_bios_read_config_dword,
-      pci_bios_write_config_byte,
-      pci_bios_write_config_word,
-      pci_bios_write_config_dword
+      read:	pci_bios_read,
+      write:	pci_bios_write
 };
 
 /*
@@ -551,8 +500,6 @@
 		&& ((pci_root_ops = pci_find_bios()))) {
 		pci_probe |= PCI_BIOS_SORT;
 		pci_bios_present = 1;
-		pci_config_read = pci_bios_read;
-		pci_config_write = pci_bios_write;
 	}
 	return 0;
 }
diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c	Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c	Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
 #define PCI_word_BAD (pos & 1)
 #define PCI_dword_BAD (pos & 3)
 
-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
 {									\
 	int res;							\
 	unsigned long flags;						\
+	u32 data;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	res = dev->bus->ops->rw##_##size(dev, pos, value);		\
+	res = dev->bus->ops->read(dev, pos, len, &data);		\
+	*value = (type)data;						\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{									\
+	int res;							\
+	unsigned long flags;						\
+	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
+	spin_lock_irqsave(&pci_lock, flags);				\
+	res = dev->bus->ops->write(dev, pos, len, value);		\
+	spin_unlock_irqrestore(&pci_lock, flags);			\
+	return res;							\
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
 
 EXPORT_SYMBOL(pci_read_config_byte);
 EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/asm-i386/pci.h linux-2.5.31-patched/include/asm-i386/pci.h
--- linux-2.5.31-vanilla/include/asm-i386/pci.h	Sat Aug 10 18:41:27 2002
+++ linux-2.5.31-patched/include/asm-i386/pci.h	Wed Aug 14 10:58:52 2002
@@ -22,8 +22,6 @@
 
 void pcibios_config_init(void);
 struct pci_bus * pcibios_scan_root(int bus);
-extern int (*pci_config_read)(int seg, int bus, int dev, int fn, int reg, int len, u32 *value);
-extern int (*pci_config_write)(int seg, int bus, int dev, int fn, int reg, int len, u32 value);
 
 void pcibios_set_master(struct pci_dev *dev);
 void pcibios_penalize_isa_irq(int irq);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h	Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h	Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
 /* Low-level architecture-dependent routines */
 
 struct pci_ops {
-	int (*read_byte)(struct pci_dev *, int where, u8 *val);
-	int (*read_word)(struct pci_dev *, int where, u16 *val);
-	int (*read_dword)(struct pci_dev *, int where, u32 *val);
-	int (*write_byte)(struct pci_dev *, int where, u8 val);
-	int (*write_word)(struct pci_dev *, int where, u16 val);
-	int (*write_dword)(struct pci_dev *, int where, u32 val);
+	int (*read)(struct pci_dev *, int where, int size, u32 *val);
+	int (*write)(struct pci_dev *, int where, int size, u32 val);
 };
 
 struct pbus_set_ranges_data

Re: [patch] PCI Cleanup

[Martin Mares]

Hi!

> It wants to force its own conf1/conf2 over the BIOS even if BIOS is
> preferred because some BIOSes dont honour the size requested and the
> hardware has bugs.
> 
> That to me says there may well be cleaner approaches.

I haven't looked at the current source yet, but the PCI access code
used to work this way: if both BIOS and direct access were enabled
(which was the default) and both worked, BIOS was used only for getting
information about interrupt routing and last bus number and direct
access for everything else. Hence the problematic BIOS calls were used
only if there were no other possibility or the user explicitly told
us to.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
No fortune, no luck, no sig!

Re: [patch] PCI Cleanup

[Matthew Dobson]

Linus Torvalds wrote:
> On Tue, 13 Aug 2002, Matthew Dobson wrote:
> 
>>	I think I'm getting what you're aiming for here.  See if this patch comes close 
>>to what you're looking for.
> 
> This seems to be more what I was thinking of, yes (although please drop 
> the "size_independent" part of the names, that just looks too horrible ;)
Fair enough..  Couldn't think of a fun creative name anyway...  Just took the 
road more traveled and renamed the old pci_conf1_read __pci_conf1_read and the 
old pci_conf1_read_size_indep pci_conf1_read.  Much prettier.  And it even 
seems to work on i386.  I definitely wouldn't try booting this on any other 
arch's though! ;)

-Matt



diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c 
linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c	Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c	Tue Aug 13 16:14:40 2002
@@ -13,7 +13,7 @@
  #define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
  	(0x80000000 | (bus << 16) | (dev << 11) | (fn << 8) | (reg & ~3))

-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int 
len, u32 *value)
+static int __pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int 
len, u32 *value)
  {
  	unsigned long flags;

@@ -41,7 +41,7 @@
  	return 0;
  }

-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int 
len, u32 value)
+static int __pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int 
len, u32 value)
  {
  	unsigned long flags;

@@ -69,75 +69,23 @@
  	return 0;
  }

-
  #undef PCI_CONF1_ADDRESS

-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *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;
+ 
return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ 
	PCI_FUNC(dev->devfn), where, size, value);
  }

-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 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);
+ 
return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ 
	PCI_FUNC(dev->devfn), where, size, 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
+ 
read: 
pci_conf1_read,
+ 
write: 
pci_conf1_write
  };


@@ -147,7 +95,7 @@

  #define PCI_CONF2_ADDRESS(dev, reg)	(u16)(0xC000 | (dev << 8) | reg)

-static int pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int 
len, u32 *value)
+static int __pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int 
len, u32 *value)
  {
  	unsigned long flags;

@@ -181,7 +129,7 @@
  	return 0;
  }

-static int pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int 
len, u32 value)
+static int __pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int 
len, u32 value)
  {
  	unsigned long flags;

@@ -217,57 +165,21 @@

  #undef PCI_CONF2_ADDRESS

-static int pci_conf2_read_config_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),
- 
	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)
-{
- 
int result;
- 
u32 data;
- 
result = pci_conf2_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)
-{
- 
return pci_conf2_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_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
  {
- 
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- 
	PCI_FUNC(dev->devfn), where, 1, value);
+ 
return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ 
	PCI_FUNC(dev->devfn), where, size, value);
  }

-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
  {
- 
return pci_conf2_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)
-{
- 
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- 
	PCI_FUNC(dev->devfn), where, 4, value);
+ 
return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ 
	PCI_FUNC(dev->devfn), where, size, 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
+ 
read: 
pci_conf2_read,
+ 
write: 
pci_conf2_write
  };


@@ -292,16 +204,16 @@
  	bus.number = 0;
  	dev.bus = &bus;
  	for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
- 
	if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+ 
	if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
  		     (x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
- 
	    (!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+ 
	    (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
  		     (x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
  	 
	return 1;
  	DBG("PCI: Sanity check failed\n");
  	return 0;
  }

-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
  {
  	unsigned int tmp;
  	unsigned long flags;
@@ -321,8 +233,10 @@
  	 
	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_conf1;
+ 
		return 0;
  		}
  		outl (tmp, 0xCF8);
  	}
@@ -339,28 +253,15 @@
  	 
	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_conf2;
+ 
		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;
  }

diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c 
linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c	Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c	Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
  #define PCI_word_BAD (pos & 1)
  #define PCI_dword_BAD (pos & 3)

-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
  {	 
							\
  	int res;				 
		\
  	unsigned long flags;						\
+ 
u32 data;			 
			\
  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
  	spin_lock_irqsave(&pci_lock, flags);				\
- 
res = dev->bus->ops->rw##_##size(dev, pos, value);		\
+ 
res = dev->bus->ops->read(dev, pos, len, &data);		\
+ 
*value = (type)data;						\
  	spin_unlock_irqrestore(&pci_lock, flags);			\
  	return res;							\
  }

-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{ 
								\
+ 
int res;			 
			\
+ 
unsigned long flags;						\
+ 
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
+ 
spin_lock_irqsave(&pci_lock, flags);				\
+ 
res = dev->bus->ops->write(dev, pos, len, value);		\
+ 
spin_unlock_irqrestore(&pci_lock, flags);			\
+ 
return res;							\
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)

  EXPORT_SYMBOL(pci_read_config_byte);
  EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h 
linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h	Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h	Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
  /* Low-level architecture-dependent routines */

  struct pci_ops {
- 
int (*read_byte)(struct pci_dev *, int where, u8 *val);
- 
int (*read_word)(struct pci_dev *, int where, u16 *val);
- 
int (*read_dword)(struct pci_dev *, int where, u32 *val);
- 
int (*write_byte)(struct pci_dev *, int where, u8 val);
- 
int (*write_word)(struct pci_dev *, int where, u16 val);
- 
int (*write_dword)(struct pci_dev *, int where, u32 val);
+ 
int (*read)(struct pci_dev *, int where, int size, u32 *val);
+ 
int (*write)(struct pci_dev *, int where, int size, u32 val);
  };

  struct pbus_set_ranges_data

Re: [patch] PCI Cleanup

[Linus Torvalds]

On Tue, 13 Aug 2002, Matthew Dobson wrote:
> 	I think I'm getting what you're aiming for here.  See if this patch comes close 
> to what you're looking for.

This seems to be more what I was thinking of, yes (although please drop 
the "size_independent" part of the names, that just looks too horrible ;)

		Linus

Re: [patch] PCI Cleanup

[Matthew Dobson]

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

Linus,
	I think I'm getting what you're aiming for here.  See if this patch comes close 
to what you're looking for.

	The basic idea is we get rid of the byte/word/dword functions and just put a 
size arg in the generic calls.  I put back the different struct pci_ops for 
conf1 and conf2, so that drivers and other code could use either config option. 
  I also fixed the macros in access.h so that it correctly calls the new functions.

Please *don't* apply this as it will break a bunch of stuff (just about all PCI 
code ;)

Cheers!

-Matt


Linus Torvalds wrote:
> On 13 Aug 2002, Alan Cox wrote:
> 
>>>OK, that IDE thing smacks of unmitigated evil to me, but if things are relying 
>>>on it, we shouldn't change it.
>>
>>It wants to force its own conf1/conf2 over the BIOS even if BIOS is
>>preferred because some BIOSes dont honour the size requested and the
>>hardware has bugs.
>>
>>That to me says there may well be cleaner approaches.
> 
> 
> The thing I liked about the separate structures for function pointers for 
> conf1/conf2 is that I could at least _see_ that the IDE driver might some 
> day be changed to just do
> 
> 	..
> 	conf2_struct->pci_config_read_byte(..)
> 	..
> 
> even if (judging by past performance) this would never happen ;)
> 
> This is why I'd like to continue with the notion of having a well-defined 
> structure that contains all the pointers (and one default case). Now, 
> shrinking those structures down to 2 entries instead of 6 sounds like a 
> fine idea to me, but short-circuiting them internally sounds bad because 
> it loses the ability to use the pci config space functions independently 
> of each other.
> 
> 		Linus
> 
> 


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

diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c	Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c	Tue Aug 13 15:22:03 2002
@@ -69,75 +69,23 @@
 	return 0;
 }
 
-
 #undef PCI_CONF1_ADDRESS
 
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read_size_indep(struct pci_dev *dev, int where, int size, u32 *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);
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_conf1_write_size_indep(struct pci_dev *dev, int where, int size, u32 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);
+		PCI_FUNC(dev->devfn), where, size, 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
+	pci_conf1_read_size_indep,
+	pci_conf1_write_size_indep
 };
 
 
@@ -217,57 +165,21 @@
 
 #undef PCI_CONF2_ADDRESS
 
-static int pci_conf2_read_config_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), 
-		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)
-{
-	int result; 
-	u32 data;
-	result = pci_conf2_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_conf2_read_size_indep(struct pci_dev *dev, int where, int size, u32 *value)
 {
 	return pci_conf2_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)
-{
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 1, value);
+		PCI_FUNC(dev->devfn), where, size, value);
 }
 
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write_size_indep(struct pci_dev *dev, int where, int size, u32 value)
 {
 	return pci_conf2_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)
-{
-	return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), 
-		PCI_FUNC(dev->devfn), where, 4, value);
+		PCI_FUNC(dev->devfn), where, size, 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
+	pci_conf2_read_size_indep,
+	pci_conf2_write_size_indep
 };
 
 
@@ -292,16 +204,16 @@
 	bus.number = 0;
 	dev.bus = &bus;
 	for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
-		if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+		if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
 		     (x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
-		    (!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+		    (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
 		     (x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
 			return 1;
 	DBG("PCI: Sanity check failed\n");
 	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,6 +224,8 @@
 	 * 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);
@@ -321,8 +235,10 @@
 			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_conf1;
+			return 0;
 		}
 		outl (tmp, 0xCF8);
 	}
@@ -331,6 +247,8 @@
 	 * 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);
@@ -339,28 +257,15 @@
 			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_conf2;
+			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;
 }
 
diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c	Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c	Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
 #define PCI_word_BAD (pos & 1)
 #define PCI_dword_BAD (pos & 3)
 
-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
 {									\
 	int res;							\
 	unsigned long flags;						\
+	u32 data;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	res = dev->bus->ops->rw##_##size(dev, pos, value);		\
+	res = dev->bus->ops->read(dev, pos, len, &data);		\
+	*value = (type)data;						\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{									\
+	int res;							\
+	unsigned long flags;						\
+	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
+	spin_lock_irqsave(&pci_lock, flags);				\
+	res = dev->bus->ops->write(dev, pos, len, value);		\
+	spin_unlock_irqrestore(&pci_lock, flags);			\
+	return res;							\
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
 
 EXPORT_SYMBOL(pci_read_config_byte);
 EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h	Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h	Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
 /* Low-level architecture-dependent routines */
 
 struct pci_ops {
-	int (*read_byte)(struct pci_dev *, int where, u8 *val);
-	int (*read_word)(struct pci_dev *, int where, u16 *val);
-	int (*read_dword)(struct pci_dev *, int where, u32 *val);
-	int (*write_byte)(struct pci_dev *, int where, u8 val);
-	int (*write_word)(struct pci_dev *, int where, u16 val);
-	int (*write_dword)(struct pci_dev *, int where, u32 val);
+	int (*read)(struct pci_dev *, int where, int size, u32 *val);
+	int (*write)(struct pci_dev *, int where, int size, u32 val);
 };
 
 struct pbus_set_ranges_data

Re: [patch] PCI Cleanup

[Linus Torvalds]

On 13 Aug 2002, Alan Cox wrote:
> > 
> > OK, that IDE thing smacks of unmitigated evil to me, but if things are relying 
> > on it, we shouldn't change it.
> 
> It wants to force its own conf1/conf2 over the BIOS even if BIOS is
> preferred because some BIOSes dont honour the size requested and the
> hardware has bugs.
> 
> That to me says there may well be cleaner approaches.

The thing I liked about the separate structures for function pointers for 
conf1/conf2 is that I could at least _see_ that the IDE driver might some 
day be changed to just do

	..
	conf2_struct->pci_config_read_byte(..)
	..

even if (judging by past performance) this would never happen ;)

This is why I'd like to continue with the notion of having a well-defined 
structure that contains all the pointers (and one default case). Now, 
shrinking those structures down to 2 entries instead of 6 sounds like a 
fine idea to me, but short-circuiting them internally sounds bad because 
it loses the ability to use the pci config space functions independently 
of each other.

		Linus

Re: [patch] PCI Cleanup

[Alan Cox]

On Tue, 2002-08-13 at 20:57, Martin J. Bligh wrote:

> > to do conf2 accesses, and nothing else. So it duplicates its own conf2 
> > functions right now, because it has no way to hook into the generic ones).
> 
> OK, that IDE thing smacks of unmitigated evil to me, but if things are relying 
> on it, we shouldn't change it.

It wants to force its own conf1/conf2 over the BIOS even if BIOS is
preferred because some BIOSes dont honour the size requested and the
hardware has bugs.

That to me says there may well be cleaner approaches.

Alan

Re: [patch] PCI Cleanup

[Martin J. Bligh]

> Quite frankly, to me it looks like the real issue is that you don't want 
> to have the byte/word/dword stuff replicated three times.

Exactly - we can do that, but it just seems messy.
 
> And your cleanup avoids the replication, but I think it has other badness 
> in it: in particular it depends on those two global pointers. Which makes 
> it really hard to have (for example) per-segment functions, or hard for 
> drivers to hook into it (there's one IDE driver in particular that wants 
> to do conf2 accesses, and nothing else. So it duplicates its own conf2 
> functions right now, because it has no way to hook into the generic ones).

OK, that IDE thing smacks of unmitigated evil to me, but if things are relying 
on it, we shouldn't change it.
 
>   And I'd suggest multiplexing them at a higher level. Instead of 
>   six different pcibios_read_config_byte etc functions, move the 
>   multiplexing up, make make them just two functions at _that_ level (and 
>   make siz #defines to make it compatible with existing users).

Yup, that sounds like ultimately the correct thing to do. Will try to get
that way done instead. Should clean things up nicely ....

M.

Re: [patch] PCI Cleanup

[Linus Torvalds]

On Tue, 13 Aug 2002, Matthew Dobson wrote:
>
> I think that it is definitely a simplification, although I am a bit biased ;) 
> It makes it easier for other configuration types to hook into the system as 
> well (I'm partial to NUMA-Q as well ;).  All they have to do is hijack the 
> pci_config_(read|write) function pointers.

Hmm..

Quite frankly, to me it looks like the real issue is that you don't want 
to have the byte/word/dword stuff replicated three times.

And your cleanup avoids the replication, but I think it has other badness 
in it: in particular it depends on those two global pointers. Which makes 
it really hard to have (for example) per-segment functions, or hard for 
drivers to hook into it (there's one IDE driver in particular that wants 
to do conf2 accesses, and nothing else. So it duplicates its own conf2 
functions right now, because it has no way to hook into the generic ones).

If that is the case, wouldn't the _real_ cleanup be to just change the 
format of pci_config_read() entirely, and just make it get the size.

Another way of saying this:

  Right now the config interface is all based around having unique 
  functions for all sizes. So callers do "pci_read_config_word()" and that 
  name-based size calling convention is propagated all the way down.

  Your patch multiplexes the sizes at the lowest level.

  And I'd suggest multiplexing them at a higher level. Instead of 
  six different pcibios_read_config_byte etc functions, move the 
  multiplexing up, make make them just two functions at _that_ level (and 
  make siz #defines to make it compatible with existing users).

Ehh?

		Linus

Re: [patch] PCI Cleanup

[Matthew Dobson]

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

Alan Cox wrote:
> I pointed out before the null check was flawed. And all I see is the
> same identical patch churned out again. Regardless of whether that
> paticular stupid error was in the old code, not fixing it in the new
> code when its pointed out is a bit of a mess.
The NULL check has been removed.  The current version is attatched.  It now 
pulls out 75 lines of code.  I must have missed your complaints about the null 
check in previous emails...  The check was done in the old pci_conf1 funcs, and 
  not in the pci_conf2 ones.. I thought this was strange as well, but I just 
left them in...

> I'm not sure its a simplification either. More function pointers don't
> always make for neater - but thats a side issue. If the NULL check goes
> I'm not too worried about the other stuff.
I think that it is definitely a simplification, although I am a bit biased ;) 
It makes it easier for other configuration types to hook into the system as 
well (I'm partial to NUMA-Q as well ;).  All they have to do is hijack the 
pci_config_(read|write) function pointers.

Also, has anyone had a chance to look at it with pci_conf2?  I have no 
particular reason to believe it doesn't work, as the code paths are almost 
identical, I'd just be nice to have a confirmation.

Cheers!

-Matt

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

diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c	Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c	Tue Aug 13 09:45:12 2002
@@ -69,76 +69,7 @@
 	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,58 @@
 
 #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), 
+	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), 
+	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), 
+	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 +235,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 +246,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 +269,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;
 }
 

Re: [patch] PCI Cleanup

[Martin J. Bligh]

> I pointed out before the null check was flawed. And all I see is the
> same identical patch churned out again. Regardless of whether that
> paticular stupid error was in the old code, not fixing it in the new
> code when its pointed out is a bit of a mess.
> 
> I'm not sure its a simplification either. More function pointers don't
> always make for neater - but thats a side issue. If the NULL check goes
> I'm not too worried about the other stuff.

OK, that's easily disposed of, and it'll remove even more lines of
redundant code.

Thanks,

M.

Re: [patch] PCI Cleanup

[Alan Cox]

On Tue, 2002-08-13 at 15:55, Martin J. Bligh wrote:
> BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out
> of it, and was just the core code cleanup. He's replacing 
> pci_conf1_read_config_byte and pci_conf2_read_config_byte

Apologies that part of the changing between the two diffs I didn't
notice. I've just stripped the value check out of 2.4 except for the
BIOS mode ones (they tend to produce -weird- tracebacks) where it
BUG()'s on that.

Re: [patch] PCI Cleanup

[Alan Cox]

On Tue, 2002-08-13 at 15:17, Martin J. Bligh wrote:
> Alan, please *look* at the patch. The NULL check is already
> there, he's REMOVING about 60 lines of duplicated code,
> reducing the complexity, and shifting the indirection up one
> level to get rid of redundancy.
> 
> If you want to delete the NULL check as well, that's fine, but
> totally a side issue. Ironically, the very snippet of code you
> quoted is all prefaced with "-", no?

I pointed out before the null check was flawed. And all I see is the
same identical patch churned out again. Regardless of whether that
paticular stupid error was in the old code, not fixing it in the new
code when its pointed out is a bit of a mess.

I'm not sure its a simplification either. More function pointers don't
always make for neater - but thats a side issue. If the NULL check goes
I'm not too worried about the other stuff.

Re: [patch] PCI Cleanup

[Martin J. Bligh]

> This stil has the same problems as it did last time you posted it. The
> pointless NULL check and the increased complexity over duplicating about
> 60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
> 
> The !value check is extremely bad because it turns a critical debuggable
> software error into a silent unnoticed mistake.
> 
> Fixing the code instead of just resending it might improve the changes
> of it being merged no end.

BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out
of it, and was just the core code cleanup. He's replacing 
pci_conf1_read_config_byte and pci_conf2_read_config_byte
with pci_conf_read_config_byte. The indirection level has now switched
from pci_root_ops to pci_config_(read|write). These were set up before,
but never seemed to be actually used.

I'm perfectly prepared to accept that the patch doesn't work, or is 
bad, but not for the reasons you gave. To make the thing easier to 
read, what he's effectively doing is this

-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_conf2_read_config_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), 
- 		PCI_FUNC(dev->devfn), where, 1, &data);
- 	*value = (u8)data;
- 	return result;
- }
-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;
+
+	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;
+}

Re: [patch] PCI Cleanup

[Martin J. Bligh]

> On Tue, 2002-08-13 at 01:08, Matthew Dobson wrote:
> 
>> -	if (!value) 
>> -		return -EINVAL;
>> -
>> -	result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
>> -		PCI_FUNC(dev->devfn), where, 2, &data);
>> -
> 
> This stil has the same problems as it did last time you posted it. The
> pointless NULL check and the increased complexity over duplicating about
> 60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
> 
> The !value check is extremely bad because it turns a critical debuggable
> software error into a silent unnoticed mistake.
> 
> Fixing the code instead of just resending it might improve the changes
> of it being merged no end.

Alan, please *look* at the patch. The NULL check is already
there, he's REMOVING about 60 lines of duplicated code,
reducing the complexity, and shifting the indirection up one
level to get rid of redundancy.

If you want to delete the NULL check as well, that's fine, but
totally a side issue. Ironically, the very snippet of code you
quoted is all prefaced with "-", no?

M.

Re: [patch] PCI Cleanup

[Alan Cox]

On Tue, 2002-08-13 at 01:08, Matthew Dobson wrote:

> -	if (!value) 
> -		return -EINVAL;
> -
> -	result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), 
> -		PCI_FUNC(dev->devfn), where, 2, &data);
> -

This stil has the same problems as it did last time you posted it. The
pointless NULL check and the increased complexity over duplicating about
60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.

The !value check is extremely bad because it turns a critical debuggable
software error into a silent unnoticed mistake.

Fixing the code instead of just resending it might improve the changes
of it being merged no end.

Alan

[patch] PCI Cleanup

[Matthew Dobson]

[-- 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;
 }