Greg asked in a previous janitors thread:
"What we need is a simple "Is this pci device present right now" type
function, to solve the mess that logic like this needs."
OK. How about this one? It uses pci_get_device but instead of returning
the dev it returns 1 if the device is present and 0 if it isnt. This take the
burdon off the driver from having to know when to use pci_dev_put or
not and should be cleaner for future maintenance work.
Ive tested it with two patches that will follow.
Hanna Linder
IBM Linux Technology Center
Signed-off-by: Hanna Linder <[email protected]>
----
diff -Nrup linux-2.6.9-rc2-mm2cln/drivers/pci/search.c linux-2.6.9-rc2-mm2patch/drivers/pci/search.c
--- linux-2.6.9-rc2-mm2cln/drivers/pci/search.c 2004-09-23 11:49:04.000000000 -0700
+++ linux-2.6.9-rc2-mm2patch/drivers/pci/search.c 2004-09-23 15:03:58.000000000 -0700
@@ -271,6 +271,30 @@ pci_get_device(unsigned int vendor, unsi
return pci_get_subsys(vendor, device, PCI_ANY_ID, PCI_ANY_ID, from);
}
+/**
+ * pci_dev_present - Returns 1 if device is present, 0 if device is not.
+ * @vendor: PCI vendor id to match, or %PCI_ANY_ID to match all vendor ids
+ * @device: PCI device id to match, or %PCI_ANY_ID to match all device ids
+ * @from: Previous PCI device found in search, or %NULL for new search.
+ *
+ * If pci_get_device returns a pci_dev pointer then the device exists and the
+ * reference count is decremented before returning 1. If pci_get_device
+ * returns %NULL then 0 is returned to indicate the device was not
+ * present. Obvious fact: You do not have a reference to the device so if
+ * it is removed from the system before this function returns the value
+ * will be stale.
+ */
+int
+pci_dev_present(unsigned int vendor, unsigned int device, struct pci_dev *from)
+{
+ struct pci_dev *dev;
+ dev = pci_get_device(vendor, device, from);
+ if (dev){
+ pci_dev_put(dev);
+ return 1;
+ }
+ return 0;
+}
/**
* pci_find_device_reverse - begin or continue searching for a PCI device by vendor/device id
@@ -352,3 +376,5 @@ EXPORT_SYMBOL(pci_get_device);
EXPORT_SYMBOL(pci_get_subsys);
EXPORT_SYMBOL(pci_get_slot);
EXPORT_SYMBOL(pci_get_class);
+EXPORT_SYMBOL(pci_dev_present);
+
diff -Nrup linux-2.6.9-rc2-mm2cln/include/linux/pci.h linux-2.6.9-rc2-mm2patch/include/linux/pci.h
--- linux-2.6.9-rc2-mm2cln/include/linux/pci.h 2004-09-23 11:49:27.000000000 -0700
+++ linux-2.6.9-rc2-mm2patch/include/linux/pci.h 2004-09-23 15:03:01.000000000 -0700
@@ -733,6 +733,8 @@ struct pci_dev *pci_get_subsys (unsigned
struct pci_dev *pci_get_slot (struct pci_bus *bus, unsigned int devfn);
struct pci_dev *pci_get_class (unsigned int class, struct pci_dev *from);
+int pci_dev_present(unsigned int vendor, unsigned int device, struct pci_dev *from);
+
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);
@@ -900,6 +902,9 @@ unsigned int ss_vendor, unsigned int ss_
static inline struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
{ return NULL; }
+static inline int pci_dev_present(unsigned int vendor, unsigned int device, struct pci_dev *from)
+{return -1; }
+
static inline void pci_set_master(struct pci_dev *dev) { }
static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
static inline void pci_disable_device(struct pci_dev *dev) { }
On Thu, Sep 23, 2004 at 03:26:40PM -0700, Hanna Linder wrote:
> + * @from: Previous PCI device found in search, or %NULL for new search.
from is not needed in a function like this. Just vendor and device
should work.
> +static inline int pci_dev_present(unsigned int vendor, unsigned int device, struct pci_dev *from)
> +{return -1; }
> +
This should return 0, not -1.
Care to redo your patches based on this?
thanks,
greg k-h
On Thu, Sep 23, 2004 at 03:26:40PM -0700, Hanna Linder wrote:
>
> Greg asked in a previous janitors thread:
> "What we need is a simple "Is this pci device present right now" type
> function, to solve the mess that logic like this needs."
>
> OK. How about this one? It uses pci_get_device but instead of returning
> the dev it returns 1 if the device is present and 0 if it isnt. This take the
> burdon off the driver from having to know when to use pci_dev_put or
> not and should be cleaner for future maintenance work.
>
> Ive tested it with two patches that will follow.
Please include subdevice/subvendor id
On Fri, Sep 24, 2004 at 08:02:31PM +0100, Christoph Hellwig wrote:
> On Thu, Sep 23, 2004 at 03:26:40PM -0700, Hanna Linder wrote:
> >
> > Greg asked in a previous janitors thread:
> > "What we need is a simple "Is this pci device present right now" type
> > function, to solve the mess that logic like this needs."
> >
> > OK. How about this one? It uses pci_get_device but instead of returning
> > the dev it returns 1 if the device is present and 0 if it isnt. This take the
> > burdon off the driver from having to know when to use pci_dev_put or
> > not and should be cleaner for future maintenance work.
> >
> > Ive tested it with two patches that will follow.
>
> Please include subdevice/subvendor id
Good idea, but do you see any places in the kernel that would use those
fields, instead of always setting them to PCI_ANY_ID?
thanks,
greg k-h
Greg KH wrote:
>
> Good idea, but do you see any places in the kernel that would use those
> fields, instead of always setting them to PCI_ANY_ID?
>
Does it matter what it is currently used for? It *is* one way you can find
out about specific board bugs. I'd think you'd want all of (VID, DID, RID,
SVID, SID).
-hpa
On Gwe, 2004-09-24 at 22:19, Greg KH wrote:
> > Please include subdevice/subvendor id
>
> Good idea, but do you see any places in the kernel that would use those
> fields, instead of always setting them to PCI_ANY_ID?
If you are taking that path then make it take a pci_device_id table.
That makes it behave like other interfaces of the same form, and makes
the implementation remarkably trivial.
On Fri, Sep 24, 2004 at 10:00:47PM +0100, Alan Cox wrote:
> On Gwe, 2004-09-24 at 22:19, Greg KH wrote:
> > > Please include subdevice/subvendor id
> >
> > Good idea, but do you see any places in the kernel that would use those
> > fields, instead of always setting them to PCI_ANY_ID?
>
> If you are taking that path then make it take a pci_device_id table.
> That makes it behave like other interfaces of the same form, and makes
> the implementation remarkably trivial.
Ah, yes, that is a very good idea. Here's just such an implementation
(compile tested, nothing else, still need to add comments describing the
function...) Does everyone like this interface? Hanna, look ok to you?
thanks,
greg k-h
--- 1.42/drivers/pci/pci-driver.c 2004-09-22 16:24:29 -07:00
+++ edited/drivers/pci/pci-driver.c 2004-09-26 06:44:32 -07:00
@@ -14,27 +14,6 @@
* Registration of PCI drivers and handling of hot-pluggable devices.
*/
-/**
- * pci_match_one_device - Tell if a PCI device structure has a matching
- * PCI device id structure
- * @id: single PCI device id structure to match
- * @dev: the PCI device structure to match against
- *
- * Returns the matching pci_device_id structure or %NULL if there is no match.
- */
-
-static inline const struct pci_device_id *
-pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
-{
- if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
- (id->device == PCI_ANY_ID || id->device == dev->device) &&
- (id->subvendor == PCI_ANY_ID || id->subvendor == dev->subsystem_vendor) &&
- (id->subdevice == PCI_ANY_ID || id->subdevice == dev->subsystem_device) &&
- !((id->class ^ dev->class) & id->class_mask))
- return id;
- return NULL;
-}
-
/*
* Dynamic device IDs are disabled for !CONFIG_HOTPLUG
*/
===== drivers/pci/pci.h 1.18 vs edited =====
--- 1.18/drivers/pci/pci.h 2004-09-22 07:07:44 -07:00
+++ edited/drivers/pci/pci.h 2004-09-26 06:44:28 -07:00
@@ -66,3 +66,24 @@
extern int pcie_mch_quirk;
extern void pcie_rootport_aspm_quirk(struct pci_dev *pdev);
extern struct device_attribute pci_dev_attrs[];
+
+/**
+ * pci_match_one_device - Tell if a PCI device structure has a matching
+ * PCI device id structure
+ * @id: single PCI device id structure to match
+ * @dev: the PCI device structure to match against
+ *
+ * Returns the matching pci_device_id structure or %NULL if there is no match.
+ */
+static inline const struct pci_device_id *
+pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
+{
+ if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
+ (id->device == PCI_ANY_ID || id->device == dev->device) &&
+ (id->subvendor == PCI_ANY_ID || id->subvendor == dev->subsystem_vendor) &&
+ (id->subdevice == PCI_ANY_ID || id->subdevice == dev->subsystem_device) &&
+ !((id->class ^ dev->class) & id->class_mask))
+ return id;
+ return NULL;
+}
+
===== drivers/pci/search.c 1.31 vs edited =====
--- 1.31/drivers/pci/search.c 2004-08-26 15:16:41 -07:00
+++ edited/drivers/pci/search.c 2004-09-26 07:02:57 -07:00
@@ -11,6 +11,7 @@
#include <linux/pci.h>
#include <linux/module.h>
#include <linux/interrupt.h>
+#include "pci.h"
spinlock_t pci_bus_lock = SPIN_LOCK_UNLOCKED;
@@ -343,6 +344,29 @@
spin_unlock(&pci_bus_lock);
return dev;
}
+
+int pci_dev_present(const struct pci_device_id *ids)
+{
+ struct pci_dev *dev;
+ int found = 0;
+
+ WARN_ON(in_interrupt());
+ spin_lock(&pci_bus_lock);
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ list_for_each_entry(dev, &pci_devices, global_list) {
+ if (pci_match_one_device(ids, dev)) {
+ found = 1;
+ goto exit;
+ }
+ }
+ ids++;
+ }
+exit:
+ spin_unlock(&pci_bus_lock);
+ return found;
+}
+EXPORT_SYMBOL(pci_dev_present);
+
EXPORT_SYMBOL(pci_find_bus);
EXPORT_SYMBOL(pci_find_device);
On Sun, Sep 26, 2004 at 07:10:02AM -0700, Greg KH wrote:
> > If you are taking that path then make it take a pci_device_id table.
> > That makes it behave like other interfaces of the same form, and makes
> > the implementation remarkably trivial.
>
> Ah, yes, that is a very good idea. Here's just such an implementation
> (compile tested, nothing else, still need to add comments describing the
> function...) Does everyone like this interface? Hanna, look ok to you?
Ok, here's the patch that I applied to my trees, and I'll follow this up
with a conversion of Hanna's two patches that I respun to use the new
parameters of this function.
thanks,
greg k-h
----
PCI: Create new function to see if a pci device is present
This is needed to help get rid of the pci_find_device() usage in the tree.
Signed-off-by: Hanna Linder <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c 2004-09-28 10:21:02 -07:00
+++ b/drivers/pci/pci-driver.c 2004-09-28 10:21:02 -07:00
@@ -14,27 +14,6 @@
* Registration of PCI drivers and handling of hot-pluggable devices.
*/
-/**
- * pci_match_one_device - Tell if a PCI device structure has a matching
- * PCI device id structure
- * @id: single PCI device id structure to match
- * @dev: the PCI device structure to match against
- *
- * Returns the matching pci_device_id structure or %NULL if there is no match.
- */
-
-static inline const struct pci_device_id *
-pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
-{
- if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
- (id->device == PCI_ANY_ID || id->device == dev->device) &&
- (id->subvendor == PCI_ANY_ID || id->subvendor == dev->subsystem_vendor) &&
- (id->subdevice == PCI_ANY_ID || id->subdevice == dev->subsystem_device) &&
- !((id->class ^ dev->class) & id->class_mask))
- return id;
- return NULL;
-}
-
/*
* Dynamic device IDs are disabled for !CONFIG_HOTPLUG
*/
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- a/drivers/pci/pci.h 2004-09-28 10:21:02 -07:00
+++ b/drivers/pci/pci.h 2004-09-28 10:21:02 -07:00
@@ -66,3 +66,24 @@
extern int pcie_mch_quirk;
extern void pcie_rootport_aspm_quirk(struct pci_dev *pdev);
extern struct device_attribute pci_dev_attrs[];
+
+/**
+ * pci_match_one_device - Tell if a PCI device structure has a matching
+ * PCI device id structure
+ * @id: single PCI device id structure to match
+ * @dev: the PCI device structure to match against
+ *
+ * Returns the matching pci_device_id structure or %NULL if there is no match.
+ */
+static inline const struct pci_device_id *
+pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
+{
+ if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
+ (id->device == PCI_ANY_ID || id->device == dev->device) &&
+ (id->subvendor == PCI_ANY_ID || id->subvendor == dev->subsystem_vendor) &&
+ (id->subdevice == PCI_ANY_ID || id->subdevice == dev->subsystem_device) &&
+ !((id->class ^ dev->class) & id->class_mask))
+ return id;
+ return NULL;
+}
+
diff -Nru a/drivers/pci/search.c b/drivers/pci/search.c
--- a/drivers/pci/search.c 2004-09-28 10:21:02 -07:00
+++ b/drivers/pci/search.c 2004-09-28 10:21:02 -07:00
@@ -11,6 +11,7 @@
#include <linux/pci.h>
#include <linux/module.h>
#include <linux/interrupt.h>
+#include "pci.h"
spinlock_t pci_bus_lock = SPIN_LOCK_UNLOCKED;
@@ -343,6 +344,39 @@
spin_unlock(&pci_bus_lock);
return dev;
}
+
+/**
+ * pci_dev_present - Returns 1 if device matching the device list is present, 0 if not.
+ * @ids: A pointer to a null terminated list of struct pci_device_id structures
+ * that describe the type of PCI device the caller is trying to find.
+ *
+ * Obvious fact: You do not have a reference to any device that might be found
+ * by this function, so if that device is removed from the system right after
+ * this function is finished, the value will be stale. Use this function to
+ * find devices that are usually built into a system, or for a general hint as
+ * to if another device happens to be present at this specific moment in time.
+ */
+int pci_dev_present(const struct pci_device_id *ids)
+{
+ struct pci_dev *dev;
+ int found = 0;
+
+ WARN_ON(in_interrupt());
+ spin_lock(&pci_bus_lock);
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ list_for_each_entry(dev, &pci_devices, global_list) {
+ if (pci_match_one_device(ids, dev)) {
+ found = 1;
+ goto exit;
+ }
+ }
+ ids++;
+ }
+exit:
+ spin_unlock(&pci_bus_lock);
+ return found;
+}
+EXPORT_SYMBOL(pci_dev_present);
EXPORT_SYMBOL(pci_find_bus);
EXPORT_SYMBOL(pci_find_device);
diff -Nru a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h 2004-09-28 10:21:02 -07:00
+++ b/include/linux/pci.h 2004-09-28 10:21:02 -07:00
@@ -733,6 +733,7 @@
struct pci_dev *from);
struct pci_dev *pci_get_slot (struct pci_bus *bus, unsigned int devfn);
struct pci_dev *pci_get_class (unsigned int class, struct pci_dev *from);
+int pci_dev_present(const struct pci_device_id *ids);
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);
@@ -900,6 +901,8 @@
static inline struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
{ return NULL; }
+static inline int pci_dev_present(const struct pci_device_id *ids)
+{ return 0; }
static inline void pci_set_master(struct pci_dev *dev) { }
static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
On Tue, Sep 28, 2004 at 10:24:26AM -0700, Greg KH wrote:
> Ok, here's the patch that I applied to my trees, and I'll follow this up
> with a conversion of Hanna's two patches that I respun to use the new
> parameters of this function.
Here's the irq.c patch:
----------
PCI: change irq.c to use pci_dev_present
Signed-off-by: Hanna Linder <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/arch/i386/pci/irq.c b/arch/i386/pci/irq.c
--- a/arch/i386/pci/irq.c 2004-09-28 10:21:40 -07:00
+++ b/arch/i386/pci/irq.c 2004-09-28 10:21:40 -07:00
@@ -452,21 +452,17 @@
#endif
-
static __init int intel_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
- struct pci_dev *dev1, *dev2;
+ static struct pci_device_id pirq_440gx[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443GX_0) },
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443GX_2) },
+ { },
+ };
/* 440GX has a proprietary PIRQ router -- don't use it */
- dev1 = pci_get_device(PCI_VENDOR_ID_INTEL,
- PCI_DEVICE_ID_INTEL_82443GX_0, NULL);
- dev2 = pci_get_device(PCI_VENDOR_ID_INTEL,
- PCI_DEVICE_ID_INTEL_82443GX_2, NULL);
- if ((dev1 != NULL) || (dev2 != NULL)) {
- pci_dev_put(dev1);
- pci_dev_put(dev2);
+ if (pci_dev_present(pirq_440gx))
return 0;
- }
switch(device)
{
On Tue, Sep 28, 2004 at 10:24:26AM -0700, Greg KH wrote:
> Ok, here's the patch that I applied to my trees, and I'll follow this up
> with a conversion of Hanna's two patches that I respun to use the new
> parameters of this function.
Here's the cyrix.c patch.
-------
PCI: change cyrix.c driver to use pci_dev_present
Signed-off-by: Hanna Linder <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff -Nru a/arch/i386/kernel/cpu/cyrix.c b/arch/i386/kernel/cpu/cyrix.c
--- a/arch/i386/kernel/cpu/cyrix.c 2004-09-28 10:21:10 -07:00
+++ b/arch/i386/kernel/cpu/cyrix.c 2004-09-28 10:21:10 -07:00
@@ -187,12 +187,19 @@
}
+#ifdef CONFIG_PCI
+static struct pci_device_id cyrix_55x0[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_5510) },
+ { PCI_DEVICE(PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_5520) },
+ { },
+};
+#endif
+
static void __init init_cyrix(struct cpuinfo_x86 *c)
{
unsigned char dir0, dir0_msn, dir0_lsn, dir1 = 0;
char *buf = c->x86_model_id;
const char *p = NULL;
- struct pci_dev *dev;
/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
@@ -275,16 +282,8 @@
/*
* The 5510/5520 companion chips have a funky PIT.
*/
- dev = pci_get_device(PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_5510, NULL);
- if (dev) {
- pci_dev_put(dev);
- pit_latch_buggy = 1;
- }
- dev = pci_get_device(PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_5520, NULL);
- if (dev) {
- pci_dev_put(dev);
+ if (pci_dev_present(cyrix_55x0))
pit_latch_buggy = 1;
- }
/* GXm supports extended cpuid levels 'ala' AMD */
if (c->cpuid_level == 2) {