[RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Roman Kagan]

  Hi,

Here's a quick patch to add very basic sysfs support to ATM devices.  It
creates class/atm and provides a couple of attributes for each ATM
device and hotplug callout for device addition/removal, which was the
main motivation for me to write it up.

With this patch, an utility to set up a network interface on the newly
registered ATM device can be called automatically via hotplug, just as
it is done with net_device.

Compared to the first version sent to linux-atm-general mailing list, it
fixes the accidentally sneaked in bug using class_device_unregister
instead of class_device_del, and replaces mutual references of
class_device and atm_dev with container_of.

The patch is against 2.6.10.  Please comment.

Thanks,
  Roman.

 include/linux/atmdev.h |    2
 net/atm/Makefile       |    2
 net/atm/atm_sysfs.c    |  113 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/atm/common.c       |    7 ++-
 net/atm/common.h       |    2
 net/atm/resources.c    |   31 ++++++++++---
 net/atm/resources.h    |    3 +
 7 files changed, 152 insertions(+), 8 deletions(-)


diff -rupN -x '*~' linux-2.6.10.orig/include/linux/atmdev.h linux-2.6.10/include/linux/atmdev.h
--- linux-2.6.10.orig/include/linux/atmdev.h	2004-12-25 00:35:24.000000000 +0300
+++ linux-2.6.10/include/linux/atmdev.h	2005-01-19 16:44:09.000000000 +0300
@@ -8,6 +8,7 @@
 
 
 #include <linux/config.h>
+#include <linux/device.h>
 #include <linux/atmapi.h>
 #include <linux/atm.h>
 #include <linux/atmioc.h>
@@ -337,6 +338,7 @@ struct atm_dev {
 	struct proc_dir_entry *proc_entry; /* proc entry */
 	char *proc_name;		/* proc entry name */
 #endif
+	struct class_device class_dev;	/* sysfs class device */
 	struct list_head dev_list;	/* linkage */
 };
 
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/atm_sysfs.c linux-2.6.10/net/atm/atm_sysfs.c
--- linux-2.6.10.orig/net/atm/atm_sysfs.c	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6.10/net/atm/atm_sysfs.c	2005-01-21 04:03:46.000000000 +0300
@@ -0,0 +1,113 @@
+/* ATM driver model support. */
+
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/atmdev.h>
+#include "common.h"
+#include "resources.h"
+
+#define to_atm_dev(cldev) container_of(cldev, struct atm_dev, class_dev)
+
+static ssize_t show_type(struct class_device *cdev, char *buf)
+{
+	struct atm_dev *adev = to_atm_dev(cdev);
+	return sprintf(buf, "%s\n", adev->type);
+}
+
+static ssize_t show_address(struct class_device *cdev, char *buf)
+{
+	int i;
+	char *pos = buf;
+	struct atm_dev *adev = to_atm_dev(cdev);
+
+	for (i = 0; i < sizeof(adev->esi); i++)
+		pos += sprintf(pos, "%02x:", adev->esi[i]);
+	pos += sprintf(pos, "%02x\n", adev->esi[i]);
+
+	return pos - buf;
+}
+
+static CLASS_DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
+static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
+
+static struct class_device_attribute *atm_attrs[] = {
+	&class_device_attr_type,
+	&class_device_attr_address,
+	NULL
+};
+
+#ifdef CONFIG_HOTPLUG
+static int atm_hotplug(struct class_device *cdev, char **envp, int num_envp, char *buf, int size)
+{
+	struct atm_dev *adev;
+	int i = 0;
+	int length = 0;
+
+	if (!cdev)
+		return -ENODEV;
+
+	adev = to_atm_dev(cdev);
+	if (!adev)
+		return -ENODEV;
+
+        if (add_hotplug_env_var(envp, num_envp, &i, buf, size, &length,
+				"INTERFACE=%d", adev->number))
+		return -ENOMEM;
+
+	envp[i] = NULL;
+	return 0;
+}
+#endif
+
+static void atm_release(struct class_device *cdev)
+{
+	struct atm_dev *adev = to_atm_dev(cdev);
+
+	kfree(adev);
+}
+
+static struct class atm_class = {
+	.name		= "atm",
+	.release	= atm_release,
+#ifdef CONFIG_HOTPLUG
+	.hotplug	= atm_hotplug,
+#endif
+};
+
+int atm_register_sysfs(struct atm_dev *adev)
+{
+	struct class_device *cdev = &adev->class_dev;
+	int i, err;
+
+	cdev->class = &atm_class;
+	class_set_devdata(cdev, adev);
+
+	snprintf(cdev->class_id, BUS_ID_SIZE, "%d", adev->number);
+	err = class_device_register(cdev);
+	if (err < 0)
+		return err;
+
+	for (i = 0; atm_attrs[i]; i++)
+		class_device_create_file(cdev, atm_attrs[i]);
+
+	return 0;
+}
+
+void atm_unregister_sysfs(struct atm_dev *adev)
+{
+	struct class_device *cdev = &adev->class_dev;
+
+	class_device_del(cdev);
+}
+
+int __init atm_sysfs_init(void)
+{
+	return class_register(&atm_class);
+}
+
+void __exit atm_sysfs_exit(void)
+{
+	class_unregister(&atm_class);
+}
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/common.c linux-2.6.10/net/atm/common.c
--- linux-2.6.10.orig/net/atm/common.c	2004-12-25 00:35:50.000000000 +0300
+++ linux-2.6.10/net/atm/common.c	2005-01-19 17:01:26.000000000 +0300
@@ -793,6 +793,10 @@ static int __init atm_init(void)
 		printk(KERN_ERR "atm_proc_init() failed with %d\n",error);
 		goto failure;
 	}
