[RFC PATCH] debugfs - yet another in-kernel file system

[RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

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.

To create a file using debugfs the call is just:
  struct dentry *debugfs_create_file(const char *name, mode_t mode,
			             struct dentry *parent, void *data,
				     struct file_operations *fops);

If that looks too complex for you, and you just want to export a single
value to userspace, how about:
  struct dentry *debugfs_create_u8(const char *name, mode_t mode, struct dentry *parent, u8 *value);

That's it, one line of code and you have a variable that can be read and
written to from userspace.  Dave Hansen, you can stop complaining about
complexity right now :)

And if you want to create a directory to put all of your stuff in (which
is a good idea, as no one is going to even try to manage the debugfs
namespace):
  struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)

That will give you a dentry to use in the other debugfs functions.

So, here's a patch against 2.6.10-rc3 (it will probably apply cleanly
against 2.6.9, but I haven't tried it.)  It still needs a bit more
cleanup (it shouldn't be a module and the init level needs to be bumped
up if core kernel code uses it) but it works and I'm asking for comments
about it.

Do we want more simple types of files?  A "string" type isn't really
needed as it's only about 3 lines of code to create your own "read"
function these days.

And I'll follow this patch up with a patch for the USB uhci driver that
converts from using /proc/driver/uhci to use debugfs, which makes that
driver smaller.

thanks,

greg k-h

p.s. I think the recently posted infiband driver can take advantage of
this fs instead of having to create it's own debug filesystem.


-------------------


diff -Nru a/fs/Makefile b/fs/Makefile
--- a/fs/Makefile	2004-12-09 16:32:32 -08:00
+++ b/fs/Makefile	2004-12-09 16:32:32 -08:00
@@ -94,3 +94,4 @@
 obj-$(CONFIG_BEFS_FS)		+= befs/
 obj-$(CONFIG_HOSTFS)		+= hostfs/
 obj-$(CONFIG_HPPFS)		+= hppfs/
