debugfs - yet another in-kernel file system

diff -Nru a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c

drivers/usb/host/uhci-debug.c +2 -2

@@ -11,7 +11,7 @@
 
 #include <linux/config.h>
 #include <linux/kernel.h>
-#include <linux/proc_fs.h>
+#include <linux/debugfs.h>
 #include <linux/smp_lock.h>
 #include <asm/io.h>
 

@@ -507,7 +496,7 @@
 
 #define MAX_OUTPUT	(64 * 1024)
 
-static struct proc_dir_entry *uhci_proc_root = NULL;
+static struct dentry *uhci_debugfs_root = NULL;
 
 struct uhci_proc {
 	int size;

drivers/usb/host/uhci-debug.c +1 -2

-	const struct proc_dir_entry *dp = PDE(inode);
-	struct uhci_hcd *uhci = dp->data;
+	struct uhci_hcd *uhci = inode->u.generic_ip;
 	struct uhci_proc *up;
 	int ret = -ENOMEM;
 

diff -Nru a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c

drivers/usb/host/uhci-hcd.c +4 -9

@@ -46,7 +46,7 @@
 #include <linux/unistd.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
-#include <linux/proc_fs.h>
+#include <linux/debugfs.h>
 #include <linux/pm.h>
 #include <linux/dmapool.h>
 #include <linux/dma-mapping.h>

@@ -1927,12 +1927,10 @@
 		uhci->fl = NULL;
 	}
 
-#ifdef CONFIG_PROC_FS
-	if (uhci->proc_entry) {
-		remove_proc_entry(uhci->hcd.self.bus_name, uhci_proc_root);
-		uhci->proc_entry = NULL;
+	if (uhci->dentry) {
+		debugfs_remove(uhci->dentry);
+		uhci->dentry = NULL;
 	}
-#endif
 }
 
 static int uhci_reset(struct usb_hcd *hcd)

@@ -1972,25 +1970,17 @@
 	unsigned io_size;
 	dma_addr_t dma_handle;
 	struct usb_device *udev;
-#ifdef CONFIG_PROC_FS
-	struct proc_dir_entry *ent;
-#endif

drivers/usb/host/uhci-hcd.c +4 -11

+		dev_err(uhci_dev(uhci), "couldn't create uhci debugfs entry\n");
 		retval = -ENOMEM;
 		goto err_create_proc_entry;
 	}
-
-	ent->data = uhci;
-	ent->proc_fops = &uhci_proc_operations;
-	ent->size = 0;
-	uhci->proc_entry = ent;
-#endif
+	uhci->dentry = dentry;
 
 	uhci->fsbr = 0;
 	uhci->fsbrtimeout = 0;

@@ -2192,13 +2182,10 @@
 	uhci->fl = NULL;
 
 err_alloc_fl:
-#ifdef CONFIG_PROC_FS
-	remove_proc_entry(hcd->self.bus_name, uhci_proc_root);
-	uhci->proc_entry = NULL;
+	debugfs_remove(uhci->dentry);
+	uhci->dentry = NULL;
 
 err_create_proc_entry:
-#endif
-
 	return retval;
 }
 

Quoted 2 lines · Greg KH

