PCI: clean up the dynamic pci id logic

Hi,

The dynamic pci id logic has been bothering me for a while, and now that I started to look into how to move some of this to the driver core, I thought it was time to clean it all up.

So here's a patch against 2.6.13-rc1 that does so. It ends up making the code smaller, and easier to follow, and fixes a few bugs at the same time (dynamic ids were not being matched everywhere, and so could be missed on some call paths for new devices, semaphore not needed to be grabbed when adding a new id and calling the driver core, etc.)

Matt, any objection to this patch?

I also renamed the function pci_match_device() to pci_match_id() as that's what it really does, but maybe I should just put it back to be compatible. Nah...

thanks,

greg k-h

---
 arch/i386/kernel/cpu/cpufreq/gx-suspmod.c |    2 
 drivers/char/hw_random.c                  |    2 
 drivers/char/watchdog/i8xx_tco.c          |    2 
 drivers/ide/setup-pci.c                   |    2 
 drivers/parport/parport_pc.c              |    2 
 drivers/pci/pci-driver.c                  |  196 ++++++++++--------------------
 include/linux/pci-dynids.h                |   18 --
 include/linux/pci.h                       |    3 
 sound/pci/bt87x.c                         |    2 
 9 files changed, 79 insertions(+), 150 deletions(-)

@@ -190,7 +190,7 @@
 
 	/* detect which companion chip is used */
 	while ((gx_pci = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, gx_pci)) != NULL) {
-		if ((pci_match_device (gx_chipset_tbl, gx_pci)) != NULL) {
+		if ((pci_match_id(gx_chipset_tbl, gx_pci)) != NULL) {
 			return gx_pci;
 		}
 	}

@@ -579,7 +579,7 @@
 
 	/* Probe for Intel, AMD RNGs */
 	for_each_pci_dev(pdev) {
-		ent = pci_match_device (rng_pci_tbl, pdev);
+		ent = pci_match_id(rng_pci_tbl, pdev);
 		if (ent) {
 			rng_ops = &rng_vendor_ops[ent->driver_data];
 			goto match;

@@ -401,7 +401,7 @@
 	 */
 
 	while ((dev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
-		if (pci_match_device(i8xx_tco_pci_tbl, dev)) {
+		if (pci_match_id(i8xx_tco_pci_tbl, dev)) {
 			i8xx_tco_pci = dev;
 			break;
 		}

@@ -847,7 +847,7 @@
 		d = list_entry(l, struct pci_driver, node);
 		if(d->id_table)
 		{
-			const struct pci_device_id *id = pci_match_device(d->id_table, dev);
+			const struct pci_device_id *id = pci_match_id(d->id_table, dev);
 			if(id != NULL)
 			{
 				if(d->probe(dev, id) >= 0)

@@ -3008,7 +3008,7 @@
 	int ret = 0;
 
 	while ((pdev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL) {
-		id = pci_match_device (parport_pc_pci_tbl, pdev);
+		id = pci_match_id(parport_pc_pci_tbl, pdev);
 		if (id == NULL || id->driver_data >= last_sio)
 			continue;
 

@@ -860,7 +860,8 @@
 void pci_unregister_driver(struct pci_driver *);
 void pci_remove_behind_bridge(struct pci_dev *);
 struct pci_driver *pci_dev_driver(const struct pci_dev *);
-const struct pci_device_id *pci_match_device(const struct pci_device_id *ids, const struct pci_dev *dev);
+const struct pci_device_id *pci_match_device(struct pci_driver *drv, struct pci_dev *dev);
+const struct pci_device_id *pci_match_id(const struct pci_device_id *ids, struct pci_dev *dev);
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass);
 
 /* kmem_cache style wrapper around pci_alloc_consistent() */

@@ -804,7 +804,7 @@
 	int i;
 	const struct pci_device_id *supported;
 
-	supported = pci_match_device(snd_bt87x_ids, pci);
+	supported = pci_match_device(driver, pci);
 	if (supported)
 		return supported->driver_data;
 

@@ -1,18 +0,0 @@
-/*
- *	PCI defines and function prototypes
- *	Copyright 2003 Dell Inc.
- *        by Matt Domsch <[email protected]>
- */
-
-#ifndef LINUX_PCI_DYNIDS_H
-#define LINUX_PCI_DYNIDS_H
-
-#include <linux/list.h>
-#include <linux/mod_devicetable.h>
-
-struct dynid {
-	struct list_head        node;
-	struct pci_device_id    id;
-};
-
-#endif

Hi Greg,

I was wondering why we need dynamic id support in the driver core. Is there an issue where the bind/unbind mechanism requires this feature? I was hoping bind/unbind would replace it.

I understand that there are PCI drivers that use .driver_data and read from their ID table (e.g. pci_serial), but we don't really want the user modifying these IDs because they're often attached to some device specific tables.

It was my understanding that the *probe function should be responsible for accepting any device, and then gracefully fail if it knows it will be unable to support it. For some drivers this could include failing if it's missing from the ID table. If the driver developer requires the driver to match to an unknown pool of devices, then the *probe function could be made more advanced.

Thanks,
Adam

No, bind/unbind (well bind, unbind doesn't care) only will work if the driver's probe function can accept the device. Without dynamic ids, it's not possible to have a driver bind to a device that it does not know about.

I want to put the sysfs file, and a common callback in the driver core, to make it easier for busses to support dynamic ids. That's why I am suggesting we add it there. But I'll play with implementing it and see if it's really worth it or not.

Either way, this patch fixes up the pci code to make the dynamic ids for it, much more readable and smaller.

True, if you look at the current pci dynamic id stuff, we don't set the driver_data field at all. Some USB drivers use this field too.

For this to happen, we would have to rewrite all drivers :( Right now they assume that the MODULE_TABLE must have matched, in order for the probe() call to be called.

Remember, probe() of most busses pass in a valid id structure too...

If the bus/driver core supports dynamic ids, then the individual drivers don't have to be changed at all :)

thanks,

greg k-h

This could be easily changed. We would just have to make some tweaks to the bus drivers. See below...

Yeah, agreed.

Personally, I'd like to remove .driver_data in favor of internal driver heuristics (possibly with some <insert bus here> helper functions). The only driver I've ever seen that wouldn't have completely trivial breakage is "serial-pci".

The right way to fix this driver would be to have it bind to the PCI serial class code. It would then have an internal ID table with flags and instructions for each device. If the device isn't in the table, and controlling it is not obvious, ->probe would fail.

Yes, but the vast majority (though not all) of driver-level probe functions completely ignore it. We could fix drivers that need it to handle a null ID structure. Most of these drivers are not going to be able to handle an unknown target device anyway, so they could just fail if there isn't an ID.

I understand. I'm just trying to avoid some extra complexity, code, and memory usage. I'd like to have a more direct approach :)

Thanks,
Adam

P.S.: One final thought, the bus driver could create a new ID ( based on available identification data) on the fly when a driver is bound to the device.

Also, don't we need a way to turn off automatic driver binding? The end user might want to select a specific driver for a device. Removing the automatically bound driver every time is a bit ugly (and probably sometimes impossible depending on the bus/hardware).