[PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Tejun Heo]

Hello,

Changes from the last take[L] are,

* bin file reads no longer go through seq_file.  It goes through a
  separate read path implemented in sysfs_bin_read().  bin files
  shouldn't see any behavior difference now.

* bin files now use a separate file_operations struct -
  sysfs_bin_operations.  Open and write paths are still shared but
  read path is separate and mmap exists only for the bin files.  While
  this is less uniform than before, it should still render itself well
  to extracting the core functionality.

* 0001-0008 are the same as before.

Patchset description follows.

Currently, sysfs's file handling is a bit weird.

* Regular and bin file paths are similar but implemented completely
  separately duplicating some hairy logics.

* Read path implements custom buffering which is essentially
  degenerate seq_file.

In addition, sysfs core implementation is planned to be separated out
so that it can be shared by other subsystems and the current file
handling is too restrictive and quirky to spread further to other
parts of the kernel.  It'd be a lot more desirable to have read path
completely handled by seq_file which is a lot more versatile and would
also increase overall behavior consistency.

This patchset updates file handling such that read is handled by
seq_file and then merges bin file handling into regular file path.
While some changes introduces behavior changes in extreme corner
cases, they are highly unlikely to be noticeable (please refer to the
description of each patch for details) and generally bring sysfs's
behavior closer to those of procfs or any pseudo filesystem which
makes use of seq_file.

After the conversion, LOC is reduced by ~150 lines and read path is
fully handled by seq_file, which allows defining a new seq_file based
core interface which will enable sharing sysfs from other subsystems.

This patchset contains the following patches.

 0001-sysfs-remove-unused-sysfs_buffer-pos.patch
 0002-sysfs-remove-sysfs_buffer-needs_read_fill.patch
 0003-sysfs-remove-sysfs_buffer-ops.patch
 0004-sysfs-add-sysfs_open_file_mutex.patch
 0005-sysfs-rename-sysfs_buffer-to-sysfs_open_file.patch
 0006-sysfs-add-sysfs_open_file-sd-and-file.patch
 0007-sysfs-use-transient-write-buffer.patch
 0008-sysfs-use-seq_file-when-reading-regular-files.patch
 0009-sysfs-skip-bin_buffer-buffer-while-reading.patch
 0010-sysfs-collapse-fs-sysfs-bin.c-fill_read-into-read.patch
 0011-sysfs-prepare-path-write-for-unified-regular-bin-fil.patch
 0012-sysfs-add-sysfs_bin_read.patch
 0013-sysfs-copy-bin-mmap-support-from-fs-sysfs-bin.c-to-f.patch
 0014-sysfs-prepare-open-path-for-unified-regular-bin-file.patch
 0015-sysfs-merge-regular-and-bin-file-handling.patch

0001-0006 are misc preps.

0007 makes write path use transient buffer instead of the one
persistent during open.

0008 makes read path use seq_file.

0009-0011 prepare for merging bin and regular file handling.

0012-0015 merge bin file handling into regular file support.

The patches are on top of

  linus#master c2d95729e3 ("Merge branch 'akpm' (patches from Andrew Morton)")
+ [1] [PATCHSET] sysfs: disentangle kobject namespace handling from sysfs
+ [2] [PATCHSET] sysfs: implement sysfs_remove()

and available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-sysfs-seq_file