+obj-$(CONFIG_DEBUG_FS)		+= debugfs/
diff -Nru a/fs/debugfs/Makefile b/fs/debugfs/Makefile
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/fs/debugfs/Makefile	2004-12-09 16:32:32 -08:00
@@ -0,0 +1,4 @@
+debugfs-objs	:= inode.o file.o
+
+obj-$(CONFIG_DEBUG_FS)	+= debugfs.o
+
diff -Nru a/fs/debugfs/debugfs.h b/fs/debugfs/debugfs.h
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/fs/debugfs/debugfs.h	2004-12-09 16:32:32 -08:00
@@ -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
+
+extern struct file_operations debugfs_file_operations;
+
diff -Nru a/fs/debugfs/debugfs_test.c b/fs/debugfs/debugfs_test.c
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/fs/debugfs/debugfs_test.c	2004-12-09 16:32:32 -08:00
@@ -0,0 +1,61 @@
+#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/proc_fs.h>
+#include <linux/usb.h>
+#include <linux/namei.h>
+#include <linux/usbdevice_fs.h>
+#include <linux/smp_lock.h>
+#include <linux/parser.h>
+#include <asm/byteorder.h>
+#include "debugfs.h"
+
+
+static struct dentry *test_dentry;
+static struct dentry *test_dir;
+static struct dentry *test_u32_dentry;
+static u32 value = 42;
+
+
+static ssize_t default_read_file(struct file *file, char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	char buf[10];
+
+	sprintf(buf, "foo\n");
+	return simple_read_from_buffer(user_buf, count, ppos, buf, sizeof(buf));
+}
+
+
+static int default_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static struct file_operations default_file_operations = {
+	.read =		default_read_file,
+	.open =		default_open,
+};
+
+static int __init test_init(void)
+{
+	test_dir = debugfs_create_dir("foo_dir", NULL);
+	test_dentry = debugfs_create_file("foo", 0444, test_dir, NULL, &default_file_operations);
+	test_u32_dentry = debugfs_create_u32("foo_u32", 0444, test_dir, &value);
+	return 0;
+}
+
+static void __exit test_exit(void)
+{
+	debugfs_remove(test_u32_dentry);
+	debugfs_remove(test_dentry);
+	debugfs_remove(test_dir);
+}
+
+module_init(test_init);
+module_exit(test_exit);
+MODULE_LICENSE("GPL");
+
diff -Nru a/fs/debugfs/file.c b/fs/debugfs/file.c
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/fs/debugfs/file.c	2004-12-09 16:32:32 -08:00
@@ -0,0 +1,165 @@
+/*
+ *  debugfs.c  - a tiny little debug file system for people to use instead of /proc or /sys
+ *
+ *  Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
+ *  Copyright (C) 2004 IBM Inc.
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/pagemap.h>
+#include <linux/debugfs.h>
+#include "debugfs.h"
+
+static ssize_t default_read_file(struct file *file, char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	return 0;
+}
+
+static ssize_t default_write_file(struct file *file, const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	return count;
+}
+
+static loff_t default_file_lseek(struct file *file, loff_t offset, int orig)
+{
+	loff_t retval = -EINVAL;
+
+	down(&file->f_dentry->d_inode->i_sem);
+	switch(orig) {
+	case 0:
+		if (offset > 0) {
+			file->f_pos = offset;
+			retval = file->f_pos;
+		} 
+		break;
+	case 1:
+		if ((offset + file->f_pos) > 0) {
+			file->f_pos += offset;
+			retval = file->f_pos;
+		} 
+		break;
+	default:
+		break;
+	}
+	up(&file->f_dentry->d_inode->i_sem);
+	return retval;
+}
+
+static int default_open(struct inode *inode, struct file *file)
+{
+	if (inode->u.generic_ip)
+		file->private_data = inode->u.generic_ip;
+
+	return 0;
+}
+
+struct file_operations debugfs_file_operations = {
+	.read =		default_read_file,
+	.write =	default_write_file,
+	.open =		default_open,
+	.llseek =	default_file_lseek,
+};
+
+
+#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)	\
+{													\
+	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)	\
+{													\
+	return debugfs_create_file(name, mode, parent, value, &fops_##type);				\
+}
+
+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':
+	case 'Y':
+	case '1':
+		*val = 1;
+		break;
+	case 'n':
+	case 'N':
+	case '0':
+		*val = 0;
+		break;
+	}
+	
+	return count;
+}
+
+static struct file_operations fops_bool = {
+	.read =		read_file_bool,
+	.write =	write_file_bool,
+	.open =		default_open,
+	.llseek =	default_file_lseek,
+};
+
+struct dentry *debugfs_create_bool(const char *name, mode_t mode, struct dentry *parent, u32 *value)
+{
+	return debugfs_create_file(name, mode, parent, value, &fops_bool);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_bool);
+
diff -Nru a/fs/debugfs/inode.c b/fs/debugfs/inode.c
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/fs/debugfs/inode.c	2004-12-09 16:32:32 -08:00
@@ -0,0 +1,229 @@
+/*
+ *  debugfs.c  - a tiny little debug file system for people to use instead of /proc or /sys
+ *
+ *  Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
+ *  Copyright (C) 2004 IBM Inc.
+ */
+
+#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>
+#include <linux/debugfs.h>
+#include "debugfs.h"
+
+#define DEBUG_MAGIC	0x64626720
+static struct vfsmount *debugfs_mount;
+static int debugfs_mount_count;
+
+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;
+		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);
+			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) */
+			inode->i_nlink++;
+			break;
+		}
+	}
+	return inode; 
+}
+
+/* SMP-safe */
+static int debugfs_mknod(struct inode *dir, struct dentry *dentry,
+			 int mode, dev_t dev)
+{
+	struct inode *inode = debugfs_get_inode(dir->i_sb, mode, dev);
+	int error = -EPERM;
+
+	if (dentry->d_inode)
+		return -EEXIST;
+
+	if (inode) {
+		d_instantiate(dentry, inode);
+		dget(dentry);
+		error = 0;
+	}
+	return error;
+}
+
+static int debugfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
+{
+	int res;
+
+	mode = (mode & (S_IRWXUGO | S_ISVTX)) | S_IFDIR;
+	res = debugfs_mknod(dir, dentry, mode, 0);
+	if (!res)
+		dir->i_nlink++;
+	return res;
+}
+
+static int debugfs_create(struct inode *dir, struct dentry *dentry, int mode)
+{
+	mode = (mode & S_IALLUGO) | S_IFREG;
+	return debugfs_mknod(dir, dentry, mode, 0);
+}
+
+static inline int debugfs_positive(struct dentry *dentry)
+{
+	return dentry->d_inode && !d_unhashed(dentry);
+}
+
+static int debug_fill_super(struct super_block *sb, void *data, int silent)
+{
+	static struct tree_descr debug_files[] = {{""}};
+
+	return simple_fill_super(sb, DEBUG_MAGIC, debug_files);
+}
+
+static struct dentry * get_dentry(struct dentry *parent, const char *name)
+{               
+	struct qstr qstr;
+
+	qstr.name = name;
+	qstr.len = strlen(name);
+	qstr.hash = full_name_hash(name,qstr.len);
+	return lookup_hash(&qstr,parent);
+}               
+
+static struct super_block *debug_get_sb(struct file_system_type *fs_type,
+				        int flags, const char *dev_name,
+					void *data)
+{
+	return get_sb_single(fs_type, flags, data, debug_fill_super);
+}
+
+static struct file_system_type debug_fs_type = {
+	.owner =	THIS_MODULE,
+	.name =		"debugfs",
+	.get_sb =	debug_get_sb,
+	.kill_sb =	kill_litter_super,
+};
+
+static int debugfs_create_by_name(const char *name, mode_t mode,
+				  struct dentry *parent,
+				  struct dentry **dentry)
+{
+	int error = 0;
+
+	/* If the parent is not specified, we create it in the root.
+	 * We need the root dentry to do this, which is in the super 
+	 * block. A pointer to that is in the struct vfsmount that we
+	 * have around.
+	 */
+	if (!parent ) {
+		if (debugfs_mount && debugfs_mount->mnt_sb) {
+			parent = debugfs_mount->mnt_sb->s_root;
+		}
+	}
+	if (!parent) {
+		dbg("Ah! can not find a parent!\n");
+		return -EFAULT;
+	}
+
+	*dentry = NULL;
+	down(&parent->d_inode->i_sem);
+	*dentry = get_dentry (parent, name);
+	if (!IS_ERR(dentry)) {
+		if ((mode & S_IFMT) == S_IFDIR)
+			error = debugfs_mkdir(parent->d_inode, *dentry, mode);
+		else 
+			error = debugfs_create(parent->d_inode, *dentry, mode);
+	} else
+		error = PTR_ERR(dentry);
+	up(&parent->d_inode->i_sem);
+
+	return error;
+}
+
+struct dentry *debugfs_create_file(const char *name, mode_t mode,
+				   struct dentry *parent, void *data,
+				   struct file_operations *fops)
+{
+	struct dentry *dentry = NULL;
+	int error;
+
+	dbg("creating file '%s'\n",name);
+
+	error = simple_pin_fs("debugfs", &debugfs_mount, &debugfs_mount_count);
+	if (error)
+		goto exit;
+
+	error = debugfs_create_by_name(name, mode, parent, &dentry);
+	if (error) {
+		dentry = NULL;
+		goto exit;
+	}
+
+	if (dentry->d_inode) {
+		if (data)
+			dentry->d_inode->u.generic_ip = data;
+		if (fops)
+			dentry->d_inode->i_fop = fops;
+	}
+exit:
+	return dentry;
+}
+EXPORT_SYMBOL_GPL(debugfs_create_file);
+
+void debugfs_remove(struct dentry *dentry)
+{
+	struct dentry *parent;
+	
+	if (!dentry)
+		return;
+
+	parent = dentry->d_parent;
+	if (!parent || !parent->d_inode)
+		return;
+
+	down(&parent->d_inode->i_sem);
+	if (debugfs_positive(dentry)) {
+		if (dentry->d_inode) {
+			if (S_ISDIR(dentry->d_inode->i_mode))
+				simple_rmdir(parent->d_inode, dentry);
+			else
+				simple_unlink(parent->d_inode, dentry);
+		dput(dentry);
+		}
+	}
+	up(&parent->d_inode->i_sem);
+	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+}
+EXPORT_SYMBOL_GPL(debugfs_remove);
+
+static int __init debugfs_init(void)
+{
+	return register_filesystem(&debug_fs_type);
+}
+
+static void __exit debugfs_exit(void)
+{
+	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+	unregister_filesystem(&debug_fs_type);
+}
+
+module_init(debugfs_init);
+module_exit(debugfs_exit);
+MODULE_LICENSE("GPL");
+
diff -Nru a/include/linux/debugfs.h b/include/linux/debugfs.h
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/include/linux/debugfs.h	2004-12-09 16:32:32 -08:00
@@ -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 <greg@kroah.com>
+ *  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); }
+
+static inline void debugfs_remove(struct dentry *dentry) { }
+
+static inline struct dentry *debugfs_create_u8(const char *name, mode_t mode, struct dentry *parent, u8 *value)
+{ return ERR_PTR(-ENODEV); }
+static inline struct dentry *debugfs_create_u16(const char *name, mode_t mode, struct dentry *parent, u16 *value)
+{ return ERR_PTR(-ENODEV); }
+static inline struct dentry *debugfs_create_u32(const char *name, mode_t mode, struct dentry *parent, u32 *value)
+{ return ERR_PTR(-ENODEV); }
+static inline struct dentry *debugfs_create_bool(const char *name, mode_t mode, struct dentry *parent, u32 *value)
+{ return EFF_PTR(-ENODEV); }
+
+#endif
+
+#endif
diff -Nru a/lib/Kconfig.debug b/lib/Kconfig.debug
--- a/lib/Kconfig.debug	2004-12-09 16:32:17 -08:00
+++ b/lib/Kconfig.debug	2004-12-09 16:32:17 -08:00
@@ -107,6 +107,18 @@
         If you're truly short on disk space or don't expect to report any
         bugs back to the UML developers, say N, otherwise say Y.
 