+        if ((error = atm_sysfs_init()) < 0) {
+		printk(KERN_ERR "atm_sysfs_init() failed with %d\n",error);
+		goto failure;
+	}
 	return 0;
 
 failure:
@@ -804,11 +808,12 @@ failure:
 static void __exit atm_exit(void)
 {
 	atm_proc_exit();
+	atm_sysfs_exit();
 	atmsvc_exit();
 	atmpvc_exit();
 }
 
-module_init(atm_init);
+subsys_initcall(atm_init);
 module_exit(atm_exit);
 
 MODULE_LICENSE("GPL");
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/common.h linux-2.6.10/net/atm/common.h
--- linux-2.6.10.orig/net/atm/common.h	2004-12-25 00:35:23.000000000 +0300
+++ linux-2.6.10/net/atm/common.h	2005-01-19 16:09:16.000000000 +0300
@@ -28,6 +28,8 @@ int atmpvc_init(void);
 void atmpvc_exit(void);
 int atmsvc_init(void);
 void atmsvc_exit(void);
+int atm_sysfs_init(void);
+void atm_sysfs_exit(void);
 
 #ifdef CONFIG_PROC_FS
 int atm_proc_init(void);
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/Makefile linux-2.6.10/net/atm/Makefile
--- linux-2.6.10.orig/net/atm/Makefile	2004-12-25 00:34:00.000000000 +0300
+++ linux-2.6.10/net/atm/Makefile	2005-01-19 17:04:29.000000000 +0300
@@ -2,7 +2,7 @@
 # Makefile for the ATM Protocol Families.
 #
 
-atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o
+atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o atm_sysfs.o
 mpoa-objs	:= mpc.o mpoa_caches.o mpoa_proc.o
 
 obj-$(CONFIG_ATM) += atm.o
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/resources.c linux-2.6.10/net/atm/resources.c
--- linux-2.6.10.orig/net/atm/resources.c	2004-12-25 00:35:29.000000000 +0300
+++ linux-2.6.10/net/atm/resources.c	2005-01-21 04:03:01.000000000 +0300
@@ -47,6 +47,12 @@ static void __free_atm_dev(struct atm_de
 	kfree(dev);
 }
 
+static void release_atm_dev(struct atm_dev *dev)
+{
+	/* will free via class release */
+	class_device_put(&dev->class_dev);
+}
+
 static struct atm_dev *__atm_dev_lookup(int number)
 {
 	struct atm_dev *dev;
@@ -114,14 +120,25 @@ struct atm_dev *atm_dev_register(const c
 		printk(KERN_ERR "atm_dev_register: "
 		       "atm_proc_dev_register failed for dev %s\n",
 		       type);
-		spin_lock(&atm_dev_lock);
-		list_del(&dev->dev_list);
-		spin_unlock(&atm_dev_lock);
-		__free_atm_dev(dev);
-		return NULL;
+		goto reg_fail;
+	}
+
+	if (atm_register_sysfs(dev) < 0) {
+		printk(KERN_ERR "atm_dev_register: "
+		       "atm_register_sysfs failed for dev %s\n",
+		       type);
+		atm_proc_dev_deregister(dev);
+		goto reg_fail;
 	}
 
 	return dev;
+	
+reg_fail:
+	spin_lock(&atm_dev_lock);
+	list_del(&dev->dev_list);
+	spin_unlock(&atm_dev_lock);
+	__free_atm_dev(dev);
+	return NULL;
 }
 
 