diffstat follows, thanks.

 Documentation/DocBook/filesystems.tmpl |    1
 fs/sysfs/Makefile                      |    3
 fs/sysfs/bin.c                         |  502 ---------------------
 fs/sysfs/dir.c                         |    2
 fs/sysfs/file.c                        |  766 ++++++++++++++++++++++++---------
 fs/sysfs/inode.c                       |    2
 fs/sysfs/sysfs.h                       |    7
 7 files changed, 567 insertions(+), 716 deletions(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.kernel/1569578
[1] http://thread.gmane.org/gmane.linux.kernel/1560372
[2] http://thread.gmane.org/gmane.linux.kernel/1564002

[PATCH 01/15] sysfs: remove unused sysfs_buffer->pos

[Tejun Heo]

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 1656a79..81e3f72 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -44,7 +44,6 @@ struct sysfs_open_dirent {
 
 struct sysfs_buffer {
 	size_t			count;
-	loff_t			pos;
 	char			*page;
 	const struct sysfs_ops	*ops;
 	struct mutex		mutex;
-- 
1.8.3.1

[PATCH 02/15] sysfs: remove sysfs_buffer->needs_read_fill

[Tejun Heo]

->needs_read_fill is used to implement the following behaviors.

1. Ensure buffer filling on the first read.
2. Force buffer filling after a write.
3. Force buffer filling after a successful poll.

However, #2 and #3 don't really work as sysfs doesn't reset file
position.  While the read buffer would be refilled, the next read
would continue from the position after the last read or write,
requiring an explicit seek to the start for it to be useful, which
makes ->needs_read_fill superflous as read buffer is always refilled
if f_pos == 0.

Update sysfs_read_file() to test buffer->page for #1 instead and
remove ->needs_read_fill.  While this changes behavior in extreme
corner cases - e.g. re-reading a sysfs file after seeking to non-zero
position after a write or poll, it's highly unlikely to lead to actual
breakage.  This change is to prepare for using seq_file in the read
path.

While at it, reformat a comment in fill_write_buffer().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay@vrfy.org>
---
 fs/sysfs/file.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 81e3f72..e2fafc0 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -47,7 +47,6 @@ struct sysfs_buffer {
 	char			*page;
 	const struct sysfs_ops	*ops;
 	struct mutex		mutex;
-	int			needs_read_fill;
 	int			event;
 	struct list_head	list;
 };
@@ -95,12 +94,10 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 		/* Try to struggle along */
 		count = PAGE_SIZE - 1;
 	}
-	if (count >= 0) {
-		buffer->needs_read_fill = 0;
+	if (count >= 0)
 		buffer->count = count;
-	} else {
+	else
 		ret = count;
-	}
 	return ret;
 }
 
@@ -130,7 +127,11 @@ sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	ssize_t retval = 0;
 
 	mutex_lock(&buffer->mutex);
-	if (buffer->needs_read_fill || *ppos == 0) {
+	/*
+	 * 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 || !buffer->page) {
 		retval = fill_read_buffer(file->f_path.dentry, buffer);
 		if (retval)
 			goto out;
@@ -166,14 +167,15 @@ static int fill_write_buffer(struct sysfs_buffer *buffer,
 	if (count >= PAGE_SIZE)
 		count = PAGE_SIZE - 1;
 	error = copy_from_user(buffer->page, buf, count);
-	buffer->needs_read_fill = 1;
-	/* if buf is assumed to contain a string, terminate it by \0,
-	   so e.g. sscanf() can scan the string easily */
+
+	/*
+	 * If buf is assumed to contain a string, terminate it by \0, so
+	 * e.g. sscanf() can scan the string easily.
+	 */
 	buffer->page[count] = 0;
 	return error ? -EFAULT : count;
 }
 
-
 /**
  *	flush_write_buffer - push buffer to kobject.
  *	@dentry:	dentry to the attribute
@@ -368,7 +370,6 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		goto err_out;
 
 	mutex_init(&buffer->mutex);
-	buffer->needs_read_fill = 1;
 	buffer->ops = ops;
 	file->private_data = buffer;
 
@@ -435,7 +436,6 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 	return DEFAULT_POLLMASK;
 
  trigger:
-	buffer->needs_read_fill = 1;
 	return DEFAULT_POLLMASK|POLLERR|POLLPRI;
 }
 
-- 
1.8.3.1

[PATCH 03/15] sysfs: remove sysfs_buffer->ops

[Tejun Heo]

Currently, sysfs_ops is fetched during sysfs_open_file() and cached in
sysfs_buffer->ops to be used while the file is open.  This patch
removes the caching and makes each operation directly fetch sysfs_ops.

This patch doesn't introduce any behavior difference and is to prepare
for merging regular and bin file supports.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e2fafc0..7dfcc33 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -45,12 +45,23 @@ struct sysfs_open_dirent {
 struct sysfs_buffer {
 	size_t			count;
 	char			*page;
-	const struct sysfs_ops	*ops;
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
 };
 
+/*
+ * Determine ktype->sysfs_ops for the given sysfs_dirent.  This function
+ * must be called while holding an active reference.
+ */
+static const struct sysfs_ops *sysfs_file_ops(struct sysfs_dirent *sd)
+{
+	struct kobject *kobj = sd->s_parent->s_dir.kobj;
+
+	lockdep_assert_held(sd);
+	return kobj->ktype ? kobj->ktype->sysfs_ops : NULL;
+}
+
 /**
  *	fill_read_buffer - allocate and fill buffer from object.
  *	@dentry:	dentry pointer.
@@ -66,7 +77,7 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 {
 	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	const struct sysfs_ops *ops = buffer->ops;
+	const struct sysfs_ops *ops;
 	int ret = 0;
 	ssize_t count;
 
@@ -80,6 +91,8 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 		return -ENODEV;
 
 	buffer->event = atomic_read(&attr_sd->s_attr.open->event);
+
+	ops = sysfs_file_ops(attr_sd);
 	count = ops->show(kobj, attr_sd->s_attr.attr, buffer->page);
 
 	sysfs_put_active(attr_sd);
@@ -191,13 +204,14 @@ static int flush_write_buffer(struct dentry *dentry,
 {
 	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	const struct sysfs_ops *ops = buffer->ops;
+	const struct sysfs_ops *ops;
 	int rc;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
 	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
+	ops = sysfs_file_ops(attr_sd);
 	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
 
 	sysfs_put_active(attr_sd);
@@ -205,7 +219,6 @@ static int flush_write_buffer(struct dentry *dentry,
 	return rc;
 }
 
-
 /**
  *	sysfs_write_file - write an attribute.
  *	@file:	file pointer
@@ -334,14 +347,11 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	/* every kobject with an attribute needs a ktype assigned */
-	if (kobj->ktype && kobj->ktype->sysfs_ops)
-		ops = kobj->ktype->sysfs_ops;
-	else {
-		WARN(1, KERN_ERR
-		     "missing sysfs attribute operations for kobject: %s\n",
-		     kobject_name(kobj));
+	ops = sysfs_file_ops(attr_sd);
+	if (WARN(!ops, KERN_ERR
+		 "missing sysfs attribute operations for kobject: %s\n",
+		 kobject_name(kobj)))
 		goto err_out;
-	}
 
 	/* File needs write support.
 	 * The inode's perms must say it's ok,
@@ -370,7 +380,6 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		goto err_out;
 
 	mutex_init(&buffer->mutex);
-	buffer->ops = ops;
 	file->private_data = buffer;
 
 	/* make sure we have open dirent struct */
-- 
1.8.3.1

[PATCH 04/15] sysfs: add sysfs_open_file_mutex

[Tejun Heo]

Add a separate mutex to protect sysfs_open_dirent->buffers list.  This
will allow performing sleepable operations while traversing
sysfs_buffers, which will be renamed to sysfs_open_file.

Note that currently sysfs_open_dirent->buffers list isn't being used
for anything and this patch doesn't make any functional difference.
It will be used to merge regular and bin file supports.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 7dfcc33..499cff8 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -25,15 +25,17 @@
 #include "sysfs.h"
 
 /*
- * There's one sysfs_buffer for each open file and one
- * sysfs_open_dirent for each sysfs_dirent with one or more open
- * files.
+ * There's one sysfs_buffer for each open file and one sysfs_open_dirent
+ * for each sysfs_dirent with one or more open files.
  *
- * filp->private_data points to sysfs_buffer and
- * sysfs_dirent->s_attr.open points to sysfs_open_dirent.  s_attr.open
- * is protected by sysfs_open_dirent_lock.
+ * 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_buffer which is chained at
+ * sysfs_open_dirent->buffers, which is protected by sysfs_open_file_mutex.
  */
 static DEFINE_SPINLOCK(sysfs_open_dirent_lock);
+static DEFINE_MUTEX(sysfs_open_file_mutex);
 
 struct sysfs_open_dirent {
 	atomic_t		refcnt;
@@ -272,6 +274,7 @@ static int sysfs_get_open_dirent(struct sysfs_dirent *sd,
 	struct sysfs_open_dirent *od, *new_od = NULL;
 
  retry:
+	mutex_lock(&sysfs_open_file_mutex);
 	spin_lock_irq(&sysfs_open_dirent_lock);
 
 	if (!sd->s_attr.open && new_od) {
@@ -286,6 +289,7 @@ static int sysfs_get_open_dirent(struct sysfs_dirent *sd,
 	}
 
 	spin_unlock_irq(&sysfs_open_dirent_lock);
+	mutex_unlock(&sysfs_open_file_mutex);
 
 	if (od) {
 		kfree(new_od);
@@ -321,6 +325,7 @@ static void sysfs_put_open_dirent(struct sysfs_dirent *sd,
 	struct sysfs_open_dirent *od = sd->s_attr.open;
 	unsigned long flags;
 
+	mutex_lock(&sysfs_open_file_mutex);
 	spin_lock_irqsave(&sysfs_open_dirent_lock, flags);
 
 	list_del(&buffer->list);
@@ -330,6 +335,7 @@ static void sysfs_put_open_dirent(struct sysfs_dirent *sd,
 		od = NULL;
 
 	spin_unlock_irqrestore(&sysfs_open_dirent_lock, flags);
+	mutex_unlock(&sysfs_open_file_mutex);
 
 	kfree(od);
 }
-- 
1.8.3.1

[PATCH 05/15] sysfs: rename sysfs_buffer to sysfs_open_file

[Tejun Heo]

sysfs read path will be converted to use seq_file which will handle
buffering making sysfs_buffer a misnomer.  Rename sysfs_buffer to
sysfs_open_file, and sysfs_open_dirent->buffers to ->files.

This path is pure rename.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 127 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 63 insertions(+), 64 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 499cff8..4b55bcf 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -25,14 +25,14 @@
 #include "sysfs.h"
 
 /*
- * There's one sysfs_buffer for each open file and one sysfs_open_dirent
+ * There's one sysfs_open_file for each open file and one sysfs_open_dirent
  * for each sysfs_dirent with one or more open files.
  *
  * 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_buffer which is chained at
- * sysfs_open_dirent->buffers, which is protected by sysfs_open_file_mutex.
+ * filp->private_data points to sysfs_open_file which is chained at
+ * sysfs_open_dirent->files, which is protected by sysfs_open_file_mutex.
  */
 static DEFINE_SPINLOCK(sysfs_open_dirent_lock);
 static DEFINE_MUTEX(sysfs_open_file_mutex);
@@ -41,10 +41,10 @@ struct sysfs_open_dirent {
 	atomic_t		refcnt;
 	atomic_t		event;
 	wait_queue_head_t	poll;
-	struct list_head	buffers; /* goes through sysfs_buffer.list */
+	struct list_head	files; /* goes through sysfs_open_file.list */
 };
 
-struct sysfs_buffer {
+struct sysfs_open_file {
 	size_t			count;
 	char			*page;
 	struct mutex		mutex;
@@ -75,7 +75,7 @@ static const struct sysfs_ops *sysfs_file_ops(struct sysfs_dirent *sd)
  *	This is called only once, on the file's first read unless an error
  *	is returned.
  */
-static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
+static int fill_read_buffer(struct dentry *dentry, struct sysfs_open_file *of)
 {
 	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
@@ -83,19 +83,19 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 	int ret = 0;
 	ssize_t count;
 
-	if (!buffer->page)
-		buffer->page = (char *) get_zeroed_page(GFP_KERNEL);
-	if (!buffer->page)
+	if (!of->page)
+		of->page = (char *) get_zeroed_page(GFP_KERNEL);
+	if (!of->page)
 		return -ENOMEM;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
 	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
-	buffer->event = atomic_read(&attr_sd->s_attr.open->event);
+	of->event = atomic_read(&attr_sd->s_attr.open->event);
 
 	ops = sysfs_file_ops(attr_sd);
-	count = ops->show(kobj, attr_sd->s_attr.attr, buffer->page);
+	count = ops->show(kobj, attr_sd->s_attr.attr, of->page);
 
 	sysfs_put_active(attr_sd);
 
@@ -110,7 +110,7 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 		count = PAGE_SIZE - 1;
 	}
 	if (count >= 0)
-		buffer->count = count;
+		of->count = count;
 	else
 		ret = count;
 	return ret;
@@ -138,63 +138,62 @@ static int fill_read_buffer(struct dentry *dentry, struct sysfs_buffer *buffer)
 static ssize_t
 sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
-	struct sysfs_buffer *buffer = file->private_data;
+	struct sysfs_open_file *of = file->private_data;
 	ssize_t retval = 0;
 
-	mutex_lock(&buffer->mutex);
+	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 || !buffer->page) {
-		retval = fill_read_buffer(file->f_path.dentry, buffer);
+	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, buffer->page);
-	retval = simple_read_from_buffer(buf, count, ppos, buffer->page,
-					 buffer->count);
+		 __func__, count, *ppos, of->page);
+	retval = simple_read_from_buffer(buf, count, ppos, of->page, of->count);
 out:
-	mutex_unlock(&buffer->mutex);
+	mutex_unlock(&of->mutex);
 	return retval;
 }
 
 /**
  *	fill_write_buffer - copy buffer from userspace.
- *	@buffer:	data buffer for file.
+ *	@of:		open file struct.
  *	@buf:		data from user.
  *	@count:		number of bytes in @userbuf.
  *
- *	Allocate @buffer->page if it hasn't been already, then
- *	copy the user-supplied buffer into it.
+ *	Allocate @of->page if it hasn't been already, then copy the
+ *	user-supplied buffer into it.
  */
-static int fill_write_buffer(struct sysfs_buffer *buffer,
+static int fill_write_buffer(struct sysfs_open_file *of,
 			     const char __user *buf, size_t count)
 {
 	int error;
 
-	if (!buffer->page)
-		buffer->page = (char *)get_zeroed_page(GFP_KERNEL);
-	if (!buffer->page)
+	if (!of->page)
+		of->page = (char *)get_zeroed_page(GFP_KERNEL);
+	if (!of->page)
 		return -ENOMEM;
 
 	if (count >= PAGE_SIZE)
 		count = PAGE_SIZE - 1;
-	error = copy_from_user(buffer->page, buf, count);
+	error = copy_from_user(of->page, buf, count);
 
 	/*
 	 * If buf is assumed to contain a string, terminate it by \0, so
 	 * e.g. sscanf() can scan the string easily.
 	 */
-	buffer->page[count] = 0;
+	of->page[count] = 0;
 	return error ? -EFAULT : count;
 }
 
 /**
  *	flush_write_buffer - push buffer to kobject.
  *	@dentry:	dentry to the attribute
- *	@buffer:	data buffer for file.
+ *	@of:		open file
  *	@count:		number of bytes
  *
  *	Get the correct pointers for the kobject and the attribute we're
@@ -202,7 +201,7 @@ static int fill_write_buffer(struct sysfs_buffer *buffer,
  *	passing the buffer that we acquired in fill_write_buffer().
  */
 static int flush_write_buffer(struct dentry *dentry,
-			      struct sysfs_buffer *buffer, size_t count)
+			      struct sysfs_open_file *of, size_t count)
 {
 	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
@@ -214,7 +213,7 @@ static int flush_write_buffer(struct dentry *dentry,
 		return -ENODEV;
 
 	ops = sysfs_file_ops(attr_sd);
-	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
+	rc = ops->store(kobj, attr_sd->s_attr.attr, of->page, count);
 
 	sysfs_put_active(attr_sd);
 
@@ -240,27 +239,26 @@ static int flush_write_buffer(struct dentry *dentry,
 static ssize_t sysfs_write_file(struct file *file, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	struct sysfs_buffer *buffer = file->private_data;
+	struct sysfs_open_file *of = file->private_data;
 	ssize_t len;
 
-	mutex_lock(&buffer->mutex);
-	len = fill_write_buffer(buffer, buf, count);
+	mutex_lock(&of->mutex);
+	len = fill_write_buffer(of, buf, count);
 	if (len > 0)
-		len = flush_write_buffer(file->f_path.dentry, buffer, len);
+		len = flush_write_buffer(file->f_path.dentry, of, len);
 	if (len > 0)
 		*ppos += len;
-	mutex_unlock(&buffer->mutex);
+	mutex_unlock(&of->mutex);
 	return len;
 }
 
 /**
  *	sysfs_get_open_dirent - get or create sysfs_open_dirent
  *	@sd: target sysfs_dirent
- *	@buffer: sysfs_buffer for this instance of open
+ *	@of: sysfs_open_file for this instance of open
  *
  *	If @sd->s_attr.open exists, increment its reference count;
- *	otherwise, create one.  @buffer is chained to the buffers
- *	list.
+ *	otherwise, create one.  @of is chained to the files list.
  *
  *	LOCKING:
  *	Kernel thread context (may sleep).
@@ -269,7 +267,7 @@ static ssize_t sysfs_write_file(struct file *file, const char __user *buf,
  *	0 on success, -errno on failure.
  */
 static int sysfs_get_open_dirent(struct sysfs_dirent *sd,
-				 struct sysfs_buffer *buffer)
+				 struct sysfs_open_file *of)
 {
 	struct sysfs_open_dirent *od, *new_od = NULL;
 
@@ -285,7 +283,7 @@ static int sysfs_get_open_dirent(struct sysfs_dirent *sd,
 	od = sd->s_attr.open;
 	if (od) {
 		atomic_inc(&od->refcnt);
-		list_add_tail(&buffer->list, &od->buffers);
+		list_add_tail(&of->list, &od->files);
 	}
 
 	spin_unlock_irq(&sysfs_open_dirent_lock);
@@ -304,23 +302,23 @@ static int sysfs_get_open_dirent(struct sysfs_dirent *sd,
 	atomic_set(&new_od->refcnt, 0);
 	atomic_set(&new_od->event, 1);
 	init_waitqueue_head(&new_od->poll);
-	INIT_LIST_HEAD(&new_od->buffers);
+	INIT_LIST_HEAD(&new_od->files);
 	goto retry;
 }
 
 /**
  *	sysfs_put_open_dirent - put sysfs_open_dirent
  *	@sd: target sysfs_dirent
- *	@buffer: associated sysfs_buffer
+ *	@of: associated sysfs_open_file
  *
- *	Put @sd->s_attr.open and unlink @buffer from the buffers list.
- *	If reference count reaches zero, disassociate and free it.
+ *	Put @sd->s_attr.open and unlink @of from the files list.  If
+ *	reference count reaches zero, disassociate and free it.
  *
  *	LOCKING:
  *	None.
  */
 static void sysfs_put_open_dirent(struct sysfs_dirent *sd,
-				  struct sysfs_buffer *buffer)
+				  struct sysfs_open_file *of)
 {
 	struct sysfs_open_dirent *od = sd->s_attr.open;
 	unsigned long flags;
@@ -328,7 +326,7 @@ static void sysfs_put_open_dirent(struct sysfs_dirent *sd,
 	mutex_lock(&sysfs_open_file_mutex);
 	spin_lock_irqsave(&sysfs_open_dirent_lock, flags);
 
-	list_del(&buffer->list);
+	list_del(&of->list);
 	if (atomic_dec_and_test(&od->refcnt))
 		sd->s_attr.open = NULL;
 	else
@@ -344,7 +342,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 {
 	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	struct sysfs_buffer *buffer;
+	struct sysfs_open_file *of;
 	const struct sysfs_ops *ops;
 	int error = -EACCES;
 
@@ -377,19 +375,20 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 			goto err_out;
 	}
 
-	/* No error? Great, allocate a buffer for the file, and store it
-	 * it in file->private_data for easy access.
+	/*
+	 * No error? Great, allocate a sysfs_open_file for the file, and
+	 * store it it in file->private_data for easy access.
 	 */
 	error = -ENOMEM;
-	buffer = kzalloc(sizeof(struct sysfs_buffer), GFP_KERNEL);
-	if (!buffer)
+	of = kzalloc(sizeof(struct sysfs_open_file), GFP_KERNEL);
+	if (!of)
 		goto err_out;
 
-	mutex_init(&buffer->mutex);
-	file->private_data = buffer;
+	mutex_init(&of->mutex);
+	file->private_data = of;
 
 	/* make sure we have open dirent struct */
-	error = sysfs_get_open_dirent(attr_sd, buffer);
+	error = sysfs_get_open_dirent(attr_sd, of);
 	if (error)
 		goto err_free;
 
@@ -398,7 +397,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 	return 0;
 
  err_free:
-	kfree(buffer);
+	kfree(of);
  err_out:
 	sysfs_put_active(attr_sd);
 	return error;
@@ -407,13 +406,13 @@ 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_buffer *buffer = filp->private_data;
+	struct sysfs_open_file *of = filp->private_data;
 
-	sysfs_put_open_dirent(sd, buffer);
+	sysfs_put_open_dirent(sd, of);
 
-	if (buffer->page)
-		free_page((unsigned long)buffer->page);
-	kfree(buffer);
+	if (of->page)
+		free_page((unsigned long)of->page);
+	kfree(of);
 
 	return 0;
 }
@@ -433,7 +432,7 @@ static int sysfs_release(struct inode *inode, struct file *filp)
  */
 static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 {
-	struct sysfs_buffer *buffer = filp->private_data;
+	struct sysfs_open_file *of = filp->private_data;
 	struct sysfs_dirent *attr_sd = filp->f_path.dentry->d_fsdata;
 	struct sysfs_open_dirent *od = attr_sd->s_attr.open;
 
@@ -445,7 +444,7 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 
 	sysfs_put_active(attr_sd);
 
-	if (buffer->event != atomic_read(&od->event))
+	if (of->event != atomic_read(&od->event))
 		goto trigger;
 
 	return DEFAULT_POLLMASK;
-- 
1.8.3.1

[PATCH 06/15] sysfs: add sysfs_open_file->sd and ->file

[Tejun Heo]

sysfs will be converted to use seq_file for read path, which will make
it difficult to pass around multiple pointers directly.  This patch
adds sysfs_open_file->sd and ->file so that we can reach all the
necessary data structures from sysfs_open_file.

flush_write_buffer() is updated to drop @dentry which was used to
discover the sysfs_dirent as it's now available through
sysfs_open_file->sd.

This patch doesn't cause any behavior difference.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4b55bcf..af6e909 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -45,6 +45,8 @@ struct sysfs_open_dirent {
 };
 
 struct sysfs_open_file {
+	struct sysfs_dirent	*sd;
+	struct file		*file;
 	size_t			count;
 	char			*page;
 	struct mutex		mutex;
@@ -192,7 +194,6 @@ static int fill_write_buffer(struct sysfs_open_file *of,
 
 /**
  *	flush_write_buffer - push buffer to kobject.
- *	@dentry:	dentry to the attribute
  *	@of:		open file
  *	@count:		number of bytes
  *
@@ -200,22 +201,20 @@ static int fill_write_buffer(struct sysfs_open_file *of,
  *	dealing with, then call the store() method for the attribute,
  *	passing the buffer that we acquired in fill_write_buffer().
  */
-static int flush_write_buffer(struct dentry *dentry,
-			      struct sysfs_open_file *of, size_t count)
+static int flush_write_buffer(struct sysfs_open_file *of, size_t count)
 {
-	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
-	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
+	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
 	const struct sysfs_ops *ops;
 	int rc;
 
-	/* 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 */
+	if (!sysfs_get_active(of->sd))
 		return -ENODEV;
 
-	ops = sysfs_file_ops(attr_sd);
-	rc = ops->store(kobj, attr_sd->s_attr.attr, of->page, count);
+	ops = sysfs_file_ops(of->sd);
+	rc = ops->store(kobj, of->sd->s_attr.attr, of->page, count);
 
-	sysfs_put_active(attr_sd);
+	sysfs_put_active(of->sd);
 
 	return rc;
 }
@@ -245,7 +244,7 @@ static ssize_t sysfs_write_file(struct file *file, const char __user *buf,
 	mutex_lock(&of->mutex);
 	len = fill_write_buffer(of, buf, count);
 	if (len > 0)
-		len = flush_write_buffer(file->f_path.dentry, of, len);
+		len = flush_write_buffer(of, len);
 	if (len > 0)
 		*ppos += len;
 	mutex_unlock(&of->mutex);
@@ -385,6 +384,8 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		goto err_out;
 
 	mutex_init(&of->mutex);
+	of->sd = attr_sd;
+	of->file = file;
 	file->private_data = of;
 
 	/* make sure we have open dirent struct */
-- 
1.8.3.1

[PATCH 07/15] sysfs: use transient write buffer

[Tejun Heo]

There isn't much to be gained by keeping around kernel buffer while a
file is open especially as the read path planned to be converted to
use seq_file and won't use the buffer.  This patch makes
sysfs_write_file() use per-write transient buffer instead of
sysfs_open_file->page.

This simplifies the write path, enables removing sysfs_open_file->page
once read path is updated and will help merging bin file write path
which already requires the use of a transient buffer due to a locking
order issue.

As the function comments of flush_write_buffer() and
sysfs_write_buffer() are being updated anyway, reformat them so that
they're more conventional.

v2: Use min_t() instead of min() in sysfs_write_file() to avoid build
    warning on arm.  Reported by build test robot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: kbuild test robot <fengguang.wu@intel.com>
---
 fs/sysfs/file.c | 114 ++++++++++++++++++++++++++------------------------------
 1 file changed, 52 insertions(+), 62 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index af6e909..53cc096 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -162,92 +162,82 @@ out:
 }
 
 /**
- *	fill_write_buffer - copy buffer from userspace.
- *	@of:		open file struct.
- *	@buf:		data from user.
- *	@count:		number of bytes in @userbuf.
+ * flush_write_buffer - push buffer to kobject
+ * @of: open file
+ * @buf: data buffer for file
+ * @count: number of bytes
  *
- *	Allocate @of->page if it hasn't been already, then copy the
- *	user-supplied buffer into it.
+ * Get the correct pointers for the kobject and the attribute we're dealing
+ * with, then call the store() method for it with @buf.
  */
-static int fill_write_buffer(struct sysfs_open_file *of,
-			     const char __user *buf, size_t count)
-{
-	int error;
-
-	if (!of->page)
-		of->page = (char *)get_zeroed_page(GFP_KERNEL);
-	if (!of->page)
-		return -ENOMEM;
-
-	if (count >= PAGE_SIZE)
-		count = PAGE_SIZE - 1;
-	error = copy_from_user(of->page, buf, count);
-
-	/*
-	 * If buf is assumed to contain a string, terminate it by \0, so
-	 * e.g. sscanf() can scan the string easily.
-	 */
-	of->page[count] = 0;
-	return error ? -EFAULT : count;
-}
-
-/**
- *	flush_write_buffer - push buffer to kobject.
- *	@of:		open file
- *	@count:		number of bytes
- *
- *	Get the correct pointers for the kobject and the attribute we're
- *	dealing with, then call the store() method for the attribute,
- *	passing the buffer that we acquired in fill_write_buffer().
- */
-static int flush_write_buffer(struct sysfs_open_file *of, size_t count)
+static int flush_write_buffer(struct sysfs_open_file *of, char *buf,
+			      size_t count)
 {
 	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
 	const struct sysfs_ops *ops;
-	int rc;
+	int rc = 0;
 
-	/* need @of->sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active(of->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;
+	}
 
 	ops = sysfs_file_ops(of->sd);
-	rc = ops->store(kobj, of->sd->s_attr.attr, of->page, count);
+	rc = ops->store(kobj, of->sd->s_attr.attr, buf, count);
 
 	sysfs_put_active(of->sd);
+	mutex_unlock(&of->mutex);
 
 	return rc;
 }
 
 /**
- *	sysfs_write_file - write an attribute.
- *	@file:	file pointer
- *	@buf:	data to write
- *	@count:	number of bytes
- *	@ppos:	starting offset
+ * sysfs_write_file - write an attribute
+ * @file: file pointer
+ * @user_buf: data to write
+ * @count: number of bytes
+ * @ppos: starting offset
  *
- *	Similar to sysfs_read_file(), though working in the opposite direction.
- *	We allocate and fill the data from the user in fill_write_buffer(),
- *	then push it to the kobject in flush_write_buffer().
- *	There is no easy way for us to know if userspace is only doing a partial
- *	write, so we don't support them. We expect the entire buffer to come
- *	on the first write.
- *	Hint: if you're writing a value, first read the file, modify only the
- *	the value you're changing, then write entire buffer back.
+ * Copy data in from userland and pass it to the matching
+ * sysfs_ops->store() by invoking flush_write_buffer().
+ *
+ * There is no easy way for us to know if userspace is only doing a partial
+ * write, so we don't support them. We expect the entire buffer to come on
+ * the first write.  Hint: if you're writing a value, first read the file,
+ * modify only the the value you're changing, then write entire buffer
+ * back.
  */
-static ssize_t sysfs_write_file(struct file *file, const char __user *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;
-	ssize_t len;
+	ssize_t len = min_t(size_t, count, PAGE_SIZE - 1);
+	char *buf;
 
-	mutex_lock(&of->mutex);
-	len = fill_write_buffer(of, buf, count);
-	if (len > 0)
-		len = flush_write_buffer(of, len);
+	if (!len)
+		return 0;
+
+	buf = kmalloc(len + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	if (copy_from_user(buf, user_buf, len)) {
+		len = -EFAULT;
+		goto out_free;
+	}
+	buf[len] = '\0';	/* guarantee string termination */
+
+	len = flush_write_buffer(of, buf, len);
 	if (len > 0)
 		*ppos += len;
-	mutex_unlock(&of->mutex);
+out_free:
+	kfree(buf);
 	return len;
 }
 
-- 
1.8.3.1

[PATCH 08/15] sysfs: use seq_file when reading regular files

[Tejun Heo]

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 <tj@kernel.org>
Cc: Kay Sievers <kay@vrfy.org>
---
 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 53cc096..4921bda 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/limits.h>
 #include <linux/uaccess.h>
+#include <linux/seq_file.h>
 
 #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,
-- 
1.8.3.1

[PATCH 09/15] sysfs: skip bin_buffer->buffer while reading

[Tejun Heo]

After b31ca3f5dfc ("sysfs: fix deadlock"), bin read() first writes
data to bb->buffer and bounces it to a transient kernel buffer which
is then copied out to userland.  The double bouncing doesn't add
anything.  Let's just use the transient buffer directly.

While at it, rename @temp to @buf for clarity.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/bin.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index d49e6ca..d2142c0 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -72,7 +72,7 @@ read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 	int size = file_inode(file)->i_size;
 	loff_t offs = *off;
 	int count = min_t(size_t, bytes, PAGE_SIZE);
-	char *temp;
+	char *buf;
 
 	if (!bytes)
 		return 0;
@@ -84,23 +84,18 @@ read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 			count = size - offs;
 	}
 
-	temp = kmalloc(count, GFP_KERNEL);
-	if (!temp)
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
 		return -ENOMEM;
 
 	mutex_lock(&bb->mutex);
+	count = fill_read(file, buf, offs, count);
+	mutex_unlock(&bb->mutex);
 
-	count = fill_read(file, bb->buffer, offs, count);
-	if (count < 0) {
-		mutex_unlock(&bb->mutex);
+	if (count < 0)
 		goto out_free;
-	}
-
-	memcpy(temp, bb->buffer, count);
 
-	mutex_unlock(&bb->mutex);
-
-	if (copy_to_user(userbuf, temp, count)) {
+	if (copy_to_user(userbuf, buf, count)) {
 		count = -EFAULT;
 		goto out_free;
 	}
@@ -110,7 +105,7 @@ read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 	*off = offs + count;
 
  out_free:
-	kfree(temp);
+	kfree(buf);
 	return count;
 }
 
-- 
1.8.3.1

[PATCH 10/15] sysfs: collapse fs/sysfs/bin.c::fill_read() into read()

[Tejun Heo]

read() is simple enough and fill_read() being in a separate function
doesn't add anything.  Let's collapse it into read().  This will make
merging bin file handling with regular file.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/bin.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index d2142c0..60a4e78 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -44,30 +44,12 @@ struct bin_buffer {
 	struct hlist_node		list;
 };
 
-static int
-fill_read(struct file *file, char *buffer, loff_t off, size_t count)
+static ssize_t
+read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 {
 	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
 	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	int rc;
-
-	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active(attr_sd))
-		return -ENODEV;
-
-	rc = -EIO;
-	if (attr->read)
-		rc = attr->read(file, kobj, attr, buffer, off, count);
-
-	sysfs_put_active(attr_sd);
-
-	return rc;
-}
-
-static ssize_t
-read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
-{
 	struct bin_buffer *bb = file->private_data;
 	int size = file_inode(file)->i_size;
 	loff_t offs = *off;
@@ -88,8 +70,20 @@ read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 	if (!buf)
 		return -ENOMEM;
 
+	/* need attr_sd for attr, its parent for kobj */
 	mutex_lock(&bb->mutex);
-	count = fill_read(file, buf, offs, count);
+	if (!sysfs_get_active(attr_sd)) {
+		count = -ENODEV;
+		mutex_unlock(&bb->mutex);
+		goto out_free;
+	}
+
+	if (attr->read)
+		count = attr->read(file, kobj, attr, buf, offs, count);
+	else
+		count = -EIO;
+
+	sysfs_put_active(attr_sd);
 	mutex_unlock(&bb->mutex);
 
 	if (count < 0)
-- 
1.8.3.1

[PATCH 11/15] sysfs: prepare path write for unified regular / bin file handling

[Tejun Heo]

sysfs bin file handling will be merged into the regular file support.
This patch prepares the write path.

bin file write is almost identical to regular file write except that
the write length is capped by the inode size and @off is passed to the
write method.  This patch adds bin file handling to sysfs_write_file()
so that it can handle both regular and bin files.

A new file_operations struct sysfs_bin_operations is added, which
currently only hosts sysfs_write_file() and generic_file_llseek().
This isn't used yet but will eventually replace fs/sysfs/bin.c.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c  | 40 ++++++++++++++++++++++++++++++++++------
 fs/sysfs/sysfs.h |  1 +
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4921bda..b36473f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -54,6 +54,11 @@ struct sysfs_open_file {
 	struct list_head	list;
 };
 
+static bool sysfs_is_bin(struct sysfs_dirent *sd)
+{
+	return sysfs_type(sd) == SYSFS_KOBJ_BIN_ATTR;
+}
+
 static struct sysfs_open_file *sysfs_of(struct file *file)
 {
 	return ((struct seq_file *)file->private_data)->private;
@@ -138,16 +143,16 @@ static int sysfs_seq_show(struct seq_file *sf, void *v)
  * flush_write_buffer - push buffer to kobject
  * @of: open file
  * @buf: data buffer for file
+ * @off: file offset to write to
  * @count: number of bytes
  *
  * Get the correct pointers for the kobject and the attribute we're dealing
  * with, then call the store() method for it with @buf.
  */
-static int flush_write_buffer(struct sysfs_open_file *of, char *buf,
+static int flush_write_buffer(struct sysfs_open_file *of, char *buf, loff_t off,
 			      size_t count)
 {
 	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
-	const struct sysfs_ops *ops;
 	int rc = 0;
 
 	/*
@@ -161,8 +166,18 @@ static int flush_write_buffer(struct sysfs_open_file *of, char *buf,
 		return -ENODEV;
 	}
 
-	ops = sysfs_file_ops(of->sd);
-	rc = ops->store(kobj, of->sd->s_attr.attr, buf, count);
+	if (sysfs_is_bin(of->sd)) {
+		struct bin_attribute *battr = of->sd->s_bin_attr.bin_attr;
+
+		rc = -EIO;
+		if (battr->write)
+			rc = battr->write(of->file, kobj, battr, buf, off,
+					  count);
+	} else {
+		const struct sysfs_ops *ops = sysfs_file_ops(of->sd);
+
+		rc = ops->store(kobj, of->sd->s_attr.attr, buf, count);
+	}
 
 	sysfs_put_active(of->sd);
 	mutex_unlock(&of->mutex);
@@ -190,9 +205,17 @@ 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 = sysfs_of(file);
-	ssize_t len = min_t(size_t, count, PAGE_SIZE - 1);
+	ssize_t len = min_t(size_t, count, PAGE_SIZE);
 	char *buf;
 
+	if (sysfs_is_bin(of->sd)) {
+		loff_t size = file_inode(file)->i_size;
+
+		if (size <= *ppos)
+			return 0;
+		len = min_t(ssize_t, len, size - *ppos);
+	}
+
 	if (!len)
 		return 0;
 
@@ -206,7 +229,7 @@ static ssize_t sysfs_write_file(struct file *file, const char __user *user_buf,
 	}
 	buf[len] = '\0';	/* guarantee string termination */
 
-	len = flush_write_buffer(of, buf, len);
+	len = flush_write_buffer(of, buf, *ppos, len);
 	if (len > 0)
 		*ppos += len;
 out_free:
@@ -471,6 +494,11 @@ const struct file_operations sysfs_file_operations = {
 	.poll		= sysfs_poll,
 };
 
+const struct file_operations sysfs_bin_operations = {
+	.write		= sysfs_write_file,
+	.llseek		= generic_file_llseek,
+};
+
 int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
 			   const struct attribute *attr, int type,
 			   umode_t amode, const void *ns)
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 4b1d825..f103bac 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -212,6 +212,7 @@ int sysfs_inode_init(void);
  * file.c
  */
 extern const struct file_operations sysfs_file_operations;
+extern const struct file_operations sysfs_bin_operations;
 
 int sysfs_add_file(struct sysfs_dirent *dir_sd,
 		   const struct attribute *attr, int type);
-- 
1.8.3.1

[PATCH 12/15] sysfs: add sysfs_bin_read()

[Tejun Heo]

sysfs bin file handling will be merged into the regular file support.
This patch prepares the read path.

Copy fs/sysfs/bin.c::read() to fs/sysfs/file.c and make it use
sysfs_open_file instead of bin_buffer.  The function is identical copy
except for the use of sysfs_open_file.

The new function is added to sysfs_bin_operations.  This isn't used
yet but will eventually replace fs/sysfs/bin.c.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index b36473f..9ba492a 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -139,6 +139,70 @@ static int sysfs_seq_show(struct seq_file *sf, void *v)
 	return 0;
 }
 
+/*
+ * Read method for bin files.  As reading a bin file can have side-effects,
+ * the exact offset and bytes specified in read(2) call should be passed to
+ * the read callback making it difficult to use seq_file.  Implement
+ * simplistic custom buffering for bin files.
+ */
+static ssize_t sysfs_bin_read(struct file *file, char __user *userbuf,
+			      size_t bytes, loff_t *off)
+{
+	struct sysfs_open_file *of = sysfs_of(file);
+	struct bin_attribute *battr = of->sd->s_bin_attr.bin_attr;
+	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
+	int size = file_inode(file)->i_size;
+	int count = min_t(size_t, bytes, PAGE_SIZE);
+	loff_t offs = *off;
+	char *buf;
+
+	if (!bytes)
+		return 0;
+
+	if (size) {
+		if (offs > size)
+			return 0;
+		if (offs + count > size)
+			count = size - offs;
+	}
+
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* need of->sd for battr, its parent for kobj */
+	mutex_lock(&of->mutex);
+	if (!sysfs_get_active(of->sd)) {
+		count = -ENODEV;
+		mutex_unlock(&of->mutex);
+		goto out_free;
+	}
+
+	if (battr->read)
+		count = battr->read(file, kobj, battr, buf, offs, count);
+	else
+		count = -EIO;
+
+	sysfs_put_active(of->sd);
+	mutex_unlock(&of->mutex);
+
+	if (count < 0)
+		goto out_free;
+
+	if (copy_to_user(userbuf, buf, count)) {
+		count = -EFAULT;
+		goto out_free;
+	}
+
+	pr_debug("offs = %lld, *off = %lld, count = %d\n", offs, *off, count);
+
+	*off = offs + count;
+
+ out_free:
+	kfree(buf);
+	return count;
+}
+
 /**
  * flush_write_buffer - push buffer to kobject
  * @of: open file
@@ -495,6 +559,7 @@ const struct file_operations sysfs_file_operations = {
 };
 
 const struct file_operations sysfs_bin_operations = {
+	.read		= sysfs_bin_read,
 	.write		= sysfs_write_file,
 	.llseek		= generic_file_llseek,
 };
-- 
1.8.3.1

[PATCH 13/15] sysfs: copy bin mmap support from fs/sysfs/bin.c to fs/sysfs/file.c

[Tejun Heo]

sysfs bin file handling will be merged into the regular file support.
This patch copies mmap support from bin so that fs/sysfs/file.c can
handle mmapping bin files.

The code is copied mostly verbatim with the following updates.

* ->mmapped and ->vm_ops are added to sysfs_open_file and bin_buffer
  references are replaced with sysfs_open_file ones.

* Symbols are prefixed with sysfs_.

* sysfs_unmap_bin_file() grabs sysfs_open_dirent and traverses
  ->files.  Invocation of this function is added to
  sysfs_addrm_finish().

* sysfs_bin_mmap() is added to sysfs_bin_operations.

This is a preparation and the new mmap path isn't used yet.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/dir.c   |   1 +
 fs/sysfs/file.c  | 247 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/sysfs/sysfs.h |   2 +
 3 files changed, 249 insertions(+), 1 deletion(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index b518afd..c4040dd 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -595,6 +595,7 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
 		acxt->removed = sd->u.removed_list;
 
 		sysfs_deactivate(sd);
+		sysfs_unmap_bin_file(sd);
 		unmap_bin_file(sd);
 		sysfs_put(sd);
 	}
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 9ba492a..02797a1 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -22,6 +22,7 @@
 #include <linux/limits.h>
 #include <linux/uaccess.h>
 #include <linux/seq_file.h>
+#include <linux/mm.h>
 
 #include "sysfs.h"
 
@@ -52,6 +53,9 @@ struct sysfs_open_file {
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
+
+	bool			mmapped;
+	const struct vm_operations_struct *vm_ops;
 };
 
 static bool sysfs_is_bin(struct sysfs_dirent *sd)
@@ -301,6 +305,218 @@ out_free:
 	return len;
 }
 
+static void sysfs_bin_vma_open(struct vm_area_struct *vma)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+
+	if (!of->vm_ops)
+		return;
+
+	if (!sysfs_get_active(of->sd))
+		return;
+
+	if (of->vm_ops->open)
+		of->vm_ops->open(vma);
+
+	sysfs_put_active(of->sd);
+}
+
+static int sysfs_bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	int ret;
+
+	if (!of->vm_ops)
+		return VM_FAULT_SIGBUS;
+
+	if (!sysfs_get_active(of->sd))
+		return VM_FAULT_SIGBUS;
+
+	ret = VM_FAULT_SIGBUS;
+	if (of->vm_ops->fault)
+		ret = of->vm_ops->fault(vma, vmf);
+
+	sysfs_put_active(of->sd);
+	return ret;
+}
+
+static int sysfs_bin_page_mkwrite(struct vm_area_struct *vma,
+				  struct vm_fault *vmf)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	int ret;
+
+	if (!of->vm_ops)
+		return VM_FAULT_SIGBUS;
+
+	if (!sysfs_get_active(of->sd))
+		return VM_FAULT_SIGBUS;
+
+	ret = 0;
+	if (of->vm_ops->page_mkwrite)
+		ret = of->vm_ops->page_mkwrite(vma, vmf);
+	else
+		file_update_time(file);
+
+	sysfs_put_active(of->sd);
+	return ret;
+}
+
+static int sysfs_bin_access(struct vm_area_struct *vma, unsigned long addr,
+			    void *buf, int len, int write)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	int ret;
+
+	if (!of->vm_ops)
+		return -EINVAL;
+
+	if (!sysfs_get_active(of->sd))
+		return -EINVAL;
+
+	ret = -EINVAL;
+	if (of->vm_ops->access)
+		ret = of->vm_ops->access(vma, addr, buf, len, write);
+
+	sysfs_put_active(of->sd);
+	return ret;
+}
+
+#ifdef CONFIG_NUMA
+static int sysfs_bin_set_policy(struct vm_area_struct *vma,
+				struct mempolicy *new)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	int ret;
+
+	if (!of->vm_ops)
+		return 0;
+
+	if (!sysfs_get_active(of->sd))
+		return -EINVAL;
+
+	ret = 0;
+	if (of->vm_ops->set_policy)
+		ret = of->vm_ops->set_policy(vma, new);
+
+	sysfs_put_active(of->sd);
+	return ret;
+}
+
+static struct mempolicy *sysfs_bin_get_policy(struct vm_area_struct *vma,
+					      unsigned long addr)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	struct mempolicy *pol;
+
+	if (!of->vm_ops)
+		return vma->vm_policy;
+
+	if (!sysfs_get_active(of->sd))
+		return vma->vm_policy;
+
+	pol = vma->vm_policy;
+	if (of->vm_ops->get_policy)
+		pol = of->vm_ops->get_policy(vma, addr);
+
+	sysfs_put_active(of->sd);
+	return pol;
+}
+
+static int sysfs_bin_migrate(struct vm_area_struct *vma, const nodemask_t *from,
+			     const nodemask_t *to, unsigned long flags)
+{
+	struct file *file = vma->vm_file;
+	struct sysfs_open_file *of = sysfs_of(file);
+	int ret;
+
+	if (!of->vm_ops)
+		return 0;
+
+	if (!sysfs_get_active(of->sd))
+		return 0;
+
+	ret = 0;
+	if (of->vm_ops->migrate)
+		ret = of->vm_ops->migrate(vma, from, to, flags);
+
+	sysfs_put_active(of->sd);
+	return ret;
+}
+#endif
+
+static const struct vm_operations_struct sysfs_bin_vm_ops = {
+	.open		= sysfs_bin_vma_open,
+	.fault		= sysfs_bin_fault,
+	.page_mkwrite	= sysfs_bin_page_mkwrite,
+	.access		= sysfs_bin_access,
+#ifdef CONFIG_NUMA
+	.set_policy	= sysfs_bin_set_policy,
+	.get_policy	= sysfs_bin_get_policy,
+	.migrate	= sysfs_bin_migrate,
+#endif
+};
+
+static int sysfs_bin_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct sysfs_open_file *of = sysfs_of(file);
+	struct bin_attribute *battr = of->sd->s_bin_attr.bin_attr;
+	struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
+	int rc;
+
+	mutex_lock(&of->mutex);
+
+	/* need of->sd for battr, its parent for kobj */
+	rc = -ENODEV;
+	if (!sysfs_get_active(of->sd))
+		goto out_unlock;
+
+	rc = -EINVAL;
+	if (!battr->mmap)
+		goto out_put;
+
+	rc = battr->mmap(file, kobj, battr, vma);
+	if (rc)
+		goto out_put;
+
+	/*
+	 * PowerPC's pci_mmap of legacy_mem uses shmem_zero_setup()
+	 * to satisfy versions of X which crash if the mmap fails: that
+	 * substitutes a new vm_file, and we don't then want bin_vm_ops.
+	 */
+	if (vma->vm_file != file)
+		goto out_put;
+
+	rc = -EINVAL;
+	if (of->mmapped && of->vm_ops != vma->vm_ops)
+		goto out_put;
+
+	/*
+	 * It is not possible to successfully wrap close.
+	 * So error if someone is trying to use close.
+	 */
+	rc = -EINVAL;
+	if (vma->vm_ops && vma->vm_ops->close)
+		goto out_put;
+
+	rc = 0;
+	of->mmapped = 1;
+	of->vm_ops = vma->vm_ops;
+	vma->vm_ops = &sysfs_bin_vm_ops;
+out_put:
+	sysfs_put_active(of->sd);
+out_unlock:
+	mutex_unlock(&of->mutex);
+
+	return rc;
+}
+
 /**
  *	sysfs_get_open_dirent - get or create sysfs_open_dirent
  *	@sd: target sysfs_dirent
@@ -375,7 +591,9 @@ static void sysfs_put_open_dirent(struct sysfs_dirent *sd,
 	mutex_lock(&sysfs_open_file_mutex);
 	spin_lock_irqsave(&sysfs_open_dirent_lock, flags);
 
-	list_del(&of->list);
+	if (of)
+		list_del(&of->list);
+
 	if (atomic_dec_and_test(&od->refcnt))
 		sd->s_attr.open = NULL;
 	else
@@ -477,6 +695,32 @@ static int sysfs_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+void sysfs_unmap_bin_file(struct sysfs_dirent *sd)
+{
+	struct sysfs_open_dirent *od;
+	struct sysfs_open_file *of;
+
+	if (!sysfs_is_bin(sd))
+		return;
+
+	spin_lock_irq(&sysfs_open_dirent_lock);
+	od = sd->s_attr.open;
+	if (od)
+		atomic_inc(&od->refcnt);
+	spin_unlock_irq(&sysfs_open_dirent_lock);
+	if (!od)
+		return;
+
+	mutex_lock(&sysfs_open_file_mutex);
+	list_for_each_entry(of, &od->files, list) {
+		struct inode *inode = file_inode(of->file);
+		unmap_mapping_range(inode->i_mapping, 0, 0, 1);
+	}
+	mutex_unlock(&sysfs_open_file_mutex);
+
+	sysfs_put_open_dirent(sd, NULL);
+}
+
 /* Sysfs attribute files are pollable.  The idea is that you read
  * the content and then you use 'poll' or 'select' to wait for
  * the content to change.  When the content changes (assuming the
@@ -562,6 +806,7 @@ const struct file_operations sysfs_bin_operations = {
 	.read		= sysfs_bin_read,
 	.write		= sysfs_write_file,
 	.llseek		= generic_file_llseek,
+	.mmap		= sysfs_bin_mmap,
 };
 
 int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index f103bac..c960d62 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -220,6 +220,8 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd,
 int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
 			   const struct attribute *attr, int type,
 			   umode_t amode, const void *ns);
+void sysfs_unmap_bin_file(struct sysfs_dirent *sd);
+
 /*
  * bin.c
  */
-- 
1.8.3.1

[PATCH 14/15] sysfs: prepare open path for unified regular / bin file handling

[Tejun Heo]

sysfs bin file handling will be merged into the regular file support.
This patch prepares the open path.

This patch updates sysfs_open_file() such that it can handle both
regular and bin files.

This is a preparation and the new bin file path isn't used yet.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/sysfs/file.c | 58 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 02797a1..417d005 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -610,38 +610,40 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
 	struct sysfs_open_file *of;
-	const struct sysfs_ops *ops;
+	bool has_read, has_write;
 	int error = -EACCES;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
 	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
-	/* every kobject with an attribute needs a ktype assigned */
-	ops = sysfs_file_ops(attr_sd);
-	if (WARN(!ops, KERN_ERR
-		 "missing sysfs attribute operations for kobject: %s\n",
-		 kobject_name(kobj)))
-		goto err_out;
+	if (sysfs_is_bin(attr_sd)) {
+		struct bin_attribute *battr = attr_sd->s_bin_attr.bin_attr;
 
-	/* File needs write support.
-	 * The inode's perms must say it's ok,
-	 * and we must have a store method.
-	 */
-	if (file->f_mode & FMODE_WRITE) {
-		if (!(inode->i_mode & S_IWUGO) || !ops->store)
-			goto err_out;
-	}
+		has_read = battr->read || battr->mmap;
+		has_write = battr->write || battr->mmap;
+	} else {
+		const struct sysfs_ops *ops = sysfs_file_ops(attr_sd);
 
-	/* File needs read support.
-	 * The inode's perms must say it's ok, and we there
-	 * must be a show method for it.
-	 */
-	if (file->f_mode & FMODE_READ) {
-		if (!(inode->i_mode & S_IRUGO) || !ops->show)
+		/* every kobject with an attribute needs a ktype assigned */
+		if (WARN(!ops, KERN_ERR
+			 "missing sysfs attribute operations for kobject: %s\n",
+			 kobject_name(kobj)))
 			goto err_out;
+
+		has_read = ops->show;
+		has_write = ops->store;
 	}
 
+	/* check perms and supported operations */
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (!(inode->i_mode & S_IWUGO) || !has_write))
+		goto err_out;
+
+	if ((file->f_mode & FMODE_READ) &&
+	    (!(inode->i_mode & S_IRUGO) || !has_read))
+		goto err_out;
+
 	/* allocate a sysfs_open_file for the file */
 	error = -ENOMEM;
 	of = kzalloc(sizeof(struct sysfs_open_file), GFP_KERNEL);
@@ -653,11 +655,14 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 	of->file = file;
 
 	/*
-	 * Always instantiate seq_file even if read access is not
-	 * implemented or requested.  This unifies private data access and
-	 * most files are readable anyway.
+	 * Always instantiate seq_file even if read access doesn't use
+	 * seq_file or is not requested.  This unifies private data access
+	 * and readable regular files are the vast majority anyway.
 	 */
-	error = single_open(file, sysfs_seq_show, of);
+	if (sysfs_is_bin(attr_sd))
+		error = single_open(file, NULL, of);
+	else
+		error = single_open(file, sysfs_seq_show, of);
 	if (error)
 		goto err_free;
 
@@ -807,6 +812,9 @@ const struct file_operations sysfs_bin_operations = {
 	.write		= sysfs_write_file,
 	.llseek		= generic_file_llseek,
 	.mmap		= sysfs_bin_mmap,
+	.open		= sysfs_open_file,
+	.release	= sysfs_release,
+	.poll		= sysfs_poll,
 };
 
 int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
-- 
1.8.3.1

[PATCH 15/15] sysfs: merge regular and bin file handling

[Tejun Heo]

With the previous changes, sysfs regular file code is ready to handle
bin files too.  This patch makes bin files share the regular file
path.

* sysfs_create/remove_bin_file() are moved to fs/sysfs/file.c.

* sysfs_init_inode() is updated to use the new sysfs_bin_operations
  instead of bin_fops for bin files.

* fs/sysfs/bin.c and the related pieces are removed.

This patch shouldn't introduce any behavior difference to bin file
accesses.

Overall, this unification reduces the amount of duplicate logic, makes
behaviors more consistent and paves the road for building simpler and
more versatile interface which will allow other subsystems to make use
of sysfs for their pseudo filesystems.

v2: Stale fs/sysfs/bin.c reference dropped from
    Documentation/DocBook/filesystems.tmpl.  Reported by kbuild test
    robot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay@vrfy.org>
Cc: kbuild test robot <fengguang.wu@intel.com>
---
 Documentation/DocBook/filesystems.tmpl |   1 -
 fs/sysfs/Makefile                      |   3 +-
 fs/sysfs/bin.c                         | 491 ---------------------------------
 fs/sysfs/dir.c                         |   1 -
 fs/sysfs/file.c                        |  26 ++
 fs/sysfs/inode.c                       |   2 +-
 fs/sysfs/sysfs.h                       |   6 -
 7 files changed, 28 insertions(+), 502 deletions(-)
 delete mode 100644 fs/sysfs/bin.c

diff --git a/Documentation/DocBook/filesystems.tmpl b/Documentation/DocBook/filesystems.tmpl
index 25b58ef..4f67683 100644
--- a/Documentation/DocBook/filesystems.tmpl
+++ b/Documentation/DocBook/filesystems.tmpl
@@ -91,7 +91,6 @@
      <title>The Filesystem for Exporting Kernel Objects</title>
 !Efs/sysfs/file.c
 !Efs/sysfs/symlink.c
-!Efs/sysfs/bin.c
   </chapter>
 
   <chapter id="debugfs">
diff --git a/fs/sysfs/Makefile b/fs/sysfs/Makefile
index 7a1ceb9..8876ac1 100644
--- a/fs/sysfs/Makefile
+++ b/fs/sysfs/Makefile
@@ -2,5 +2,4 @@
 # Makefile for the sysfs virtual filesystem
 #
 
-obj-y		:= inode.o file.o dir.o symlink.o mount.o bin.o \
-		   group.o
+obj-y		:= inode.o file.o dir.o symlink.o mount.o group.o
diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
deleted file mode 100644
index 60a4e78..0000000
--- a/fs/sysfs/bin.c
+++ /dev/null
@@ -1,491 +0,0 @@
-/*
- * fs/sysfs/bin.c - sysfs binary file implementation
- *
- * Copyright (c) 2003 Patrick Mochel
- * Copyright (c) 2003 Matthew Wilcox
- * Copyright (c) 2004 Silicon Graphics, Inc.
- * Copyright (c) 2007 SUSE Linux Products GmbH
- * Copyright (c) 2007 Tejun Heo <teheo@suse.de>
- *
- * This file is released under the GPLv2.
- *
- * Please see Documentation/filesystems/sysfs.txt for more information.
- */
-
-#undef DEBUG
-
-#include <linux/errno.h>
-#include <linux/fs.h>
-#include <linux/kernel.h>
-#include <linux/kobject.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/mutex.h>
-#include <linux/mm.h>
-#include <linux/uaccess.h>
-
-#include "sysfs.h"
-
-/*
- * There's one bin_buffer for each open file.
- *
- * filp->private_data points to bin_buffer and
- * sysfs_dirent->s_bin_attr.buffers points to a the bin_buffer s
- * sysfs_dirent->s_bin_attr.buffers is protected by sysfs_bin_lock
- */
-static DEFINE_MUTEX(sysfs_bin_lock);
-
-struct bin_buffer {
-	struct mutex			mutex;
-	void				*buffer;
-	int				mmapped;
-	const struct vm_operations_struct *vm_ops;
-	struct file			*file;
-	struct hlist_node		list;
-};
-
-static ssize_t
-read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
-{
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
-	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	struct bin_buffer *bb = file->private_data;
-	int size = file_inode(file)->i_size;
-	loff_t offs = *off;
-	int count = min_t(size_t, bytes, PAGE_SIZE);
-	char *buf;
-
-	if (!bytes)
-		return 0;
-
-	if (size) {
-		if (offs > size)
-			return 0;
-		if (offs + count > size)
-			count = size - offs;
-	}
-
-	buf = kmalloc(count, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	/* need attr_sd for attr, its parent for kobj */
-	mutex_lock(&bb->mutex);
-	if (!sysfs_get_active(attr_sd)) {
-		count = -ENODEV;
-		mutex_unlock(&bb->mutex);
-		goto out_free;
-	}
-
-	if (attr->read)
-		count = attr->read(file, kobj, attr, buf, offs, count);
-	else
-		count = -EIO;
-
-	sysfs_put_active(attr_sd);
-	mutex_unlock(&bb->mutex);
-
-	if (count < 0)
-		goto out_free;
-
-	if (copy_to_user(userbuf, buf, count)) {
-		count = -EFAULT;
-		goto out_free;
-	}
-
-	pr_debug("offs = %lld, *off = %lld, count = %d\n", offs, *off, count);
-
-	*off = offs + count;
-
- out_free:
-	kfree(buf);
-	return count;
-}
-
-static int
-flush_write(struct file *file, char *buffer, loff_t offset, size_t count)
-{
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
-	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	int rc;
-
-	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active(attr_sd))
-		return -ENODEV;
-
-	rc = -EIO;
-	if (attr->write)
-		rc = attr->write(file, kobj, attr, buffer, offset, count);
-
-	sysfs_put_active(attr_sd);
-
-	return rc;
-}
-
-static ssize_t write(struct file *file, const char __user *userbuf,
-		     size_t bytes, loff_t *off)
-{
-	struct bin_buffer *bb = file->private_data;
-	int size = file_inode(file)->i_size;
-	loff_t offs = *off;
-	int count = min_t(size_t, bytes, PAGE_SIZE);
-	char *temp;
-
-	if (!bytes)
-		return 0;
-
-	if (size) {
-		if (offs > size)
-			return 0;
-		if (offs + count > size)
-			count = size - offs;
-	}
-
-	temp = memdup_user(userbuf, count);
-	if (IS_ERR(temp))
-		return PTR_ERR(temp);
-
-	mutex_lock(&bb->mutex);
-
-	memcpy(bb->buffer, temp, count);
-
-	count = flush_write(file, bb->buffer, offs, count);
-	mutex_unlock(&bb->mutex);
-
-	if (count > 0)
-		*off = offs + count;
-
-	kfree(temp);
-	return count;
-}
-
-static void bin_vma_open(struct vm_area_struct *vma)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-
-	if (!bb->vm_ops)
-		return;
-
-	if (!sysfs_get_active(attr_sd))
-		return;
-
-	if (bb->vm_ops->open)
-		bb->vm_ops->open(vma);
-
-	sysfs_put_active(attr_sd);
-}
-
-static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	int ret;
-
-	if (!bb->vm_ops)
-		return VM_FAULT_SIGBUS;
-
-	if (!sysfs_get_active(attr_sd))
-		return VM_FAULT_SIGBUS;
-
-	ret = VM_FAULT_SIGBUS;
-	if (bb->vm_ops->fault)
-		ret = bb->vm_ops->fault(vma, vmf);
-
-	sysfs_put_active(attr_sd);
-	return ret;
-}
-
-static int bin_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	int ret;
-
-	if (!bb->vm_ops)
-		return VM_FAULT_SIGBUS;
-
-	if (!sysfs_get_active(attr_sd))
-		return VM_FAULT_SIGBUS;
-
-	ret = 0;
-	if (bb->vm_ops->page_mkwrite)
-		ret = bb->vm_ops->page_mkwrite(vma, vmf);
-	else
-		file_update_time(file);
-
-	sysfs_put_active(attr_sd);
-	return ret;
-}
-
-static int bin_access(struct vm_area_struct *vma, unsigned long addr,
-		  void *buf, int len, int write)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	int ret;
-
-	if (!bb->vm_ops)
-		return -EINVAL;
-
-	if (!sysfs_get_active(attr_sd))
-		return -EINVAL;
-
-	ret = -EINVAL;
-	if (bb->vm_ops->access)
-		ret = bb->vm_ops->access(vma, addr, buf, len, write);
-
-	sysfs_put_active(attr_sd);
-	return ret;
-}
-
-#ifdef CONFIG_NUMA
-static int bin_set_policy(struct vm_area_struct *vma, struct mempolicy *new)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	int ret;
-
-	if (!bb->vm_ops)
-		return 0;
-
-	if (!sysfs_get_active(attr_sd))
-		return -EINVAL;
-
-	ret = 0;
-	if (bb->vm_ops->set_policy)
-		ret = bb->vm_ops->set_policy(vma, new);
-
-	sysfs_put_active(attr_sd);
-	return ret;
-}
-
-static struct mempolicy *bin_get_policy(struct vm_area_struct *vma,
-					unsigned long addr)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	struct mempolicy *pol;
-
-	if (!bb->vm_ops)
-		return vma->vm_policy;
-
-	if (!sysfs_get_active(attr_sd))
-		return vma->vm_policy;
-
-	pol = vma->vm_policy;
-	if (bb->vm_ops->get_policy)
-		pol = bb->vm_ops->get_policy(vma, addr);
-
-	sysfs_put_active(attr_sd);
-	return pol;
-}
-
-static int bin_migrate(struct vm_area_struct *vma, const nodemask_t *from,
-			const nodemask_t *to, unsigned long flags)
-{
-	struct file *file = vma->vm_file;
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	int ret;
-
-	if (!bb->vm_ops)
-		return 0;
-
-	if (!sysfs_get_active(attr_sd))
-		return 0;
-
-	ret = 0;
-	if (bb->vm_ops->migrate)
-		ret = bb->vm_ops->migrate(vma, from, to, flags);
-
-	sysfs_put_active(attr_sd);
-	return ret;
-}
-#endif
-
-static const struct vm_operations_struct bin_vm_ops = {
-	.open		= bin_vma_open,
-	.fault		= bin_fault,
-	.page_mkwrite	= bin_page_mkwrite,
-	.access		= bin_access,
-#ifdef CONFIG_NUMA
-	.set_policy	= bin_set_policy,
-	.get_policy	= bin_get_policy,
-	.migrate	= bin_migrate,
-#endif
-};
-
-static int mmap(struct file *file, struct vm_area_struct *vma)
-{
-	struct bin_buffer *bb = file->private_data;
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
-	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
-	int rc;
-
-	mutex_lock(&bb->mutex);
-
-	/* need attr_sd for attr, its parent for kobj */
-	rc = -ENODEV;
-	if (!sysfs_get_active(attr_sd))
-		goto out_unlock;
-
-	rc = -EINVAL;
-	if (!attr->mmap)
-		goto out_put;
-
-	rc = attr->mmap(file, kobj, attr, vma);
-	if (rc)
-		goto out_put;
-
-	/*
-	 * PowerPC's pci_mmap of legacy_mem uses shmem_zero_setup()
-	 * to satisfy versions of X which crash if the mmap fails: that
-	 * substitutes a new vm_file, and we don't then want bin_vm_ops.
-	 */
-	if (vma->vm_file != file)
-		goto out_put;
-
-	rc = -EINVAL;
-	if (bb->mmapped && bb->vm_ops != vma->vm_ops)
-		goto out_put;
-
-	/*
-	 * It is not possible to successfully wrap close.
-	 * So error if someone is trying to use close.
-	 */
-	rc = -EINVAL;
-	if (vma->vm_ops && vma->vm_ops->close)
-		goto out_put;
-
-	rc = 0;
-	bb->mmapped = 1;
-	bb->vm_ops = vma->vm_ops;
-	vma->vm_ops = &bin_vm_ops;
-out_put:
-	sysfs_put_active(attr_sd);
-out_unlock:
-	mutex_unlock(&bb->mutex);
-
-	return rc;
-}
-
-static int open(struct inode *inode, struct file *file)
-{
-	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
-	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
-	struct bin_buffer *bb = NULL;
-	int error;
-
-	/* binary file operations requires both @sd and its parent */
-	if (!sysfs_get_active(attr_sd))
-		return -ENODEV;
-
-	error = -EACCES;
-	if ((file->f_mode & FMODE_WRITE) && !(attr->write || attr->mmap))
-		goto err_out;
-	if ((file->f_mode & FMODE_READ) && !(attr->read || attr->mmap))
-		goto err_out;
-
-	error = -ENOMEM;
-	bb = kzalloc(sizeof(*bb), GFP_KERNEL);
-	if (!bb)
-		goto err_out;
-
-	bb->buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!bb->buffer)
-		goto err_out;
-
-	mutex_init(&bb->mutex);
-	bb->file = file;
-	file->private_data = bb;
-
-	mutex_lock(&sysfs_bin_lock);
-	hlist_add_head(&bb->list, &attr_sd->s_bin_attr.buffers);
-	mutex_unlock(&sysfs_bin_lock);
-
-	/* open succeeded, put active references */
-	sysfs_put_active(attr_sd);
-	return 0;
-
- err_out:
-	sysfs_put_active(attr_sd);
-	kfree(bb);
-	return error;
-}
-
-static int release(struct inode *inode, struct file *file)
-{
-	struct bin_buffer *bb = file->private_data;
-
-	mutex_lock(&sysfs_bin_lock);
-	hlist_del(&bb->list);
-	mutex_unlock(&sysfs_bin_lock);
-
-	kfree(bb->buffer);
-	kfree(bb);
-	return 0;
-}
-
-const struct file_operations bin_fops = {
-	.read		= read,
-	.write		= write,
-	.mmap		= mmap,
-	.llseek		= generic_file_llseek,
-	.open		= open,
-	.release	= release,
-};
-
-
-void unmap_bin_file(struct sysfs_dirent *attr_sd)
-{
-	struct bin_buffer *bb;
-
-	if (sysfs_type(attr_sd) != SYSFS_KOBJ_BIN_ATTR)
-		return;
-
-	mutex_lock(&sysfs_bin_lock);
-
-	hlist_for_each_entry(bb, &attr_sd->s_bin_attr.buffers, list) {
-		struct inode *inode = file_inode(bb->file);
-
-		unmap_mapping_range(inode->i_mapping, 0, 0, 1);
-	}
-
-	mutex_unlock(&sysfs_bin_lock);
-}
-
-/**
- *	sysfs_create_bin_file - create binary file for object.
- *	@kobj:	object.
- *	@attr:	attribute descriptor.
- */
-int sysfs_create_bin_file(struct kobject *kobj,
-			  const struct bin_attribute *attr)
-{
-	BUG_ON(!kobj || !kobj->sd || !attr);
-
-	return sysfs_add_file(kobj->sd, &attr->attr, SYSFS_KOBJ_BIN_ATTR);
-}
-EXPORT_SYMBOL_GPL(sysfs_create_bin_file);
-
-/**
- *	sysfs_remove_bin_file - remove binary file for object.
- *	@kobj:	object.
- *	@attr:	attribute descriptor.
- */
-void sysfs_remove_bin_file(struct kobject *kobj,
-			   const struct bin_attribute *attr)
-{
-	sysfs_hash_and_remove(kobj->sd, attr->attr.name, NULL);
-}
-EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index c4040dd..f6025c8 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -596,7 +596,6 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
 
 		sysfs_deactivate(sd);
 		sysfs_unmap_bin_file(sd);
-		unmap_bin_file(sd);
 		sysfs_put(sd);
 	}
 }
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 417d005..5f7a955 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -987,6 +987,32 @@ void sysfs_remove_file_from_group(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_file_from_group);
 
+/**
+ *	sysfs_create_bin_file - create binary file for object.
+ *	@kobj:	object.
+ *	@attr:	attribute descriptor.
+ */
+int sysfs_create_bin_file(struct kobject *kobj,
+			  const struct bin_attribute *attr)
+{
+	BUG_ON(!kobj || !kobj->sd || !attr);
+
+	return sysfs_add_file(kobj->sd, &attr->attr, SYSFS_KOBJ_BIN_ATTR);
+}
+EXPORT_SYMBOL_GPL(sysfs_create_bin_file);
+
+/**
+ *	sysfs_remove_bin_file - remove binary file for object.
+ *	@kobj:	object.
+ *	@attr:	attribute descriptor.
+ */
+void sysfs_remove_bin_file(struct kobject *kobj,
+			   const struct bin_attribute *attr)
+{
+	sysfs_hash_and_remove(kobj->sd, attr->attr.name, NULL);
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
+
 struct sysfs_schedule_callback_struct {
 	struct list_head	workq_list;
 	struct kobject		*kobj;
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 63f755e..2cb1b6b 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -260,7 +260,7 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
 	case SYSFS_KOBJ_BIN_ATTR:
 		bin_attr = sd->s_bin_attr.bin_attr;
 		inode->i_size = bin_attr->size;
-		inode->i_fop = &bin_fops;
+		inode->i_fop = &sysfs_bin_operations;
 		break;
 	case SYSFS_KOBJ_LINK:
 		inode->i_op = &sysfs_symlink_inode_operations;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index c960d62..4e01d3b 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -223,12 +223,6 @@ int sysfs_add_file_mode_ns(struct sysfs_dirent *dir_sd,
 void sysfs_unmap_bin_file(struct sysfs_dirent *sd);
 
 /*
- * bin.c
- */
-extern const struct file_operations bin_fops;
-void unmap_bin_file(struct sysfs_dirent *attr_sd);
-
-/*
  * symlink.c
  */
 extern const struct inode_operations sysfs_symlink_inode_operations;
-- 
1.8.3.1

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Greg KH]

On Tue, Oct 01, 2013 at 05:41:54PM -0400, Tejun Heo wrote:
> Hello,
> 
> Changes from the last take[L] are,
> 
> * bin file reads no longer go through seq_file.  It goes through a
>   separate read path implemented in sysfs_bin_read().  bin files
>   shouldn't see any behavior difference now.
> 
> * bin files now use a separate file_operations struct -
>   sysfs_bin_operations.  Open and write paths are still shared but
>   read path is separate and mmap exists only for the bin files.  While
>   this is less uniform than before, it should still render itself well
>   to extracting the core functionality.
> 
> * 0001-0008 are the same as before.

Nice, I've applied all of these now.

> Patchset description follows.
> 
> Currently, sysfs's file handling is a bit weird.
> 
> * Regular and bin file paths are similar but implemented completely
>   separately duplicating some hairy logics.
> 
> * Read path implements custom buffering which is essentially
>   degenerate seq_file.
> 
> In addition, sysfs core implementation is planned to be separated out
> so that it can be shared by other subsystems and the current file
> handling is too restrictive and quirky to spread further to other
> parts of the kernel.  It'd be a lot more desirable to have read path
> completely handled by seq_file which is a lot more versatile and would
> also increase overall behavior consistency.
> 
> This patchset updates file handling such that read is handled by
> seq_file and then merges bin file handling into regular file path.
> While some changes introduces behavior changes in extreme corner
> cases, they are highly unlikely to be noticeable (please refer to the
> description of each patch for details) and generally bring sysfs's
> behavior closer to those of procfs or any pseudo filesystem which
> makes use of seq_file.
> 
> After the conversion, LOC is reduced by ~150 lines and read path is
> fully handled by seq_file, which allows defining a new seq_file based
> core interface which will enable sharing sysfs from other subsystems.

I'm glad to see the sysfs cleanups, thanks for doing all of this, it's
much needed.

How are you going to use sysfs from another kernel subsystem?  Is this
for another filesystem, or do you want to use kobjects for some other
subsystem that doesn't export them using sysfs?

thanks,

greg k-h

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Tejun Heo]

Hello,

On Sat, Oct 05, 2013 at 05:40:32PM -0700, Greg KH wrote:
> > * 0001-0008 are the same as before.
> 
> Nice, I've applied all of these now.

Great!

> How are you going to use sysfs from another kernel subsystem?  Is this
> for another filesystem, or do you want to use kobjects for some other
> subsystem that doesn't export them using sysfs?

I was at first trying to convert sysfs interface to not use kobject
but that caused too much disruption and I'm now trying to separate out
the core functionality into a separate filesystem - currently named
kernfs, so sysfs will be a consumer of kernfs translating between
kobject and kernfs interface, and cgroup will be the second user which
replaces its own pseudo filesystem implementation with kernfs.

Thanks a lot.

-- 
tejun

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Greg KH]

On Mon, Oct 07, 2013 at 01:00:05PM -0400, Tejun Heo wrote:
> > How are you going to use sysfs from another kernel subsystem?  Is this
> > for another filesystem, or do you want to use kobjects for some other
> > subsystem that doesn't export them using sysfs?
> 
> I was at first trying to convert sysfs interface to not use kobject
> but that caused too much disruption and I'm now trying to separate out
> the core functionality into a separate filesystem - currently named
> kernfs, so sysfs will be a consumer of kernfs translating between
> kobject and kernfs interface, and cgroup will be the second user which
> replaces its own pseudo filesystem implementation with kernfs.

Ah, that would be nice, if it works out well, I might have to move
debugfs over to it also, as the current implementation of it has some
major issues.

Good luck :)

greg k-h

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Yinghai Lu]

On Mon, Oct 7, 2013 at 4:11 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 07, 2013 at 01:00:05PM -0400, Tejun Heo wrote:
>> > How are you going to use sysfs from another kernel subsystem?  Is this
>> > for another filesystem, or do you want to use kobjects for some other
>> > subsystem that doesn't export them using sysfs?
>>
>> I was at first trying to convert sysfs interface to not use kobject
>> but that caused too much disruption and I'm now trying to separate out
>> the core functionality into a separate filesystem - currently named
>> kernfs, so sysfs will be a consumer of kernfs translating between
>> kobject and kernfs interface, and cgroup will be the second user which
>> replaces its own pseudo filesystem implementation with kernfs.
>
> Ah, that would be nice, if it works out well, I might have to move
> debugfs over to it also, as the current implementation of it has some
> major issues.
>
got warning from drivers core next.


[  448.189960] ------------[ cut here ]------------
[  448.195214] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
sysfs_file_ops+0x59/0x70()
[  448.204188] Modules linked in:
[  448.207630] CPU: 6 PID: 69219 Comm: hald Not tainted
3.12.0-rc4-yh-01397-gaac2c70 #1976
[  448.216608] Hardware name:
[  448.226753]  0000000000000009 ffff8ddee8731b88 ffffffff820f6b7e
0000000000000046
[  448.235143]  0000000000000000 ffff8ddee8731bc8 ffffffff8109758c
0000000000000246
[  448.243540]  ffff8ddf424c6098 ffff8ddf31c84480 ffff8ddf424c6098
ffff8ddee8731f24
[  448.251937] Call Trace:
[  448.254711]  [<ffffffff820f6b7e>] dump_stack+0x46/0x58
[  448.260496]  [<ffffffff8109758c>] warn_slowpath_common+0x8c/0xc0
[  448.267249]  [<ffffffff810975da>] warn_slowpath_null+0x1a/0x20
[  448.273805]  [<ffffffff81248bd9>] sysfs_file_ops+0x59/0x70
[  448.279973]  [<ffffffff81249628>] sysfs_open_file+0x88/0x330
[  448.286334]  [<ffffffff812495a0>] ? sysfs_release+0x70/0x70
[  448.292606]  [<ffffffff811d3a49>] do_dentry_open.isra.15+0x1d9/0x2b0
[  448.299745]  [<ffffffff811d3b4c>] finish_open+0x2c/0x40
[  448.305621]  [<ffffffff811e5657>] do_last.isra.47+0xa37/0xc50
[  448.312089]  [<ffffffff811e1bcd>] ? inode_permission+0x4d/0x50
[  448.318655]  [<ffffffff811e5ac2>] path_openat+0x252/0x660
[  448.324734]  [<ffffffff81100078>] ? trace_hardirqs_off_caller+0x28/0x160
[  448.332264]  [<ffffffff811e62d3>] do_filp_open+0x43/0xb0
[  448.338240]  [<ffffffff811f38e1>] ? __alloc_fd+0x121/0x140
[  448.344409]  [<ffffffff811d4ea0>] do_sys_open+0x170/0x210
[  448.350476]  [<ffffffff811d4f62>] SyS_open+0x22/0x30
[  448.356057]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
[  448.361629] ---[ end trace d1986b41b84640de ]---
[  448.366842] ------------[ cut here ]------------
[  448.372030] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
sysfs_file_ops+0x59/0x70()
[  448.381002] Modules linked in:
[  448.384459] CPU: 6 PID: 69219 Comm: hald Tainted: G        W
3.12.0-rc4-yh-01397-gaac2c70 #1976
[  448.394497] Hardware name:
[  448.404630]  0000000000000009 ffff8ddee8731db8 ffffffff820f6b7e
0000000000004480
[  448.413020]  0000000000000000 ffff8ddee8731df8 ffffffff8109758c
0000000000000246
[  448.421418]  ffff8ddf424c6098 ffff8ddf30fb4e00 ffff8ddf424c6098
ffff8ddf4668d000
[  448.429805] Call Trace:
[  448.432575]  [<ffffffff820f6b7e>] dump_stack+0x46/0x58
[  448.438354]  [<ffffffff8109758c>] warn_slowpath_common+0x8c/0xc0
[  448.445102]  [<ffffffff810975da>] warn_slowpath_null+0x1a/0x20
[  448.451640]  [<ffffffff81248bd9>] sysfs_file_ops+0x59/0x70
[  448.457806]  [<ffffffff81248cbb>] sysfs_seq_show+0xcb/0x180
[  448.464069]  [<ffffffff811f99ec>] seq_read+0x19c/0x3a0
[  448.469847]  [<ffffffff811d624d>] vfs_read+0xbd/0x170
[  448.475517]  [<ffffffff811d6495>] SyS_read+0x55/0xb0
[  448.481104]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
[  448.486678] ---[ end trace d1986b41b84640df ]---
[  448.662721]  [<ffffffff81100078>] ? trace_hardirqs_off_caller+0x28/0x160
[  448.662730]  [<ffffffff811e62d3>] do_filp_open+0x43/0xb0
[  448.662738]  [<ffffffff811f38e1>] ? __alloc_fd+0x121/0x140
[  448.662747]  [<ffffffff811d4ea0>] do_sys_open+0x170/0x210
[  448.662755]  [<ffffffff811d4f62>] SyS_open+0x22/0x30
[  448.662763]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
[  448.662766] ---[ end trace d1986b41b84640ea ]---
[  448.662786] ------------[ cut here ]------------
....

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Greg KH]

