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

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

[Russell King]

On Tue, Sep 09, 2003 at 10:22:59PM +0100, Alan Cox wrote:
> On Maw, 2003-09-09 at 22:04, Russell King wrote:
> > I want this to be foolproof, because its me people bug when their cardbus
> > cards oops when they insert the damned things.  If people are happy to
> > ignore this issue, I'm happy to ignore the bug reports.
> > 
> > It basically isn't something I want to deal with, and we need to find a
> > way to stop these stupidities appearing in the first place.
> > 
> > Any ideas?
> 
> You've already got symbols for initdata start and end, just check the 
> pointers in the pci_register code. I guess you want a per platform
> 
> BUG_IF_INIT(x)

That would work for built-in drivers.  We could couple that with an idea
Kai came up with (in private mail) to catch them in modpost.  However,
the problem with modpost is that it gets false positives for these
drivers which explicitly want to discard their module device id tables.

As we currently stand, there seem to be only four drivers which want to
discard their driver id tables.  Is it really worth adding extra code
to the kernel to try to trap these, or just not mark the device id
tables with __initdata or __devinitdata and detect the bad guys with
a grep?

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

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

[Zwane Mwaikambo]

On Tue, 9 Sep 2003, Alan Cox wrote:

> On Maw, 2003-09-09 at 22:04, Russell King wrote:
> > I want this to be foolproof, because its me people bug when their cardbus
> > cards oops when they insert the damned things.  If people are happy to
> > ignore this issue, I'm happy to ignore the bug reports.
> > 
> > It basically isn't something I want to deal with, and we need to find a
> > way to stop these stupidities appearing in the first place.
> > 
> > Any ideas?
> 
> You've already got symbols for initdata start and end, just check the 
> pointers in the pci_register code. I guess you want a per platform
> 
> BUG_IF_INIT(x)

I hacked up something similar, but i'm not sure what to do about modules.

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

[Alan Cox]

On Maw, 2003-09-09 at 22:04, Russell King wrote:
> I want this to be foolproof, because its me people bug when their cardbus
> cards oops when they insert the damned things.  If people are happy to
> ignore this issue, I'm happy to ignore the bug reports.
> 
> It basically isn't something I want to deal with, and we need to find a
> way to stop these stupidities appearing in the first place.
> 
> Any ideas?

You've already got symbols for initdata start and end, just check the 
pointers in the pci_register code. I guess you want a per platform

BUG_IF_INIT(x)

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

[Russell King]

