Skip to contentSkip to conversation

staging: vc04_services: no need to check debugfs return values

PATCH New Greg Kroah-Hartman 2 participants 7 messages Jun 1, 2018

Subsystems (2)
Staging Broadcom BCM2711/BCM2835 Arm architecture
lore
Share & tools
lore

Greg Kroah-Hartman

author maintainer
#0 lore raw

PATCH 1/6 staging: vc04_services: no need to check debugfs return values

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Clean up the vchiq_arm code by not caring about the value of debugfs calls. This ends up removing a number of lines of code that are not needed.

Cc Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 13 +---
 .../interface/vchiq_arm/vchiq_debugfs.c       | 72 +++----------------
 .../interface/vchiq_arm/vchiq_debugfs.h       |  4 +-
 3 files changed, 15 insertions(+), 74 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +2 -11
index aaa264f3b598..bc05c69383b8 100644
@@ -2018,7 +2018,6 @@ vchiq_open(struct inode *inode, struct file *file)
 	vchiq_log_info(vchiq_arm_log_level, "vchiq_open");
 	switch (dev) {
 	case VCHIQ_MINOR: {
-		int ret;
 		VCHIQ_STATE_T *state = vchiq_get_state();
 		VCHIQ_INSTANCE_T instance;
 

@@ -2035,11 +2034,7 @@ vchiq_open(struct inode *inode, struct file *file)
 		instance->state = state;
 		instance->pid = current->tgid;
 
-		ret = vchiq_debugfs_add_instance(instance);
-		if (ret != 0) {
-			kfree(instance);
-			return ret;
-		}
+		vchiq_debugfs_add_instance(instance);
 
 		sema_init(&instance->insert_event, 0);
 		sema_init(&instance->remove_event, 0);

@@ -3630,9 +3625,7 @@ static int vchiq_probe(struct platform_device *pdev)
 		goto failed_device_create;
 
 	/* create debugfs entries */
-	err = vchiq_debugfs_init();
-	if (err != 0)
-		goto failed_debugfs_init;
+	vchiq_debugfs_init();
 
 	vchiq_log_info(vchiq_arm_log_level,
 		"vchiq: initialised - version %d (min %d), device %d.%d",

@@ -3645,8 +3638,6 @@ static int vchiq_probe(struct platform_device *pdev)
 
 	return 0;
 
-failed_debugfs_init:
-	device_destroy(vchiq_class, vchiq_devid);
 failed_device_create:
 	class_destroy(vchiq_class);
 failed_class_create:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +11 -61

132 lines not on this page · raw message

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.h +2 -2
index 1d95e3d70621..3af6397ada19 100644
@@ -40,11 +40,11 @@ typedef struct vchiq_debugfs_node_struct {
     struct dentry *dentry;
 } VCHIQ_DEBUGFS_NODE_T;
 
-int vchiq_debugfs_init(void);
+void vchiq_debugfs_init(void);
 
 void vchiq_debugfs_deinit(void);
 
-int vchiq_debugfs_add_instance(VCHIQ_INSTANCE_T instance);
+void vchiq_debugfs_add_instance(VCHIQ_INSTANCE_T instance);
 
 void vchiq_debugfs_remove_instance(VCHIQ_INSTANCE_T instance);
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 2/6 staging: vc04_services: remove odd vchiq_debugfs_top() wrapper

vchiq_debugfs_top() is only a wrapper around a pointer to a dentry, so just use the dentry directly instead, making it a static variable instead of part of a static structure.

This also removes the pointless BUG_ON() when checking that dentry as no one should ever care if debugfs is working or not, and the kernel should really not panic over something as trivial as that.

Suggested-by Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../interface/vchiq_arm/vchiq_debugfs.c       | 24 +++++++------------
 1 file changed, 8 insertions(+), 16 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +8 -16
index 103fec955e2c..8b46256a97fc 100644
@@ -53,9 +53,6 @@
 
 /* Top-level debug info */
 struct vchiq_debugfs_info {
-	/* Global 'vchiq' debugfs entry used by all instances */
-	struct dentry *vchiq_cfg_dir;
-
 	/* one entry per client process */
 	struct dentry *clients;
 

@@ -65,6 +62,9 @@ struct vchiq_debugfs_info {
 
 static struct vchiq_debugfs_info debugfs_info;
 
+/* Global 'vchiq' debugfs entry used by all instances */
+struct dentry *vchiq_dbg_dir;
+
 /* Log category debugfs entries */
 struct vchiq_debugfs_log_entry {
 	const char *name;

@@ -82,7 +82,6 @@ static struct vchiq_debugfs_log_entry vchiq_debugfs_log_entries[] = {
 static int n_log_entries = ARRAY_SIZE(vchiq_debugfs_log_entries);
 
 static struct dentry *vchiq_clients_top(void);
-static struct dentry *vchiq_debugfs_top(void);
 
 static int debugfs_log_show(struct seq_file *f, void *offset)
 {

@@ -163,7 +162,7 @@ static void vchiq_debugfs_create_log_entries(struct dentry *top)
 	struct dentry *dir;
 	size_t i;
 
-	dir = debugfs_create_dir("log", vchiq_debugfs_top());
+	dir = debugfs_create_dir("log", vchiq_dbg_dir);
 	debugfs_info.log_categories = dir;
 
 	for (i = 0; i < n_log_entries; i++) {

@@ -286,17 +285,16 @@ void vchiq_debugfs_remove_instance(VCHIQ_INSTANCE_T instance)
 
 void vchiq_debugfs_init(void)
 {
-	debugfs_info.vchiq_cfg_dir = debugfs_create_dir("vchiq", NULL);
-	debugfs_info.clients = debugfs_create_dir("clients",
-				vchiq_debugfs_top());
+	vchiq_dbg_dir = debugfs_create_dir("vchiq", NULL);
+	debugfs_info.clients = debugfs_create_dir("clients", vchiq_dbg_dir);
 
-	vchiq_debugfs_create_log_entries(vchiq_debugfs_top());
+	vchiq_debugfs_create_log_entries(vchiq_dbg_dir);
 }
 
 /* remove all the debugfs entries */
 void vchiq_debugfs_deinit(void)
 {
-	debugfs_remove_recursive(vchiq_debugfs_top());
+	debugfs_remove_recursive(vchiq_dbg_dir);
 }
 
 static struct dentry *vchiq_clients_top(void)

@@ -304,12 +302,6 @@ static struct dentry *vchiq_clients_top(void)
 	return debugfs_info.clients;
 }
 
-static struct dentry *vchiq_debugfs_top(void)
-{
-	BUG_ON(debugfs_info.vchiq_cfg_dir == NULL);
-	return debugfs_info.vchiq_cfg_dir;
-}
-
 #else /* CONFIG_DEBUG_FS */
 
 void vchiq_debugfs_init(void)
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

PATCH 3/6 staging: vc04_services: move client dbg directory into static variable

In reply to Greg Kroah-Hartman

This does not need to be part of a wrapper function, or in a structure, just properly reference it directly as a single variable.

The whole variable will be going away soon anyway, this is just a step toward that direction.

Suggested-by Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../interface/vchiq_arm/vchiq_debugfs.c        | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +4 -14
index 8b46256a97fc..12ed560c95a7 100644
@@ -53,17 +53,15 @@
 
 /* Top-level debug info */
 struct vchiq_debugfs_info {
-	/* one entry per client process */
-	struct dentry *clients;
-
 	/* log categories */
 	struct dentry *log_categories;
 };
 
 static struct vchiq_debugfs_info debugfs_info;
 
-/* Global 'vchiq' debugfs entry used by all instances */
+/* Global 'vchiq' debugfs and clients entry used by all instances */
 struct dentry *vchiq_dbg_dir;
+struct dentry *vchiq_dbg_clients;
 
 /* Log category debugfs entries */
 struct vchiq_debugfs_log_entry {

@@ -81,8 +79,6 @@ static struct vchiq_debugfs_log_entry vchiq_debugfs_log_entries[] = {
 };
 static int n_log_entries = ARRAY_SIZE(vchiq_debugfs_log_entries);
 
-static struct dentry *vchiq_clients_top(void);
-
 static int debugfs_log_show(struct seq_file *f, void *offset)
 {
 	int *levp = f->private;

@@ -262,12 +258,11 @@ void vchiq_debugfs_add_instance(VCHIQ_INSTANCE_T instance)
 {
 	char pidstr[16];
 	struct dentry *top;
-	struct dentry *clients = vchiq_clients_top();
 
 	snprintf(pidstr, sizeof(pidstr), "%d",
 		 vchiq_instance_get_pid(instance));
 
-	top = debugfs_create_dir(pidstr, clients);
+	top = debugfs_create_dir(pidstr, vchiq_dbg_clients);
 
 	debugfs_create_file("use_count", 0444, top, instance,
 			    &debugfs_usecount_fops);

@@ -286,7 +281,7 @@ void vchiq_debugfs_remove_instance(VCHIQ_INSTANCE_T instance)
 void vchiq_debugfs_init(void)
 {
 	vchiq_dbg_dir = debugfs_create_dir("vchiq", NULL);
-	debugfs_info.clients = debugfs_create_dir("clients", vchiq_dbg_dir);
+	vchiq_dbg_clients = debugfs_create_dir("clients", vchiq_dbg_dir);
 
 	vchiq_debugfs_create_log_entries(vchiq_dbg_dir);
 }

@@ -297,11 +292,6 @@ void vchiq_debugfs_deinit(void)
 	debugfs_remove_recursive(vchiq_dbg_dir);
 }
 
-static struct dentry *vchiq_clients_top(void)
-{
-	return debugfs_info.clients;
-}
-
 #else /* CONFIG_DEBUG_FS */
 
 void vchiq_debugfs_init(void)
Signature · 1 line
2.17.1
author maintainer
#3 lore raw

PATCH 4/6 staging: vc04_services: remove struct vchiq_debugfs_info

In reply to Greg Kroah-Hartman

This structure, and the one static variable that was declared with it, were not being used for anything. The log_categories field was being set, but never used again. So just remove it entirely as it is not needed at all.

Suggested-by Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../interface/vchiq_arm/vchiq_debugfs.c            | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +1 -13
index 12ed560c95a7..f18cd56c3634 100644
@@ -51,14 +51,6 @@
 #define VCHIQ_LOG_INFO_STR    "info"
 #define VCHIQ_LOG_TRACE_STR   "trace"
 
-/* Top-level debug info */
-struct vchiq_debugfs_info {
-	/* log categories */
-	struct dentry *log_categories;
-};
-
-static struct vchiq_debugfs_info debugfs_info;
-
 /* Global 'vchiq' debugfs and clients entry used by all instances */
 struct dentry *vchiq_dbg_dir;
 struct dentry *vchiq_dbg_clients;

@@ -159,16 +151,12 @@ static void vchiq_debugfs_create_log_entries(struct dentry *top)
 	size_t i;
 
 	dir = debugfs_create_dir("log", vchiq_dbg_dir);
-	debugfs_info.log_categories = dir;
 
 	for (i = 0; i < n_log_entries; i++) {
 		void *levp = (void *)vchiq_debugfs_log_entries[i].plevel;
 
 		dir = debugfs_create_file(vchiq_debugfs_log_entries[i].name,
-					  0644,
-					  debugfs_info.log_categories,
-					  levp,
-					  &debugfs_log_fops);
+					  0644, dir, levp, &debugfs_log_fops);
 		vchiq_debugfs_log_entries[i].dir = dir;
 	}
 }
Signature · 1 line
2.17.1
author maintainer
#4 lore raw

PATCH 5/6 staging: vc04_services: vchiq_debugfs_log_entry can be a void *

In reply to Greg Kroah-Hartman

There's no need to set this to be int * when it is only used as a void *. This lets us remove the unneeded cast, and unneeded temporary variable the one place it is referenced in the code.

Suggested-by Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../vc04_services/interface/vchiq_arm/vchiq_debugfs.c     | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +4 -4
index f18cd56c3634..2b353d2d25ce 100644
@@ -58,7 +58,7 @@ struct dentry *vchiq_dbg_clients;
 /* Log category debugfs entries */
 struct vchiq_debugfs_log_entry {
 	const char *name;
-	int *plevel;
+	void *plevel;
 	struct dentry *dir;
 };
 

@@ -153,10 +153,10 @@ static void vchiq_debugfs_create_log_entries(struct dentry *top)
 	dir = debugfs_create_dir("log", vchiq_dbg_dir);
 
 	for (i = 0; i < n_log_entries; i++) {
-		void *levp = (void *)vchiq_debugfs_log_entries[i].plevel;
-
 		dir = debugfs_create_file(vchiq_debugfs_log_entries[i].name,
-					  0644, dir, levp, &debugfs_log_fops);
+					  0644, dir,
+					  vchiq_debugfs_log_entries[i].plevel,
+					  &debugfs_log_fops);
 		vchiq_debugfs_log_entries[i].dir = dir;
 	}
 }
Signature · 1 line
2.17.1
author maintainer
#5 lore raw

PATCH 6/6 staging: vc04_services: no need to save the log debufs dentries

In reply to Greg Kroah-Hartman

The log entry dentries are only set, never referenced, so no need to keep them around. Remove the pointer from struct vchiq_debugfs_log_entry as it is not needed anymore and get rid of the separate vchiq_debugfs_create_log_entries() function as it is only used in one place.

Suggested-by Eric Anholt <[email protected]>
Cc Stefan Wahren <[email protected]>
Cc Kees Cook <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Keerthi Reddy <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 .../interface/vchiq_arm/vchiq_debugfs.c       | 29 +++++++------------
 1 file changed, 10 insertions(+), 19 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c +10 -19
index 2b353d2d25ce..38805504d462 100644
@@ -59,7 +59,6 @@ struct dentry *vchiq_dbg_clients;
 struct vchiq_debugfs_log_entry {
 	const char *name;
 	void *plevel;
-	struct dentry *dir;
 };
 
 static struct vchiq_debugfs_log_entry vchiq_debugfs_log_entries[] = {

@@ -144,23 +143,6 @@ static const struct file_operations debugfs_log_fops = {
 	.release	= single_release,
 };
 
-/* create an entry under <debugfs>/vchiq/log for each log category */
-static void vchiq_debugfs_create_log_entries(struct dentry *top)
-{
-	struct dentry *dir;
-	size_t i;
-
-	dir = debugfs_create_dir("log", vchiq_dbg_dir);
-
-	for (i = 0; i < n_log_entries; i++) {
-		dir = debugfs_create_file(vchiq_debugfs_log_entries[i].name,
-					  0644, dir,
-					  vchiq_debugfs_log_entries[i].plevel,
-					  &debugfs_log_fops);
-		vchiq_debugfs_log_entries[i].dir = dir;
-	}
-}
-
 static int debugfs_usecount_show(struct seq_file *f, void *offset)
 {
 	VCHIQ_INSTANCE_T instance = f->private;

@@ -268,10 +250,19 @@ void vchiq_debugfs_remove_instance(VCHIQ_INSTANCE_T instance)
 
 void vchiq_debugfs_init(void)
 {
+	struct dentry *dir;
+	int i;
+
 	vchiq_dbg_dir = debugfs_create_dir("vchiq", NULL);
 	vchiq_dbg_clients = debugfs_create_dir("clients", vchiq_dbg_dir);
 
-	vchiq_debugfs_create_log_entries(vchiq_dbg_dir);
+	/* create an entry under <debugfs>/vchiq/log for each log category */
+	dir = debugfs_create_dir("log", vchiq_dbg_dir);
+
+	for (i = 0; i < n_log_entries; i++)
+		debugfs_create_file(vchiq_debugfs_log_entries[i].name, 0644,
+				    dir, vchiq_debugfs_log_entries[i].plevel,
+				    &debugfs_log_fops);
 }
 
 /* remove all the debugfs entries */
Signature · 1 line
2.17.1
#6 lore raw

PATCH 1/6 staging: vc04_services: no need to check debugfs return values

In reply to Greg Kroah-Hartman
Quoted 7 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Clean up the vchiq_arm code by not caring about the value of debugfs calls. This ends up removing a number of lines of code that are not needed.

This series is:

Reviewed-by Eric Anholt <[email protected]>

Thanks for the cleanups!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180601/d86564ed/attachment.sig>