On Fri, Oct 11, 2013 at 12:11:18PM -0700, Yinghai Lu wrote:
> On Mon, Oct 7, 2013 at 4:11 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 07, 2013 at 01:00:05PM -0400, Tejun Heo wrote:
> >> > How are you going to use sysfs from another kernel subsystem?  Is this
> >> > for another filesystem, or do you want to use kobjects for some other
> >> > subsystem that doesn't export them using sysfs?
> >>
> >> I was at first trying to convert sysfs interface to not use kobject
> >> but that caused too much disruption and I'm now trying to separate out
> >> the core functionality into a separate filesystem - currently named
> >> kernfs, so sysfs will be a consumer of kernfs translating between
> >> kobject and kernfs interface, and cgroup will be the second user which
> >> replaces its own pseudo filesystem implementation with kernfs.
> >
> > Ah, that would be nice, if it works out well, I might have to move
> > debugfs over to it also, as the current implementation of it has some
> > major issues.
> >
> got warning from drivers core next.
> 
> 
> [  448.189960] ------------[ cut here ]------------
> [  448.195214] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
> sysfs_file_ops+0x59/0x70()
> [  448.204188] Modules linked in:
> [  448.207630] CPU: 6 PID: 69219 Comm: hald Not tainted
> 3.12.0-rc4-yh-01397-gaac2c70 #1976
> [  448.216608] Hardware name:
> [  448.226753]  0000000000000009 ffff8ddee8731b88 ffffffff820f6b7e
> 0000000000000046
> [  448.235143]  0000000000000000 ffff8ddee8731bc8 ffffffff8109758c
> 0000000000000246
> [  448.243540]  ffff8ddf424c6098 ffff8ddf31c84480 ffff8ddf424c6098
> ffff8ddee8731f24
> [  448.251937] Call Trace:
> [  448.254711]  [<ffffffff820f6b7e>] dump_stack+0x46/0x58
> [  448.260496]  [<ffffffff8109758c>] warn_slowpath_common+0x8c/0xc0
> [  448.267249]  [<ffffffff810975da>] warn_slowpath_null+0x1a/0x20
> [  448.273805]  [<ffffffff81248bd9>] sysfs_file_ops+0x59/0x70
> [  448.279973]  [<ffffffff81249628>] sysfs_open_file+0x88/0x330
> [  448.286334]  [<ffffffff812495a0>] ? sysfs_release+0x70/0x70
> [  448.292606]  [<ffffffff811d3a49>] do_dentry_open.isra.15+0x1d9/0x2b0
> [  448.299745]  [<ffffffff811d3b4c>] finish_open+0x2c/0x40
> [  448.305621]  [<ffffffff811e5657>] do_last.isra.47+0xa37/0xc50
> [  448.312089]  [<ffffffff811e1bcd>] ? inode_permission+0x4d/0x50
> [  448.318655]  [<ffffffff811e5ac2>] path_openat+0x252/0x660
> [  448.324734]  [<ffffffff81100078>] ? trace_hardirqs_off_caller+0x28/0x160
> [  448.332264]  [<ffffffff811e62d3>] do_filp_open+0x43/0xb0
> [  448.338240]  [<ffffffff811f38e1>] ? __alloc_fd+0x121/0x140
> [  448.344409]  [<ffffffff811d4ea0>] do_sys_open+0x170/0x210
> [  448.350476]  [<ffffffff811d4f62>] SyS_open+0x22/0x30
> [  448.356057]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
> [  448.361629] ---[ end trace d1986b41b84640de ]---
> [  448.366842] ------------[ cut here ]------------
> [  448.372030] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
> sysfs_file_ops+0x59/0x70()
> [  448.381002] Modules linked in:
> [  448.384459] CPU: 6 PID: 69219 Comm: hald Tainted: G        W
> 3.12.0-rc4-yh-01397-gaac2c70 #1976
> [  448.394497] Hardware name:
> [  448.404630]  0000000000000009 ffff8ddee8731db8 ffffffff820f6b7e
> 0000000000004480
> [  448.413020]  0000000000000000 ffff8ddee8731df8 ffffffff8109758c
> 0000000000000246
> [  448.421418]  ffff8ddf424c6098 ffff8ddf30fb4e00 ffff8ddf424c6098
> ffff8ddf4668d000
> [  448.429805] Call Trace:
> [  448.432575]  [<ffffffff820f6b7e>] dump_stack+0x46/0x58
> [  448.438354]  [<ffffffff8109758c>] warn_slowpath_common+0x8c/0xc0
> [  448.445102]  [<ffffffff810975da>] warn_slowpath_null+0x1a/0x20
> [  448.451640]  [<ffffffff81248bd9>] sysfs_file_ops+0x59/0x70
> [  448.457806]  [<ffffffff81248cbb>] sysfs_seq_show+0xcb/0x180
> [  448.464069]  [<ffffffff811f99ec>] seq_read+0x19c/0x3a0
> [  448.469847]  [<ffffffff811d624d>] vfs_read+0xbd/0x170
> [  448.475517]  [<ffffffff811d6495>] SyS_read+0x55/0xb0
> [  448.481104]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
> [  448.486678] ---[ end trace d1986b41b84640df ]---
> [  448.662721]  [<ffffffff81100078>] ? trace_hardirqs_off_caller+0x28/0x160
> [  448.662730]  [<ffffffff811e62d3>] do_filp_open+0x43/0xb0
> [  448.662738]  [<ffffffff811f38e1>] ? __alloc_fd+0x121/0x140
> [  448.662747]  [<ffffffff811d4ea0>] do_sys_open+0x170/0x210
> [  448.662755]  [<ffffffff811d4f62>] SyS_open+0x22/0x30
> [  448.662763]  [<ffffffff82113d6a>] tracesys+0xd4/0xd9
> [  448.662766] ---[ end trace d1986b41b84640ea ]---
> [  448.662786] ------------[ cut here ]------------
> ....


