iwl: cleanup: remove unneeded error handling

PATCH In review Dan Carpenter 2 participants 3 messages Apr 28, 2010 – Apr 29, 2010

Subsystems (4)

Networking Network drivers Kernel janitors Networking drivers (wireless)

lore

Share & tools

lore

Dan Carpenter

author

2010-04-28 09:01 UTC #0 lore raw

PATCH 1/1 iwl: cleanup: remove unneeded error handling

This is just a cleanup and doesn't change how the code works.

debugfs_create_dir() and debugfs_create_file() return an error pointer (-ENODEV) if CONFIG_DEBUG_FS is not enabled, otherwise if an error occurs they return NULL. This is how they are implemented and what it says in the DebugFS documentation. DebugFS can not be compiled as a module.

As a result, we only need to check for error pointers and particularly -ENODEV one time to know that DebugFS is enabled. This patch keeps the first check for error pointers and removes the rest.

The other reason for this patch, is that it silences some Smatch warnings. Smatch sees the condition "(result != -ENODEV)" and assumes that it's possible for "result" to equal -ENODEV. If it were possible it would lead to an error pointer dereference. But since it's not, we can just remove the check.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/wireless/iwmc3200wifi/debugfs.c +0 ~~-63~~

index cbb81be..10eeeeb 100644
@@ -438,40 +438,10 @@ int iwm_debugfs_init(struct iwm_priv *iwm)
 	snprintf(devdir, sizeof(devdir), "%s", wiphy_name(iwm_to_wiphy(iwm)));
 
 	iwm->dbg.devdir = debugfs_create_dir(devdir, iwm->dbg.rootdir);
-	result = PTR_ERR(iwm->dbg.devdir);
-	if (IS_ERR(iwm->dbg.devdir) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create devdir: %d\n", result);
-		goto error;
-	}
-
 	iwm->dbg.dbgdir = debugfs_create_dir("debug", iwm->dbg.devdir);
-	result = PTR_ERR(iwm->dbg.dbgdir);
-	if (IS_ERR(iwm->dbg.dbgdir) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create dbgdir: %d\n", result);
-		goto error;
-	}
-
 	iwm->dbg.rxdir = debugfs_create_dir("rx", iwm->dbg.devdir);
-	result = PTR_ERR(iwm->dbg.rxdir);
-	if (IS_ERR(iwm->dbg.rxdir) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create rx dir: %d\n", result);
-		goto error;
-	}
-
 	iwm->dbg.txdir = debugfs_create_dir("tx", iwm->dbg.devdir);
-	result = PTR_ERR(iwm->dbg.txdir);
-	if (IS_ERR(iwm->dbg.txdir) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create tx dir: %d\n", result);
-		goto error;
-	}
-
 	iwm->dbg.busdir = debugfs_create_dir("bus", iwm->dbg.devdir);
-	result = PTR_ERR(iwm->dbg.busdir);
-	if (IS_ERR(iwm->dbg.busdir) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create bus dir: %d\n", result);
-		goto error;
-	}
-
 	if (iwm->bus_ops->debugfs_init) {
 		result = iwm->bus_ops->debugfs_init(iwm, iwm->dbg.busdir);
 		if (result < 0) {

@@ -480,27 +450,15 @@ int iwm_debugfs_init(struct iwm_priv *iwm)
 		}
 	}
 
-
 	iwm->dbg.dbg_level = IWM_DL_NONE;
 	iwm->dbg.dbg_level_dentry  		debugfs_create_file("level", 0200, iwm->dbg.dbgdir, iwm,
 				    &fops_iwm_dbg_level);
