2007-12-16 16:38:18

by Mattias Nissler

[permalink] [raw]
Subject: [PATCH] debugfs: Revamp debugfs_create_{u,x,s}{8,16,32,64} to support signed integers

This makes debugfs use its own file_operations for the value accessor files
created by debugfs_create_XXX. Having that, we can also have proper versions
for signed integers.

Signed-off-by: Mattias Nissler <[email protected]>
---

When writing some debugfs code for mac80211 I wanted to have access to
some s32 variables. I thought it's better to make debugfs support signed
integers instead of adding the functionality locally. I assume generic
versions will be useful for other people as well.

Please CC me for replies.


fs/debugfs/file.c | 291 +++++++++++++++++++++++++++++++++++++++--------
include/linux/debugfs.h | 45 +++++++
2 files changed, 287 insertions(+), 49 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index fa6b7f7..8808bc0 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -56,18 +56,121 @@ const struct inode_operations debugfs_link_operations = {
.follow_link = debugfs_follow_link,
};

-static void debugfs_u8_set(void *data, u64 val)
+/* Simple numeric attributes */
+struct debugfs_num_attr
{
- *(u8 *)data = val;
+ void (*get)(void *, char *);
+ void (*set)(void *, char *);
+ char get_buf[24];
+ char set_buf[24];
+ void *pnum;
+ struct mutex mutex;
+};
+
+static int debugfs_num_attr_open(struct inode *inode, struct file *file,
+ void (*get)(void *, char*),
+ void (*set)(void *, char*))
+{
+ struct debugfs_num_attr *attr;
+
+ attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+
+ attr->get = get;
+ attr->set = set;
+ attr->pnum = inode->i_private;
+ mutex_init(&attr->mutex);
+
+ file->private_data = attr;
+
+ return nonseekable_open(inode, file);
+}
+
+static int debugfs_num_attr_close(struct inode *inode, struct file *file)
+{
+ kfree(file->private_data);
+
+ return 0;
+}
+
+static ssize_t debugfs_num_attr_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ struct debugfs_num_attr *attr;
+ size_t size;
+ ssize_t ret;
+
+ attr = file->private_data;
+
+ mutex_lock(&attr->mutex);
+ if (!*ppos) {
+ /* first read */
+ attr->get(attr->pnum, attr->get_buf);
+ attr->get_buf[sizeof(attr->get_buf) - 1] = '\0';
+ }
+
+ size = strlen(attr->get_buf);
+ ret = simple_read_from_buffer(buf, len, ppos, attr->get_buf, size);
+ mutex_unlock(&attr->mutex);
+
+ return ret;
}
-static u64 debugfs_u8_get(void *data)
+
+static ssize_t debugfs_num_attr_write(struct file *file, const char __user *buf,
+ size_t len, loff_t *ppos)
{
- return *(u8 *)data;
+ struct debugfs_num_attr *attr;
+ size_t size;
+ ssize_t ret;
+
+ attr = file->private_data;
+
+ mutex_lock(&attr->mutex);
+ ret = -EFAULT;
+ size = min(sizeof(attr->set_buf) - 1, len);
+ if (copy_from_user(attr->set_buf, buf, size))
+ goto out;
+
+ ret = len; /* claim we got the whole input */
+ attr->set_buf[size] = '\0';
+ attr->set(attr->pnum, attr->set_buf);
+out:
+ mutex_unlock(&attr->mutex);
+ return ret;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%llu\n");
+
+#define DEFINE_NUM_ATTR(__fops, __type, __format) \
+static void __fops ## _get(void *data, char *buf) \
+{ \
+ scnprintf(buf, 24, __format, *(__type *) data); \
+} \
+static void __fops ## _set(void *data, char *buf) \
+{ \
+ sscanf(buf, __format, (__type *) data); \
+} \
+static int __fops ## _open(struct inode *inode, struct file *file) \
+{ \
+ return debugfs_num_attr_open(inode, file, __fops ## _get, \
+ __fops ## _set); \
+} \
+static struct file_operations __fops = { \
+ .owner = THIS_MODULE, \
+ .open = __fops ## _open, \
+ .release = debugfs_num_attr_close, \
+ .read = debugfs_num_attr_read, \
+ .write = debugfs_num_attr_write, \
+};
+
+DEFINE_NUM_ATTR(fops_u8, u8, "%hhu\n")
+DEFINE_NUM_ATTR(fops_u16, u16, "%hu\n")
+DEFINE_NUM_ATTR(fops_u32, u32, "%u\n")
+DEFINE_NUM_ATTR(fops_u64, u64, "%llu\n")

/**
- * debugfs_create_u8 - create a debugfs file that is used to read and write an unsigned 8-bit value
+ * debugfs_create_u8 - create a debugfs file that is used to read and write an
+ * unsigned 8-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -97,18 +200,10 @@ struct dentry *debugfs_create_u8(const char *name, mode_t mode,
}
EXPORT_SYMBOL_GPL(debugfs_create_u8);

-static void debugfs_u16_set(void *data, u64 val)
-{
- *(u16 *)data = val;
-}
-static u64 debugfs_u16_get(void *data)
-{
- return *(u16 *)data;
-}
-DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%llu\n");
-
/**
- * debugfs_create_u16 - create a debugfs file that is used to read and write an unsigned 16-bit value
+ * debugfs_create_u16 - create a debugfs file that is used to read and write an
+ * unsigned 16-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -138,18 +233,10 @@ struct dentry *debugfs_create_u16(const char *name, mode_t mode,
}
EXPORT_SYMBOL_GPL(debugfs_create_u16);

-static void debugfs_u32_set(void *data, u64 val)
-{
- *(u32 *)data = val;
-}
-static u64 debugfs_u32_get(void *data)
-{
- return *(u32 *)data;
-}
-DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%llu\n");
-
/**
- * debugfs_create_u32 - create a debugfs file that is used to read and write an unsigned 32-bit value
+ * debugfs_create_u32 - create a debugfs file that is used to read and write an
+ * unsigned 32-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -179,19 +266,10 @@ struct dentry *debugfs_create_u32(const char *name, mode_t mode,
}
EXPORT_SYMBOL_GPL(debugfs_create_u32);

-static void debugfs_u64_set(void *data, u64 val)
-{
- *(u64 *)data = val;
-}
-
-static u64 debugfs_u64_get(void *data)
-{
- return *(u64 *)data;
-}
-DEFINE_SIMPLE_ATTRIBUTE(fops_u64, debugfs_u64_get, debugfs_u64_set, "%llu\n");
-
/**
- * debugfs_create_u64 - create a debugfs file that is used to read and write an unsigned 64-bit value
+ * debugfs_create_u64 - create a debugfs file that is used to read and write an
+ * unsigned 64-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -221,14 +299,104 @@ struct dentry *debugfs_create_u64(const char *name, mode_t mode,
}
EXPORT_SYMBOL_GPL(debugfs_create_u64);

-DEFINE_SIMPLE_ATTRIBUTE(fops_x8, debugfs_u8_get, debugfs_u8_set, "0x%02llx\n");
+DEFINE_NUM_ATTR(fops_s8, s8, "%hhd\n")
+DEFINE_NUM_ATTR(fops_s16, s16, "%hd\n")
+DEFINE_NUM_ATTR(fops_s32, s32, "%d\n")
+DEFINE_NUM_ATTR(fops_s64, s64, "%lld\n")
+
+/*
+ * debugfs_create_s{8,16,32,64} - create a debugfs file that is used to read
+ * and write a signed {8,16,32,64}-bit value
+ *
+ * These functions are exactly the same as the above functions (but use a hex
+ * output for the decimal challenged). For details look at the above unsigned
+ * decimal functions.
+ */
+
+/**
+ * debugfs_create_s8 - create a debugfs file that is used to read and write a
+ * signed 8-bit value
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+struct dentry *debugfs_create_s8(const char *name, mode_t mode,
+ struct dentry *parent, s8 *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_s8);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_s8);
+
+/**
+ * debugfs_create_s16 - create a debugfs file that is used to read and write a
+ * signed 16-bit value
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+struct dentry *debugfs_create_s16(const char *name, mode_t mode,
+ struct dentry *parent, s16 *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_s16);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_s16);
+
+/**
+ * debugfs_create_s32 - create a debugfs file that is used to read and write a
+ * signed 32-bit value
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+struct dentry *debugfs_create_s32(const char *name, mode_t mode,
+ struct dentry *parent, s32 *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_s32);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_s32);

-DEFINE_SIMPLE_ATTRIBUTE(fops_x16, debugfs_u16_get, debugfs_u16_set, "0x%04llx\n");
+/**
+ * debugfs_create_s64 - create a debugfs file that is used to read and write a
+ * signed 32-bit value
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+struct dentry *debugfs_create_s64(const char *name, mode_t mode,
+ struct dentry *parent, s64 *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_s64);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_s64);

-DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n");
+DEFINE_NUM_ATTR(fops_x8, u8, "%hhx\n")
+DEFINE_NUM_ATTR(fops_x16, u16, "%hx\n")
+DEFINE_NUM_ATTR(fops_x32, u32, "%x\n")
+DEFINE_NUM_ATTR(fops_x64, u64, "%llx\n")

/*
- * debugfs_create_x{8,16,32} - create a debugfs file that is used to read and write an unsigned {8,16,32}-bit value
+ * debugfs_create_x{8,16,32,64} - create a debugfs file that is used to read
+ * and write an unsigned {8,16,32,64}-bit value
*
* These functions are exactly the same as the above functions (but use a hex
* output for the decimal challenged). For details look at the above unsigned
@@ -236,7 +404,9 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_x32, debugfs_u32_get, debugfs_u32_set, "0x%08llx\n"
*/

/**
- * debugfs_create_x8 - create a debugfs file that is used to read and write an unsigned 8-bit value
+ * debugfs_create_x8 - create a debugfs file that is used to read and write an
+ * unsigned 8-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -253,7 +423,9 @@ struct dentry *debugfs_create_x8(const char *name, mode_t mode,
EXPORT_SYMBOL_GPL(debugfs_create_x8);

/**
- * debugfs_create_x16 - create a debugfs file that is used to read and write an unsigned 16-bit value
+ * debugfs_create_x16 - create a debugfs file that is used to read and write an
+ * unsigned 16-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -263,14 +435,16 @@ EXPORT_SYMBOL_GPL(debugfs_create_x8);
* from.
*/
struct dentry *debugfs_create_x16(const char *name, mode_t mode,
- struct dentry *parent, u16 *value)
+ struct dentry *parent, u16 *value)
{
return debugfs_create_file(name, mode, parent, value, &fops_x16);
}
EXPORT_SYMBOL_GPL(debugfs_create_x16);

/**
- * debugfs_create_x32 - create a debugfs file that is used to read and write an unsigned 32-bit value
+ * debugfs_create_x32 - create a debugfs file that is used to read and write an
+ * unsigned 32-bit value
+ *
* @name: a pointer to a string containing the name of the file to create.
* @mode: the permission that the file should have
* @parent: a pointer to the parent dentry for this file. This should be a
@@ -280,12 +454,31 @@ EXPORT_SYMBOL_GPL(debugfs_create_x16);
* from.
*/
struct dentry *debugfs_create_x32(const char *name, mode_t mode,
- struct dentry *parent, u32 *value)
+ struct dentry *parent, u32 *value)
{
return debugfs_create_file(name, mode, parent, value, &fops_x32);
}
EXPORT_SYMBOL_GPL(debugfs_create_x32);

