some sysfs patches for 2.6.0 [0/4]

Here are 4 sysfs related patches for 2.6.0. They do the following:

- fix an oops in sysfs when a kobject has been unregistered before it's child has. An example of this is the oops that happens in 2.6.0 whenever a usb-serial device is removed that has a port open by a user (almost all Pilot devices do this by virtue of the protocol...)

- add sysfs support to the following char drivers:
- mem devices (/dev/null and friends)
- misc devices (/dev/psaux and other odd devices)
- vc devices

Please apply these to 2.6.0 or your -mm tree depending on how comfortable you are with them.

thanks,

greg k-h

This fixes an oops when a kobject is unregistered before it's child is. The usb-serial devices show this bug very easily (yank out a device while its port is opened...)

Patch was originally written by Mike Gorse <[email protected]>

diff -Nru a/fs/sysfs/dir.c b/fs/sysfs/dir.c
@@ -83,7 +83,8 @@
 	struct dentry * parent = dget(d->d_parent);
 	down(&parent->d_inode->i_sem);
 	d_delete(d);
-	simple_rmdir(parent->d_inode,d);
+	if (d->d_inode)
+		simple_rmdir(parent->d_inode,d);
 
 	pr_debug(" o %s removing done (%d)\n",d->d_name.name,
 		 atomic_read(&d->d_count));

This adds /sys/class/mem which enables all mem char devices to show up properly in udev.

Has been posted to linux-kernel every so often since last July, and acked by a number of other kernel developers.

diff -Nru a/drivers/char/mem.c b/drivers/char/mem.c
@@ -24,6 +24,7 @@
 #include <linux/smp_lock.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/ptrace.h>
+#include <linux/device.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -657,7 +658,7 @@
 	.open		= memory_open,	/* just a selector for the real open */
 };
 
