[PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[Kefeng Wang]

Like proc_create_single/seq[,_data] in procfs, we could provide similar debugfs
helper to reduce losts of boilerplate code.

debugfs_create_single[,_data]
  creates a file in debugfs with the extra data and a seq_file show callback.
debugfs_create_seq[,_data]
  creates a file in debugfs with the extra data and a seq_operations.

There is a object dynamically allocated in the helper, which is used to store
extra data, we need free it when remove the debugfs file.

If the change is acceptable, we could change the caller one by one.

Kefeng Wang (3):
  debugfs: Provide debugfs_[set|clear|test]_lowest_bit()
  debugfs: introduce debugfs_create_single[,_data]
  debugfs: introduce debugfs_create_seq[,_data]

 fs/debugfs/file.c       | 127 +++++++++++++++++++++++++++++++++++-----
 fs/debugfs/inode.c      |  11 ++--
 fs/debugfs/internal.h   |   5 +-
 include/linux/debugfs.h |  34 +++++++++++
 4 files changed, 157 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH next 1/3] debugfs: Provide debugfs_[set|clear|test]_lowest_bit()

[Kefeng Wang]

Provide debugfs_[set|clear|test]_lowest_bit() helper, which
could test, set and clear the lowest bit of a pointer, and
will be used in the subsequent patches.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/debugfs/file.c     | 7 +++----
 fs/debugfs/inode.c    | 7 +++----
 fs/debugfs/internal.h | 5 ++++-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index dede25247b81..38c17a99eb17 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -50,7 +50,7 @@ const struct file_operations *debugfs_real_fops(const struct file *filp)
 {
 	struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
 
-	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) {
+	if (debugfs_test_lowest_bit(fsd)) {
 		/*
 		 * Urgh, we've been called w/o a protecting
 		 * debugfs_file_get().
@@ -84,15 +84,14 @@ int debugfs_file_get(struct dentry *dentry)
 	void *d_fsd;
 
 	d_fsd = READ_ONCE(dentry->d_fsdata);
-	if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+	if (!debugfs_test_lowest_bit(d_fsd)) {
 		fsd = d_fsd;
 	} else {
 		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
 		if (!fsd)
 			return -ENOMEM;
 
-		fsd->real_fops = (void *)((unsigned long)d_fsd &
-					~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+		fsd->real_fops = debugfs_clear_lowest_bit(d_fsd);
 		refcount_set(&fsd->active_users, 1);
 		init_completion(&fsd->active_users_drained);
 		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 7b975dbb2bb4..cc24e97686d0 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -211,7 +211,7 @@ static void debugfs_release_dentry(struct dentry *dentry)
 {
 	void *fsd = dentry->d_fsdata;
 
-	if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
+	if (!debugfs_test_lowest_bit(fsd))
 		kfree(dentry->d_fsdata);
 }
 
@@ -398,8 +398,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
 
 	inode->i_op = &debugfs_file_inode_operations;
 	inode->i_fop = proxy_fops;
-	dentry->d_fsdata = (void *)((unsigned long)real_fops |
-				DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+	dentry->d_fsdata = debugfs_set_lowest_bit(real_fops);
 
 	d_instantiate(dentry, inode);
 	fsnotify_create(d_inode(dentry->d_parent), dentry);
@@ -679,7 +678,7 @@ static void __debugfs_file_removed(struct dentry *dentry)
 	 */
 	smp_mb();
 	fsd = READ_ONCE(dentry->d_fsdata);
-	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
+	if (debugfs_test_lowest_bit(fsd))
 		return;
 	if (!refcount_dec_and_test(&fsd->active_users))
 		wait_for_completion(&fsd->active_users_drained);
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index f0d73d86cc1a..4dd6df4bc172 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -27,6 +27,9 @@ struct debugfs_fsdata {
  * In order to distinguish between these two cases, a real fops
  * pointer gets its lowest bit set.
  */
-#define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
+
+#define debugfs_set_lowest_bit(ptr)	((void *)((unsigned long)(ptr) | BIT(0)))
+#define debugfs_clear_lowest_bit(ptr)	((void *)((unsigned long)(ptr) & ~BIT(0)))
+#define debugfs_test_lowest_bit(ptr)	((unsigned long)(ptr) & BIT(0))
 
 #endif /* _DEBUGFS_INTERNAL_H_ */
-- 
2.20.1

[PATCH next 2/3] debugfs: introduce debugfs_create_single[,_data]

[Kefeng Wang]

Like proc_create_single[,_data] in procfs, introduce a similar
debugfs_create_single[,_data] function, which could drastically
reduces the boilerplate code.

Rename debugfs_devm_entry to debugfs_entry, and reuse debugfs_entry_ops
code. The struct debugfs_entry instance is dynamically allocated
and taken as data in debugfs_create_single_data, which need to be
freed when calling debugfs_remove, using lowest bit of inode->i_private
to distinguish it.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/debugfs/file.c       | 58 ++++++++++++++++++++++++++++++++++-------
 fs/debugfs/inode.c      |  4 +++
 include/linux/debugfs.h | 18 +++++++++++++
 3 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 38c17a99eb17..68f0c4b19bef 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1106,21 +1106,24 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
 
 #endif /* CONFIG_HAS_IOMEM */
 
-struct debugfs_devm_entry {
+struct debugfs_entry {
 	int (*read)(struct seq_file *seq, void *data);
-	struct device *dev;
+	void *data;
 };
 
-static int debugfs_devm_entry_open(struct inode *inode, struct file *f)
+static int debugfs_entry_open(struct inode *inode, struct file *f)
 {
-	struct debugfs_devm_entry *entry = inode->i_private;
+	struct debugfs_entry *entry = inode->i_private;
 
-	return single_open(f, entry->read, entry->dev);
+	if (debugfs_test_lowest_bit(entry))
+		entry = debugfs_clear_lowest_bit(entry);
+
+	return single_open(f, entry->read, entry->data);
 }
 
-static const struct file_operations debugfs_devm_entry_ops = {
+static const struct file_operations debugfs_entry_ops = {
 	.owner = THIS_MODULE,
-	.open = debugfs_devm_entry_open,
+	.open = debugfs_entry_open,
 	.release = single_release,
 	.read = seq_read,
 	.llseek = seq_lseek
@@ -1141,7 +1144,7 @@ struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char *name,
 					   int (*read_fn)(struct seq_file *s,
 							  void *data))
 {
-	struct debugfs_devm_entry *entry;
+	struct debugfs_entry *entry;
 
 	if (IS_ERR(parent))
 		return ERR_PTR(-ENOENT);
@@ -1151,10 +1154,45 @@ struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char *name,
 		return ERR_PTR(-ENOMEM);
 
 	entry->read = read_fn;
-	entry->dev = dev;
+	entry->data = dev;
 
 	return debugfs_create_file(name, S_IRUGO, parent, entry,
-				   &debugfs_devm_entry_ops);
+				   &debugfs_entry_ops);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_devm_seqfile);
 
+/**
+ * debugfs_create_single_data - create a file in the debugfs filesystem
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have.
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @data: a pointer to something that the caller will want to get to later
+ *        on.  The inode.i_private pointer will point to this value on
+ *        the open() call.
+ * @read_fn: function pointer called to print the seq_file content.
+ *
+ * This function creates a file in debugfs with the extra data and a seq_file
+ * show callback.
+ */
+struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
+					  struct dentry *parent, void *data,
+					  int (*read_fn)(struct seq_file *s,
+							 void *data))
+{
+	struct debugfs_entry *entry;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return ERR_PTR(-ENOMEM);
+
+	entry->read = read_fn;
+	entry->data = data;
+
+	entry = debugfs_set_lowest_bit(entry);
+
+	return debugfs_create_file(name, mode, parent, entry,
+				   &debugfs_entry_ops);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_single_data);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index cc24e97686d0..58cbb7af2a55 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -197,6 +197,10 @@ static void debugfs_free_inode(struct inode *inode)
 {
 	if (S_ISLNK(inode->i_mode))
 		kfree(inode->i_link);
+
+	if (debugfs_test_lowest_bit(inode->i_private))
+		kfree(debugfs_clear_lowest_bit(inode->i_private));
+
 	free_inode_nonrcu(inode);
 }
 
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index bf9b6cafa4c2..82171f183e93 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -142,6 +142,11 @@ struct dentry *debugfs_create_devm_seqfile(struct device *dev, const char *name,
 					   int (*read_fn)(struct seq_file *s,
 							  void *data));
 
+struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
+					  struct dentry *parent, void *data,
+					  int (*read_fn)(struct seq_file *s,
+							 void *data));
+
 bool debugfs_initialized(void);
 
 ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
@@ -336,6 +341,16 @@ static inline struct dentry *debugfs_create_devm_seqfile(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct dentry *debugfs_create_single_data(const char *name,
+							umode_t mode,
+							struct dentry *parent,
+							void *data,
+					 int (*read_fn)(struct seq_file *s,
+							void *data))
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline ssize_t debugfs_read_file_bool(struct file *file,
 					     char __user *user_buf,
 					     size_t count, loff_t *ppos)
@@ -352,6 +367,9 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
 
 #endif
 
+#define debugfs_create_single(name, mode, parent, read) \
+	debugfs_create_single_data(name, mode, parent, NULL, read)
+
 /**
  * debugfs_create_xul - create a debugfs file that is used to read and write an
  * unsigned long value, formatted in hexadecimal
-- 
2.20.1

Re: [PATCH next 2/3] debugfs: introduce debugfs_create_single[,_data]

[Dan Carpenter]

[ How do I fetch 0day git branchs?
  git fetch https://github.com/0day-ci/linux/commits/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
  doesn't work. - dan ]

Hi Kefeng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20191128]
[cannot apply to v5.4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
base:    d26b0e226f222c22c3b7e9d16e5b886e7c51057a


If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
fs/debugfs/file.c:1193 debugfs_create_single_data() warn: overwrite may leak 'entry'

Old smatch warnings:
include/linux/compiler.h:247 __write_once_size() warn: potential memory corrupting cast 8 vs 4 bytes

# https://github.com/0day-ci/linux/commit/198a4ea9768d6790a169e8b802e702c208aafbd1
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 198a4ea9768d6790a169e8b802e702c208aafbd1
vim +/entry +1193 fs/debugfs/file.c

198a4ea9768d67 Kefeng Wang      2019-11-29  1179  struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
198a4ea9768d67 Kefeng Wang      2019-11-29  1180  					  struct dentry *parent, void *data,
198a4ea9768d67 Kefeng Wang      2019-11-29  1181  					  int (*read_fn)(struct seq_file *s,
198a4ea9768d67 Kefeng Wang      2019-11-29  1182  							 void *data))
198a4ea9768d67 Kefeng Wang      2019-11-29  1183  {
198a4ea9768d67 Kefeng Wang      2019-11-29  1184  	struct debugfs_entry *entry;
198a4ea9768d67 Kefeng Wang      2019-11-29  1185  
198a4ea9768d67 Kefeng Wang      2019-11-29  1186  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
198a4ea9768d67 Kefeng Wang      2019-11-29  1187  	if (!entry)
198a4ea9768d67 Kefeng Wang      2019-11-29  1188  		return ERR_PTR(-ENOMEM);
198a4ea9768d67 Kefeng Wang      2019-11-29  1189  
198a4ea9768d67 Kefeng Wang      2019-11-29  1190  	entry->read = read_fn;
198a4ea9768d67 Kefeng Wang      2019-11-29  1191  	entry->data = data;
198a4ea9768d67 Kefeng Wang      2019-11-29  1192  
198a4ea9768d67 Kefeng Wang      2019-11-29 @1193  	entry = debugfs_set_lowest_bit(entry);
                                                        ^^^^^^^^
I haven't looked at the surrounding code but how would we free "entry"
when we write over it here?

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Re: [kbuild-all] Re: [PATCH next 2/3] debugfs: introduce debugfs_create_single[,_data]

[Rong Chen]

On 12/3/19 4:38 PM, Dan Carpenter wrote:
> [ How do I fetch 0day git branchs?
>    git fetch https://github.com/0day-ci/linux/commits/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
>    doesn't work. - dan ]

Hi Dan,

I can fetch it by the following steps:

nfs@shao2-debian:~/linux$ git remote add 0day-linux-review 
https://github.com/0day-ci/linux.git
nfs@shao2-debian:~/linux$ git fetch --no-tags 0day-linux-review 
Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440 

remote: Enumerating objects: 25261, done.
remote: Counting objects: 100% (25261/25261), done.
remote: Total 33246 (delta 25260), reused 25260 (delta 25260), 
pack-reused 7985
Receiving objects: 100% (33246/33246), 12.03 MiB | 1.05 MiB/s, done.
Resolving deltas: 100% (28148/28148), completed with 6149 local objects.
 From https://github.com/0day-ci/linux
  * branch 
Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440 
-> FETCH_HEAD
  * [new branch] 
Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440 
-> 
0day-linux-review/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440

Best Regards,
Rong Chen

>
> Hi Kefeng,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on next-20191128]
> [cannot apply to v5.4]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> base:    d26b0e226f222c22c3b7e9d16e5b886e7c51057a
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> fs/debugfs/file.c:1193 debugfs_create_single_data() warn: overwrite may leak 'entry'
>
> Old smatch warnings:
> include/linux/compiler.h:247 __write_once_size() warn: potential memory corrupting cast 8 vs 4 bytes
>
> # https://github.com/0day-ci/linux/commit/198a4ea9768d6790a169e8b802e702c208aafbd1
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 198a4ea9768d6790a169e8b802e702c208aafbd1
> vim +/entry +1193 fs/debugfs/file.c
>
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1179  struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1180  					  struct dentry *parent, void *data,
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1181  					  int (*read_fn)(struct seq_file *s,
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1182  							 void *data))
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1183  {
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1184  	struct debugfs_entry *entry;
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1185
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1186  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1187  	if (!entry)
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1188  		return ERR_PTR(-ENOMEM);
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1189
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1190  	entry->read = read_fn;
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1191  	entry->data = data;
> 198a4ea9768d67 Kefeng Wang      2019-11-29  1192
> 198a4ea9768d67 Kefeng Wang      2019-11-29 @1193  	entry = debugfs_set_lowest_bit(entry);
>                                                          ^^^^^^^^
> I haven't looked at the surrounding code but how would we free "entry"
> when we write over it here?
>
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org

Re: [kbuild-all] Re: [PATCH next 2/3] debugfs: introduce debugfs_create_single[,_data]

[Dan Carpenter]

On Tue, Dec 03, 2019 at 05:02:54PM +0800, Rong Chen wrote:
> 
> 
> On 12/3/19 4:38 PM, Dan Carpenter wrote:
> > [ How do I fetch 0day git branchs?
> >    git fetch https://github.com/0day-ci/linux/commits/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> >    doesn't work. - dan ]
> 
> Hi Dan,
> 
> I can fetch it by the following steps:
> 
> nfs@shao2-debian:~/linux$ git remote add 0day-linux-review
> https://github.com/0day-ci/linux.git
> nfs@shao2-debian:~/linux$ git fetch --no-tags 0day-linux-review
> Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> 
> remote: Enumerating objects: 25261, done.
> remote: Counting objects: 100% (25261/25261), done.
> remote: Total 33246 (delta 25260), reused 25260 (delta 25260), pack-reused
> 7985
> Receiving objects: 100% (33246/33246), 12.03 MiB | 1.05 MiB/s, done.
> Resolving deltas: 100% (28148/28148), completed with 6149 local objects.
> From https://github.com/0day-ci/linux
>  * branch
> Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> -> FETCH_HEAD
>  * [new branch]
> Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> -> 0day-linux-review/Kefeng-Wang/debugfs-introduce-debugfs_create_single-seq-_data/20191129-173440
> 

Tracking a remote is unworkably slow on my system.  It's way better to
try fetch a specific branch if possible.

regards,
dan carpenter

[PATCH next 3/3] debugfs: introduce debugfs_create_seq[,_data]

[Kefeng Wang]

Like proc_create_seq[,_data] in procfs, introduce a similar
debugfs_create_seq[,_data] function, which could drastically
reduces the boilerplate code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/debugfs/file.c       | 62 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/debugfs.h | 16 +++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 68f0c4b19bef..77522717c9fb 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1107,7 +1107,10 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
 #endif /* CONFIG_HAS_IOMEM */
 
 struct debugfs_entry {
-	int (*read)(struct seq_file *seq, void *data);
+	union {
+		const struct seq_operations *seq_ops;
+		int (*read)(struct seq_file *seq, void *data);
+	};
 	void *data;
 };
 
@@ -1196,3 +1199,60 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
 				   &debugfs_entry_ops);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_single_data);
+
+static int debugfs_entry_seq_open(struct inode *inode, struct file *file)
+{
+	struct debugfs_entry *entry = inode->i_private;
+	int ret;
+
+	entry = debugfs_clear_lowest_bit(entry);
+
+	ret = seq_open(file, entry->seq_ops);
+	if (!ret && entry->data) {
+		struct seq_file *seq = file->private_data;
+		seq->private = entry->data;
+	}
+
+	return ret;
+}
+
+static const struct file_operations debugfs_seq_fops = {
+	.open		= debugfs_entry_seq_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+
+/**
+ * debugfs_create_seq_data - create a file in the debugfs filesystem
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have.
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @data: a pointer to something that the caller will want to get to later
+ *        on.  The inode.i_private pointer will point to this value on
+ *        the open() call.
+ * @seq_ops: seq_operations pointer of the seqfile.
+ *
+ * This function creates a file in debugfs with the extra data and a seq_ops.
+ */
+struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
+				       struct dentry *parent, void *data,
+				       const struct seq_operations *seq_ops)
+{
+	struct debugfs_entry *entry;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return ERR_PTR(-ENOMEM);
+
+	entry->seq_ops = seq_ops;
+	entry->data = data;
+
+	entry = debugfs_set_lowest_bit(entry);
+
+	return debugfs_create_file(name, mode, parent, entry,
+				   &debugfs_seq_fops);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_seq_data);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 82171f183e93..d32d49983547 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -147,6 +147,10 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
 					  int (*read_fn)(struct seq_file *s,
 							 void *data));
 
+struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
+				       struct dentry *parent, void *data,
+				       const struct seq_operations *seq_ops);
+
 bool debugfs_initialized(void);
 
 ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
@@ -351,6 +355,15 @@ static inline struct dentry *debugfs_create_single_data(const char *name,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct dentry *debugfs_create_seq_data(const char *name,
+						     umode_t mode,
+						     struct dentry *parent,
+						     void *data,
+				       const struct seq_operations *seq_ops)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline ssize_t debugfs_read_file_bool(struct file *file,
 					     char __user *user_buf,
 					     size_t count, loff_t *ppos)
@@ -370,6 +383,9 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
 #define debugfs_create_single(name, mode, parent, read) \
 	debugfs_create_single_data(name, mode, parent, NULL, read)
 
+#define debugfs_create_seq(name, mode, parent, ops) \
+	debugfs_create_seq_data(name, mode, parent, NULL, ops)
+
 /**
  * debugfs_create_xul - create a debugfs file that is used to read and write an
  * unsigned long value, formatted in hexadecimal
-- 
2.20.1

Re: [PATCH next 3/3] debugfs: introduce debugfs_create_seq[,_data]

[Greg KH]

On Fri, Nov 29, 2019 at 05:27:52PM +0800, Kefeng Wang wrote:
> Like proc_create_seq[,_data] in procfs, introduce a similar
> debugfs_create_seq[,_data] function, which could drastically
> reduces the boilerplate code.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  fs/debugfs/file.c       | 62 ++++++++++++++++++++++++++++++++++++++++-
>  include/linux/debugfs.h | 16 +++++++++++
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 68f0c4b19bef..77522717c9fb 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -1107,7 +1107,10 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
>  #endif /* CONFIG_HAS_IOMEM */
>  
>  struct debugfs_entry {
> -	int (*read)(struct seq_file *seq, void *data);
> +	union {
> +		const struct seq_operations *seq_ops;
> +		int (*read)(struct seq_file *seq, void *data);
> +	};
>  	void *data;
>  };
>  
> @@ -1196,3 +1199,60 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
>  				   &debugfs_entry_ops);
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_single_data);
> +
> +static int debugfs_entry_seq_open(struct inode *inode, struct file *file)
> +{
> +	struct debugfs_entry *entry = inode->i_private;
> +	int ret;
> +
> +	entry = debugfs_clear_lowest_bit(entry);
> +
> +	ret = seq_open(file, entry->seq_ops);
> +	if (!ret && entry->data) {
> +		struct seq_file *seq = file->private_data;
> +		seq->private = entry->data;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations debugfs_seq_fops = {
> +	.open		= debugfs_entry_seq_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
> +};
> +
> +/**
> + * debugfs_create_seq_data - create a file in the debugfs filesystem
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have.
> + * @parent: a pointer to the parent dentry for this file.  This should be a
> + *          directory dentry if set.  If this parameter is NULL, then the
> + *          file will be created in the root of the debugfs filesystem.
> + * @data: a pointer to something that the caller will want to get to later
> + *        on.  The inode.i_private pointer will point to this value on
> + *        the open() call.
> + * @seq_ops: seq_operations pointer of the seqfile.
> + *
> + * This function creates a file in debugfs with the extra data and a seq_ops.
> + */
> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
> +				       struct dentry *parent, void *data,
> +				       const struct seq_operations *seq_ops)
> +{
> +	struct debugfs_entry *entry;
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return ERR_PTR(-ENOMEM);
> +
> +	entry->seq_ops = seq_ops;
> +	entry->data = data;
> +
> +	entry = debugfs_set_lowest_bit(entry);
> +
> +	return debugfs_create_file(name, mode, parent, entry,
> +				   &debugfs_seq_fops);
> +}
> +EXPORT_SYMBOL_GPL(debugfs_create_seq_data);
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 82171f183e93..d32d49983547 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -147,6 +147,10 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
>  					  int (*read_fn)(struct seq_file *s,
>  							 void *data));
>  
> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
> +				       struct dentry *parent, void *data,
> +				       const struct seq_operations *seq_ops);
> +
>  bool debugfs_initialized(void);
>  
>  ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,

