Re: agpgart support for intel SHG2 motherboard, serverworks chipset

Re: agpgart support for intel SHG2 motherboard, serverworks chipset

[Matt Domsch]

> > Anatoly, I've cc'd Greg on this one, as you managed to break the
> > sysfs new_id stuff that he wrote, so I think he may be interested
> > in fixing that up 8-)

agp_serverworks_probe() is marked __init.  Thus the static lookup 
called by the new_id code fails as this function is no longer in the 
kernel.  The fix is to remove __init from the probe routines.  I'm looking 
to see how often this occurs elsewhere.

sworks-agp.c also can't make effective use of the new_id code because it 
registers a single all-covering serverworks pci_device_id, then its probe 
routine checks for three specific device IDs and bails if it's not them.  
The new_id code can't help here.  The "right" way would be to register 
three separate entries in the pci_table and not test for them in the probe 
routine.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: Buggy PCI drivers - do not mark pci_device_id as discardable data

[Matt Domsch]

> > agp_serverworks_probe() is marked __init.  Thus the static lookup
> Ah, Russell just got a patch for this into the tree today.

Thanks Russell.  However, I believe your patch only fixes the
pci_device_id tables marked __initdata, not the probe functions (or
anything they call) being marked __init, which is what Anatoly tripped up.  

At least these have probe functions marked __init in -test5.

drivers/net/irda/via-ircc.c:static int __init via_init_one
drivers/net/tokenring/abyss.c:static int __init abyss_attach
drivers/net/tokenring/tmspci.c:static int __init tms_pci_attach
drivers/pcmcia/i82092.c:static int __init i82092aa_pci_probe
sound/oss/ali5455.c:static int __init ali_probe
sound/oss/ali5455.c:ali_ac97_init
sound/oss/ali5455.c:ali_configure_clocking
sound/oss/i810_audio.c:static int __init i810_probe
sound/oss/i810_audio.c:i810_ac97_init
sound/oss/i810_audio.c:i810_configure_clocking
sound/oss/maestro3.c:static int __init m3_probe
sound/oss/maestro3.c:m3_codec_install
sound/oss/trident.c:static int __init trident_probe
sound/oss/trident.c:trident_ac97_init
sound/oss/via82cxxx_audio.c:static int __init via_init_one
   (and some related functions)
drivers/char/agp/*.c


Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: agpgart support for intel SHG2 motherboard, serverworks chipset

[Greg KH]

On Tue, Sep 09, 2003 at 05:22:00PM -0500, Matt Domsch wrote:
> > > Anatoly, I've cc'd Greg on this one, as you managed to break the
> > > sysfs new_id stuff that he wrote, so I think he may be interested
> > > in fixing that up 8-)
> 
> agp_serverworks_probe() is marked __init.  Thus the static lookup 
> called by the new_id code fails as this function is no longer in the 
> kernel.  The fix is to remove __init from the probe routines.  I'm looking 
> to see how often this occurs elsewhere.

Ah, Russell just got a patch for this into the tree today.

thanks,

greg k-h

Re: Buggy PCI drivers - do not mark pci_device_id as discardable data

[Greg KH]

On Tue, Sep 09, 2003 at 06:12:48PM -0500, Matt Domsch wrote:
> > > agp_serverworks_probe() is marked __init.  Thus the static lookup
> > Ah, Russell just got a patch for this into the tree today.
> 
> Thanks Russell.  However, I believe your patch only fixes the
> pci_device_id tables marked __initdata, not the probe functions (or
> anything they call) being marked __init, which is what Anatoly tripped up.  
> 
> At least these have probe functions marked __init in -test5.

These either need to be marked __devinit and make "new_id" dependant on
CONFIG_HOTPLUG, or we need to remove the __init marker on these
functions.

Any throughts about which?

thanks,

greg k-h

Re: Buggy PCI drivers - do not mark pci_device_id as discardable data

[Matt Domsch]

> > At least these have probe functions marked __init in -test5.
> 
> These either need to be marked __devinit and make "new_id" dependant on
> CONFIG_HOTPLUG, or we need to remove the __init marker on these
> functions.
> 
> Any throughts about which?

I expect the CONFIG_EMBEDDED folks would much prefer the 
__devinit/CONFIG_HOTPLUG path, so it all disappears for them.

-- 
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: Buggy PCI drivers - do not mark pci_device_id as discardable data

[Matt Domsch]

> > These either need to be marked __devinit and make "new_id" dependant on
> > CONFIG_HOTPLUG

Patch below moves all the new_id code under CONFIG_HOTPLUG.  Tested
with both CONFIG_HOTPLUG enabled and disabled.  No significant code
changes, merely code moving, and in 2 cases, stub functions added.

Please review and apply.

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

--- linux-2.5/drivers/pci/pci-driver.c	Thu Sep  4 16:36:45 2003
+++ linux-2.5-devinit/drivers/pci/pci-driver.c	Wed Sep 10 10:18:55 2003
@@ -19,7 +19,7 @@
  *                        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.
  */
 