-static const struct {
+static const struct mem_dev {
 	unsigned int		minor;
 	char			*name;
 	umode_t			mode;

@@ -676,6 +677,23 @@
 	{11,"kmsg",    S_IRUGO | S_IWUSR,           &kmsg_fops},
 };
 
+static void release_mem_dev(struct class_device *class_dev)
+{
+	kfree(class_dev);
+}
+
+static struct class mem_class = {
+	.name		= "mem",
+	.release	= &release_mem_dev,
+};
+
+static ssize_t show_dev(struct class_device *class_dev, char *buf)
+{
+	struct mem_dev *mem_dev = class_get_devdata(class_dev);
+	return print_dev_t(buf, MKDEV(MEM_MAJOR, mem_dev->minor));
+}
+static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
+
 static int __init chr_dev_init(void)
 {
 	int i;

@@ -683,7 +701,20 @@
 	if (register_chrdev(MEM_MAJOR,"mem",&memory_fops))
 		printk("unable to get major %d for memory devs\n", MEM_MAJOR);
 
+	class_register(&mem_class);
 	for (i = 0; i < ARRAY_SIZE(devlist); i++) {
+		struct class_device *class_dev;
+
+		class_dev = kmalloc(sizeof(*class_dev), GFP_KERNEL);
+		if (class_dev) {
+			memset(class_dev, 0x00, sizeof(*class_dev));
+			class_dev->class = &mem_class;
+			strncpy(class_dev->class_id, devlist[i].name, BUS_ID_SIZE);
+			class_set_devdata(class_dev, (void *)&devlist[i]);
+			if (!class_device_register(class_dev));
+				class_device_create_file(class_dev, &class_device_attr_dev);
+		}
+
 		devfs_mk_cdev(MKDEV(MEM_MAJOR, devlist[i].minor),
 				S_IFCHR | devlist[i].mode, devlist[i].name);
 	}

This adds /sys/class/mem which enables all misc char devices to show up properly in udev.

Note, the misc_init() call has been moved to a subsys_initcall as it seems there are a lot of platform specific misc devices that are calling misc_register before misc_init() is called.

Has been posted to lkml a few times in the past and tested by a wide range of people.

diff -Nru a/drivers/char/misc.c b/drivers/char/misc.c
@@ -47,7 +47,7 @@
 #include <linux/devfs_fs_kernel.h>
 #include <linux/stat.h>
 #include <linux/init.h>
-
+#include <linux/device.h>
 #include <linux/tty.h>
 #include <linux/kmod.h>
 

@@ -180,6 +180,91 @@
 	return err;
 }
 
+/* Misc class implementation */
+
+/* 
+ * TODO for 2.7:
+ *  - add a struct class_device to struct miscdevice and make all usages of
+ *    them dynamic.  This will let us get rid of struct misc_dev below.
+ */
+struct misc_dev {
+	struct list_head node;
+	dev_t dev;
+	struct class_device class_dev;
+};
+#define to_misc_dev(d) container_of(d, struct misc_dev, class_dev)
+
+static LIST_HEAD(misc_dev_list);
+static spinlock_t misc_dev_list_lock = SPIN_LOCK_UNLOCKED;
+
+static void release_misc_dev(struct class_device *class_dev)
+{
+	struct misc_dev *misc_dev = to_misc_dev(class_dev);
+	kfree(misc_dev);
+}
+
+static struct class misc_class = {
+	.name		= "misc",
+	.release	= &release_misc_dev,
+};
+
+static ssize_t show_dev(struct class_device *class_dev, char *buf)
+{
+	struct misc_dev *misc_dev = to_misc_dev(class_dev);
+	return print_dev_t(buf, misc_dev->dev);
+}
+static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
+
+static void misc_add_class_device(struct miscdevice *misc)
+{
+	struct misc_dev *misc_dev = NULL;
+	int retval;
+
+	misc_dev = kmalloc(sizeof(*misc_dev), GFP_KERNEL);
+	if (!misc_dev)
+		return;
+	memset(misc_dev, 0x00, sizeof(*misc_dev));
+
+	misc_dev->dev = MKDEV(MISC_MAJOR, misc->minor);
+	misc_dev->class_dev.dev = misc->dev;
+	misc_dev->class_dev.class = &misc_class;
+	snprintf(misc_dev->class_dev.class_id, BUS_ID_SIZE, "%s", misc->name);
+	retval = class_device_register(&misc_dev->class_dev);
+	if (retval)
+		goto error;
+	class_device_create_file(&misc_dev->class_dev, &class_device_attr_dev);
+	spin_lock(&misc_dev_list_lock);
+	list_add(&misc_dev->node, &misc_dev_list);
+	spin_unlock(&misc_dev_list_lock);
+	return;
+error:
+	kfree(misc_dev);
+}
+
+static void misc_remove_class_device(struct miscdevice *misc)
+{
+	struct misc_dev *misc_dev = NULL;
+	struct list_head *tmp;
+	int found = 0;
+
+	spin_lock(&misc_dev_list_lock);
+	list_for_each(tmp, &misc_dev_list) {
+		misc_dev = list_entry(tmp, struct misc_dev, node);
+		if ((MINOR(misc_dev->dev) == misc->minor)) {
+			found = 1;
+			break;
+		}
+	}
+	if (found) {
+		list_del(&misc_dev->node);
+		spin_unlock(&misc_dev_list_lock);
+		class_device_unregister(&misc_dev->class_dev);
+	} else {
+		spin_unlock(&misc_dev_list_lock);
+	}
+}
+
+
 static struct file_operations misc_fops = {
 	.owner		= THIS_MODULE,
 	.open		= misc_open,

@@ -236,6 +321,7 @@
 
 	devfs_mk_cdev(MKDEV(MISC_MAJOR, misc->minor),
 			S_IFCHR|S_IRUSR|S_IWUSR|S_IRGRP, misc->devfs_name);
+	misc_add_class_device(misc);
 
 	/*
 	 * Add it to the front, so that later devices can "override"

@@ -265,6 +351,7 @@
 
 	down(&misc_sem);
 	list_del(&misc->list);
+	misc_remove_class_device(misc);
 	devfs_remove(misc->devfs_name);
 	if (i < DYNAMIC_MINORS && i>0) {
 		misc_minors[i>>3] &= ~(1 << (misc->minor & 7));

@@ -285,6 +372,7 @@
 	if (ent)
 		ent->proc_fops = &misc_proc_fops;
 #endif
+	class_register(&misc_class);
 #ifdef CONFIG_MVME16x
 	rtc_MK48T08_init();
 #endif

@@ -319,4 +407,4 @@
 	}
 	return 0;
 }
-module_init(misc_init);
+subsys_initcall(misc_init);

diff -Nru a/include/linux/miscdevice.h b/include/linux/miscdevice.h
@@ -36,12 +36,15 @@
 
 #define TUN_MINOR	     200
 
+struct device;
+
 struct miscdevice 
 {
 	int minor;
 	const char *name;
 	struct file_operations *fops;
 	struct list_head list;
+	struct device *dev;
 	char devfs_name[64];
 };
 

This adds /sys/class/vc which enables all vc char devices to show up properly in udev.

Has been posted to lkml a few times in the past and tested by a wide range of people.

diff -Nru a/drivers/char/vc_screen.c b/drivers/char/vc_screen.c
@@ -36,6 +36,7 @@
 #include <linux/kbd_kern.h>
 #include <linux/console.h>
 #include <linux/smp_lock.h>
+#include <linux/device.h>
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
 #include <asm/unaligned.h>

@@ -469,6 +470,85 @@
 	.open		= vcs_open,
 };
 
+/* vc class implementation */
+
+struct vc_dev {
+	struct list_head node;
+	dev_t dev;
+	struct class_device class_dev;
+};
+#define to_vc_dev(d) container_of(d, struct vc_dev, class_dev)
+
+static LIST_HEAD(vc_dev_list);
+static spinlock_t vc_dev_list_lock = SPIN_LOCK_UNLOCKED;
+
+static void release_vc_dev(struct class_device *class_dev)
+{
+	struct vc_dev *vc_dev = to_vc_dev(class_dev);
+	kfree(vc_dev);
+}
+
+static struct class vc_class = {
+	.name		= "vc",
+	.release	= &release_vc_dev,
+};
+
+static ssize_t show_dev(struct class_device *class_dev, char *buf)
+{
+	struct vc_dev *vc_dev = to_vc_dev(class_dev);
+	return print_dev_t(buf, vc_dev->dev);
+}
+static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
+
+static int vc_add_class_device(dev_t dev, char *name, int minor)
+{
+	struct vc_dev *vc_dev = NULL;
+	int retval;
+
+	vc_dev = kmalloc(sizeof(*vc_dev), GFP_KERNEL);
+	if (!vc_dev)
+		return -ENOMEM;
+	memset(vc_dev, 0x00, sizeof(*vc_dev));
+
+	vc_dev->dev = dev;
+	vc_dev->class_dev.class = &vc_class;
+	snprintf(vc_dev->class_dev.class_id, BUS_ID_SIZE, name, minor);
+	retval = class_device_register(&vc_dev->class_dev);
+	if (retval)
+		goto error;
+	class_device_create_file(&vc_dev->class_dev, &class_device_attr_dev);
+	spin_lock(&vc_dev_list_lock);
+	list_add(&vc_dev->node, &vc_dev_list);
+	spin_unlock(&vc_dev_list_lock);
+	return 0;
+error:
+	kfree(vc_dev);
+	return retval;
+}
+
+static void vc_remove_class_device(int minor)
+{
+	struct vc_dev *vc_dev = NULL;
+	struct list_head *tmp;
+	int found = 0;
+
+	spin_lock(&vc_dev_list_lock);
+	list_for_each(tmp, &vc_dev_list) {
+		vc_dev = list_entry(tmp, struct vc_dev, node);
+		if (MINOR(vc_dev->dev) == minor) {
+			found = 1;
+			break;
+		}
+	}
+	if (found) {
+		list_del(&vc_dev->node);
+		spin_unlock(&vc_dev_list_lock);
+		class_device_unregister(&vc_dev->class_dev);
+	} else {
+		spin_unlock(&vc_dev_list_lock);
+	}
+}
+
 void vcs_make_devfs(struct tty_struct *tty)
 {
 	devfs_mk_cdev(MKDEV(VCS_MAJOR, tty->index + 1),

@@ -477,19 +557,26 @@
 	devfs_mk_cdev(MKDEV(VCS_MAJOR, tty->index + 129),
 			S_IFCHR|S_IRUSR|S_IWUSR,
 			"vcc/a%u", tty->index + 1);
+	vc_add_class_device(MKDEV(VCS_MAJOR, tty->index + 1), "vcs%u", tty->index + 1);
+	vc_add_class_device(MKDEV(VCS_MAJOR, tty->index + 129), "vcsa%u", tty->index + 1);
 }
 void vcs_remove_devfs(struct tty_struct *tty)
 {
 	devfs_remove("vcc/%u", tty->index + 1);
 	devfs_remove("vcc/a%u", tty->index + 1);
+	vc_remove_class_device(tty->index + 1);
+	vc_remove_class_device(tty->index + 129);
 }
 
 int __init vcs_init(void)
 {
 	if (register_chrdev(VCS_MAJOR, "vcs", &vcs_fops))
 		panic("unable to get major %d for vcs device", VCS_MAJOR);
+	class_register(&vc_class);
 
 	devfs_mk_cdev(MKDEV(VCS_MAJOR, 0), S_IFCHR|S_IRUSR|S_IWUSR, "vcc/0");
 	devfs_mk_cdev(MKDEV(VCS_MAJOR, 128), S_IFCHR|S_IRUSR|S_IWUSR, "vcc/a0");
+	vc_add_class_device(MKDEV(VCS_MAJOR, 0), "vcs", 0);
+	vc_add_class_device(MKDEV(VCS_MAJOR, 128), "vcsa", 128);
 	return 0;
 }

Same thing agin. This is virtual console code and shouldn't be in sysfs.

Also pasting the same code n times into different subsystems isn't exactly a good sign of code reuse..

Oops, that should say, "/sys/class/misc". Sorry for the typo.

greg k-h

So how are misc devices a device class now? This is just a random coolection of drivers that traditionally didn't have their own major. Killing misc devices sounds like abetter plan than codifying this in sysfs..

This is pointless. The original point of sysfs and co was to present the physical device tree, where these devices absolutely fit into. Why are you doing this at all? Creating thse through udev doesn't make sense as they need to be present anyway..

Creating them via udev is the point.

Remember, the ultimate goal is to have udev in initramfs during early boot, and all of these vital devices will be created.

For udev to work as intended, all devices on the system must be represented in sysfs.

Rob Love

Oh, I think I get your point, now.

The devices actually do not need to be in sysfs, but we do need to generate a hotplug event for them.

Nonetheless, I think it makes sense to also put them in sysfs. I think of sysfs has a "device tree" and not necessarily a "physical device tree".

Rob Love

I disagree. For fully static devices like the mem devices the udev indirection is completely superflous.

I see your point, so I really do not want to argue, but here is my rationale for why everything should be done seamlessly via udev:

In a nutshell, we want a single, clean, automatic solution to device naming. If some "static" devices are hard coded, we introduce a special case. Why do that? Why have special cases when udev can seamlessly manage the whole thing? Say we decide to remove /dev/foo in the kernel
- that should be reflected in udev simply by way of it no longer being created on boot.

That is my thoughts. I dislike special casing. And without it, udev can seamlessly handle everything, automatically.

But I _do_ see your point. It is silly to generate a hotplug event for a static device on every boot, etc. etc. But I think the cleanliness of not special casing certain devices in the udev solution is worth it.

Rob Love

It can be considered superfluous, but OTOH I think when creating a clean interface it's desirable to keep the number of exceptional items as small as possible, IOW zero.

Stephan

Trying to get rid of them for the sake of getting rid of them can very well make interface (a) not clean and (b) harder to cleanup up later.

If sysfs does not contain data on mem devices, we will need makedev.

devfs did replace makedev. until udev can create all devices, it would need to re-introduce makedev.

Andreas

So what?

So maybe suggest an solution rather than shooting one down all the time (which do seem logical, and is only apposed by one person currently
- namely you =).

I currently run a system with Greg's patches + udev, and all the devices are generated via udev and a modified version of Robert's initscript (running much earlier), with only alsa's nodes generated via another script as it is not sysfs-ified yet. So basically the initramfs idea is fully plausible if initramfs will get there (or I get time to have a look).

Martin Schlemmer

Nah, most of us just trust Christoph to fight the good fight for us ;-)

I for one certainly agree with him that for static stuff, we don't need (or want) udev. For inherently hotplug stuff like USB cameras, or large SCSI raid arrays, it's nice, but not for basic things like mem devices and the disk devices I'm booting from - it's just added complexity.

If it works as is, don't screw with it.

M.

heh =)

Well, its inclusion do not mean you have to use it - you have to physically walk all the classes in /sys to get udev to create the nodes, as they are already there when booted. And as the code is only a few lines for each device, it is not much overhead to get:

1) a full sysfs tree of all physical and 'virtual' (?) devices.