+config DEBUG_FS
+	tristate "Debug Filesystem"
+	help
+	  debugfs is a virtual file system that kernel developers use to put
+	  debugging files into.  Enable this option to be able to read and
+	  write to these files.
+
+	  If unsure, say N.
+
+	  To compile this as a module, choose M here: the module will be called
+	  debugfs.
+
 if !X86_64
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"

[RFC PATCH] convert uhci-hcd to use debugfs

[Greg KH]

And here's the uhci-hcd patch.

Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

thanks,

greg k-h


-----------------


diff -Nru a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
--- a/drivers/usb/host/uhci-debug.c	2004-12-09 16:32:17 -08:00
+++ b/drivers/usb/host/uhci-debug.c	2004-12-09 16:32:17 -08:00
@@ -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;
@@ -517,8 +506,7 @@
 
 static int uhci_proc_open(struct inode *inode, struct file *file)
 {
-	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
--- a/drivers/usb/host/uhci-hcd.c	2004-12-09 16:32:31 -08:00
+++ b/drivers/usb/host/uhci-hcd.c	2004-12-09 16:32:31 -08:00
@@ -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
+	struct dentry *dentry;
 
 	io_size = pci_resource_len(to_pci_dev(uhci_dev(uhci)), hcd->region);
 
-#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) {
+		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;
 }
 