static int uhci_proc_open(struct inode *inode, struct file *file) {

Didn't want to rename this uhci_debug_open?

drivers/usb/host/uhci-hcd.c line 1978

Quoted 6 lines

drivers/usb/host/uhci-hcd.c +2 -4

-#ifdef CONFIG_PROC_FS
-	ent = create_proc_entry(hcd->self.bus_name, S_IFREG|S_IRUGO|S_IWUSR, uhci_proc_root);
-	if (!ent) {
-		dev_err(uhci_dev(uhci), "couldn't create uhci proc entry\n");
+	dentry = debugfs_create_file(hcd->self.bus_name, S_IFREG|S_IRUGO|S_IWUSR, uhci_debugfs_root, uhci, &uhci_proc_operations);
+	if (!dentry) {

That won't work if debugfs is not configured. You'll get back (void *) -ENODEV, which is not NULL.

Quoted 10 lines

drivers/usb/host/uhci-hcd.c line 1978

Quoted 7 lines

drivers/usb/host/uhci-hcd.c +2 -4

-#ifdef CONFIG_PROC_FS
-	ent = create_proc_entry(hcd->self.bus_name, S_IFREG|S_IRUGO|S_IWUSR, uhci_proc_root);
-	if (!ent) {
-		dev_err(uhci_dev(uhci), "couldn't create uhci proc entry\n");
+	dentry = debugfs_create_file(hcd->self.bus_name, S_IFREG|S_IRUGO|S_IWUSR, uhci_debugfs_root, uhci, &uhci_proc_operations);
+	if (!dentry) {

That won't work if debugfs is not configured. You'll get back (void *) -ENODEV, which is not NULL.

That's exactly correct. We do _not_ want NULL to be returned if debugfs is not configured in. We want to be able to detect if an error happened, not if the feature was just not configured. Otherwise, if we did return NULL if debugfs was not enabled, this code would just fail and the uhci startup would never happen.

This is why people have to wrap proc functions in #ifdef, and why no one ever checks the return values from devfs calls. It's a real problem and I don't want to duplicate that again.

Hello!

Historically, users, like me, have been able to download a "stable" kernel. With the new development model, where it is up to distributions to "stabilise" the kernel, these days are gone. This is a, sort-of, regression from my POV, making it so much harder for DIY-"distros" like LFS (and to a lesser extent gentoo).

What if there was a in-kernel filesystem that was explicitly just for putting debugging stuff? Some place other than proc and sysfs, and that was easier than both of them to use. Yet it needed to also be able to handle complex stuff like seq file and raw file_ops if needed.

Quoted 4 lines

What if there was a in-kernel filesystem that was explicitly just for putting debugging stuff? Some place other than proc and sysfs, and that was easier than both of them to use. Yet it needed to also be able to handle complex stuff like seq file and raw file_ops if needed.

As for modules, they could just wrap a variable in a module_param, don't they?

I have to admit that adding another filesystem that is very like procfs or sysfs make some kind of redundancy.

Quoted 2 lines

I have to admit that adding another filesystem that is very like procfs or sysfs make some kind of redundancy.

Why? The main issue is the discussion usually goes like this:

Me:  Hey, the /proc/driver/foo/foo_value really shouldn't be in proc.
Developer:  Ok, but it has a lot of really good debug stuff in it.  Can

I put it in sysfs?
Me: No, sysfs is for one-value-per-file whenever possible. It needs to go somewhere else.
Developer: Well, if you don't have anywhere else to put it, why are you even bringing this up at all. Go away and leave me alone.

As for "another filesystem", it's tiny due to using libfs, and it will compile away into nothing if not selected (so in the end, provides the ability to make the final kernel image smaller.)

Quoted 5 lines

Quoted 2 lines

I have to admit that adding another filesystem that is very like procfs or sysfs make some kind of redundancy.

Why? The main issue is the discussion usually goes like this:

Quoted 7 lines

Me:  Hey, the /proc/driver/foo/foo_value really shouldn't be in proc.
Developer:  Ok, but it has a lot of really good debug stuff in it.  Can

I put it in sysfs?
Me: No, sysfs is for one-value-per-file whenever possible. It needs to go somewhere else.
Developer: Well, if you don't have anywhere else to put it, why are you even bringing this up at all. Go away and leave me alone.

So how about adding seqfiles (or multi-value-per-file things) to sysfs?

A while ago a comment from another kernel developer about why they put a huge file in sysfs (one that was bigger than a single page and contained more than just 1 type of information), was something like, "well, it was just so easy, and there was no other place to put debugging stuff like that," got me to thinking.

What if there was a in-kernel filesystem that was explicitly just for putting debugging stuff? Some place other than proc and sysfs, and that was easier than both of them to use. Yet it needed to also be able to handle complex stuff like seq file and raw file_ops if needed.

Thus debugfs was born (yes, I know there's a userspace program called debugfs, this is an in-kernel filesystem and has nothing to do with that.) debugfs is ment for putting stuff that kernel developers need to see exported to userspace, yet don't always want hanging around.

fs/debugfs/debugfs.h new file +3 -0

+
+extern struct file_operations debugfs_file_operations;
+

fs/debugfs/file.c new file +5 -0

+ *
+ *  Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
+ *  Copyright (C) 2004 IBM Inc.
+ */
+

fs/debugfs/file.c new file +33 -0

+
+simple_type(u8, "%c", unsigned long, simple_strtoul);
+simple_type(u16, "%hi", unsigned long, simple_strtoul);
+simple_type(u32, "%i", unsigned long, simple_strtoul);
+EXPORT_SYMBOL_GPL(debugfs_create_u8);
+EXPORT_SYMBOL_GPL(debugfs_create_u16);
+EXPORT_SYMBOL_GPL(debugfs_create_u32);
+
+static ssize_t read_file_bool(struct file *file, char __user *user_buf, size_t count, loff_t *ppos)
+{
+	char buf[2];
+	u32 *val = file->private_data;
+	
+	if (val)
+		buf[0] = 'Y';
+	else
+		buf[0] = 'N';
+	buf[1] = 0x00;
+	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t write_file_bool(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	char buf[32];
+	int buf_size;
+	u32 *val = file->private_data;
+
+	buf_size = min(count, (sizeof(buf)-1));
+	if (copy_from_user(buf, user_buf, buf_size))
+		return -EFAULT;
+
+	switch (buf[0]) {
+	case 'y':

fs/debugfs/inode.c new file +6 -0

+ *
+ *  Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
+ *  Copyright (C) 2004 IBM Inc.
+ */
+
+#include <linux/config.h>

diff -Nru a/include/linux/debugfs.h b/include/linux/debugfs.h

include/linux/debugfs.h new file +33 -0

@@ -0,0 +1,47 @@
+/*
+ *  debugfs.h  - a tiny little debug file system for people to use instead of /proc or /sys
+ *
+ *  Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
+ *  Copyright (C) 2004 IBM Inc.
+ */
+
+#ifndef _DEBUGFS_H_
+#define _DEBUGFS_H_
+
+#if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEBUG_FS_MODULE)
+struct dentry *debugfs_create_file(const char *name, mode_t mode,
+				   struct dentry *parent, void *data,
+				   struct file_operations *fops);
+
+static inline struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
+{
+	return debugfs_create_file(name, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO, parent, NULL, NULL);
+}
+
+void debugfs_remove(struct dentry *dentry);
+
+struct dentry *debugfs_create_u8(const char *name, mode_t mode, struct dentry *parent, u8 *value);
+struct dentry *debugfs_create_u16(const char *name, mode_t mode, struct dentry *parent, u16 *value);
+struct dentry *debugfs_create_u32(const char *name, mode_t mode, struct dentry *parent, u32 *value);
+struct dentry *debugfs_create_bool(const char *name, mode_t mode, struct dentry *parent, u32 *value);
+
+#else
+static inline struct dentry *debugfs_create_file(const char *name, mode_t mode, struct dentry *parent, void *data, struct file_operations *fops)
+{ return ERR_PTR(-ENODEV); }
+static inline struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
+{ return ERR_PTR(-ENODEV); }
+

fs/debugfs/debugfs.h line 7

Quoted 11 lines · Greg KH

diff -Nru a/fs/debugfs/debugfs.h b/fs/debugfs/debugfs.h

fs/debugfs/debugfs.h new file +7 -0

@@ -0,0 +1,10 @@
+#define DEBUG
+
+#ifdef DEBUG
+#define dbg(format, arg...) printk(KERN_DEBUG "%s: " format , __FILE__ , ## arg)
+#else
+#define dbg(format, arg...) do {} while (0)
+#endif

Fine for now, but if it gets merged should pr_debug be used?

fs/debugfs/file.c line 2

Quoted 6 lines

diff -Nru a/fs/debugfs/file.c b/fs/debugfs/file.c

fs/debugfs/file.c new file +2 -0

@@ -0,0 +1,165 @@
+/*
+ *  debugfs.c  - a tiny little debug file system for people to use instead of /proc or /sys

<NIT>
Wrong file name :).
</NIT>

Quoted 20 lines

fs/debugfs/file.c new file +20 -0

+static ssize_t write_file_bool(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	char buf[32];
+	int buf_size;
+	u32 *val = file->private_data;
+
+	buf_size = min(count, (sizeof(buf)-1));
+	if (copy_from_user(buf, user_buf, buf_size))
+		return -EFAULT;
+
+	switch (buf[0]) {
+	case 'y':
+	case 'Y':
+	case '1':
+		*val = 1;
+		break;
+	case 'n':
+	case 'N':
+	case '0':
+		*val = 0;

fs/debugfs/file.c line 24

Quoted 2 lines

fs/debugfs/file.c new file +2 -0

+		break;
+	}

Writing 'Y', 'y', or '1' is allowed, but only 'Y' is ever returned by the read function (similar for the "false" case). That can be confusing to a program if it writes '1', and then checks to see if the value it wrote took and it gets back 'Y'. Maybe only allow what you are going to return for bool values in the write case?

fs/debugfs/file.c line 25

Quoted 1 line

fs/debugfs/file.c new file +1 -0

+#define DEBUG_MAGIC	0x64626720

#define DEBUGFS_MAGIC

fs/debugfs/file.c line 27

Quoted 2 lines

fs/debugfs/file.c new file +2 -0

+static inline struct dentry *debugfs_create_bool(const char *name, mode_t mode, struct dentry *parent, u32 *value)
+{ return EFF_PTR(-ENODEV); }

Could these just return NULL perhaps? Would be more like procfs then, which is what I'd assume most drivers will be converting from.

Quoted 6 lines

Writing 'Y', 'y', or '1' is allowed, but only 'Y' is ever returned by the read function (similar for the "false" case). That can be confusing to a program if it writes '1', and then checks to see if the value it wrote took and it gets back 'Y'. Maybe only allow what you are going to return for bool values in the write case?

No, this is the exact same way (and pretty much the same code) as the module param stuff is in sysfs today. Just trying to be consistant here.

Quoted 4 lines

Quoted 2 lines

+#define DEBUG_MAGIC 0x64626720

#define DEBUGFS_MAGIC

Hm, ok, sure. That doesn't really matter.

Quoted 6 lines

include/linux/debugfs.h line 43

Quoted 3 lines

include/linux/debugfs.h new file +2 -0

+static inline struct dentry *debugfs_create_bool(const char *name, mode_t mode, struct dentry *parent, u32 *value)
+{ return EFF_PTR(-ENODEV); }

Could these just return NULL perhaps? Would be more like procfs then, which is what I'd assume most drivers will be converting from.

No, see my previous response as to why this would be a bad thing to do. We should all learn from the mistakes made in the past and try not to repeat them.

Thus debugfs was born (yes, I know there's a userspace program called debugfs, this is an in-kernel filesystem and has nothing to do with that.) debugfs is ment for putting stuff that kernel developers need to see exported to userspace, yet don't always want hanging around.

diff -Nru a/fs/debugfs/debugfs_test.c b/fs/debugfs/debugfs_test.c

fs/debugfs/file.c new file +33 -0

+{													\
+	char buf[32];											\
+	type *val = file->private_data;									\
+													\
+	snprintf(buf, sizeof(buf), format, *val);							\
+	return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));			\
+}													\
+static ssize_t write_file_##type(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos)	\
+{													\
+	char *endp;											\
+	char buf[32];											\
+	int buf_size;											\
+	type *val = file->private_data;									\
+	temptype tmp;											\
+													\
+	memset(buf, 0x00, sizeof(buf));									\
+	buf_size = min(count, (sizeof(buf)-1));								\
+	if (copy_from_user(buf, user_buf, buf_size))							\
+		return -EFAULT;										\
+													\
+	tmp = strtolfn(buf, &endp, 0);									\
+	if ((endp == buf) || ((type)tmp != tmp))							\
+		return -EINVAL;										\
+	*val = tmp;											\
+	return count;											\
+}													\
+static struct file_operations fops_##type = {								\
+	.read =		read_file_##type,								\
+	.write =	write_file_##type,								\
+	.open =		default_open,									\
+	.llseek =	default_file_lseek,								\
+};													\
+struct dentry *debugfs_create_##type(const char *name, mode_t mode, struct dentry *parent, type *value)	\

fs/debugfs/inode.c new file +4 -0

+static struct inode *debugfs_get_inode(struct super_block *sb, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+

fs/debugfs/inode.c new file +9 -0

+			break;
+		case S_IFREG:
+			inode->i_fop = &debugfs_file_operations;
+			break;
+		case S_IFDIR:
+			inode->i_op = &simple_dir_inode_operations;
+			inode->i_fop = &simple_dir_operations;
+
+			/* directory inodes start off with i_nlink == 2 (for "." entry) */

Two-word summary: cool stuff! Thanks!

Quoted 5 lines · Greg KH

Thus debugfs was born (yes, I know there's a userspace program called debugfs, this is an in-kernel filesystem and has nothing to do with that.) debugfs is ment for putting stuff that kernel developers need to see exported to userspace, yet don't always want hanging around.

In principle, it is the same as /proc, just with the explicit information that binary compatibility will never be a goal, right?

Quoted 1 line

diff -Nru a/fs/debugfs/debugfs_test.c b/fs/debugfs/debugfs_test.c

Nice example code. But I'd vote for either killing it or renaming it to debugfs_example.c. Just to document that anyone actually compiling it in is stupid.

fs/debugfs/debugfs_test.c line 24

Quoted 2 lines

fs/debugfs/debugfs_test.c new file +2 -0

+static ssize_t default_read_file(struct file *file, char __user *user_buf,
+				 size_t count, loff_t *ppos)

For a similar reason, I'd call this example_read_file(). You actually fooled me for a moment and I was wondering why the heck this should be part of debugfs. ;)

fs/debugfs/file.c line 69

Quoted 2 lines

fs/debugfs/file.c new file +2 -0

+#define simple_type(type, format, temptype, strtolfn)	\
+static ssize_t read_file_##type(struct file *file, char __user *user_buf, size_t count, loff_t *ppos)	\

Long lines. Taste varies, but...

fs/debugfs/file.c line 112

Quoted 6 lines

fs/debugfs/file.c new file +6 -0

+simple_type(u8, "%c", unsigned long, simple_strtoul);
+simple_type(u16, "%hi", unsigned long, simple_strtoul);
+simple_type(u32, "%i", unsigned long, simple_strtoul);
+EXPORT_SYMBOL_GPL(debugfs_create_u8);
+EXPORT_SYMBOL_GPL(debugfs_create_u16);
+EXPORT_SYMBOL_GPL(debugfs_create_u32);

Move above three lines into the macro? Or do you prefer to me the export move obvious?

fs/debugfs/debugfs_test.c line 9

Quoted 7 lines

fs/debugfs/debugfs_test.c new file +7 -0

+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/mount.h>
+#include <linux/pagemap.h>
+#include <linux/init.h>
+#include <linux/namei.h>

I like to sort the above alphabetically. Shouldn't matter, but it looks neat and since there is no other natural order...

Quoted 9 lines

fs/debugfs/inode.c new file +9 -0

+static struct inode *debugfs_get_inode(struct super_block *sb, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+
+	if (inode) {
+		inode->i_mode = mode;
+		inode->i_uid = 0;
+		inode->i_gid = 0;
+		inode->i_blksize = PAGE_CACHE_SIZE;

fs/debugfs/inode.c line 35

Quoted 5 lines

fs/debugfs/inode.c new file +5 -0

+		inode->i_blocks = 0;
+		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		switch (mode & S_IFMT) {
+		default:
+			init_special_inode(inode, mode, dev);

Just out of curiosity: why would anyone want special nodes under /debug?

Quoted 12 lines · J?rn Engel

fs/debugfs/debugfs_test.c line 9

Quoted 7 lines · Greg KH

fs/debugfs/debugfs_test.c new file +7 -0

+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/mount.h>
+#include <linux/pagemap.h>
+#include <linux/init.h>
+#include <linux/namei.h>

I like to sort the above alphabetically. Shouldn't matter, but it looks neat and since there is no other natural order...

Well config.h should be first. After that, sometimes it matters, but usually not.

Just being "correct" :)
I don't think they would want special nodes, but hey, let's not prevent anyone from doing anything. If some looney person wants to make device nodes in debugfs, then they deserve the ridicule they will get when doing it...

What if there was a in-kernel filesystem that was explicitly just for putting debugging stuff?  Some place other than proc and sysfs, and that was easier than both of them to use.  Yet it needed to also be able to handle complex stuff like seq file and raw file_ops if needed.

Quoted 4 lines · Greg KH

What if there was a in-kernel filesystem that was explicitly just for putting debugging stuff? ?Some place other than proc and sysfs, and that was easier than both of them to use. ?Yet it needed to also be able to handle complex stuff like seq file and raw file_ops if needed.

The problem with sysfs here is: no seq_file support. Otherwise it solves the basic "where to put the debug files associated with "device X" or "driver Y" problems in a good non-confusing way: there are directories already set up for devices and for drivers.

The problem with procfs here is that it doesn't have such a naming solution: there's no automatic mapping betwen a /proc/driver/...file and its device, or vice versa. That issue is shared with debugfs.

Couldn't debugfs just be a thin shim on top of sysfs, adding seq_file support? Or on top of procfs, adding device/driver naming domains, and maybe file-per-value read/write support for drivers that want them?

What I'd really want out of a debug file API is to resolve the naming issues, work with seq_file, and "softly and silently vanish away". I think this patch has the last two, but not the first one!

Quoted 6 lines

The problem with sysfs here is: no seq_file support. Otherwise it solves the basic "where to put the debug files associated with "device X" or "driver Y" problems in a good non-confusing way: there are directories already set up for devices and for drivers.

Yes, but that's a design decision for sysfs. no seq_file support is a feature, not a shortcoming :)

Quoted 4 lines

What I'd really want out of a debug file API is to resolve the naming issues, work with seq_file, and "softly and silently vanish away". I think this patch has the last two, but not the first one!

I considered adding a kobject as a paramater to the debugfs interface. The file created would be equal to the path that the kobject has. Would that work for you?

Quoted 8 lines · Greg KH

Quoted 4 lines

What I'd really want out of a debug file API is to resolve the naming issues, work with seq_file, and "softly and silently vanish away". I think this patch has the last two, but not the first one!

I considered adding a kobject as a paramater to the debugfs interface. The file created would be equal to the path that the kobject has. Would that work for you?

If I could pass device->kobj or driver->kobj, that'd be good.

Will there be a /debug/devices tree parallel to /sys/devices?

Greg> I considered adding a kobject as a paramater to the debugfs Greg> interface. The file created would be equal to the path that Greg> the kobject has. Would that work for you?

I'd really prefer this to be optional at least. Somethings it's nice to use kobjects for, but creating hierarchies of subdirectories is not really one of them.