2) Optional feature to generate /dev with one simple script for those that want it, which should be the less complex option at initramfs time.

With an already populated /dev, sure :/

Thanks,

Martin Schlemmer

My suggestion is to just use MAKEDEV asis for devices that are static like the memdevices. Dynamic solutions do not buy us anything for those.

They do buy when using tmpfs for /dev.

Tomasz Torcz            There exists no separation between gods and men:
[email protected]   one blends softly casual into the other. 

And it will anyhow require a full evaluation of MAKEDEV (debian's one anyhow), as it really put a lot of extra crap in. With Greg's patches I only need to worry about /dev/core, /dev/std{in,out,err} and alsa. The question is now also how do you 'qualify' an device to have sysfs support. Sure some is static like /dev/null, but over here /dev/hda is pretty static as well =)

Martin Schlemmer

No. The point of sysfs and co was to present the physical _and_ logical device tree. Mem devices are devices. This patch also provides a place to move the /proc/sys/kernel/random files out of proc and into sysfs.

In order for tools like udev to work, we must export all char devices that are registered with the kernel. We can't do this at register_chrdev() time, as that only allocates a whole major. And people haven't converted over to using register_chrdev_region only when they really have a device present yet.

With devices such as the misc devices, we only care about the devices we really have in the system at that time. It also gives us the ability to show the linkage between the logical device, and the physical one (for misc devices.)