@@ -129,6 +146,8 @@ void atm_dev_deregister(struct atm_dev *
 {
 	unsigned long warning_time;
 
+	atm_unregister_sysfs(dev);
+
 	atm_proc_dev_deregister(dev);
 
 	spin_lock(&atm_dev_lock);
@@ -147,7 +166,7 @@ void atm_dev_deregister(struct atm_dev *
                 }
         }
 
-	__free_atm_dev(dev);
+	release_atm_dev(dev);
 }
 
 void shutdown_atm_dev(struct atm_dev *dev)
diff -rupN -x '*~' linux-2.6.10.orig/net/atm/resources.h linux-2.6.10/net/atm/resources.h
--- linux-2.6.10.orig/net/atm/resources.h	2004-12-25 00:36:01.000000000 +0300
+++ linux-2.6.10/net/atm/resources.h	2005-01-19 16:55:20.000000000 +0300
@@ -43,4 +43,7 @@ static inline void atm_proc_dev_deregist
 
 #endif /* CONFIG_PROC_FS */
 
+int atm_register_sysfs(struct atm_dev *adev);
+void atm_unregister_sysfs(struct atm_dev *adev);
+
 #endif

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[chas williams - CONTRACTOR]

In message <20050121085123.GA2471@katya>,Roman Kagan writes:
>The patch is against 2.6.10.  Please comment.

i guess i would prefer that entries be named typeN, like he0
instead of just a number.

you were printing 7 octets for the address.

if !CONFIG_SYSFS, then __free_atm_dev() is going to need to do the
kfree.

i added some other fields that i think are interesting.

comments?


# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/04 13:05:34-05:00 chas@relax.cmf.nrl.navy.mil 
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# net/atm/atm_sysfs.c
#   2005/02/04 13:05:17-05:00 chas@relax.cmf.nrl.navy.mil +181 -0
# 
# net/atm/resources.h
#   2005/02/04 13:05:17-05:00 chas@relax.cmf.nrl.navy.mil +3 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# net/atm/resources.c
#   2005/02/04 13:05:17-05:00 chas@relax.cmf.nrl.navy.mil +22 -6
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# net/atm/atm_sysfs.c
#   2005/02/04 13:05:17-05:00 chas@relax.cmf.nrl.navy.mil +0 -0
#   BitKeeper file /scratch/chas/LATEST/2.6/net/atm/atm_sysfs.c
# 
# net/atm/common.h
#   2005/02/04 13:05:16-05:00 chas@relax.cmf.nrl.navy.mil +2 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# net/atm/common.c
#   2005/02/04 13:05:16-05:00 chas@relax.cmf.nrl.navy.mil +6 -1
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# net/atm/Makefile
#   2005/02/04 13:05:16-05:00 chas@relax.cmf.nrl.navy.mil +1 -1
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
# include/linux/atmdev.h
#   2005/02/04 13:05:16-05:00 chas@relax.cmf.nrl.navy.mil +2 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <rkagan@mail.ru>)
# 
diff -Nru a/include/linux/atmdev.h b/include/linux/atmdev.h
--- a/include/linux/atmdev.h	2005-02-04 13:06:48 -05:00
+++ b/include/linux/atmdev.h	2005-02-04 13:06:48 -05:00
@@ -8,6 +8,7 @@
 
 
 #include <linux/config.h>
+#include <linux/device.h>
 #include <linux/atmapi.h>
 #include <linux/atm.h>
 #include <linux/atmioc.h>
@@ -345,6 +346,7 @@
 	struct proc_dir_entry *proc_entry; /* proc entry */
 	char *proc_name;		/* proc entry name */
 #endif
+	struct class_device class_dev;	/* sysfs class device */
 	struct list_head dev_list;	/* linkage */
 };
 