Ick.  Tejun, any ideas?

Re: [PATCHSET v2] sysfs: use seq_file and unify regular and bin file handling

[Tejun Heo]

Hello, guys.

Sorry about the delay.

On Fri, Oct 11, 2013 at 12:11:18PM -0700, Yinghai Lu wrote:
> [  448.189960] ------------[ cut here ]------------
> [  448.195214] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
> sysfs_file_ops+0x59/0x70()

So, that's "lockdep_assert_held(sd);" in sysfs_file_ops().

> [  448.273805]  [<ffffffff81248bd9>] sysfs_file_ops+0x59/0x70
> [  448.279973]  [<ffffffff81249628>] sysfs_open_file+0x88/0x330

Triggering from sysfs_open_file()

> [  448.366842] ------------[ cut here ]------------
> [  448.372030] WARNING: CPU: 6 PID: 69219 at fs/sysfs/file.c:79
> sysfs_file_ops+0x59/0x70()
...
> [  448.457806]  [<ffffffff81248cbb>] sysfs_seq_show+0xcb/0x180

and then from sysfs_seq_show(), which is weird because
lockdep_assert_held(sd) checks whether sd's active ref is held and
both do hold them.  Hmm.... ah, right, I forgot about
->ignore_lockdep, we need to skip the assertion for files marked with
ignore_lockdep.  Will send a patch soon.