If you notice, I have been removing the return value of the
debugfs_create_* functions over the past few kernel versions (look at
5.5-rc1 for a lot more).  Please don't add any new functions that also
return a dentry that I then need to go and remove.

Just have these be void functions, no need to return anything.

thanks,

greg k-h

Re: [PATCH next 3/3] debugfs: introduce debugfs_create_seq[,_data]

[Kefeng Wang]

On 2019/11/29 22:22, Greg KH wrote:
> On Fri, Nov 29, 2019 at 05:27:52PM +0800, Kefeng Wang wrote:
>> Like proc_create_seq[,_data] in procfs, introduce a similar
>> debugfs_create_seq[,_data] function, which could drastically
>> reduces the boilerplate code.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  fs/debugfs/file.c       | 62 ++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/debugfs.h | 16 +++++++++++
>>  2 files changed, 77 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 68f0c4b19bef..77522717c9fb 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -1107,7 +1107,10 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
>>  #endif /* CONFIG_HAS_IOMEM */
>>  
>>  struct debugfs_entry {
>> -	int (*read)(struct seq_file *seq, void *data);
>> +	union {
>> +		const struct seq_operations *seq_ops;
>> +		int (*read)(struct seq_file *seq, void *data);
>> +	};
>>  	void *data;
>>  };
>>  
>> @@ -1196,3 +1199,60 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
>>  				   &debugfs_entry_ops);
>>  }
>>  EXPORT_SYMBOL_GPL(debugfs_create_single_data);
>> +
>> +static int debugfs_entry_seq_open(struct inode *inode, struct file *file)
>> +{
>> +	struct debugfs_entry *entry = inode->i_private;
>> +	int ret;
>> +
>> +	entry = debugfs_clear_lowest_bit(entry);
>> +
>> +	ret = seq_open(file, entry->seq_ops);
>> +	if (!ret && entry->data) {
>> +		struct seq_file *seq = file->private_data;
>> +		seq->private = entry->data;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations debugfs_seq_fops = {
>> +	.open		= debugfs_entry_seq_open,
>> +	.read		= seq_read,
>> +	.llseek		= seq_lseek,
>> +	.release	= seq_release,
>> +};
>> +
>> +/**
>> + * debugfs_create_seq_data - create a file in the debugfs filesystem
>> + * @name: a pointer to a string containing the name of the file to create.
>> + * @mode: the permission that the file should have.
>> + * @parent: a pointer to the parent dentry for this file.  This should be a
>> + *          directory dentry if set.  If this parameter is NULL, then the
>> + *          file will be created in the root of the debugfs filesystem.
>> + * @data: a pointer to something that the caller will want to get to later
>> + *        on.  The inode.i_private pointer will point to this value on
>> + *        the open() call.
>> + * @seq_ops: seq_operations pointer of the seqfile.
>> + *
>> + * This function creates a file in debugfs with the extra data and a seq_ops.
>> + */
>> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
>> +				       struct dentry *parent, void *data,
>> +				       const struct seq_operations *seq_ops)
>> +{
>> +	struct debugfs_entry *entry;
>> +
>> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	entry->seq_ops = seq_ops;
>> +	entry->data = data;
>> +
>> +	entry = debugfs_set_lowest_bit(entry);
>> +
>> +	return debugfs_create_file(name, mode, parent, entry,
>> +				   &debugfs_seq_fops);
>> +}
>> +EXPORT_SYMBOL_GPL(debugfs_create_seq_data);
>> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
>> index 82171f183e93..d32d49983547 100644
>> --- a/include/linux/debugfs.h
>> +++ b/include/linux/debugfs.h
>> @@ -147,6 +147,10 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
>>  					  int (*read_fn)(struct seq_file *s,
>>  							 void *data));
>>  
>> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
>> +				       struct dentry *parent, void *data,
>> +				       const struct seq_operations *seq_ops);
>> +
>>  bool debugfs_initialized(void);
>>  
>>  ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> 
> If you notice, I have been removing the return value of the
> debugfs_create_* functions over the past few kernel versions (look at
> 5.5-rc1 for a lot more).  Please don't add any new functions that also
> return a dentry that I then need to go and remove.