@@ -2405,11 +2392,9 @@
 			goto errbuf_failed;
 	}
 
-#ifdef CONFIG_PROC_FS
-	uhci_proc_root = create_proc_entry("driver/uhci", S_IFDIR, NULL);
-	if (!uhci_proc_root)
+	uhci_debugfs_root = debugfs_create_dir("uhci", NULL);
+	if (!uhci_debugfs_root)
 		goto proc_failed;
-#endif
 
 	uhci_up_cachep = kmem_cache_create("uhci_urb_priv",
 		sizeof(struct urb_priv), 0, 0, NULL, NULL);
@@ -2427,12 +2412,9 @@
 		warn("not all urb_priv's were freed!");
 
 up_failed:
-
-#ifdef CONFIG_PROC_FS
-	remove_proc_entry("driver/uhci", NULL);
+	debugfs_remove(uhci_debugfs_root);
 
 proc_failed:
-#endif
 	if (errbuf)
 		kfree(errbuf);
 
@@ -2448,9 +2430,7 @@
 	if (kmem_cache_destroy(uhci_up_cachep))
 		warn("not all urb_priv's were freed!");
 
-#ifdef CONFIG_PROC_FS
-	remove_proc_entry("driver/uhci", NULL);
-#endif
+	debugfs_remove(uhci_debugfs_root);
 
 	if (errbuf)
 		kfree(errbuf);