Thanks.

-- 
tejun

[PATCH driver-core-next] sysfs: make sysfs_file_ops() follow ignore_lockdep flag

[Tejun Heo]

375b611e60 ("sysfs: remove sysfs_buffer->ops") introduced
sysfs_file_ops() which determines the associated file operation of a
given sysfs_dirent.  As file ops access should be protected by an
active reference, the new function includes a lockdep assertion on the
sysfs_dirent; unfortunately, I forgot to take attr->ignore_lockdep
flag into account and the lockdep assertion trips spuriously for files
which opt out from active reference lockdep checking.

# cat /sys/devices/pci0000:00/0000:00:01.2/usb1/authorized

 ------------[ cut here ]------------
 WARNING: CPU: 1 PID: 540 at /work/os/work/fs/sysfs/file.c:79 sysfs_file_ops+0x4e/0x60()
 Modules linked in:
 CPU: 1 PID: 540 Comm: cat Not tainted 3.11.0-work+ #3
 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
  0000000000000009 ffff880016205c08 ffffffff81ca0131 0000000000000000
  ffff880016205c40 ffffffff81096d0d ffff8800166cb898 ffff8800166f6f60
  ffffffff8125a220 ffff880011ab1ec0 ffff88000aff0c78 ffff880016205c50
 Call Trace:
  [<ffffffff81ca0131>] dump_stack+0x4e/0x82
  [<ffffffff81096d0d>] warn_slowpath_common+0x7d/0xa0
  [<ffffffff81096dea>] warn_slowpath_null+0x1a/0x20
  [<ffffffff8125994e>] sysfs_file_ops+0x4e/0x60
  [<ffffffff8125a274>] sysfs_open_file+0x54/0x300
  [<ffffffff811df612>] do_dentry_open.isra.17+0x182/0x280
  [<ffffffff811df820>] finish_open+0x30/0x40
  [<ffffffff811f0623>] do_last+0x503/0xd90
  [<ffffffff811f0f6b>] path_openat+0xbb/0x6d0
  [<ffffffff811f23ba>] do_filp_open+0x3a/0x90
  [<ffffffff811e09a9>] do_sys_open+0x129/0x220
  [<ffffffff811e0abe>] SyS_open+0x1e/0x20
  [<ffffffff81caf3c2>] system_call_fastpath+0x16/0x1b
 ---[ end trace aa48096b111dafdb ]---