diff -Nru a/net/atm/Makefile b/net/atm/Makefile
--- a/net/atm/Makefile	2005-02-04 13:06:48 -05:00
+++ b/net/atm/Makefile	2005-02-04 13:06:48 -05:00
@@ -2,7 +2,7 @@
 # Makefile for the ATM Protocol Families.
 #
 
-atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o
+atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o atm_sysfs.o
 mpoa-objs	:= mpc.o mpoa_caches.o mpoa_proc.o
 
 obj-$(CONFIG_ATM) += atm.o
diff -Nru a/net/atm/atm_sysfs.c b/net/atm/atm_sysfs.c
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/net/atm/atm_sysfs.c	2005-02-04 13:06:48 -05:00
@@ -0,0 +1,181 @@
+/* ATM driver model support. */
+
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/atmdev.h>
+#include "common.h"
+#include "resources.h"
+
+#define to_atm_dev(cldev) container_of(cldev, struct atm_dev, class_dev)
+
+static ssize_t show_type(struct class_device *cdev, char *buf)
+{
+	struct atm_dev *adev = to_atm_dev(cdev);
+	return sprintf(buf, "%s\n", adev->type);
+}
+
+static ssize_t show_address(struct class_device *cdev, char *buf)
+{
+	char *pos = buf;
+	struct atm_dev *adev = to_atm_dev(cdev);
+	int i;
+
+	for (i = 0; i < (ESI_LEN - 1); i++)
+		pos += sprintf(pos, "%02x:", adev->esi[i]);
+	pos += sprintf(pos, "%02x\n", adev->esi[i]);
+
+	return pos - buf;
+}
+
+static ssize_t show_atmaddress(struct class_device *cdev, char *buf)
+{
+        unsigned long flags;
+	char *pos = buf;
+	struct atm_dev *adev = to_atm_dev(cdev);
+        struct atm_dev_addr *aaddr;
+	int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin;
+	int i, j;
+
+        spin_lock_irqsave(&adev->lock, flags);
+        list_for_each_entry(aaddr, &adev->local, entry) {
+		for(i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) {
+			if (j == *fmt) {
+				pos += sprintf(pos, ".");
+				++fmt;
+				j = 0;
+			}
+			pos += sprintf(pos, "%02x", aaddr->addr.sas_addr.prv[i]);
+		}
+		pos += sprintf(pos, "\n");
+	}
+        spin_unlock_irqrestore(&adev->lock, flags);
+
+	return pos - buf;
+}
+
+static ssize_t show_carrier(struct class_device *cdev, char *buf)
+{
+	char *pos = buf;
+	struct atm_dev *adev = to_atm_dev(cdev);
+
+	pos += sprintf(pos, "%d\n",
+		       adev->signal == ATM_PHY_SIG_LOST ? 0 : 1);
+		
+	return pos - buf;
+}
+
+static ssize_t show_link_rate(struct class_device *cdev, char *buf)
+{
+	char *pos = buf;
+	struct atm_dev *adev = to_atm_dev(cdev);
+	int link_rate;
+
+	/* show the link rate, not the data rate */
+	switch (adev->link_rate) {
+		case ATM_OC3_PCR:
+			link_rate = 155520000;
+			break;
+		case ATM_OC12_PCR:
+			link_rate = 622080000;
+			break;
+		case ATM_25_PCR:
+			link_rate = 25600000;
+			break;
+		default:
+			link_rate = adev->link_rate * 8 * 53;
+	}
+	pos += sprintf(pos, "%d\n", link_rate);
+		
+	return pos - buf;
+}
+
+static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
+static CLASS_DEVICE_ATTR(atmaddress, S_IRUGO, show_atmaddress, NULL);
+static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
+static CLASS_DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
+static CLASS_DEVICE_ATTR(link_rate, S_IRUGO, show_link_rate, NULL);
+
+static struct class_device_attribute *atm_attrs[] = {
+	&class_device_attr_atmaddress,
+	&class_device_attr_address,
+	&class_device_attr_carrier,
+	&class_device_attr_type,
+	&class_device_attr_link_rate,
+	NULL
+};
+
+#ifdef CONFIG_HOTPLUG
+static int atm_hotplug(struct class_device *cdev, char **envp, int num_envp, char *buf, int size)
+{
+	struct atm_dev *adev;
+	int i = 0;
+	int length = 0;
+
+	if (!cdev)
+		return -ENODEV;
+
+	adev = to_atm_dev(cdev);
+	if (!adev)
+		return -ENODEV;
+
+        if (add_hotplug_env_var(envp, num_envp, &i, buf, size, &length,
+				"INTERFACE=%s%d", adev->type, adev->number))
+		return -ENOMEM;
+
+	envp[i] = NULL;
+	return 0;
+}
+#endif
+
+static void atm_release(struct class_device *cdev)
+{
+	struct atm_dev *adev = to_atm_dev(cdev);
+
+	kfree(adev);
+}
+
+static struct class atm_class = {
+	.name		= "atm",
+	.release	= atm_release,
+#ifdef CONFIG_HOTPLUG
+	.hotplug	= atm_hotplug,
+#endif
+};
+
+int atm_register_sysfs(struct atm_dev *adev)
+{
+	struct class_device *cdev = &adev->class_dev;
+	int i, err;
+
+	cdev->class = &atm_class;
+	class_set_devdata(cdev, adev);
+
+	snprintf(cdev->class_id, BUS_ID_SIZE, "%s%d", adev->type, adev->number);
+	err = class_device_register(cdev);
+	if (err < 0)
+		return err;
+
+	for (i = 0; atm_attrs[i]; i++)
+		class_device_create_file(cdev, atm_attrs[i]);
+
+	return 0;
+}
+
+void atm_unregister_sysfs(struct atm_dev *adev)
+{
+	struct class_device *cdev = &adev->class_dev;
+
+	class_device_del(cdev);
+}
+
+int __init atm_sysfs_init(void)
+{
+	return class_register(&atm_class);
+}
+
+void __exit atm_sysfs_exit(void)
+{
+	class_unregister(&atm_class);
+}
diff -Nru a/net/atm/common.c b/net/atm/common.c
--- a/net/atm/common.c	2005-02-04 13:06:48 -05:00
+++ b/net/atm/common.c	2005-02-04 13:06:48 -05:00
@@ -793,6 +793,10 @@
 		printk(KERN_ERR "atm_proc_init() failed with %d\n",error);
 		goto failure;
 	}