Hi Greg, see function debugfs_create_seq_data()and debugfs_create_single_data(),
they are wrapper function of debugfs_create_file(), when remove the debugfs file
called debugfs_remove(), some drivers could do such thing, we must return a dentry
to the caller.

> 
> Just have these be void functions, no need to return anything.
> 
> thanks,
> 
> greg k-h
> 
> .
> 

Re: [PATCH next 3/3] debugfs: introduce debugfs_create_seq[,_data]

[Greg KH]

On Fri, Nov 29, 2019 at 11:03:47PM +0800, Kefeng Wang wrote:
> 
> 
> On 2019/11/29 22:22, Greg KH wrote:
> > On Fri, Nov 29, 2019 at 05:27:52PM +0800, Kefeng Wang wrote:
> >> Like proc_create_seq[,_data] in procfs, introduce a similar
> >> debugfs_create_seq[,_data] function, which could drastically
> >> reduces the boilerplate code.
> >>
> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >> ---
> >>  fs/debugfs/file.c       | 62 ++++++++++++++++++++++++++++++++++++++++-
> >>  include/linux/debugfs.h | 16 +++++++++++
> >>  2 files changed, 77 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> >> index 68f0c4b19bef..77522717c9fb 100644
> >> --- a/fs/debugfs/file.c
> >> +++ b/fs/debugfs/file.c
> >> @@ -1107,7 +1107,10 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
> >>  #endif /* CONFIG_HAS_IOMEM */
> >>  
> >>  struct debugfs_entry {
> >> -	int (*read)(struct seq_file *seq, void *data);
> >> +	union {
> >> +		const struct seq_operations *seq_ops;
> >> +		int (*read)(struct seq_file *seq, void *data);
> >> +	};
> >>  	void *data;
> >>  };
> >>  
> >> @@ -1196,3 +1199,60 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
> >>  				   &debugfs_entry_ops);
> >>  }
> >>  EXPORT_SYMBOL_GPL(debugfs_create_single_data);
> >> +
> >> +static int debugfs_entry_seq_open(struct inode *inode, struct file *file)
> >> +{
> >> +	struct debugfs_entry *entry = inode->i_private;
> >> +	int ret;
> >> +
> >> +	entry = debugfs_clear_lowest_bit(entry);
> >> +
> >> +	ret = seq_open(file, entry->seq_ops);
> >> +	if (!ret && entry->data) {
> >> +		struct seq_file *seq = file->private_data;
> >> +		seq->private = entry->data;
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static const struct file_operations debugfs_seq_fops = {
> >> +	.open		= debugfs_entry_seq_open,
> >> +	.read		= seq_read,
> >> +	.llseek		= seq_lseek,
> >> +	.release	= seq_release,
> >> +};
> >> +
> >> +/**
> >> + * debugfs_create_seq_data - create a file in the debugfs filesystem
> >> + * @name: a pointer to a string containing the name of the file to create.
> >> + * @mode: the permission that the file should have.
> >> + * @parent: a pointer to the parent dentry for this file.  This should be a
> >> + *          directory dentry if set.  If this parameter is NULL, then the
> >> + *          file will be created in the root of the debugfs filesystem.
> >> + * @data: a pointer to something that the caller will want to get to later
> >> + *        on.  The inode.i_private pointer will point to this value on
> >> + *        the open() call.
> >> + * @seq_ops: seq_operations pointer of the seqfile.
> >> + *
> >> + * This function creates a file in debugfs with the extra data and a seq_ops.
> >> + */
> >> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
> >> +				       struct dentry *parent, void *data,
> >> +				       const struct seq_operations *seq_ops)
> >> +{
> >> +	struct debugfs_entry *entry;
> >> +
> >> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> >> +	if (!entry)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	entry->seq_ops = seq_ops;
> >> +	entry->data = data;
> >> +
> >> +	entry = debugfs_set_lowest_bit(entry);
> >> +
> >> +	return debugfs_create_file(name, mode, parent, entry,
> >> +				   &debugfs_seq_fops);
> >> +}
> >> +EXPORT_SYMBOL_GPL(debugfs_create_seq_data);
> >> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> >> index 82171f183e93..d32d49983547 100644
> >> --- a/include/linux/debugfs.h
> >> +++ b/include/linux/debugfs.h
> >> @@ -147,6 +147,10 @@ struct dentry *debugfs_create_single_data(const char *name, umode_t mode,
> >>  					  int (*read_fn)(struct seq_file *s,
> >>  							 void *data));
> >>  
> >> +struct dentry *debugfs_create_seq_data(const char *name, umode_t mode,
> >> +				       struct dentry *parent, void *data,
> >> +				       const struct seq_operations *seq_ops);
> >> +
> >>  bool debugfs_initialized(void);
> >>  
> >>  ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> > 
> > If you notice, I have been removing the return value of the
> > debugfs_create_* functions over the past few kernel versions (look at
> > 5.5-rc1 for a lot more).  Please don't add any new functions that also
> > return a dentry that I then need to go and remove.
> 
> Hi Greg, see function debugfs_create_seq_data()and debugfs_create_single_data(),
> they are wrapper function of debugfs_create_file(), when remove the debugfs file
> called debugfs_remove(), some drivers could do such thing, we must return a dentry
> to the caller.

Again, no, we should not return a dentry.  Files should go in
directories and then removed all at once if they need to, which is why I
have been making these types of changes.

Yes, for some existing files that does not work, but again, please do
not create a new api that does this.  Only use it for the files that do
not need their dentries saved.

thanks,

greg k-h

Re: [PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[Greg KH]

On Fri, Nov 29, 2019 at 05:27:49PM +0800, Kefeng Wang wrote:
> Like proc_create_single/seq[,_data] in procfs, we could provide similar debugfs
> helper to reduce losts of boilerplate code.
> 
> debugfs_create_single[,_data]
>   creates a file in debugfs with the extra data and a seq_file show callback.
> debugfs_create_seq[,_data]
>   creates a file in debugfs with the extra data and a seq_operations.
> 
> There is a object dynamically allocated in the helper, which is used to store
> extra data, we need free it when remove the debugfs file.
> 
> If the change is acceptable, we could change the caller one by one.

I would like to see a user of this and how you would convert it, in
order to see if this is worth it or not.

When you redo this series, can you add that to the end of it?

thanks,

greg k-h

Re: [PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[Kefeng Wang]

On 2019/11/29 22:21, Greg KH wrote:
> On Fri, Nov 29, 2019 at 05:27:49PM +0800, Kefeng Wang wrote:
>> Like proc_create_single/seq[,_data] in procfs, we could provide similar debugfs
>> helper to reduce losts of boilerplate code.
>>
>> debugfs_create_single[,_data]
>>   creates a file in debugfs with the extra data and a seq_file show callback.
>> debugfs_create_seq[,_data]
>>   creates a file in debugfs with the extra data and a seq_operations.
>>
>> There is a object dynamically allocated in the helper, which is used to store
>> extra data, we need free it when remove the debugfs file.
>>
>> If the change is acceptable, we could change the caller one by one.
> 
> I would like to see a user of this and how you would convert it, in
> order to see if this is worth it or not.

I have some diff patches, the conversion is in progress. current statistics
are as follows,

1) debugfs: switch to debugfs_create_seq[,_data]
19 files changed, 85 insertions(+), 620 deletions(-)
2) debugfs: switch to debugfs_create_single[,_data]
70 files changed, 249 insertions(+), 1482 deletions(-)

Here are some examples,
1) debugfs_create_seq
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 78d53378db99..62c26772f24c 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -2057,18 +2057,6 @@ static const struct seq_operations unusable_op = {
 	.show	= unusable_show,
 };