Rename fs/sysfs/dir.c::ignore_lockdep() to sysfs_ignore_lockdep() and
move it to fs/sysfs/sysfs.h and make sysfs_file_ops() skip lockdep
assertion if sysfs_ignore_lockdep() is true.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Yinghai Lu <yinghai@kernel.org>
---
 fs/sysfs/dir.c   |   22 ++--------------------
 fs/sysfs/file.c  |    3 ++-
 fs/sysfs/sysfs.h |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 21 deletions(-)

--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -144,24 +144,6 @@ static void sysfs_unlink_sibling(struct
 		sd->s_parent->s_flags &= ~SYSFS_FLAG_HAS_NS;
 }
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-/* Test for attributes that want to ignore lockdep for read-locking */
-static bool ignore_lockdep(struct sysfs_dirent *sd)
-{
-	return sysfs_type(sd) == SYSFS_KOBJ_ATTR &&
-			sd->s_attr.attr->ignore_lockdep;
-}
-
-#else
-
-static inline bool ignore_lockdep(struct sysfs_dirent *sd)
-{
-	return true;
-}
-
-#endif
-
 /**
  *	sysfs_get_active - get an active reference to sysfs_dirent
  *	@sd: sysfs_dirent to get an active reference to
@@ -180,7 +162,7 @@ struct sysfs_dirent *sysfs_get_active(st
 	if (!atomic_inc_unless_negative(&sd->s_active))
 		return NULL;
 
-	if (likely(!ignore_lockdep(sd)))
+	if (likely(!sysfs_ignore_lockdep(sd)))
 		rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
 	return sd;
 }
@@ -199,7 +181,7 @@ void sysfs_put_active(struct sysfs_diren
 	if (unlikely(!sd))
 		return;
 
-	if (likely(!ignore_lockdep(sd)))
+	if (likely(!sysfs_ignore_lockdep(sd)))
 		rwsem_release(&sd->dep_map, 1, _RET_IP_);
 	v = atomic_dec_return(&sd->s_active);
 	if (likely(v != SD_DEACTIVATED_BIAS))
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -76,7 +76,8 @@ static const struct sysfs_ops *sysfs_fil
 {
 	struct kobject *kobj = sd->s_parent->s_dir.kobj;
 
-	lockdep_assert_held(sd);
+	if (!sysfs_ignore_lockdep(sd))
+		lockdep_assert_held(sd);
 	return kobj->ktype ? kobj->ktype->sysfs_ops : NULL;
 }
 
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -103,6 +103,7 @@ static inline unsigned int sysfs_type(st
 }
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 #define sysfs_dirent_init_lockdep(sd)				\
 do {								\
 	struct attribute *attr = sd->s_attr.attr;		\
@@ -112,8 +113,23 @@ do {								\
 								\
 	lockdep_init_map(&sd->dep_map, "s_active", key, 0);	\
 } while (0)
+
+/* Test for attributes that want to ignore lockdep for read-locking */
+static inline bool sysfs_ignore_lockdep(struct sysfs_dirent *sd)
+{
+	return sysfs_type(sd) == SYSFS_KOBJ_ATTR &&
+		sd->s_attr.attr->ignore_lockdep;
+}
+
 #else