+        if ((error = atm_sysfs_init()) < 0) {
+		printk(KERN_ERR "atm_sysfs_init() failed with %d\n",error);
+		goto failure;
+	}
 	return 0;
 
 failure:
@@ -804,11 +808,12 @@
 static void __exit atm_exit(void)
 {
 	atm_proc_exit();
+	atm_sysfs_exit();
 	atmsvc_exit();
 	atmpvc_exit();
 }
 
-module_init(atm_init);
+subsys_initcall(atm_init);
 module_exit(atm_exit);
 
 MODULE_LICENSE("GPL");
diff -Nru a/net/atm/common.h b/net/atm/common.h
--- a/net/atm/common.h	2005-02-04 13:06:48 -05:00
+++ b/net/atm/common.h	2005-02-04 13:06:48 -05:00
@@ -28,6 +28,8 @@
 void atmpvc_exit(void);
 int atmsvc_init(void);
 void atmsvc_exit(void);
+int atm_sysfs_init(void);
+void atm_sysfs_exit(void);
 
 #ifdef CONFIG_PROC_FS
 int atm_proc_init(void);
diff -Nru a/net/atm/resources.c b/net/atm/resources.c
--- a/net/atm/resources.c	2005-02-04 13:06:48 -05:00
+++ b/net/atm/resources.c	2005-02-04 13:06:48 -05:00
@@ -47,7 +47,11 @@
 
 static void __free_atm_dev(struct atm_dev *dev)
 {
+#ifdef CONFIG_SYSFS
+	class_device_put(&dev->class_dev);
+#else
 	kfree(dev);
+#endif
 }
 
 static struct atm_dev *__atm_dev_lookup(int number)
@@ -91,7 +95,7 @@
 		if ((inuse = __atm_dev_lookup(number))) {
 			atm_dev_put(inuse);
 			spin_unlock(&atm_dev_lock);
-			__free_atm_dev(dev);
+			kfree(dev);
 			return NULL;
 		}
 		dev->number = number;
@@ -117,14 +121,25 @@
 		printk(KERN_ERR "atm_dev_register: "
 		       "atm_proc_dev_register failed for dev %s\n",
 		       type);