-	result = PTR_ERR(iwm->dbg.dbg_level_dentry);
-	if (IS_ERR(iwm->dbg.dbg_level_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create dbg_level: %d\n", result);
-		goto error;
-	}
-
 
 	iwm->dbg.dbg_modules = IWM_DM_DEFAULT;
 	iwm->dbg.dbg_modules_dentry  		debugfs_create_file("modules", 0200, iwm->dbg.dbgdir, iwm,
 				    &fops_iwm_dbg_modules);
-	result = PTR_ERR(iwm->dbg.dbg_modules_dentry);
-	if (IS_ERR(iwm->dbg.dbg_modules_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create dbg_modules: %d\n", result);
-		goto error;
-	}
 
 	for (i = 0; i < __IWM_DM_NR; i++)
 		add_dbg_module(iwm->dbg, iwm_debug_module[i].name,

@@ -509,39 +467,18 @@ int iwm_debugfs_init(struct iwm_priv *iwm)
 	iwm->dbg.txq_dentry = debugfs_create_file("queues", 0200,
 						  iwm->dbg.txdir, iwm,
 						  &iwm_debugfs_txq_fops);
-	result = PTR_ERR(iwm->dbg.txq_dentry);
-	if (IS_ERR(iwm->dbg.txq_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create tx queue: %d\n", result);
-		goto error;
-	}
 
 	iwm->dbg.tx_credit_dentry = debugfs_create_file("credits", 0200,
 						   iwm->dbg.txdir, iwm,
 						   &iwm_debugfs_tx_credit_fops);
-	result = PTR_ERR(iwm->dbg.tx_credit_dentry);
-	if (IS_ERR(iwm->dbg.tx_credit_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create tx credit: %d\n", result);
-		goto error;
-	}
 
 	iwm->dbg.rx_ticket_dentry = debugfs_create_file("tickets", 0200,
 						  iwm->dbg.rxdir, iwm,
 						  &iwm_debugfs_rx_ticket_fops);
-	result = PTR_ERR(iwm->dbg.rx_ticket_dentry);
-	if (IS_ERR(iwm->dbg.rx_ticket_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create rx ticket: %d\n", result);
-		goto error;
-	}
 
 	iwm->dbg.fw_err_dentry = debugfs_create_file("last_fw_err", 0200,
 						     iwm->dbg.dbgdir, iwm,
 						     &iwm_debugfs_fw_err_fops);
-	result = PTR_ERR(iwm->dbg.fw_err_dentry);
-	if (IS_ERR(iwm->dbg.fw_err_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create last FW err: %d\n", result);
-		goto error;
-	}
-
 
 	return 0;

Zhu Yi

2010-04-29 02:12 UTC #1 lore raw

Quoted 11 lines · Dan Carpenter

This is just a cleanup and doesn't change how the code works.
debugfs_create_dir() and debugfs_create_file() return an error pointer (-ENODEV) if CONFIG_DEBUG_FS is not enabled, otherwise if an error occurs they return NULL. This is how they are implemented and what it says in the DebugFS documentation. DebugFS can not be compiled as a module.
As a result, we only need to check for error pointers and particularly -ENODEV one time to know that DebugFS is enabled. This patch keeps the first check for error pointers and removes the rest.

Quoted 7 lines

The other reason for this patch, is that it silences some Smatch warnings. Smatch sees the condition "(result != -ENODEV)" and assumes that it's possible for "result" to equal -ENODEV. If it were possible it would lead to an error pointer dereference. But since it's not, we can just remove the check.
Signed-off-by Dan Carpenter <[email protected]>

Thanks. But looks like you missed the one in if_sdio_debugfs_init().

I don't think we even need to check -ENODEV ourselves because if DEBUG_FS is not compiled in, all the debugfs utility functions will become no-op.

Signed-off-by Dan Carpenter <[email protected]>

Signed-off-by Zhu Yi <[email protected]>

drivers/net/wireless/iwmc3200wifi/bus.h +1 -1

index 836663e..62edd58 100644
@@ -31,7 +31,7 @@ struct iwm_if_ops {
 	int (*disable)(struct iwm_priv *iwm);
 	int (*send_chunk)(struct iwm_priv *iwm, u8* buf, int count);
 
-	int (*debugfs_init)(struct iwm_priv *iwm, struct dentry *parent_dir);
+	void (*debugfs_init)(struct iwm_priv *iwm, struct dentry *parent_dir);
 	void (*debugfs_exit)(struct iwm_priv *iwm);
 
 	const char *umac_name;

drivers/net/wireless/iwmc3200wifi/debug.h +2 -2

index e35c9b6..f98bf12 100644
@@ -113,10 +113,10 @@ struct iwm_debugfs {
 };
 
 #ifdef CONFIG_IWM_DEBUG
-int iwm_debugfs_init(struct iwm_priv *iwm);
+void iwm_debugfs_init(struct iwm_priv *iwm);
 void iwm_debugfs_exit(struct iwm_priv *iwm);
 #else
-static inline int iwm_debugfs_init(struct iwm_priv *iwm)
+static inline void iwm_debugfs_init(struct iwm_priv *iwm)
 {
 	return 0;
 }

drivers/net/wireless/iwmc3200wifi/debugfs.c +10 ~~-100~~

153 lines not on this page · raw message

drivers/net/wireless/iwmc3200wifi/sdio.c +2 ~~-15~~

index 1eafd6d..1acea37 100644
@@ -366,21 +366,13 @@ static const struct file_operations iwm_debugfs_sdio_fops = {
 	.read =		iwm_debugfs_sdio_read,
 };
 
-static int if_sdio_debugfs_init(struct iwm_priv *iwm, struct dentry *parent_dir)
+static void if_sdio_debugfs_init(struct iwm_priv *iwm, struct dentry *parent_dir)
 {
-	int result;
 	struct iwm_sdio_priv *hw = iwm_to_if_sdio(iwm);
 
 	hw->cccr_dentry = debugfs_create_file("cccr", 0200,
 					      parent_dir, iwm,
 					      &iwm_debugfs_sdio_fops);
-	result = PTR_ERR(hw->cccr_dentry);
-	if (IS_ERR(hw->cccr_dentry) && (result != -ENODEV)) {
-		IWM_ERR(iwm, "Couldn't create CCCR entry: %d\n", result);
-		return result;
-	}
-
-	return 0;
 }
 
 static void if_sdio_debugfs_exit(struct iwm_priv *iwm)

@@ -440,11 +432,7 @@ static int iwm_sdio_probe(struct sdio_func *func,
 	hw = iwm_private(iwm);
 	hw->iwm = iwm;
 
-	ret = iwm_debugfs_init(iwm);
-	if (ret < 0) {
-		IWM_ERR(iwm, "Debugfs registration failed\n");
-		goto if_free;
-	}
+	iwm_debugfs_init(iwm);
 
 	sdio_set_drvdata(func, hw);

@@ -473,7 +461,6 @@ static int iwm_sdio_probe(struct sdio_func *func,
 	destroy_workqueue(hw->isr_wq);
  debugfs_exit:
 	iwm_debugfs_exit(iwm);
- if_free:
 	iwm_if_free(iwm);
 	return ret;
 }

Dan Carpenter

author Acked-by

2010-04-29 09:29 UTC #2 lore raw

Quoted 3 lines · Zhu Yi

Thanks. But looks like you missed the one in if_sdio_debugfs_init().

Yup.

Quoted 4 lines

I don't think we even need to check -ENODEV ourselves because if DEBUG_FS is not compiled in, all the debugfs utility functions will become no-op.

Good point.

Quoted 1 line

Signed-off-by Dan Carpenter <[email protected]>

Probably this could get changed to just "Reported-by" but I'm happy to take credit for other peoples work. :)

Quoted 2 lines

Signed-off-by Zhu Yi <[email protected]>

Acked-by Dan Carpenter <[email protected]>

regards,
dan carpenter