@@ -35,6 +35,166 @@ pci_match_one_device(const struct pci_de
 	return NULL;
 }
 
+/*
+ * Dynamic device IDs are disabled for !CONFIG_HOTPLUG
+ */
+
+#ifdef CONFIG_HOTPLUG
+/**
+ * pci_device_probe_dynamic()
+ *
+ * Walk the dynamic ID list looking for a match.
+ * returns 0 and sets pci_dev->driver when drv claims pci_dev, else error.
+ */
+static int
+pci_device_probe_dynamic(struct pci_driver *drv, struct pci_dev *pci_dev)
+{
+	int error = -ENODEV;
+	struct list_head *pos;
+	struct dynid *dynid;
+
+	spin_lock(&drv->dynids.lock);
+	list_for_each(pos, &drv->dynids.list) {
+		dynid = list_entry(pos, struct dynid, node);
+		if (pci_match_one_device(&dynid->id, pci_dev)) {
+			spin_unlock(&drv->dynids.lock);
+			error = drv->probe(pci_dev, &dynid->id);
+			if (error >= 0) {
+				pci_dev->driver = drv;
+				return 0;
+			}
+			return error;
+		}
+	}
+	spin_unlock(&drv->dynids.lock);
+	return error;
+}
+static inline void
+dynid_init(struct dynid *dynid)
+{
+	memset(dynid, 0, sizeof(*dynid));
+	INIT_LIST_HEAD(&dynid->node);
+}
+
+/**
+ * store_new_id
+ * @ pdrv
+ * @ buf
+ * @ count
+ *
+ * Adds a new dynamic pci device ID to this driver,
+ * and causes the driver to probe for all devices again.
+ */
+static inline ssize_t
+store_new_id(struct device_driver * driver, const char * buf, size_t count)
+{
+	struct dynid *dynid;
+	struct bus_type * bus;
+	struct pci_driver *pdrv = to_pci_driver(driver);
+	__u32 vendor=PCI_ANY_ID, device=PCI_ANY_ID, subvendor=PCI_ANY_ID,
+		subdevice=PCI_ANY_ID, class=0, class_mask=0;
+	unsigned long driver_data=0;
+	int fields=0;
+
+	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
+			&vendor, &device, &subvendor, &subdevice,
+			&class, &class_mask, &driver_data);
+	if (fields < 0)
+		return -EINVAL;
+
+	dynid = kmalloc(sizeof(*dynid), GFP_KERNEL);
+	if (!dynid)
+		return -ENOMEM;
+	dynid_init(dynid);
+
+	dynid->id.vendor = vendor;
+	dynid->id.device = device;
+	dynid->id.subvendor = subvendor;
+	dynid->id.subdevice = subdevice;
+	dynid->id.class = class;
+	dynid->id.class_mask = class_mask;
+	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
+		driver_data : 0UL;
+
+	spin_lock(&pdrv->dynids.lock);
+	list_add_tail(&pdrv->dynids.list, &dynid->node);
+	spin_unlock(&pdrv->dynids.lock);
+
+	bus = get_bus(pdrv->driver.bus);
+	if (bus) {
+		if (get_driver(&pdrv->driver)) {
+			down_write(&bus->subsys.rwsem);
+			driver_attach(&pdrv->driver);
+			up_write(&bus->subsys.rwsem);
+			put_driver(&pdrv->driver);
+		}
+		put_bus(bus);
+	}
+
+	return count;
+}
+
+static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
+static inline void
+pci_init_dynids(struct pci_dynids *dynids)
+{
+	memset(dynids, 0, sizeof(*dynids));
+	spin_lock_init(&dynids->lock);
+	INIT_LIST_HEAD(&dynids->list);
+}
+
+static void
+pci_free_dynids(struct pci_driver *drv)
+{
+	struct list_head *pos, *n;
+	struct dynid *dynid;
+
+	spin_lock(&drv->dynids.lock);
+	list_for_each_safe(pos, n, &drv->dynids.list) {
+		dynid = list_entry(pos, struct dynid, node);
+		list_del(&dynid->node);
+		kfree(dynid);
+	}
+	spin_unlock(&drv->dynids.lock);
+}
+
+static int
+pci_create_newid_file(struct pci_driver * drv)
+{
+	int error = 0;
+	if (drv->probe != NULL)
+		error = sysfs_create_file(&drv->driver.kobj,
+					  &driver_attr_new_id.attr);
+	return error;
+}
+
+static int
+pci_bus_match_dynids(const struct pci_dev * pci_dev, const struct pci_driver * pci_drv)
+{
+	struct list_head *pos;
+	struct dynid *dynid;
+
+	spin_lock(&pci_drv->dynids.lock);
+	list_for_each(pos, &pci_drv->dynids.list) {
+		dynid = list_entry(pos, struct dynid, node);
+		if (pci_match_one_device(&dynid->id, pci_dev)) {
+			spin_unlock(&pci_drv->dynids.lock);
+			return 1;
+		}
+	}
+	spin_unlock(&pci_drv->dynids.lock);
+	return 0;
+}
+
+#else /* !CONFIG_HOTPLUG */
+#define pci_device_probe_dynamic(drv,pci_dev) (-ENODEV)
+#define dynid_init(dynid) do {} while (0)
+#define pci_init_dynids(dynids) do {} while (0)
+#define pci_free_dynids(drv) do {} while (0)
+#define pci_create_newid_file(drv) (0)
+#define pci_bus_match_dynids(pci_dev, pci_drv) (0)
+#endif
+
 /**
  * pci_match_device - Tell if a PCI device structure has a matching
  *                    PCI device id structure
@@ -80,36 +240,6 @@ pci_device_probe_static(struct pci_drive
 }
 
 /**
- * pci_device_probe_dynamic()
- * 
- * Walk the dynamic ID list looking for a match.
- * returns 0 and sets pci_dev->driver when drv claims pci_dev, else error.
- */
-static int
-pci_device_probe_dynamic(struct pci_driver *drv, struct pci_dev *pci_dev)
-{		   
-	int error = -ENODEV;
-	struct list_head *pos;
-	struct dynid *dynid;
-
-	spin_lock(&drv->dynids.lock);
-	list_for_each(pos, &drv->dynids.list) {
-		dynid = list_entry(pos, struct dynid, node);
-		if (pci_match_one_device(&dynid->id, pci_dev)) {
-			spin_unlock(&drv->dynids.lock);
-			error = drv->probe(pci_dev, &dynid->id);
-			if (error >= 0) {
-				pci_dev->driver = drv;
-				return 0;
-			}
-			return error;
-		}
-	}
-	spin_unlock(&drv->dynids.lock);
-	return error;
-}
-
-/**
  * __pci_device_probe()
  * 
  * returns 0  on success, else error.
@@ -178,72 +308,6 @@ static int pci_device_resume(struct devi
 	return 0;
 }
 
-static inline void
-dynid_init(struct dynid *dynid)
-{
-	memset(dynid, 0, sizeof(*dynid));
-	INIT_LIST_HEAD(&dynid->node);
-}
-
-/**
- * store_new_id
- * @ pdrv
- * @ buf
- * @ count
- *
- * Adds a new dynamic pci device ID to this driver,
- * and causes the driver to probe for all devices again.
- */
-static inline ssize_t
-store_new_id(struct device_driver * driver, const char * buf, size_t count)
-{
-	struct dynid *dynid;
-	struct bus_type * bus;
-	struct pci_driver *pdrv = to_pci_driver(driver);
-	__u32 vendor=PCI_ANY_ID, device=PCI_ANY_ID, subvendor=PCI_ANY_ID,
-		subdevice=PCI_ANY_ID, class=0, class_mask=0;
-	unsigned long driver_data=0;
-	int fields=0;
-
-	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
-			&vendor, &device, &subvendor, &subdevice,
-			&class, &class_mask, &driver_data);
-	if (fields < 0)
-		return -EINVAL;
-
-	dynid = kmalloc(sizeof(*dynid), GFP_KERNEL);
-	if (!dynid)
-		return -ENOMEM;
-	dynid_init(dynid);
-
-	dynid->id.vendor = vendor;
-	dynid->id.device = device;
-	dynid->id.subvendor = subvendor;
-	dynid->id.subdevice = subdevice;
-	dynid->id.class = class;
-	dynid->id.class_mask = class_mask;
-	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
-		driver_data : 0UL;
-
-	spin_lock(&pdrv->dynids.lock);
-	list_add_tail(&pdrv->dynids.list, &dynid->node);
-	spin_unlock(&pdrv->dynids.lock);
-
-	bus = get_bus(pdrv->driver.bus);
-	if (bus) {
-		if (get_driver(&pdrv->driver)) {
-			down_write(&bus->subsys.rwsem);
-			driver_attach(&pdrv->driver);
-			up_write(&bus->subsys.rwsem);
-			put_driver(&pdrv->driver);
-		}
-		put_bus(bus);
-	}
-	
-	return count;
-}
-
-static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
 
 #define kobj_to_pci_driver(obj) container_of(obj, struct device_driver, kobj)
 #define attr_to_driver_attribute(obj) container_of(obj, struct driver_attribute, attr)
