> > 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 http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
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
> > 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 http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
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
> > 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 http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
> > 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 http://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);
}
/**
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);
}