+
 #define sysfs_dirent_init_lockdep(sd) do {} while (0)
+
+static inline bool sysfs_ignore_lockdep(struct sysfs_dirent *sd)
+{
+	return true;
+}
+
 #endif
 
 /*

Re: [PATCH driver-core-next] sysfs: make sysfs_file_ops() follow ignore_lockdep flag

[Yinghai Lu]

On Mon, Oct 14, 2013 at 6:27 AM, Tejun Heo <tj@kernel.org> wrote:
> 375b611e60 ("sysfs: remove sysfs_buffer->ops") introduced
> sysfs_file_ops() which determines the associated file operation of a
> given sysfs_dirent.  As file ops access should be protected by an
> active reference, the new function includes a lockdep assertion on the
> sysfs_dirent; unfortunately, I forgot to take attr->ignore_lockdep
> flag into account and the lockdep assertion trips spuriously for files
> which opt out from active reference lockdep checking.
>
> # cat /sys/devices/pci0000:00/0000:00:01.2/usb1/authorized
>
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 540 at /work/os/work/fs/sysfs/file.c:79 sysfs_file_ops+0x4e/0x60()
>  Modules linked in:
>  CPU: 1 PID: 540 Comm: cat Not tainted 3.11.0-work+ #3
>  Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
>   0000000000000009 ffff880016205c08 ffffffff81ca0131 0000000000000000
>   ffff880016205c40 ffffffff81096d0d ffff8800166cb898 ffff8800166f6f60
>   ffffffff8125a220 ffff880011ab1ec0 ffff88000aff0c78 ffff880016205c50
>  Call Trace:
>   [<ffffffff81ca0131>] dump_stack+0x4e/0x82
>   [<ffffffff81096d0d>] warn_slowpath_common+0x7d/0xa0
>   [<ffffffff81096dea>] warn_slowpath_null+0x1a/0x20
>   [<ffffffff8125994e>] sysfs_file_ops+0x4e/0x60
>   [<ffffffff8125a274>] sysfs_open_file+0x54/0x300
>   [<ffffffff811df612>] do_dentry_open.isra.17+0x182/0x280
>   [<ffffffff811df820>] finish_open+0x30/0x40
>   [<ffffffff811f0623>] do_last+0x503/0xd90
>   [<ffffffff811f0f6b>] path_openat+0xbb/0x6d0
>   [<ffffffff811f23ba>] do_filp_open+0x3a/0x90
>   [<ffffffff811e09a9>] do_sys_open+0x129/0x220
>   [<ffffffff811e0abe>] SyS_open+0x1e/0x20
>   [<ffffffff81caf3c2>] system_call_fastpath+0x16/0x1b
>  ---[ end trace aa48096b111dafdb ]---
>
> Rename fs/sysfs/dir.c::ignore_lockdep() to sysfs_ignore_lockdep() and
> move it to fs/sysfs/sysfs.h and make sysfs_file_ops() skip lockdep
> assertion if sysfs_ignore_lockdep() is true.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Yinghai Lu <yinghai@kernel.org>

Tested-by: Yinghai Lu <yinghai@kernel.org>