diff -Nru a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
--- a/drivers/usb/host/uhci-hcd.h	2004-12-09 16:32:30 -08:00
+++ b/drivers/usb/host/uhci-hcd.h	2004-12-09 16:32:30 -08:00
@@ -326,10 +326,8 @@
 struct uhci_hcd {
 	struct usb_hcd hcd;		/* must come first! */
 
-#ifdef CONFIG_PROC_FS
-	/* procfs */
-	struct proc_dir_entry *proc_entry;
-#endif
+	/* debugfs */
+	struct dentry *dentry;
 
 	/* Grabbed from PCI */
 	unsigned long io_addr;

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Jan Engelhardt]

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


Jan Engelhardt
-- 
ENOSPC

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 08:07:20AM +0100, Jan Engelhardt wrote:
> >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?

For input stuff, yes.  And for simple types that are not debug stuff,
yes, that is the proper place for them.

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


Now we have a place to put this stuff.  If you look through the way
drivers use /proc these days, there is still a lot of stuff that needs
to get moved out, that can go in debugfs.

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.)

thanks,

greg k-h

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Jan Engelhardt]

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

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

>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.)

Ah that's good news.



Jan Engelhardt
-- 
ENOSPC

Re: [RFC PATCH] convert uhci-hcd to use debugfs

[Josh Boyer]

On Thu, 2004-12-09 at 18:55, Greg KH wrote:
> diff -Nru a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
> --- a/drivers/usb/host/uhci-debug.c	2004-12-09 16:32:17 -08:00
> +++ b/drivers/usb/host/uhci-debug.c	2004-12-09 16:32:17 -08:00
> @@ -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;
> @@ -517,8 +506,7 @@
>  
>  static int uhci_proc_open(struct inode *inode, struct file *file)
>  {

Didn't want to rename this uhci_debug_open?

> -	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
> --- a/drivers/usb/host/uhci-hcd.c	2004-12-09 16:32:31 -08:00
> +++ b/drivers/usb/host/uhci-hcd.c	2004-12-09 16:32:31 -08:00
> @@ -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
> +	struct dentry *dentry;
>  
>  	io_size = pci_resource_len(to_pci_dev(uhci_dev(uhci)), hcd->region);
>  
> -#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.

> +		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;
>  }
>  
> @@ -2405,11 +2392,9 @@
>  			goto errbuf_failed;
>  	}
>  
> -#ifdef CONFIG_PROC_FS
> -	uhci_proc_root = create_proc_entry("driver/uhci", S_IFDIR, NULL);
> -	if (!uhci_proc_root)
> +	uhci_debugfs_root = debugfs_create_dir("uhci", NULL);
> +	if (!uhci_debugfs_root)

Same thing here.

>  		goto proc_failed;
> -#endif
>  
>  	uhci_up_cachep = kmem_cache_create("uhci_urb_priv",
>  		sizeof(struct urb_priv), 0, 0, NULL, NULL);

josh

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Josh Boyer]

On Thu, 2004-12-09 at 18:50, Greg KH wrote:
> 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.

Cool idea.


> diff -Nru a/fs/debugfs/debugfs.h b/fs/debugfs/debugfs.h
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/fs/debugfs/debugfs.h	2004-12-09 16:32:32 -08:00
> @@ -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?

> +
> +extern struct file_operations debugfs_file_operations;
> +

<snip>

> diff -Nru a/fs/debugfs/file.c b/fs/debugfs/file.c
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/fs/debugfs/file.c	2004-12-09 16:32:32 -08:00
> @@ -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>

> + *
> + *  Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
> + *  Copyright (C) 2004 IBM Inc.
> + */
> +

<snip>

> +
> +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':
> +	case 'Y':
> +	case '1':
> +		*val = 1;
> +		break;
> +	case 'n':
> +	case 'N':
> +	case '0':
> +		*val = 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?


> diff -Nru a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/fs/debugfs/inode.c	2004-12-09 16:32:32 -08:00
> @@ -0,0 +1,229 @@
> +/*
> + *  debugfs.c  - a tiny little debug file system for people to use instead of /proc or /sys

Again.  Template perhaps? :)

> + *
> + *  Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
> + *  Copyright (C) 2004 IBM Inc.
> + */
> +
> +#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>
> +#include <linux/debugfs.h>
> +#include "debugfs.h"
> +
> +#define DEBUG_MAGIC	0x64626720

