Very basic sysfs support for ATM devices (updated)

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
@@ -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/common.c linux-2.6.10/net/atm/common.c
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

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 [email protected]
# [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
#

# net/atm/atm_sysfs.c
#   2005/02/04 13:05:17-05:00 [email protected] +181 -0
# 
# net/atm/resources.h
#   2005/02/04 13:05:17-05:00 [email protected] +3 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
# net/atm/resources.c
#   2005/02/04 13:05:17-05:00 [email protected] +22 -6
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
# net/atm/atm_sysfs.c
#   2005/02/04 13:05:17-05:00 [email protected] +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 [email protected] +2 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
# net/atm/common.c
#   2005/02/04 13:05:16-05:00 [email protected] +6 -1
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
# net/atm/Makefile
#   2005/02/04 13:05:16-05:00 [email protected] +1 -1
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
# include/linux/atmdev.h
#   2005/02/04 13:05:16-05:00 [email protected] +2 -0
#   [ATM]: basic sysfs support for ATM devices (from Roman Kagan <[email protected]>)
# 
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
@@ -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/common.c b/net/atm/common.c
@@ -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
@@ -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
@@ -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
@@ -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

Hi,

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

Hi Chas,

I'm fine with it.

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.

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

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
@@ -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
@@ -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
@@ -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

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

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.

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.

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.

Hi Roman,

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.

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.