From 13c589d5b0ac654d9da7e490a2dd548e6b86b4a5 Mon Sep 17 00:00:00 2001 From: Tejun Heo Date: Tue, 1 Oct 2013 17:42:02 -0400 Subject: [PATCH] sysfs: use seq_file when reading regular files sysfs read path implements its own buffering scheme between userland and kernel callbacks, which essentially is a degenerate duplicate of seq_file. This patch replaces the custom read buffering implementation in sysfs with seq_file. While the amount of code reduction is small, this reduces low level hairiness and enables future development of a new versatile API based on seq_file so that sysfs features can be shared with other subsystems. As write path was already converted to not use sysfs_open_file->page, this patch makes ->page and ->count unused and removes them. Userland behavior remains the same except for some extreme corner cases - e.g. sysfs will now regenerate the content each time a file is read after a non-contiguous seek whereas the original code would keep using the same content. While this is a userland visible behavior change, it is extremely unlikely to be noticeable and brings sysfs behavior closer to that of procfs. Signed-off-by: Tejun Heo Cc: Kay Sievers Signed-off-by: Greg Kroah-Hartman --- fs/sysfs/file.c | 164 +++++++++++++++++++++--------------------------- 1 file changed, 73 insertions(+), 91 deletions(-) diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 53cc096e6a1b..4921bda3a37a 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -21,6 +21,7 @@ #include #include #include +#include #include "sysfs.h" @@ -31,7 +32,8 @@ * sysfs_dirent->s_attr.open points to sysfs_open_dirent. s_attr.open is * protected by sysfs_open_dirent_lock. * - * filp->private_data points to sysfs_open_file which is chained at + * filp->private_data points to seq_file whose ->private points to + * sysfs_open_file. sysfs_open_files are chained at * sysfs_open_dirent->files, which is protected by sysfs_open_file_mutex. */ static DEFINE_SPINLOCK(sysfs_open_dirent_lock); @@ -47,13 +49,16 @@ struct sysfs_open_dirent { struct sysfs_open_file { struct sysfs_dirent *sd; struct file *file; - size_t count; - char *page; struct mutex mutex; int event; struct list_head list; }; +static struct sysfs_open_file *sysfs_of(struct file *file) +{ + return ((struct seq_file *)file->private_data)->private; +} + /* * Determine ktype->sysfs_ops for the given sysfs_dirent. This function * must be called while holding an active reference. @@ -66,40 +71,54 @@ static const struct sysfs_ops *sysfs_file_ops(struct sysfs_dirent *sd) return kobj->ktype ? kobj->ktype->sysfs_ops : NULL; } -/** - * fill_read_buffer - allocate and fill buffer from object. - * @dentry: dentry pointer. - * @buffer: data buffer for file. - * - * Allocate @buffer->page, if it hasn't been already, then call the - * kobject's show() method to fill the buffer with this attribute's - * data. - * This is called only once, on the file's first read unless an error - * is returned. +/* + * Reads on sysfs are handled through seq_file, which takes care of hairy + * details like buffering and seeking. The following function pipes + * sysfs_ops->show() result through seq_file. */ -static int fill_read_buffer(struct dentry *dentry, struct sysfs_open_file *of) +static int sysfs_seq_show(struct seq_file *sf, void *v) { - struct sysfs_dirent *attr_sd = dentry->d_fsdata; - struct kobject *kobj = attr_sd->s_parent->s_dir.kobj; + struct sysfs_open_file *of = sf->private; + struct kobject *kobj = of->sd->s_parent->s_dir.kobj; const struct sysfs_ops *ops; - int ret = 0; + char *buf; ssize_t count; - if (!of->page) - of->page = (char *) get_zeroed_page(GFP_KERNEL); - if (!of->page) - return -ENOMEM; + /* acquire buffer and ensure that it's >= PAGE_SIZE */ + count = seq_get_buf(sf, &buf); + if (count < PAGE_SIZE) { + seq_commit(sf, -1); + return 0; + } - /* need attr_sd for attr and ops, its parent for kobj */ - if (!sysfs_get_active(attr_sd)) + /* + * Need @of->sd for attr and ops, its parent for kobj. @of->mutex + * nests outside active ref and is just to ensure that the ops + * aren't called concurrently for the same open file. + */ + mutex_lock(&of->mutex); + if (!sysfs_get_active(of->sd)) { + mutex_unlock(&of->mutex); return -ENODEV; + } - of->event = atomic_read(&attr_sd->s_attr.open->event); + of->event = atomic_read(&of->sd->s_attr.open->event); - ops = sysfs_file_ops(attr_sd); - count = ops->show(kobj, attr_sd->s_attr.attr, of->page); + /* + * Lookup @ops and invoke show(). Control may reach here via seq + * file lseek even if @ops->show() isn't implemented. + */ + ops = sysfs_file_ops(of->sd); + if (ops->show) + count = ops->show(kobj, of->sd->s_attr.attr, buf); + else + count = 0; - sysfs_put_active(attr_sd); + sysfs_put_active(of->sd); + mutex_unlock(&of->mutex); + + if (count < 0) + return count; /* * The code works fine with PAGE_SIZE return but it's likely to @@ -111,54 +130,8 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_open_file *of) /* Try to struggle along */ count = PAGE_SIZE - 1; } - if (count >= 0) - of->count = count; - else - ret = count; - return ret; -} - -/** - * sysfs_read_file - read an attribute. - * @file: file pointer. - * @buf: buffer to fill. - * @count: number of bytes to read. - * @ppos: starting offset in file. - * - * Userspace wants to read an attribute file. The attribute descriptor - * is in the file's ->d_fsdata. The target object is in the directory's - * ->d_fsdata. - * - * We call fill_read_buffer() to allocate and fill the buffer from the - * object's show() method exactly once (if the read is happening from - * the beginning of the file). That should fill the entire buffer with - * all the data the object has to offer for that attribute. - * We then call flush_read_buffer() to copy the buffer to userspace - * in the increments specified. - */ - -static ssize_t -sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) -{ - struct sysfs_open_file *of = file->private_data; - ssize_t retval = 0; - - mutex_lock(&of->mutex); - /* - * Fill on zero offset and the first read so that silly things like - * "dd bs=1 skip=N" can work on sysfs files. - */ - if (*ppos == 0 || !of->page) { - retval = fill_read_buffer(file->f_path.dentry, of); - if (retval) - goto out; - } - pr_debug("%s: count = %zd, ppos = %lld, buf = %s\n", - __func__, count, *ppos, of->page); - retval = simple_read_from_buffer(buf, count, ppos, of->page, of->count); -out: - mutex_unlock(&of->mutex); - return retval; + seq_commit(sf, count); + return 0; } /** @@ -216,7 +189,7 @@ static int flush_write_buffer(struct sysfs_open_file *of, char *buf, static ssize_t sysfs_write_file(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos) { - struct sysfs_open_file *of = file->private_data; + struct sysfs_open_file *of = sysfs_of(file); ssize_t len = min_t(size_t, count, PAGE_SIZE - 1); char *buf; @@ -364,10 +337,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file) goto err_out; } - /* - * No error? Great, allocate a sysfs_open_file for the file, and - * store it it in file->private_data for easy access. - */ + /* allocate a sysfs_open_file for the file */ error = -ENOMEM; of = kzalloc(sizeof(struct sysfs_open_file), GFP_KERNEL); if (!of) @@ -376,20 +346,34 @@ static int sysfs_open_file(struct inode *inode, struct file *file) mutex_init(&of->mutex); of->sd = attr_sd; of->file = file; - file->private_data = of; + + /* + * Always instantiate seq_file even if read access is not + * implemented or requested. This unifies private data access and + * most files are readable anyway. + */ + error = single_open(file, sysfs_seq_show, of); + if (error) + goto err_free; + + /* seq_file clears PWRITE unconditionally, restore it if WRITE */ + if (file->f_mode & FMODE_WRITE) + file->f_mode |= FMODE_PWRITE; /* make sure we have open dirent struct */ error = sysfs_get_open_dirent(attr_sd, of); if (error) - goto err_free; + goto err_close; /* open succeeded, put active references */ sysfs_put_active(attr_sd); return 0; - err_free: +err_close: + single_release(inode, file); +err_free: kfree(of); - err_out: +err_out: sysfs_put_active(attr_sd); return error; } @@ -397,12 +381,10 @@ static int sysfs_open_file(struct inode *inode, struct file *file) static int sysfs_release(struct inode *inode, struct file *filp) { struct sysfs_dirent *sd = filp->f_path.dentry->d_fsdata; - struct sysfs_open_file *of = filp->private_data; + struct sysfs_open_file *of = sysfs_of(filp); sysfs_put_open_dirent(sd, of); - - if (of->page) - free_page((unsigned long)of->page); + single_release(inode, filp); kfree(of); return 0; @@ -423,7 +405,7 @@ static int sysfs_release(struct inode *inode, struct file *filp) */ static unsigned int sysfs_poll(struct file *filp, poll_table *wait) { - struct sysfs_open_file *of = filp->private_data; + struct sysfs_open_file *of = sysfs_of(filp); struct sysfs_dirent *attr_sd = filp->f_path.dentry->d_fsdata; struct sysfs_open_dirent *od = attr_sd->s_attr.open; @@ -481,9 +463,9 @@ void sysfs_notify(struct kobject *k, const char *dir, const char *attr) EXPORT_SYMBOL_GPL(sysfs_notify); const struct file_operations sysfs_file_operations = { - .read = sysfs_read_file, + .read = seq_read, .write = sysfs_write_file, - .llseek = generic_file_llseek, + .llseek = seq_lseek, .open = sysfs_open_file, .release = sysfs_release, .poll = sysfs_poll, -- 2.20.1