Now yeah, I can see that some people might think it's a bit overkill to move the mem devices here, but why not? They are logical devices in the system, and as stated above, it provides a place within sysfs to move user modifiable attributes of these devices out of /proc (as they do not pertain to anything related to processes.)

I do agree that the duplication of the code should be fixed, and I'll go do that right now (I should have realized that after cut-and-pasting that logic the third time, sorry about that.) If that is done, the overhead to support mem devices will drop to a very tiny ammount.

thanks,

greg k-h

Then add a random-junk-not-converted-yet devclass instead of duplicting the adhoc code everywhere. Still I'd vote for going to the proper interface directly instead of adding bad hacks all over the places. If that means waiting for 2.7 to open, so what?

But why is it tied to the obsoltet misc mechanism (or the obsolete usb major thing, etc..)

What user-modifiable attributes?

See /proc/sys/kernel/random

Junk like that should be ripped from procfs and put in sysfs corresponding to its device file.

Rob Love

Well, it's not just for /dev/random but also for all in-kernel cosumers of random numbers, so doing this as a sysctl makes quite a lot of sense.

Whether sysctl should be mached into procfs or sysfs or rather be it's own fs is an entirely different question. Interface-wise the latter would make most sense.

And /dev/random is the user-space abstraction representing the random number generator...