@@ -290,38 +354,9 @@ static struct kobj_type pci_driver_kobj_
 static int
 pci_populate_driver_dir(struct pci_driver * drv)
 {
-	int error = 0;
-
-	if (drv->probe != NULL)
-		error = sysfs_create_file(&drv->driver.kobj,
-					  &driver_attr_new_id.attr);
-	return error;
-}
-
-static inline void
-pci_init_dynids(struct pci_dynids *dynids)
-{
-	memset(dynids, 0, sizeof(*dynids));
-	spin_lock_init(&dynids->lock);
-	INIT_LIST_HEAD(&dynids->list);
-}
-
-static void
-pci_free_dynids(struct pci_driver *drv)
-{
-	struct list_head *pos, *n;
-	struct dynid *dynid;
-
-	spin_lock(&drv->dynids.lock);
-	list_for_each_safe(pos, n, &drv->dynids.list) {
-		dynid = list_entry(pos, struct dynid, node);
-		list_del(&dynid->node);
-		kfree(dynid);
-	}
-	spin_unlock(&drv->dynids.lock);
+	return pci_create_newid_file(drv);
 }
 
-
 /**
  * pci_register_driver - register a new pci driver
  * @drv: the driver structure to register
@@ -411,8 +446,6 @@ static int pci_bus_match(struct device *
 	struct pci_driver * pci_drv = to_pci_driver(drv);
 	const struct pci_device_id * ids = pci_drv->id_table;
 	const struct pci_device_id *found_id;
-	struct list_head *pos;
-	struct dynid *dynid;
 
 	if (!ids)
 		return 0;
@@ -421,17 +454,7 @@ static int pci_bus_match(struct device *
 	if (found_id)
 		return 1;
 
-	spin_lock(&pci_drv->dynids.lock);
-	list_for_each(pos, &pci_drv->dynids.list) {
-		dynid = list_entry(pos, struct dynid, node);
-		if (pci_match_one_device(&dynid->id, pci_dev)) {
-			spin_unlock(&pci_drv->dynids.lock);
-			return 1;
-		}
-	}
-	spin_unlock(&pci_drv->dynids.lock);
-
-	return 0;
+	return pci_bus_match_dynids(pci_dev, pci_drv);
 }
 
 /**

Re: Buggy PCI drivers - do not mark pci_device_id as discardable data

[Greg KH]

On Wed, Sep 10, 2003 at 12:17:34PM -0500, Matt Domsch wrote:
> > > These either need to be marked __devinit and make "new_id" dependant on
> > > CONFIG_HOTPLUG
> 
> Patch below moves all the new_id code under CONFIG_HOTPLUG.  Tested
> with both CONFIG_HOTPLUG enabled and disabled.  No significant code
> changes, merely code moving, and in 2 cases, stub functions added.
> 
> Please review and apply.

Looks good.  I've added this patch, and then applied this one on top of
yours to fix up some compiler warnings that I got when building yours.
I've also moved the #defines into static inline functions, which is a
bit nicer to do.

I'll send it on in a bit to Linus.

I'll also go through the tree and fix up any pci probe functions that
are marked __init as they should now be marked __devinit.

thanks for doing this patch,

greg k-h


# PCI: remove compiler warning from previous new_id patch
#
# Also change the #define functions into inline functions to help
# catch any future paramater mis-matches.
#
# And clean up a few minor style issue...

diff -Nru a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	Thu Sep 11 14:23:32 2003
+++ b/drivers/pci/pci-driver.c	Thu Sep 11 14:23:32 2003
@@ -69,6 +69,7 @@
 	spin_unlock(&drv->dynids.lock);
 	return error;
 }
+
 static inline void
 dynid_init(struct dynid *dynid)
 {
@@ -78,15 +79,12 @@
 
 /**
  * store_new_id
- * @ pdrv
- * @ buf
- * @ count
  *
  * Adds a new dynamic pci device ID to this driver,
  * and causes the driver to probe for all devices again.
  */
 static inline ssize_t