-static int unusable_open(struct inode *inode, struct file *file)
-{
-	return seq_open(file, &unusable_op);
-}
-
-static const struct file_operations unusable_file_ops = {
-	.open		= unusable_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= seq_release,
-};
-
 static void extfrag_show_print(struct seq_file *m,
 					pg_data_t *pgdat, struct zone *zone)
 {
@@ -2109,29 +2097,17 @@ static const struct seq_operations extfrag_op = {
 	.show	= extfrag_show,
 };

-static int extfrag_open(struct inode *inode, struct file *file)
-{
-	return seq_open(file, &extfrag_op);
-}
-
-static const struct file_operations extfrag_file_ops = {
-	.open		= extfrag_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= seq_release,
-};
-
 static int __init extfrag_debug_init(void)
 {
 	struct dentry *extfrag_debug_root;

 	extfrag_debug_root = debugfs_create_dir("extfrag", NULL);

-	debugfs_create_file("unusable_index", 0444, extfrag_debug_root, NULL,
-			    &unusable_file_ops);
+	debugfs_create_seq("unusable_index", 0444, extfrag_debug_root,
+			   &unusable_op);

-	debugfs_create_file("extfrag_index", 0444, extfrag_debug_root, NULL,
-			    &extfrag_file_ops);
+	debugfs_create_seq("extfrag_index", 0444, extfrag_debug_root,
+			   &extfrag_op);

 	return 0;
 }

2) debugfs_create_single_data()
diff --git a/net/hsr/hsr_debugfs.c b/net/hsr/hsr_debugfs.c
index 94447974a3c0..8bdd70af02c9 100644
--- a/net/hsr/hsr_debugfs.c
+++ b/net/hsr/hsr_debugfs.c
@@ -52,25 +52,6 @@ hsr_node_table_show(struct seq_file *sfp, void *data)
 	return 0;
 }