#define DEBUGFS_MAGIC

<snip>

> diff -Nru a/include/linux/debugfs.h b/include/linux/debugfs.h
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/include/linux/debugfs.h	2004-12-09 16:32:32 -08:00
> @@ -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 <greg@kroah.com>
> + *  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); }
> +
> +static inline void debugfs_remove(struct dentry *dentry) { }
> +
> +static inline struct dentry *debugfs_create_u8(const char *name, mode_t mode, struct dentry *parent, u8 *value)
> +{ return ERR_PTR(-ENODEV); }
> +static inline struct dentry *debugfs_create_u16(const char *name, mode_t mode, struct dentry *parent, u16 *value)
> +{ return ERR_PTR(-ENODEV); }
> +static inline struct dentry *debugfs_create_u32(const char *name, mode_t mode, struct dentry *parent, u32 *value)
> +{ return ERR_PTR(-ENODEV); }
> +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.

> +
> +#endif
> +
> +#endif

josh

Re: [RFC PATCH] convert uhci-hcd to use debugfs

[Greg KH]

On Fri, Dec 10, 2004 at 08:15:18AM -0600, Josh Boyer wrote:
> On Thu, 2004-12-09 at 18:55, Greg KH wrote:
> >  static int uhci_proc_open(struct inode *inode, struct file *file)
> >  {
> 
> Didn't want to rename this uhci_debug_open?

Yeah, I could :)

> > -#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.

thanks,

greg k-h

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 08:46:14AM -0600, Josh Boyer wrote:
> On Thu, 2004-12-09 at 18:50, Greg KH wrote:
> > diff -Nru a/fs/debugfs/debugfs.h b/fs/debugfs/debugfs.h
> > --- /dev/null	Wed Dec 31 16:00:00 196900
> > +++ b/fs/debugfs/debugfs.h	2004-12-09 16:32:32 -08:00
> > @@ -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?

Yeah, good point.  I'll go change it.

> > diff -Nru a/fs/debugfs/file.c b/fs/debugfs/file.c
> > --- /dev/null	Wed Dec 31 16:00:00 196900
> > +++ b/fs/debugfs/file.c	2004-12-09 16:32:32 -08:00
> > @@ -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>

Yeah, you can tell I split it up into 2 files at the last minute, can't
ya :)

> > +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;
> > +		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?

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.

> > +#define DEBUG_MAGIC	0x64626720
> 
> #define DEBUGFS_MAGIC

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

> > +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.

Thanks for reviewing the code.

greg k-h

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 09:00:22AM +0100, Jan Engelhardt wrote:
> >> 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.
> 
> So how about adding seqfiles (or multi-value-per-file things) to sysfs?

No, that is not going to happen, as it is directly opposite to how sysfs
is supposed to work (one _single_ value per file).

thanks,

greg k-h

Re: [RFC PATCH] convert uhci-hcd to use debugfs

[Josh Boyer]

On Fri, 2004-12-10 at 09:27, Greg KH wrote:
> 
> > > -#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.

Ah, ok I see now.  Good point.  I should have looked at the code instead
of just trying to infer everything from the patch ;).

josh

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Josh Boyer]

On Fri, 2004-12-10 at 09:30, Greg KH wrote:
> > 
> > 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.

Ok.  Consistency is a good thing.

> 
> > > +#define DEBUG_MAGIC	0x64626720
> > 
> > #define DEBUGFS_MAGIC
> 
> Hm, ok, sure.  That doesn't really matter.

No, not really.  Just a nit.

> 
> > > +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.

Yep, agreed.

josh

Comments on new kernel dev. model

[Peter Karlsson]

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). I realise this has been debated quite
a lot before, and as an illiterate when it comes to C-programming ("hello
world" is the limit for me right now) I cannot contribute (so far). Ok,
that's all I have to say. I'm gonna crawl under the rock from where I came
now...

Best regards

Peter Karlsson

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Jörn Engel]

Two-word summary: cool stuff!  Thanks!

On Thu, 9 December 2004 16:50:56 -0800, Greg KH wrote:
> 
> 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?

Details differ, sure.

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

> +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. ;)

> +#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...