+/**
+ * debugfs_create_x64 - create a debugfs file that is used to read and write an
+ * unsigned 32-bit value
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+struct dentry *debugfs_create_x64(const char *name, mode_t mode,
+ struct dentry *parent, u64 *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_x64);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_x64);
+
static ssize_t read_file_bool(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index f592d6d..e9991ed 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -49,12 +49,22 @@ struct dentry *debugfs_create_u32(const char *name, mode_t mode,
struct dentry *parent, u32 *value);
struct dentry *debugfs_create_u64(const char *name, mode_t mode,
struct dentry *parent, u64 *value);
+struct dentry *debugfs_create_s8(const char *name, mode_t mode,
+ struct dentry *parent, s8 *value);
+struct dentry *debugfs_create_s16(const char *name, mode_t mode,
+ struct dentry *parent, s16 *value);
+struct dentry *debugfs_create_s32(const char *name, mode_t mode,
+ struct dentry *parent, s32 *value);
+struct dentry *debugfs_create_s64(const char *name, mode_t mode,
+ struct dentry *parent, s64 *value);
struct dentry *debugfs_create_x8(const char *name, mode_t mode,
struct dentry *parent, u8 *value);
struct dentry *debugfs_create_x16(const char *name, mode_t mode,
struct dentry *parent, u16 *value);
struct dentry *debugfs_create_x32(const char *name, mode_t mode,
struct dentry *parent, u32 *value);
+struct dentry *debugfs_create_x64(const char *name, mode_t mode,
+ struct dentry *parent, u64 *value);
struct dentry *debugfs_create_bool(const char *name, mode_t mode,
struct dentry *parent, u32 *value);

@@ -128,6 +138,34 @@ static inline struct dentry *debugfs_create_u64(const char *name, mode_t mode,
return ERR_PTR(-ENODEV);
}

+static inline struct dentry *debugfs_create_s8(const char *name, mode_t mode,
+ struct dentry *parent,
+ s8 *value)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline struct dentry *debugfs_create_s16(const char *name, mode_t mode,
+ struct dentry *parent,
+ s16 *value)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline struct dentry *debugfs_create_s32(const char *name, mode_t mode,
+ struct dentry *parent,
+ s32 *value)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline struct dentry *debugfs_create_s64(const char *name, mode_t mode,
+ struct dentry *parent,
+ s64 *value)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline struct dentry *debugfs_create_x8(const char *name, mode_t mode,
struct dentry *parent,
u8 *value)
@@ -149,6 +187,13 @@ static inline struct dentry *debugfs_create_x32(const char *name, mode_t mode,
return ERR_PTR(-ENODEV);
}

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


2007-12-16 17:04:52

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] debugfs: Revamp debugfs_create_{u,x,s}{8,16,32,64} to support signed integers

On Sun, Dec 16, 2007 at 05:37:59PM +0100, Mattias Nissler wrote:
> This makes debugfs use its own file_operations for the value accessor files
> created by debugfs_create_XXX. Having that, we can also have proper versions
> for signed integers.

Why not tweak the "SIMPLE_ATTRIBUTE" code to support this instead? That
way debugfs and all other filesystems could also use these attributes?

thanks,

greg k-h

2007-12-16 17:20:09

by Mattias Nissler

[permalink] [raw]
Subject: Re: [PATCH] debugfs: Revamp debugfs_create_{u,x,s}{8,16,32,64} to support signed integers


On Sun, 2007-12-16 at 09:05 -0800, Greg KH wrote:
> On Sun, Dec 16, 2007 at 05:37:59PM +0100, Mattias Nissler wrote:
> > This makes debugfs use its own file_operations for the value accessor files
> > created by debugfs_create_XXX. Having that, we can also have proper versions
> > for signed integers.
>
> Why not tweak the "SIMPLE_ATTRIBUTE" code to support this instead? That
> way debugfs and all other filesystems could also use these attributes?

I expected that question ;-) Actually, I had a version that did this.
But it ended up being ugly, cause SIMPLE_ATTRIBUTEs expect to access the
values via get/set functions using u64. Here is what I did and why I
didn't like it:

* I made the set/get function interface more generic (as it is in the
debugfs patch), i.e. getting passed the buf, so they have to
decode/encode whatever they like into the buf instead of working with
u64
* This means either all code using SIMPLE_ATTRIBUTES must be changed, or
I need to add another wrapper macro that produces suitable get/set
functions from the u64 versions it gets passed.
* Changing all SIMPLE_ATTRIBUTE users is not what I want, cause moving
the csnprintf()/strtoull() calls out of the SIMPLE_ATTRIBUTE code means
it becomes less simple to use it, right?
* The wrapper is actually possible, but it suffers from cases where
somebody passes NULL for a get or set function (which is actually
expected, compare the code in libfs.c) It is possible to hack around
this, but it's ugly.

Then I thought, well, SIMPLE_ATTRIBUTEs are made to work with a set/get
function pair. But debugfs expects a pointer to an actual variable
anyway, so just bypass the SIMPLE_ATTRIBUTE code and make debugfs use
it's own file_operations (most of them shamelessly stolen from the
SIMPLE_ATTRIBUTE code, I admit).

Mattias