-/* hsr_node_table_open - Open the node_table file
- *
- * Description:
- * This routine opens a debugfs file node_table of specific hsr device
- */
-static int
-hsr_node_table_open(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, hsr_node_table_show, inode->i_private);
-}
-
-static const struct file_operations hsr_fops = {
-	.owner	= THIS_MODULE,
-	.open	= hsr_node_table_open,
-	.read	= seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
-
 /* hsr_debugfs_init - create hsr node_table file for dumping
  * the node table
  *
@@ -91,9 +72,9 @@ int hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)

 	priv->node_tbl_root = de;

-	de = debugfs_create_file("node_table", S_IFREG | 0444,
-				 priv->node_tbl_root, priv,
-				 &hsr_fops);
+	de = debugfs_create_single_data("node_table", S_IFREG | 0444,
+					priv->node_tbl_root, priv,
+					hsr_node_table_show);
 	if (!de) {
 		pr_err("Cannot create hsr node_table directory\n");
 		return rc;
-- 
2.20.1


> 
> When you redo this series, can you add that to the end of it?
> 
> thanks,
> 
> greg k-h
> 
> .
> 

Re: [PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[Greg KH]

On Fri, Nov 29, 2019 at 11:16:38PM +0800, Kefeng Wang wrote:
> 
> 
> On 2019/11/29 22:21, Greg KH wrote:
> > On Fri, Nov 29, 2019 at 05:27:49PM +0800, Kefeng Wang wrote:
> >> Like proc_create_single/seq[,_data] in procfs, we could provide similar debugfs
> >> helper to reduce losts of boilerplate code.
> >>
> >> debugfs_create_single[,_data]
> >>   creates a file in debugfs with the extra data and a seq_file show callback.
> >> debugfs_create_seq[,_data]
> >>   creates a file in debugfs with the extra data and a seq_operations.
> >>
> >> There is a object dynamically allocated in the helper, which is used to store
> >> extra data, we need free it when remove the debugfs file.
> >>
> >> If the change is acceptable, we could change the caller one by one.
> > 
> > I would like to see a user of this and how you would convert it, in
> > order to see if this is worth it or not.
> 
> I have some diff patches, the conversion is in progress. current statistics
> are as follows,
> 
> 1) debugfs: switch to debugfs_create_seq[,_data]
> 19 files changed, 85 insertions(+), 620 deletions(-)
> 2) debugfs: switch to debugfs_create_single[,_data]
> 70 files changed, 249 insertions(+), 1482 deletions(-)
> 
> Here are some examples,
> 1) debugfs_create_seq
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 78d53378db99..62c26772f24c 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -2057,18 +2057,6 @@ static const struct seq_operations unusable_op = {
>  	.show	= unusable_show,
>  };
> 
> -static int unusable_open(struct inode *inode, struct file *file)
> -{
> -	return seq_open(file, &unusable_op);
> -}
> -
> -static const struct file_operations unusable_file_ops = {
> -	.open		= unusable_open,
> -	.read		= seq_read,
> -	.llseek		= seq_lseek,
> -	.release	= seq_release,
> -};
> -

Can't this file just use the normal file macro/interface for debugfs
files instead?  Hm, maybe not, it seems the celf code wants to do much
the same as above, but is seq_read() really needed for these?

There are loads of places where open-coded debugfs file ops can just be
converted to use the existing macros, maybe do that work first before
adding new ones?

thanks,

greg k-h

Re: [PATCH next 0/3] debugfs: introduce debugfs_create_single/seq[,_data]

[Greg KH]

On Fri, Nov 29, 2019 at 11:19:38PM +0100, Greg KH wrote:
> On Fri, Nov 29, 2019 at 11:16:38PM +0800, Kefeng Wang wrote:
> > 
> > 
> > On 2019/11/29 22:21, Greg KH wrote:
> > > On Fri, Nov 29, 2019 at 05:27:49PM +0800, Kefeng Wang wrote:
> > >> Like proc_create_single/seq[,_data] in procfs, we could provide similar debugfs
> > >> helper to reduce losts of boilerplate code.
> > >>
> > >> debugfs_create_single[,_data]
> > >>   creates a file in debugfs with the extra data and a seq_file show callback.
> > >> debugfs_create_seq[,_data]
> > >>   creates a file in debugfs with the extra data and a seq_operations.
> > >>
> > >> There is a object dynamically allocated in the helper, which is used to store
> > >> extra data, we need free it when remove the debugfs file.
> > >>
> > >> If the change is acceptable, we could change the caller one by one.
> > > 
> > > I would like to see a user of this and how you would convert it, in
> > > order to see if this is worth it or not.
> > 
> > I have some diff patches, the conversion is in progress. current statistics
> > are as follows,
> > 
> > 1) debugfs: switch to debugfs_create_seq[,_data]
> > 19 files changed, 85 insertions(+), 620 deletions(-)
> > 2) debugfs: switch to debugfs_create_single[,_data]
> > 70 files changed, 249 insertions(+), 1482 deletions(-)
> > 
> > Here are some examples,
> > 1) debugfs_create_seq
> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > index 78d53378db99..62c26772f24c 100644
> > --- a/mm/vmstat.c
> > +++ b/mm/vmstat.c
> > @@ -2057,18 +2057,6 @@ static const struct seq_operations unusable_op = {
> >  	.show	= unusable_show,
> >  };
> > 
> > -static int unusable_open(struct inode *inode, struct file *file)
> > -{
> > -	return seq_open(file, &unusable_op);
> > -}
> > -
> > -static const struct file_operations unusable_file_ops = {
> > -	.open		= unusable_open,
> > -	.read		= seq_read,
> > -	.llseek		= seq_lseek,
> > -	.release	= seq_release,
> > -};
> > -
> 
> Can't this file just use the normal file macro/interface for debugfs
> files instead?  Hm, maybe not, it seems the celf code wants to do much
> the same as above, but is seq_read() really needed for these?

I refer to DEFINE_SIMPLE_ATTRIBUTE(), sorry for not saying that here.

And if they do not work, how about creating:
	DEFINE_SEQ_ATTRIBUTE()
in much the same way for the whole kernel to use.

thanks,

greg k-h