Hi Markus,
I was looking at the users of class_interfaces and stumbled across
I2O subsystem. As far as I understand the purpose of class interfaces
was to provide different 'views' on the hardware, not just to have
a callback to finish initialization of sysfs structures. I think it
woudl be better to remove i2o_device_class_interface and create
user/parent links right after class device registration.
Also, it looks like i2o_device_class itself is not needed - correct
me if I am wrong, but all i2o devics reside on their own bus so
i2o_devices class simply mirrors iformation from the bus and can
also be safely removed.
Please consider applying the 2 pathes below (just compile-tested,
don't have proper hardware).
--
Dmitry
I2O: cleanup - remove i2o_device_class
I2O devices reside on their own bus so there should be no reason
to also have i2c_device class that mirros i2o bus.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/message/i2o/core.h | 3 -
drivers/message/i2o/device.c | 71 +++++++------------------------------------
drivers/message/i2o/driver.c | 3 +
drivers/message/i2o/iop.c | 10 ------
include/linux/i2o.h | 2 -
5 files changed, 17 insertions(+), 72 deletions(-)
Index: work/drivers/message/i2o/device.c
===================================================================
--- work.orig/drivers/message/i2o/device.c
+++ work/drivers/message/i2o/device.c
@@ -138,17 +138,6 @@ static void i2o_device_release(struct de
kfree(i2o_dev);
}
-/**
- * i2o_device_class_release - I2O class device release function
- * @cd: I2O class device which is added to the I2O device class
- *
- * The function is just a stub - memory will be freed when
- * associated I2O device is released.
- */
-static void i2o_device_class_release(struct class_device *cd)
-{
- /* empty */
-}
/**
* i2o_device_class_show_class_id - Displays class id of I2O device
@@ -157,12 +146,13 @@ static void i2o_device_class_release(str
*
* Returns the number of bytes which are printed into the buffer.
*/
-static ssize_t i2o_device_class_show_class_id(struct class_device *cd,
- char *buf)
+static ssize_t i2o_device_show_class_id(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
{
- struct i2o_device *dev = to_i2o_device(cd->dev);
+ struct i2o_device *i2o_dev = to_i2o_device(dev);
- sprintf(buf, "0x%03x\n", dev->lct_data.class_id);
+ sprintf(buf, "0x%03x\n", i2o_dev->lct_data.class_id);
return strlen(buf) + 1;
}
@@ -173,27 +163,22 @@ static ssize_t i2o_device_class_show_cla
*
* Returns the number of bytes which are printed into the buffer.
*/
-static ssize_t i2o_device_class_show_tid(struct class_device *cd, char *buf)
+static ssize_t i2o_device_show_tid(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
{
- struct i2o_device *dev = to_i2o_device(cd->dev);
+ struct i2o_device *i2o_dev = to_i2o_device(dev);
- sprintf(buf, "0x%03x\n", dev->lct_data.tid);
+ sprintf(buf, "0x%03x\n", i2o_dev->lct_data.tid);
return strlen(buf) + 1;
}
-static struct class_device_attribute i2o_device_class_attrs[] = {
- __ATTR(class_id, S_IRUGO, i2o_device_class_show_class_id, NULL),
- __ATTR(tid, S_IRUGO, i2o_device_class_show_tid, NULL),
+struct device_attribute i2o_device_attrs[] = {
+ __ATTR(class_id, S_IRUGO, i2o_device_show_class_id, NULL),
+ __ATTR(tid, S_IRUGO, i2o_device_show_tid, NULL),
__ATTR_NULL
};
-/* I2O device class */
-static struct class i2o_device_class = {
- .name = "i2o_device",
- .release = i2o_device_class_release,
- .class_dev_attrs = i2o_device_class_attrs,
-};
-
/**
* i2o_device_alloc - Allocate a I2O device and initialize it
*
@@ -217,8 +202,6 @@ static struct i2o_device *i2o_device_all
dev->device.bus = &i2o_bus_type;
dev->device.release = &i2o_device_release;
- dev->classdev.class = &i2o_device_class;
- dev->classdev.dev = &dev->device;
return dev;
}
@@ -311,17 +294,12 @@ static struct i2o_device *i2o_device_add
snprintf(dev->device.bus_id, BUS_ID_SIZE, "%d:%03x", c->unit,
dev->lct_data.tid);
- snprintf(dev->classdev.class_id, BUS_ID_SIZE, "%d:%03x", c->unit,
- dev->lct_data.tid);
-
dev->device.parent = &c->device;
device_register(&dev->device);
list_add_tail(&dev->list, &c->devices);
- class_device_register(&dev->classdev);
-
i2o_setup_sysfs_links(dev);
i2o_driver_notify_device_add_all(dev);
@@ -343,7 +321,6 @@ void i2o_device_remove(struct i2o_device
{
i2o_driver_notify_device_remove_all(i2o_dev);
i2o_remove_sysfs_links(i2o_dev);
- class_device_unregister(&i2o_dev->classdev);
list_del(&i2o_dev->list);
device_unregister(&i2o_dev->device);
}
@@ -598,28 +575,6 @@ int i2o_parm_table_get(struct i2o_device
return size;
}
-/**
- * i2o_device_init - Initialize I2O devices
- *
- * Registers the I2O device class.
- *
- * Returns 0 on success or negative error code on failure.
- */
-int i2o_device_init(void)
-{
- return class_register(&i2o_device_class);
-}
-
-/**
- * i2o_device_exit - I2O devices exit function
- *
- * Unregisters the I2O device class.
- */
-void i2o_device_exit(void)
-{
- class_unregister(&i2o_device_class);
-}
-
EXPORT_SYMBOL(i2o_device_claim);
EXPORT_SYMBOL(i2o_device_claim_release);
EXPORT_SYMBOL(i2o_parm_field_get);
Index: work/drivers/message/i2o/driver.c
===================================================================
--- work.orig/drivers/message/i2o/driver.c
+++ work/drivers/message/i2o/driver.c
@@ -58,9 +58,12 @@ static int i2o_bus_match(struct device *
};
/* I2O bus type */
+extern struct device_attribute i2o_device_attrs[];
+
struct bus_type i2o_bus_type = {
.name = "i2o",
.match = i2o_bus_match,
+ .dev_attrs = i2o_device_attrs,
};
/**
Index: work/include/linux/i2o.h
===================================================================
--- work.orig/include/linux/i2o.h
+++ work/include/linux/i2o.h
@@ -66,8 +66,6 @@ struct i2o_device {
struct device device;
struct semaphore lock; /* device lock */
-
- struct class_device classdev; /* i2o device class */
};
/*
Index: work/drivers/message/i2o/core.h
===================================================================
--- work.orig/drivers/message/i2o/core.h
+++ work/drivers/message/i2o/core.h
@@ -36,9 +36,6 @@ extern void __exit i2o_pci_exit(void);
extern void i2o_device_remove(struct i2o_device *);
extern int i2o_device_parse_lct(struct i2o_controller *);
-extern int i2o_device_init(void);
-extern void i2o_device_exit(void);
-
/* IOP */
extern struct i2o_controller *i2o_iop_alloc(void);
extern void i2o_iop_free(struct i2o_controller *);
Index: work/drivers/message/i2o/iop.c
===================================================================
--- work.orig/drivers/message/i2o/iop.c
+++ work/drivers/message/i2o/iop.c
@@ -1246,13 +1246,9 @@ static int __init i2o_iop_init(void)
printk(KERN_INFO OSM_DESCRIPTION " v" OSM_VERSION "\n");
- rc = i2o_device_init();
- if (rc)
- goto exit;
-
if ((rc = class_register(&i2o_controller_class))) {
osm_err("can't register class i2o_controller\n");
- goto device_exit;
+ goto exit;
}
if ((rc = i2o_driver_init()))
@@ -1275,9 +1271,6 @@ static int __init i2o_iop_init(void)
class_exit:
class_unregister(&i2o_controller_class);
- device_exit:
- i2o_device_exit();
-
exit:
return rc;
}
@@ -1293,7 +1286,6 @@ static void __exit i2o_iop_exit(void)
i2o_exec_exit();
i2o_driver_exit();
class_unregister(&i2o_controller_class);
- i2o_device_exit();
};
module_init(i2o_iop_init);
I2O: remove i2o_device_class_interface misuse
The intent of class interfaces was to provide different
'views' at the same object, not just run some code every
time a new class device is registered. Kill interface
structure, make class core register default attributes
and set up sysfs links right when registering class
devices.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/message/i2o/device.c | 255 ++++++++++++++++++++-----------------------
1 files changed, 122 insertions(+), 133 deletions(-)
Index: work/drivers/message/i2o/device.c
===================================================================
--- work.orig/drivers/message/i2o/device.c
+++ work/drivers/message/i2o/device.c
@@ -45,10 +45,10 @@ static inline int i2o_device_issue_claim
writel(type, &msg->body[0]);
return i2o_msg_post_wait(dev->iop, m, 60);
-};
+}
/**
- * i2o_device_claim - claim a device for use by an OSM
+ * i2o_device_claim - claim a device for use by an OSM
* @dev: I2O device to claim
* @drv: I2O driver which wants to claim the device
*
@@ -73,7 +73,7 @@ int i2o_device_claim(struct i2o_device *
up(&dev->lock);
return rc;
-};
+}
/**
* i2o_device_claim_release - release a device that the OSM is using
@@ -119,7 +119,8 @@ int i2o_device_claim_release(struct i2o_
up(&dev->lock);
return rc;
-};
+}
+
/**
* i2o_device_release - release the memory for a I2O device
@@ -135,39 +136,62 @@ static void i2o_device_release(struct de
pr_debug("i2o: device %s released\n", dev->bus_id);
kfree(i2o_dev);
-};
+}
/**
- * i2o_device_class_release - Remove I2O device attributes
+ * i2o_device_class_release - I2O class device release function
* @cd: I2O class device which is added to the I2O device class
*
- * Removes attributes from the I2O device again. Also search each device
- * on the controller for I2O devices which refert to this device as parent
- * or user and remove this links also.
+ * The function is just a stub - memory will be freed when
+ * associated I2O device is released.
*/
static void i2o_device_class_release(struct class_device *cd)
{
- struct i2o_device *i2o_dev, *tmp;
- struct i2o_controller *c;
+ /* empty */
+}
- i2o_dev = to_i2o_device(cd->dev);
- c = i2o_dev->iop;
+/**
+ * i2o_device_class_show_class_id - Displays class id of I2O device
+ * @cd: class device of which the class id should be displayed
+ * @buf: buffer into which the class id should be printed
+ *
+ * Returns the number of bytes which are printed into the buffer.
+ */
+static ssize_t i2o_device_class_show_class_id(struct class_device *cd,
+ char *buf)
+{
+ struct i2o_device *dev = to_i2o_device(cd->dev);
- sysfs_remove_link(&i2o_dev->device.kobj, "parent");
- sysfs_remove_link(&i2o_dev->device.kobj, "user");
+ sprintf(buf, "0x%03x\n", dev->lct_data.class_id);
+ return strlen(buf) + 1;
+}
- list_for_each_entry(tmp, &c->devices, list) {
- if (tmp->lct_data.parent_tid == i2o_dev->lct_data.tid)
- sysfs_remove_link(&tmp->device.kobj, "parent");
- if (tmp->lct_data.user_tid == i2o_dev->lct_data.tid)
- sysfs_remove_link(&tmp->device.kobj, "user");
- }
+/**
+ * i2o_device_class_show_tid - Displays TID of I2O device
+ * @cd: class device of which the TID should be displayed
+ * @buf: buffer into which the class id should be printed
+ *
+ * Returns the number of bytes which are printed into the buffer.
+ */
+static ssize_t i2o_device_class_show_tid(struct class_device *cd, char *buf)
+{
+ struct i2o_device *dev = to_i2o_device(cd->dev);
+
+ sprintf(buf, "0x%03x\n", dev->lct_data.tid);
+ return strlen(buf) + 1;
+}
+
+static struct class_device_attribute i2o_device_class_attrs[] = {
+ __ATTR(class_id, S_IRUGO, i2o_device_class_show_class_id, NULL),
+ __ATTR(tid, S_IRUGO, i2o_device_class_show_tid, NULL),
+ __ATTR_NULL
};
/* I2O device class */
static struct class i2o_device_class = {
- .name = "i2o_device",
- .release = i2o_device_class_release
+ .name = "i2o_device",
+ .release = i2o_device_class_release,
+ .class_dev_attrs = i2o_device_class_attrs,
};
/**
@@ -197,7 +221,67 @@ static struct i2o_device *i2o_device_all
dev->classdev.dev = &dev->device;
return dev;
-};
+}
+
+/**
+ * i2o_setup_sysfs_links - Adds attributes to the I2O device
+ * @cd: I2O class device which is added to the I2O device class
+ *
+ * This function get called when a I2O device is added to the class. It
+ * creates the attributes for each device and creates user/parent symlink
+ * if necessary.
+ *
+ * Returns 0 on success or negative error code on failure.
+ */
+static void i2o_setup_sysfs_links(struct i2o_device *i2o_dev)
+{
+ struct i2o_controller *c = i2o_dev->iop;
+ struct i2o_device *tmp;
+
+ /* create user entries for this device */
+ tmp = i2o_iop_find_device(i2o_dev->iop, i2o_dev->lct_data.user_tid);
+ if (tmp && tmp != i2o_dev)
+ sysfs_create_link(&i2o_dev->device.kobj,
+ &tmp->device.kobj, "user");
+
+ /* create user entries refering to this device */
+ list_for_each_entry(tmp, &c->devices, list)
+ if (tmp->lct_data.user_tid == i2o_dev->lct_data.tid &&
+ tmp != i2o_dev)
+ sysfs_create_link(&tmp->device.kobj,
+ &i2o_dev->device.kobj, "user");
+
+ /* create parent entries for this device */
+ tmp = i2o_iop_find_device(i2o_dev->iop, i2o_dev->lct_data.parent_tid);
+ if (tmp && tmp != i2o_dev)
+ sysfs_create_link(&i2o_dev->device.kobj,
+ &tmp->device.kobj, "parent");
+
+ /* create parent entries refering to this device */
+ list_for_each_entry(tmp, &c->devices, list)
+ if (tmp->lct_data.parent_tid == i2o_dev->lct_data.tid &&
+ tmp != i2o_dev)
+ sysfs_create_link(&tmp->device.kobj,
+ &i2o_dev->device.kobj, "parent");
+}
+
+static void i2o_remove_sysfs_links(struct i2o_device *i2o_dev)
+{
+ struct i2o_controller *c = i2o_dev->iop;
+ struct i2o_device *tmp;
+
+ sysfs_remove_link(&i2o_dev->device.kobj, "parent");
+ sysfs_remove_link(&i2o_dev->device.kobj, "user");
+
+ list_for_each_entry(tmp, &c->devices, list) {
+ if (tmp->lct_data.parent_tid == i2o_dev->lct_data.tid)
+ sysfs_remove_link(&tmp->device.kobj, "parent");
+ if (tmp->lct_data.user_tid == i2o_dev->lct_data.tid)
+ sysfs_remove_link(&tmp->device.kobj, "user");
+ }
+}
+
+
/**
* i2o_device_add - allocate a new I2O device and add it to the IOP
@@ -222,6 +306,7 @@ static struct i2o_device *i2o_device_add
}
dev->lct_data = *entry;
+ dev->iop = c;
snprintf(dev->device.bus_id, BUS_ID_SIZE, "%d:%03x", c->unit,
dev->lct_data.tid);
@@ -229,7 +314,6 @@ static struct i2o_device *i2o_device_add
snprintf(dev->classdev.class_id, BUS_ID_SIZE, "%d:%03x", c->unit,
dev->lct_data.tid);
- dev->iop = c;
dev->device.parent = &c->device;
device_register(&dev->device);
@@ -238,12 +322,14 @@ static struct i2o_device *i2o_device_add
class_device_register(&dev->classdev);
+ i2o_setup_sysfs_links(dev);
+
i2o_driver_notify_device_add_all(dev);
pr_debug("i2o: device %s added\n", dev->device.bus_id);
return dev;
-};
+}
/**
* i2o_device_remove - remove an I2O device from the I2O core
@@ -256,10 +342,11 @@ static struct i2o_device *i2o_device_add
void i2o_device_remove(struct i2o_device *i2o_dev)
{
i2o_driver_notify_device_remove_all(i2o_dev);
+ i2o_remove_sysfs_links(i2o_dev);
class_device_unregister(&i2o_dev->classdev);
list_del(&i2o_dev->list);
device_unregister(&i2o_dev->device);
-};
+}
/**
* i2o_device_parse_lct - Parse a previously fetched LCT and create devices
@@ -337,99 +424,8 @@ int i2o_device_parse_lct(struct i2o_cont
up(&c->lct_lock);
return 0;
-};
-
-/**
- * i2o_device_class_show_class_id - Displays class id of I2O device
- * @cd: class device of which the class id should be displayed
- * @buf: buffer into which the class id should be printed
- *
- * Returns the number of bytes which are printed into the buffer.
- */
-static ssize_t i2o_device_class_show_class_id(struct class_device *cd,
- char *buf)
-{
- struct i2o_device *dev = to_i2o_device(cd->dev);
-
- sprintf(buf, "0x%03x\n", dev->lct_data.class_id);
- return strlen(buf) + 1;
-};
-
-/**
- * i2o_device_class_show_tid - Displays TID of I2O device
- * @cd: class device of which the TID should be displayed
- * @buf: buffer into which the class id should be printed
- *
- * Returns the number of bytes which are printed into the buffer.
- */
-static ssize_t i2o_device_class_show_tid(struct class_device *cd, char *buf)
-{
- struct i2o_device *dev = to_i2o_device(cd->dev);
-
- sprintf(buf, "0x%03x\n", dev->lct_data.tid);
- return strlen(buf) + 1;
-};
-
-/* I2O device class attributes */
-static CLASS_DEVICE_ATTR(class_id, S_IRUGO, i2o_device_class_show_class_id,
- NULL);
-static CLASS_DEVICE_ATTR(tid, S_IRUGO, i2o_device_class_show_tid, NULL);
-
-/**
- * i2o_device_class_add - Adds attributes to the I2O device
- * @cd: I2O class device which is added to the I2O device class
- *
- * This function get called when a I2O device is added to the class. It
- * creates the attributes for each device and creates user/parent symlink
- * if necessary.
- *
- * Returns 0 on success or negative error code on failure.
- */
-static int i2o_device_class_add(struct class_device *cd)
-{
- struct i2o_device *i2o_dev, *tmp;
- struct i2o_controller *c;
-
- i2o_dev = to_i2o_device(cd->dev);
- c = i2o_dev->iop;
-
- class_device_create_file(cd, &class_device_attr_class_id);
- class_device_create_file(cd, &class_device_attr_tid);
-
- /* create user entries for this device */
- tmp = i2o_iop_find_device(i2o_dev->iop, i2o_dev->lct_data.user_tid);
- if (tmp && (tmp != i2o_dev))
- sysfs_create_link(&i2o_dev->device.kobj, &tmp->device.kobj,
- "user");
-
- /* create user entries refering to this device */
- list_for_each_entry(tmp, &c->devices, list)
- if ((tmp->lct_data.user_tid == i2o_dev->lct_data.tid)
- && (tmp != i2o_dev))
- sysfs_create_link(&tmp->device.kobj,
- &i2o_dev->device.kobj, "user");
-
- /* create parent entries for this device */
- tmp = i2o_iop_find_device(i2o_dev->iop, i2o_dev->lct_data.parent_tid);
- if (tmp && (tmp != i2o_dev))
- sysfs_create_link(&i2o_dev->device.kobj, &tmp->device.kobj,
- "parent");
-
- /* create parent entries refering to this device */
- list_for_each_entry(tmp, &c->devices, list)
- if ((tmp->lct_data.parent_tid == i2o_dev->lct_data.tid)
- && (tmp != i2o_dev))
- sysfs_create_link(&tmp->device.kobj,
- &i2o_dev->device.kobj, "parent");
-
- return 0;
-};
+}
-/* I2O device class interface */
-static struct class_interface i2o_device_class_interface = {
- .class = &i2o_device_class,
- .add = i2o_device_class_add
-};
/*
* Run time support routines
@@ -553,11 +549,11 @@ int i2o_parm_field_get(struct i2o_device
}
/*
- * if oper == I2O_PARAMS_TABLE_GET, get from all rows
- * if fieldcount == -1 return all fields
+ * if oper == I2O_PARAMS_TABLE_GET, get from all rows
+ * if fieldcount == -1 return all fields
* ibuf and ibuflen are unused (use NULL, 0)
- * else return specific fields
- * ibuf contains fieldindexes
+ * else return specific fields
+ * ibuf contains fieldindexes
*
* if oper == I2O_PARAMS_LIST_GET, get from specific rows
* if fieldcount == -1 return all fields
@@ -611,14 +607,8 @@ int i2o_parm_table_get(struct i2o_device
*/
int i2o_device_init(void)
{
- int rc;
-
- rc = class_register(&i2o_device_class);
- if (rc)
- return rc;
-
- return class_interface_register(&i2o_device_class_interface);
-};
+ return class_register(&i2o_device_class);
+}
/**
* i2o_device_exit - I2O devices exit function
@@ -627,9 +617,8 @@ int i2o_device_init(void)
*/
void i2o_device_exit(void)
{
- class_interface_register(&i2o_device_class_interface);
class_unregister(&i2o_device_class);
-};
+}
EXPORT_SYMBOL(i2o_device_claim);
EXPORT_SYMBOL(i2o_device_claim_release);
Hello,
Dmitry Torokhov wrote:
> I was looking at the users of class_interfaces and stumbled across
> I2O subsystem. As far as I understand the purpose of class interfaces
> was to provide different 'views' on the hardware, not just to have
> a callback to finish initialization of sysfs structures. I think it
> woudl be better to remove i2o_device_class_interface and create
> user/parent links right after class device registration.
OK, think i've misunderstood the description of the class interface :-(
> Also, it looks like i2o_device_class itself is not needed - correct
> me if I am wrong, but all i2o devics reside on their own bus so
> i2o_devices class simply mirrors iformation from the bus and can
> also be safely removed.
Nope, there is one bus per controller not per device...
> Please consider applying the 2 pathes below (just compile-tested,
> don't have proper hardware).
I'll try to merge the changes (and some other patches) in the near future
and provide a changed patch...
Thank you very much.
Best regards,
Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)
Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany
Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11
E-Mail: [email protected]
URL: http://www.shadowconnect.com
On 9/13/05, Markus Lidel <[email protected]> wrote:
> > Also, it looks like i2o_device_class itself is not needed - correct
> > me if I am wrong, but all i2o devics reside on their own bus so
> > i2o_devices class simply mirrors iformation from the bus and can
> > also be safely removed.
>
> Nope, there is one bus per controller not per device...
>
That is what I was trying to say. Well, not exactly... What I was
really trying to say is AFAIKS I2O system registers only one sysfs bus
object and all I2O devices reside on it. Unlike, for exaple input
objects, that can appear on serio, gameport, usb buses and so on. So
if one wants to see all I2O devices in sysfs he could just check
/sys/bus/i2o/devices/ and see them all there.
--
Dmitry