On Tue, Sep 09, 2003 at 04:02:58PM -0400, Zwane Mwaikambo wrote:
> On Tue, 9 Sep 2003, Russell King wrote:
> 
> > --- orig/drivers/char/watchdog/amd7xx_tco.c	Sat Jun 14 22:33:48 2003
> > +++ linux/drivers/char/watchdog/amd7xx_tco.c	Tue Sep  9 20:45:16 2003
> > @@ -294,7 +294,7 @@
> >  	.fops	= &amdtco_fops
> >  };
> >  
> > -static struct pci_device_id amdtco_pci_tbl[] __initdata = {
> > +static struct pci_device_id amdtco_pci_tbl[] = {
> >  	/* AMD 766 PCI_IDs here */
> >  	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_OPUS_7443, PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ 0, }
> 
> That's not a bug.
>
> > --- orig/drivers/char/watchdog/i810-tco.c	Sun Aug  3 11:21:11 2003
> > +++ linux/drivers/char/watchdog/i810-tco.c	Tue Sep  9 20:45:16 2003
> > @@ -301,7 +301,7 @@
> >   * register a pci_driver, because someone else might one day
> >   * want to register another driver on the same PCI id.
> >   */
> > -static struct pci_device_id i810tco_pci_tbl[] __initdata = {
> > +static struct pci_device_id i810tco_pci_tbl[] = {
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_0,	PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_0,	PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0,	PCI_ANY_ID, PCI_ANY_ID, },
> 
> Neither is that.
> 
> > --- orig/drivers/char/hw_random.c	Sat Jun 14 22:33:46 2003
> > +++ linux/drivers/char/hw_random.c	Tue Sep  9 20:45:16 2003
> > @@ -149,7 +149,7 @@
> >   * register a pci_driver, because someone else might one day
> >   * want to register another driver on the same PCI id.
> >   */
> > -static struct pci_device_id rng_pci_tbl[] __initdata = {
> > +static struct pci_device_id rng_pci_tbl[] = {
> >  	{ 0x1022, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
> >  	{ 0x1022, 0x746b, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
> 
> This too

Ok, I'm happy that this aren't (after reading someone elses explaination).

Having these different makes it hard to ensure that no further bad cases
exist in the tree though.

I want this to be foolproof, because its me people bug when their cardbus
cards oops when they insert the damned things.  If people are happy to
ignore this issue, I'm happy to ignore the bug reports.

It basically isn't something I want to deal with, and we need to find a
way to stop these stupidities appearing in the first place.

Any ideas?

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

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

[Russell King]

On Tue, Sep 09, 2003 at 04:02:58PM -0400, Zwane Mwaikambo wrote:
> On Tue, 9 Sep 2003, Russell King wrote:
> 
> > --- orig/drivers/char/watchdog/amd7xx_tco.c	Sat Jun 14 22:33:48 2003
> > +++ linux/drivers/char/watchdog/amd7xx_tco.c	Tue Sep  9 20:45:16 2003
> > @@ -294,7 +294,7 @@
> >  	.fops	= &amdtco_fops
> >  };
> >  
> > -static struct pci_device_id amdtco_pci_tbl[] __initdata = {
> > +static struct pci_device_id amdtco_pci_tbl[] = {
> >  	/* AMD 766 PCI_IDs here */
> >  	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_OPUS_7443, PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ 0, }
> 
> That's not a bug.
> 
> > --- orig/drivers/char/watchdog/i810-tco.c	Sun Aug  3 11:21:11 2003
> > +++ linux/drivers/char/watchdog/i810-tco.c	Tue Sep  9 20:45:16 2003
> > @@ -301,7 +301,7 @@
> >   * register a pci_driver, because someone else might one day
> >   * want to register another driver on the same PCI id.
> >   */
> > -static struct pci_device_id i810tco_pci_tbl[] __initdata = {
> > +static struct pci_device_id i810tco_pci_tbl[] = {
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_0,	PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_0,	PCI_ANY_ID, PCI_ANY_ID, },
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0,	PCI_ANY_ID, PCI_ANY_ID, },
> 
> Neither is that.
> 
> > --- orig/drivers/char/hw_random.c	Sat Jun 14 22:33:46 2003
> > +++ linux/drivers/char/hw_random.c	Tue Sep  9 20:45:16 2003
> > @@ -149,7 +149,7 @@
> >   * register a pci_driver, because someone else might one day
> >   * want to register another driver on the same PCI id.
> >   */
> > -static struct pci_device_id rng_pci_tbl[] __initdata = {
> > +static struct pci_device_id rng_pci_tbl[] = {
> >  	{ 0x1022, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
> >  	{ 0x1022, 0x746b, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
> 
> This too

They're all bugs, plain and simple.  The pci device id tables are scanned
*whenever* a new pci device is inserted into the system, or the appropriate
numbers are echoed to the "new_id" driver model entry for the driver.

It doesn't matter if the driver doesn't care or not.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

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

[Russell King]

On Tue, Sep 09, 2003 at 09:14:37PM +0100, Dave Jones wrote:
> On Tue, Sep 09, 2003 at 08:48:03PM +0100, Russell King wrote:
>  > -static struct pci_device_id agp_ati_pci_table[] __initdata = {
>  > +static struct pci_device_id agp_ati_pci_table[] = {
> 
> Wierd. I could swear akpm had these patches in his tree no so long back.

So did I, so I checked the history in bk, and it seems it never made Linus'
tree.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

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

[Dave Jones]

On Tue, Sep 09, 2003 at 08:48:03PM +0100, Russell King wrote:

 > -static struct pci_device_id agp_ati_pci_table[] __initdata = {
 > +static struct pci_device_id agp_ati_pci_table[] = {

Wierd. I could swear akpm had these patches in his tree no so long back.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

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

[Zwane Mwaikambo]

On Tue, 9 Sep 2003, Russell King wrote:

> --- orig/drivers/char/watchdog/amd7xx_tco.c	Sat Jun 14 22:33:48 2003
> +++ linux/drivers/char/watchdog/amd7xx_tco.c	Tue Sep  9 20:45:16 2003
> @@ -294,7 +294,7 @@
>  	.fops	= &amdtco_fops
>  };
>  
> -static struct pci_device_id amdtco_pci_tbl[] __initdata = {
> +static struct pci_device_id amdtco_pci_tbl[] = {
>  	/* AMD 766 PCI_IDs here */
>  	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_OPUS_7443, PCI_ANY_ID, PCI_ANY_ID, },
>  	{ 0, }

That's not a bug.

> --- orig/drivers/char/watchdog/i810-tco.c	Sun Aug  3 11:21:11 2003
> +++ linux/drivers/char/watchdog/i810-tco.c	Tue Sep  9 20:45:16 2003
> @@ -301,7 +301,7 @@
>   * register a pci_driver, because someone else might one day
>   * want to register another driver on the same PCI id.
>   */
> -static struct pci_device_id i810tco_pci_tbl[] __initdata = {
> +static struct pci_device_id i810tco_pci_tbl[] = {
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_0,	PCI_ANY_ID, PCI_ANY_ID, },
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_0,	PCI_ANY_ID, PCI_ANY_ID, },
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0,	PCI_ANY_ID, PCI_ANY_ID, },

Neither is that.

> --- orig/drivers/char/hw_random.c	Sat Jun 14 22:33:46 2003
> +++ linux/drivers/char/hw_random.c	Tue Sep  9 20:45:16 2003
> @@ -149,7 +149,7 @@
>   * register a pci_driver, because someone else might one day
>   * want to register another driver on the same PCI id.
>   */
> -static struct pci_device_id rng_pci_tbl[] __initdata = {
> +static struct pci_device_id rng_pci_tbl[] = {
>  	{ 0x1022, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
>  	{ 0x1022, 0x746b, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },

This too

Buggy PCI drivers - do not mark pci_device_id as discardable data

[Russell King]

Guys,

All these drivers are buggy wrt PCI hotplugging/Cardbus/PCI new_id
support and are all potential sources of oops dumps.  You don't need
to have hotplugging enabled to cause your machine to oops with any
of these drivers loaded.

The authors of future drivers doing this will be sent home to write
1000 lines, using pen and paper, of: "I will not use __initdata with
pci_device_id tables."  Printed copies, trick photography, and emails
will not be accepted.

(I'm sending Linus a separate patch since I've already sent him the
AGP fixes.)

--- orig/drivers/char/agp/ati-agp.c	Mon Sep  8 23:36:52 2003
+++ linux/drivers/char/agp/ati-agp.c	Tue Sep  9 20:11:19 2003
@@ -491,7 +491,7 @@
 	agp_put_bridge(bridge);
 }
 
-static struct pci_device_id agp_ati_pci_table[] __initdata = {
+static struct pci_device_id agp_ati_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
 	.class_mask	= ~0,
--- orig/drivers/char/agp/sis-agp.c	Mon Sep  8 23:36:53 2003
+++ linux/drivers/char/agp/sis-agp.c	Tue Sep  9 20:12:38 2003
@@ -215,7 +215,7 @@
 	agp_put_bridge(bridge);
 }
 
-static struct pci_device_id agp_sis_pci_table[] __initdata = {
+static struct pci_device_id agp_sis_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
 	.class_mask	= ~0,
--- orig/drivers/char/agp/uninorth-agp.c	Thu Sep  4 16:36:58 2003
+++ linux/drivers/char/agp/uninorth-agp.c	Tue Sep  9 20:12:38 2003
@@ -350,7 +350,7 @@
 	agp_put_bridge(bridge);
 }
 
-static struct pci_device_id agp_uninorth_pci_table[] __initdata = {
+static struct pci_device_id agp_uninorth_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
 	.class_mask	= ~0,
--- orig/drivers/char/agp/via-agp.c	Mon Sep  8 23:36:53 2003
+++ linux/drivers/char/agp/via-agp.c	Tue Sep  9 20:12:38 2003
@@ -432,7 +432,7 @@
 	agp_put_bridge(bridge);
 }
 
-static struct pci_device_id agp_via_pci_table[] __initdata = {
+static struct pci_device_id agp_via_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
 	.class_mask	= ~0,
--- orig/drivers/char/watchdog/alim1535_wdt.c	Mon Sep  8 23:37:03 2003
+++ linux/drivers/char/watchdog/alim1535_wdt.c	Tue Sep  9 20:45:16 2003
@@ -317,7 +317,7 @@
  *	want to register another driver on the same PCI id.
  */
 
-static struct pci_device_id ali_pci_tbl[] __initdata = {
+static struct pci_device_id ali_pci_tbl[] = {
 	{ PCI_VENDOR_ID_AL, 1535, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, },
 };
--- orig/drivers/char/watchdog/amd7xx_tco.c	Sat Jun 14 22:33:48 2003
+++ linux/drivers/char/watchdog/amd7xx_tco.c	Tue Sep  9 20:45:16 2003
@@ -294,7 +294,7 @@
 	.fops	= &amdtco_fops
 };
 
-static struct pci_device_id amdtco_pci_tbl[] __initdata = {
+static struct pci_device_id amdtco_pci_tbl[] = {
 	/* AMD 766 PCI_IDs here */
 	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_OPUS_7443, PCI_ANY_ID, PCI_ANY_ID, },
 	{ 0, }
--- orig/drivers/char/watchdog/i810-tco.c	Sun Aug  3 11:21:11 2003
+++ linux/drivers/char/watchdog/i810-tco.c	Tue Sep  9 20:45:16 2003
@@ -301,7 +301,7 @@
  * register a pci_driver, because someone else might one day
  * want to register another driver on the same PCI id.
  */
-static struct pci_device_id i810tco_pci_tbl[] __initdata = {
+static struct pci_device_id i810tco_pci_tbl[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_0,	PCI_ANY_ID, PCI_ANY_ID, },
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_0,	PCI_ANY_ID, PCI_ANY_ID, },
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0,	PCI_ANY_ID, PCI_ANY_ID, },
--- orig/drivers/char/hw_random.c	Sat Jun 14 22:33:46 2003
+++ linux/drivers/char/hw_random.c	Tue Sep  9 20:45:16 2003
@@ -149,7 +149,7 @@
  * register a pci_driver, because someone else might one day
  * want to register another driver on the same PCI id.
  */
-static struct pci_device_id rng_pci_tbl[] __initdata = {
+static struct pci_device_id rng_pci_tbl[] = {
 	{ 0x1022, 0x7443, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
 	{ 0x1022, 0x746b, PCI_ANY_ID, PCI_ANY_ID, 0, 0, rng_hw_amd },
 
--- orig/drivers/isdn/hisax/config.c	Sun Aug  3 11:21:16 2003
+++ linux/drivers/isdn/hisax/config.c	Tue Sep  9 20:45:16 2003
@@ -2113,7 +2113,7 @@
 
 #include <linux/pci.h>
 
-static struct pci_device_id hisax_pci_tbl[] __initdata = {
+static struct pci_device_id hisax_pci_tbl[] = {
 #ifdef CONFIG_HISAX_FRITZPCI
 	{PCI_VENDOR_ID_AVM,      PCI_DEVICE_ID_AVM_A1,           PCI_ANY_ID, PCI_ANY_ID},
 #endif
--- orig/drivers/isdn/hysdn/hysdn_init.c	Sat Jun 14 22:33:52 2003
+++ linux/drivers/isdn/hysdn/hysdn_init.c	Tue Sep  9 20:45:16 2003
@@ -21,7 +21,7 @@
 
 #include "hysdn_defs.h"
 
-static struct pci_device_id hysdn_pci_tbl[] __initdata = {
+static struct pci_device_id hysdn_pci_tbl[] = {
 	{PCI_VENDOR_ID_HYPERCOPE, PCI_DEVICE_ID_HYPERCOPE_PLX, PCI_ANY_ID, PCI_SUBDEVICE_ID_HYPERCOPE_METRO},
 	{PCI_VENDOR_ID_HYPERCOPE, PCI_DEVICE_ID_HYPERCOPE_PLX, PCI_ANY_ID, PCI_SUBDEVICE_ID_HYPERCOPE_CHAMP2},
 	{PCI_VENDOR_ID_HYPERCOPE, PCI_DEVICE_ID_HYPERCOPE_PLX, PCI_ANY_ID, PCI_SUBDEVICE_ID_HYPERCOPE_ERGO},
--- orig/drivers/net/fc/iph5526.c	Thu Sep  4 16:37:13 2003
+++ linux/drivers/net/fc/iph5526.c	Tue Sep  9 20:45:17 2003
@@ -110,7 +110,7 @@
 #define ALIGNED_ADDR(addr, len) ((((unsigned long)(addr) + (len - 1)) & ~(len - 1)) - (unsigned long)(addr))
 
 
-static struct pci_device_id iph5526_pci_tbl[] __initdata = {
+static struct pci_device_id iph5526_pci_tbl[] = {
 	{ PCI_VENDOR_ID_INTERPHASE, PCI_DEVICE_ID_INTERPHASE_5526, PCI_ANY_ID, PCI_ANY_ID, },
 	{ PCI_VENDOR_ID_INTERPHASE, PCI_DEVICE_ID_INTERPHASE_55x6, PCI_ANY_ID, PCI_ANY_ID, },
 	{ }			/* Terminating entry */
--- orig/drivers/net/irda/via-ircc.c	Mon Sep  8 23:37:31 2003
+++ linux/drivers/net/irda/via-ircc.c	Tue Sep  9 20:45:17 2003
@@ -121,7 +121,7 @@
 	}
 }
 
-static struct pci_device_id via_pci_tbl[] __initdata = {
+static struct pci_device_id via_pci_tbl[] = {
 	{ PCI_VENDOR_ID_VIA, DeviceID1, PCI_ANY_ID, PCI_ANY_ID,0,0,0 },
 	{ PCI_VENDOR_ID_VIA, DeviceID2, PCI_ANY_ID, PCI_ANY_ID,0,0,1 },
 	{ PCI_VENDOR_ID_VIA, DeviceID3, PCI_ANY_ID, PCI_ANY_ID,0,0,2 },
--- orig/drivers/net/skfp/skfddi.c	Mon Sep  8 23:37:35 2003
+++ linux/drivers/net/skfp/skfddi.c	Tue Sep  9 20:45:17 2003
@@ -182,7 +182,7 @@
 extern void enable_tx_irq(struct s_smc *smc, u_short queue);
 extern void mac_drv_clear_txd(struct s_smc *smc);
 
-static struct pci_device_id skfddi_pci_tbl[] __initdata = {
+static struct pci_device_id skfddi_pci_tbl[] = {
 	{ PCI_VENDOR_ID_SK, PCI_DEVICE_ID_SK_FP, PCI_ANY_ID, PCI_ANY_ID, },
 	{ }			/* Terminating entry */
 };
--- orig/drivers/net/wan/sdladrv.c	Wed Aug 13 10:33:18 2003
+++ linux/drivers/net/wan/sdladrv.c	Tue Sep  9 20:45:17 2003
@@ -201,7 +201,7 @@
  * Note: All data must be explicitly initialized!!!
  */
 
-static struct pci_device_id sdladrv_pci_tbl[] __initdata = {
+static struct pci_device_id sdladrv_pci_tbl[] = {
 	{ V3_VENDOR_ID, V3_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID, },
 	{ }			/* Terminating entry */
 };
--- orig/drivers/net/dgrs.c	Mon Sep  8 23:37:29 2003
+++ linux/drivers/net/dgrs.c	Tue Sep  9 20:45:16 2003
@@ -120,7 +120,7 @@
 #include "dgrs_asstruct.h"
 #include "dgrs_bcomm.h"
 
-static struct pci_device_id dgrs_pci_tbl[] __initdata = {
+static struct pci_device_id dgrs_pci_tbl[] = {
 	{ SE6_PCI_VENDOR_ID, SE6_PCI_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID, },
 	{ }			/* Terminating entry */
 };
--- orig/drivers/net/hp100.c	Thu Sep  4 16:37:16 2003
+++ linux/drivers/net/hp100.c	Tue Sep  9 20:45:16 2003
@@ -284,7 +284,7 @@
 
 #define HP100_PCI_IDS_SIZE	(sizeof(hp100_pci_ids)/sizeof(struct hp100_pci_id))
 
-static struct pci_device_id hp100_pci_tbl[] __initdata = {
+static struct pci_device_id hp100_pci_tbl[] = {
 	{PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_J2585A, PCI_ANY_ID, PCI_ANY_ID,},
 	{PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_J2585B, PCI_ANY_ID, PCI_ANY_ID,},
 	{PCI_VENDOR_ID_COMPEX, PCI_DEVICE_ID_COMPEX_ENET100VG4, PCI_ANY_ID, PCI_ANY_ID,},
--- orig/drivers/net/sunhme.c	Mon Sep  8 23:37:36 2003
+++ linux/drivers/net/sunhme.c	Tue Sep  9 20:45:17 2003
@@ -179,7 +179,7 @@
    where it could be referenced at any time due to hot plugging,
    the __initdata reference should be removed. */
 
-struct pci_device_id happymeal_pci_ids[] __initdata = {
+struct pci_device_id happymeal_pci_ids[] = {
 	{
 	  .vendor	= PCI_VENDOR_ID_SUN,
 	  .device	= PCI_DEVICE_ID_SUN_HAPPYMEAL,
--- orig/drivers/video/i810/i810_main.c	Wed Aug 13 10:33:34 2003
+++ linux/drivers/video/i810/i810_main.c	Tue Sep  9 20:45:16 2003
@@ -66,7 +66,7 @@
 	"Intel(R) 815 (Internal Graphics with AGP) Framebuffer Device"
 };
 
-static struct pci_device_id i810fb_pci_tbl[] __initdata = {
+static struct pci_device_id i810fb_pci_tbl[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82810_IG1,
 	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82810_IG3,

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Linux kernel maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core