> +{													\
> +	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)	\
> +{													\
> +	return debugfs_create_file(name, mode, parent, value, &fops_##type);				\
> +}
> +
> +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?

> +#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...

> +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;
> +		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?

> +			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) */
> +			inode->i_nlink++;
> +			break;
> +		}
> +	}
> +	return inode; 
> +}


> +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); }

Nice.  Gotta remember that one.

Jörn

-- 
To recognize individual spam features you have to try to get into the
mind of the spammer, and frankly I want to spend as little time inside
the minds of spammers as possible.
-- Paul Graham

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 06:21:26PM +0100, J?rn Engel wrote:
> Two-word summary: cool stuff!  Thanks!
> 
> On Thu, 9 December 2004 16:50:56 -0800, Greg KH wrote:
> > 
> > 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?

Yes, that is correct.

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

Yeah, I forgot I left it in the patch, that was part of my initial test
code.  I'll clean it up and mark it as such.

> > +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. ;)

Heh, ok, will do.

> > +#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...

Good point, I'll fix that, thanks.

> > +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?

Hm, I don't remember why I did that.  I'll move that in the macro just
to save 2 lines of code :)

> > +#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.

> > +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;
> > +		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?

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

Thanks for reviewing the code, I appreciate it.

greg k-h

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Jörn Engel]

On Fri, 10 December 2004 09:35:56 -0800, Greg KH wrote:
> On Fri, Dec 10, 2004 at 06:21:26PM +0100, J?rn Engel wrote:
> > On Thu, 9 December 2004 16:50:56 -0800, Greg KH wrote:
> 
> > > +#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.

I buy the config.h point.  For the rest, I actually like to receive
the breakage when order matters.  It shouldn't matter, so there goes
another patch...

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

Thought so.  But hey, there may be valid reasons to do so.  And if
there are, I'd like to hear the story.

Jörn

-- 
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein

Re: [RFC PATCH] debugfs - yet another in-kernel file system

[Roland Dreier]

    Greg> p.s. I think the recently posted infiband driver can take
    Greg> advantage of this fs instead of having to create it's own
    Greg> debug filesystem.

Yes, if this gets merged, I'll convert the "ipoib_debugfs" in the
OpenIB drivers to just use debugfs stuff.

Thanks,
  Roland

Re: Comments on new kernel dev. model

[Greg KH]

On Fri, Dec 10, 2004 at 06:17:11PM +0100, Peter Karlsson wrote:
> 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).

Gentoo is having no such problem, thanks anyway :)

greg k-h

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[David Brownell]

On Thursday 09 December 2004 4:50 pm, Greg KH wrote:
> 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!

- Dave

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 05:29:01PM -0800, David Brownell wrote:
> On Thursday 09 December 2004 4:50 pm, Greg KH wrote:
> > 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.

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

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

Agreed.

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

Ick.

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

thanks,

greg k-h

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[David Brownell]

On Friday 10 December 2004 5:39 pm, Greg KH wrote:
> > 
> > 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 :)

It's an arbitrary fiat, sure; not a feature!  ;)


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

- Dave

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 06:02:00PM -0800, David Brownell wrote:
> On Friday 10 December 2004 5:39 pm, Greg KH wrote:
> > > 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.

Ok, I'll work on adding that.

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

If the kobject name is devices/foo/whatever, then  yes, it would then
match up the same, if you create a debugfs file with the kobject.  If
you don't do that, then no, I'm not going to maintain a mirror directory
image of sysfs in debugfs just for the fun of it :)

thanks,

greg k-h

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[Roland Dreier]

    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.

 - R.

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[Greg KH]

On Fri, Dec 10, 2004 at 06:26:49PM -0800, Roland Dreier wrote:
>     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.

Oh yes, it would be optional.  I still like the simple interface that
debugfs is providing so far, I'll just add
yet-another-way-to-create-a-file type function that takes a kobject.

Sound ok?

thanks,

greg k-h

Re: [linux-usb-devel] [RFC PATCH] debugfs - yet another in-kernel file system

[Roland Dreier]

    Greg> Oh yes, it would be optional.  I still like the simple
    Greg> interface that debugfs is providing so far, I'll just add
    Greg> yet-another-way-to-create-a-file type function that takes a
    Greg> kobject.

    Greg> Sound ok?

Yep, that works for me.

Thanks,
  Roland