What to do with sysctl's in the long run is a good question.

Rob Love

Not precisely. If your userspace program can obtain random numbers in some other way, it should... /dev/random shouldn't be considered as the canonical source for random bits for the entire machine.

Jeff

s/the random number generator/the kernel's random number generator (random.c)/

E.g., my point was that the device file was the user-space representation of the kernel rng, so associating the kernel rng attributes with the device file made sense. I did not mean to imply it was the be-all and end-all of rng's on the machine.

Rob Love

Junk like that is called "sysctl" and is a part of userland ABI.

Right. And I agree 100% that we need to respect that ABI.

Long-term, though, we need to think about a saner home for sysctl's than procfs. hch's suggestion of giving them their own filesystem is a good idea to start with.

Right now, though, I think it is a shining exemplar of sanity to associate device-related attributes with the device's directory in sysfs.

Rob Love

Thanks, thoses added many missing devices.

Are there usable patches to add these? url?
- floppy devices (propably not necessary)
- alsa sound devices (snd/ and sound/ (oss emulation))
- input devices
- printer devices

For my machine I compared udev and devfs and those are the only devices not present in sysfs. I searched linux-kernel archives for patches to add those, but couldn't find anything.

Regards,Andreas

Already in the main kernel, all block devices are supported.

Working on it right now.

Hanna Linder has been working on this, but I think she's on vacation right now.

USB printers are already supported :)

No, no parallel port support yet. I thought Hanna had a patch somewhere. The "simple_class" patch I just posted should make it pretty easy to add support for this if you want to try it.

thanks,

greg k-h