-store_new_id(struct device_driver * driver, const char * buf, size_t count)
+store_new_id(struct device_driver *driver, const char *buf, size_t count)
 {
 	struct dynid *dynid;
 	struct bus_type * bus;
@@ -159,7 +157,7 @@
 }
 
 static int
-pci_create_newid_file(struct pci_driver * drv)
+pci_create_newid_file(struct pci_driver *drv)
 {
 	int error = 0;
 	if (drv->probe != NULL)
@@ -169,7 +167,7 @@
 }
 
 static int
-pci_bus_match_dynids(const struct pci_dev * pci_dev, const struct pci_driver * pci_drv)
+pci_bus_match_dynids(const struct pci_dev *pci_dev, struct pci_driver *pci_drv)
 {
 	struct list_head *pos;
 	struct dynid *dynid;
@@ -187,12 +185,21 @@
 }
 
 #else /* !CONFIG_HOTPLUG */
-#define pci_device_probe_dynamic(drv,pci_dev) (-ENODEV)
-#define dynid_init(dynid) do {} while (0)
-#define pci_init_dynids(dynids) do {} while (0)
-#define pci_free_dynids(drv) do {} while (0)
-#define pci_create_newid_file(drv) (0)
-#define pci_bus_match_dynids(pci_dev, pci_drv) (0)
+static inline int pci_device_probe_dynamic(struct pci_driver *drv, struct pci_dev *pci_dev)
+{
+	return -ENODEV;
+}
+static inline void dynid_init(struct dynid *dynid) {}
+static inline void pci_init_dynids(struct pci_dynids *dynids) {}
+static inline void pci_free_dynids(struct pci_driver *drv) {}
+static inline int pci_create_newid_file(struct pci_driver *drv)
+{
+	return 0;
+}
+static inline int pci_bus_match_dynids(const struct pci_dev *pci_dev, struct pci_driver *pci_drv)
+{
+	return 0;
+}
 #endif
 
 /**
@@ -352,7 +359,7 @@
 };
 
 static int
-pci_populate_driver_dir(struct pci_driver * drv)
+pci_populate_driver_dir(struct pci_driver *drv)
 {
 	return pci_create_newid_file(drv);
 }