-		spin_lock(&atm_dev_lock);
-		list_del(&dev->dev_list);
-		spin_unlock(&atm_dev_lock);
-		__free_atm_dev(dev);
-		return NULL;
+		goto reg_fail;
+	}
+
+	if (atm_register_sysfs(dev) < 0) {
+		printk(KERN_ERR "atm_dev_register: "
+		       "atm_register_sysfs failed for dev %s\n",
+		       type);
+		atm_proc_dev_deregister(dev);
+		goto reg_fail;
 	}
 
 	return dev;
+	
+reg_fail:
+	spin_lock(&atm_dev_lock);
+	list_del(&dev->dev_list);
+	spin_unlock(&atm_dev_lock);
+	kfree(dev);
+	return NULL;
 }
 
 
@@ -132,6 +147,7 @@
 {
 	unsigned long warning_time;
 
+	atm_unregister_sysfs(dev);
 	atm_proc_dev_deregister(dev);
 
 	spin_lock(&atm_dev_lock);
diff -Nru a/net/atm/resources.h b/net/atm/resources.h
--- a/net/atm/resources.h	2005-02-04 13:06:48 -05:00
+++ b/net/atm/resources.h	2005-02-04 13:06:48 -05:00
@@ -43,4 +43,7 @@
 
 #endif /* CONFIG_PROC_FS */
 
+int atm_register_sysfs(struct atm_dev *adev);
+void atm_unregister_sysfs(struct atm_dev *adev);
+
 #endif

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[matthieu castet]

Hi,

chas williams - CONTRACTOR wrote:
> In message <20050121085123.GA2471@katya>,Roman Kagan writes:
> 
>>The patch is against 2.6.10.  Please comment.
> 
> 
> i guess i would prefer that entries be named typeN, like he0
> instead of just a number.
> 
> you were printing 7 octets for the address.
> 
> if !CONFIG_SYSFS, then __free_atm_dev() is going to need to do the
> kfree.
> 
> i added some other fields that i think are interesting.
> 
> comments?
> 
> +
> +static ssize_t show_carrier(struct class_device *cdev, char *buf)
> +{
> +	char *pos = buf;
> +	struct atm_dev *adev = to_atm_dev(cdev);
> +
> +	pos += sprintf(pos, "%d\n",
> +		       adev->signal == ATM_PHY_SIG_LOST ? 0 : 1);
> +		
> +	return pos - buf;
> +}
Shouldn't adev->signal == ATM_PHY_SIG_FOUND ? 1 : 0); be better : in the 
case of ATM_PHY_SIG_UNKNOWN, it return 1...


Matthieu

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[chas williams - CONTRACTOR]

In message <4203BE7B.2030503@free.fr>,matthieu castet writes:
>Shouldn't adev->signal == ATM_PHY_SIG_FOUND ? 1 : 0); be better : in the 
>case of ATM_PHY_SIG_UNKNOWN, it return 1...

its intentional.  if the atm device cant track the state of the link
its probably best to just assume its up.

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Roman Kagan]

  Hi Chas,

On Fri, Feb 04, 2005 at 01:11:25PM -0500, chas williams - CONTRACTOR wrote:
> In message <20050121085123.GA2471@katya>,Roman Kagan writes:
> >The patch is against 2.6.10.  Please comment.
> 
> i guess i would prefer that entries be named typeN, like he0
> instead of just a number.

I'm fine with it.

> if !CONFIG_SYSFS, then __free_atm_dev() is going to need to do the
> kfree.

Right, I forgot that sysfs was still optional.  Below is the patch on
top of yours to add this conditional to a few other places.

> i added some other fields that i think are interesting.

As a matter of fact that's what I hoped for :)

> +static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);

Maybe it should better be "esi", to match the name in struct atm_dev?

Besides, I've encountered a problem with hotplug being called from
atm_dev_register: the device actually becomes usable only when the
driver sets .dev_data, which may happen after hotplug has been called.
E.g. if I call br2684ctl in my hotplug script, and it happens to attempt
to open a socket on the device before .dev_data is set, br2684ctl fails
(rather disgracefully BTW, forgetting to cleanup nasX).  I had to work
around it by adding a sleep for a couple of seconds to the hotplug
script, and hoping that was enough for the driver to complete
initializing atm_dev.  I suspect the only way to fix it is to split the
atm_dev initialization into two stages, allocation and registration, as
it is done for net_device.

Cheers,
  Roman.

diff -ruNp linux-2.6.10.atm/net/atm/common.h linux-2.6.10.atm.new/net/atm/common.h
--- linux-2.6.10.atm/net/atm/common.h	2005-02-04 21:46:52.084591328 +0300
+++ linux-2.6.10.atm.new/net/atm/common.h	2005-02-04 22:19:41.962124424 +0300
@@ -28,8 +28,21 @@ int atmpvc_init(void);
 void atmpvc_exit(void);
 int atmsvc_init(void);
 void atmsvc_exit(void);
+
+#ifdef CONFIG_SYSFS
 int atm_sysfs_init(void);
 void atm_sysfs_exit(void);
+#else
+static inline int atm_sysfs_init(void)
+{
+	return 0;
+}
+
+static inline void atm_sysfs_exit(void)
+{
+	/* nothing */
+}
+#endif /* CONFIG_SYSFS */
 
 #ifdef CONFIG_PROC_FS
 int atm_proc_init(void);
diff -ruNp linux-2.6.10.atm/net/atm/Makefile linux-2.6.10.atm.new/net/atm/Makefile
--- linux-2.6.10.atm/net/atm/Makefile	2005-02-04 21:46:51.966609264 +0300
+++ linux-2.6.10.atm.new/net/atm/Makefile	2005-02-04 22:19:07.025435608 +0300
@@ -2,7 +2,7 @@
 # Makefile for the ATM Protocol Families.
 #
 
-atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o atm_sysfs.o
+atm-y		:= addr.o pvc.o signaling.o svc.o ioctl.o common.o atm_misc.o raw.o resources.o
 mpoa-objs	:= mpc.o mpoa_caches.o mpoa_proc.o
 
 obj-$(CONFIG_ATM) += atm.o
@@ -12,6 +12,7 @@ obj-$(CONFIG_ATM_BR2684) += br2684.o
 atm-$(subst m,y,$(CONFIG_ATM_BR2684)) += ipcommon.o
 atm-$(subst m,y,$(CONFIG_NET_SCH_ATM)) += ipcommon.o
 atm-$(CONFIG_PROC_FS) += proc.o
+atm-$(CONFIG_SYSFS) += atm_sysfs.o
 
 obj-$(CONFIG_ATM_LANE) += lec.o
 obj-$(CONFIG_ATM_MPOA) += mpoa.o
diff -ruNp linux-2.6.10.atm/net/atm/resources.h linux-2.6.10.atm.new/net/atm/resources.h
--- linux-2.6.10.atm/net/atm/resources.h	2005-02-04 21:46:52.263564120 +0300
+++ linux-2.6.10.atm.new/net/atm/resources.h	2005-02-04 22:19:19.766498672 +0300
@@ -43,7 +43,21 @@ static inline void atm_proc_dev_deregist
 
 #endif /* CONFIG_PROC_FS */
 
+#ifdef CONFIG_SYSFS
 int atm_register_sysfs(struct atm_dev *adev);
 void atm_unregister_sysfs(struct atm_dev *adev);
+#else
+
+static inline int atm_register_sysfs(struct atm_dev *dev)
+{
+	return 0;
+}
+
+static inline void atm_deregister_sysfs(struct atm_dev *dev)
+{
+	/* nothing */
+}
+
+#endif /* CONFIG_SYSFS */
 
 #endif

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[chas williams - CONTRACTOR]

In message <20050204201327.GA2439@katya>,Roman Kagan writes:
>> +static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
>Maybe it should better be "esi", to match the name in struct atm_dev?

its called address for ethernet network interfaces so i guess calling
it address here is sensible.

>script, and hoping that was enough for the driver to complete
>initializing atm_dev.  I suspect the only way to fix it is to split the
>atm_dev initialization into two stages, allocation and registration, as
>it is done for net_device.

yeah which is why i just wanted to convert atm_dev to just be a netdevice.
the code is already written and "stable".

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Roman Kagan]

On Sat, Feb 05, 2005 at 08:33:18PM -0500, chas williams - CONTRACTOR wrote:
> In message <20050204201327.GA2439@katya>,Roman Kagan writes:
> >initializing atm_dev.  I suspect the only way to fix it is to split the
> >atm_dev initialization into two stages, allocation and registration, as
> >it is done for net_device.
> 
> yeah which is why i just wanted to convert atm_dev to just be a netdevice.
> the code is already written and "stable".

Is it publically available?  Linux-ATM CVS appears to contain no
kernelspace code.  I'm just curious how atm_dev attributes are mapped to
net_device's, which doesn't look easy with the current atm_dev and
net_device.

Also, is it scheduled for inclusion in the mainline any time soon?  If
it is, maybe we shouldn't create an atm-specific user-visible interface
in sysfs and hotplug which is going to be obsoleted by the net generic
one in the near future?

Roman.

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Duncan Sands]

Hi Roman,

> Besides, I've encountered a problem with hotplug being called from
> atm_dev_register: the device actually becomes usable only when the
> driver sets .dev_data, which may happen after hotplug has been called.
> E.g. if I call br2684ctl in my hotplug script, and it happens to attempt
> to open a socket on the device before .dev_data is set, br2684ctl fails
> (rather disgracefully BTW, forgetting to cleanup nasX).  I had to work
> around it by adding a sleep for a couple of seconds to the hotplug
> script, and hoping that was enough for the driver to complete
> initializing atm_dev.  I suspect the only way to fix it is to split the
> atm_dev initialization into two stages, allocation and registration, as
> it is done for net_device.

this is a long-standing problem with ATM device initialisation: the device
becomes available before the struct atm_dev is initialised!  In theory a
driver's ATM methods could be called the moment atm_dev_register returns,
and before the driver has had a chance to fill in any fields.  That's why
all the ATM methods in the speedtouch driver check atm_dev->dev_data, and
return without doing anything if it is NULL: atm_dev->dev_data is only set
to a non-NULL value when the struct atm_dev has been fully initialised (this
is the reason for the memory barrier by the way).  When you say
"the device actually becomes usable only when the driver sets .dev_data",
I think that's a property of the speedtouch driver only.  With other drivers
it is probably worse: you get to open a connection even though the structure
is not initialized, and boom!  In the past, this race was only a theoretical
problem for SMP machines; but with your patch it hits everyone.

Ciao,

Duncan.

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Roman Kagan]

On Mon, Feb 07, 2005 at 10:03:08AM +0100, Duncan Sands wrote:
> this is a long-standing problem with ATM device initialisation: the device
> becomes available before the struct atm_dev is initialised!
> [...]
> In the past, this race was only a theoretical
> problem for SMP machines; but with your patch it hits everyone.

Indeed.  OTOH at a first glance splitting the ATM device initialisation
API into allocation and registration doesn't look a terribly difficult
thing, it would require fairly local changes in the drivers.

Unless Chas' move to struct net_device is alredy knocking on the door,
of course.

Cheers,
  Roman.

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[chas williams - CONTRACTOR]

In message <20050206191918.GA2369@katya>,Roman Kagan writes:
>Is it publically available?  Linux-ATM CVS appears to contain no
>kernelspace code.  I'm just curious how atm_dev attributes are mapped to
>net_device's, which doesn't look easy with the current atm_dev and
>net_device.

not really.  i just stuck attached the atm_dev to the netdevice's priv.
the only thing that really bothered me about this was that the atm
devices would show in an ifconfig.  this might confuse some people.

>it is, maybe we shouldn't create an atm-specific user-visible interface
>in sysfs and hotplug which is going to be obsoleted by the net generic
>one in the near future?

with sysfs support the only thing left to 'fix' would be splitting of
allocation/registration.

Re: [Linux-ATM-General] [RFC][PATCH] Very basic sysfs support for ATM devices (updated)

[Roman Kagan]

On Tue, Feb 08, 2005 at 10:02:19AM -0500, chas williams - CONTRACTOR wrote:
> >it is, maybe we shouldn't create an atm-specific user-visible interface
> >in sysfs and hotplug which is going to be obsoleted by the net generic
> >one in the near future?
> 
> with sysfs support the only thing left to 'fix' would be splitting of
> allocation/registration.

I beg your pardon, but are you saying that you've changed your mind re.
moving to net_device?  Is your plan now to cast in stone that ATM
devices live under class/atm in sysfs, and go ahead with splitting of
allocation/registration of the current atm_dev?

Roman.