Skip to contentSkip to conversation

regulator: Clean up debugfs error handling a bit

PATCH In review Mark Brown 2 participants 4 messages Feb 20, 2012 – Feb 21, 2012 1 message not in the archive Voltage and current regulator framework

lore
Share & tools
lore

Mark Brown

author
#0 lore raw

PATCH 1/1 regulator: Clean up debugfs error handling a bit

Use IS_ERR_OR_NULL() rather than open coding it and ignore errors from failure to create the supply map.

Signed-off-by Mark Brown <[email protected]>
---
 drivers/regulator/core.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/regulator/core.c +4 -5
index fecda41..d14e433 100644
@@ -2815,7 +2815,7 @@ static void rdev_init_debugfs(struct regulator_dev *rdev)
 {
 #ifdef CONFIG_DEBUG_FS
 	rdev->debugfs = debugfs_create_dir(rdev_get_name(rdev), debugfs_root);
-	if (IS_ERR(rdev->debugfs) || !rdev->debugfs) {
+	if (IS_ERR_OR_NULL(rdev->debugfs)) {
 		rdev_warn(rdev, "Failed to create debugfs directory\n");
 		rdev->debugfs = NULL;
 		return;

@@ -3231,14 +3231,13 @@ static int __init regulator_init(void)
 
 #ifdef CONFIG_DEBUG_FS
 	debugfs_root = debugfs_create_dir("regulator", NULL);
-	if (IS_ERR(debugfs_root) || !debugfs_root) {
+	if (IS_ERR_OR_NULL(debugfs_root)) {
 		pr_warn("regulator: Failed to create debugfs directory\n");
 		debugfs_root = NULL;
 	}
 
-	if (IS_ERR(debugfs_create_file("supply_map", 0444, debugfs_root,
-				       NULL, &supply_map_fops)))
-		pr_warn("regulator: Failed to create supplies debugfs\n");
+	debugfs_create_file("supply_map", 0444, debugfs_root, NULL,
+			    &supply_map_fops);
 #endif
 
 	regulator_dummy_init();
Signature · 1 line
1.7.9
#1 lore raw
Quoted 2 lines · Mark Brown

Use IS_ERR_OR_NULL() rather than open coding it and ignore errors from failure to create the supply map.

Reported-by Stephen Boyd <[email protected]>
Quoted 9 lines
Signed-off-by Mark Brown<[email protected]>
drivers/regulator/core.c +0 -1
@@ -3231,14 +3231,13 @@ static int __init regulator_init(void)
 
  #ifdef CONFIG_DEBUG_FS
  	debugfs_root = debugfs_create_dir("regulator", NULL);
-	if (IS_ERR(debugfs_root) || !debugfs_root) {

drivers/regulator/core.c static int __init regulator_init(void) · line 3240

Quoted 10 lines
drivers/regulator/core.c +3 -3
+	if (IS_ERR_OR_NULL(debugfs_root)) {
  		pr_warn("regulator: Failed to create debugfs directory\n");
  		debugfs_root = NULL;
  	}
 
-	if (IS_ERR(debugfs_create_file("supply_map", 0444, debugfs_root,
-				       NULL,&supply_map_fops)))
-		pr_warn("regulator: Failed to create supplies debugfs\n");
+	debugfs_create_file("supply_map", 0444, debugfs_root, NULL,
+			&supply_map_fops);

Do you want an else here so that if the regulator directory fails we don't accidentally create a supply_map file in the root of debugfs? How ever unlikely that sounds.

A message that is not in the archive (sent off-list or not yet received): Message-Id: <1329772500-20577-1-git-send-email-broonie@opensource.wolfsonmicro.com

#3 lore raw

PATCH 1/1 regulator: Simplify debugfs code

If CONFIG_DEBUG_FS=y debugfs functions will never return an ERR_PTR. Instead they'll return NULL. The intent is to remove ifdefs in calling code.

Update the code to reflect this. We gain an extra dentry pointer per struct regulator and struct regulator_dev but that should be ok because most distros have debugfs compiled in anyway.

Signed-off-by Stephen Boyd <[email protected]>

---

Quoted 4 lines

Personally I'd be happy to do that, the only reason I put the ifdefs in there was that it appears to be idiomatic to do so but I'm not really a big fan of it. Then again I never build kernls without debugfs support in them myself...

Here's the patch on top of your patch in case you want it. My compiler seems to be clever enough to remove the supply_map_fops when debugfs isn't enabled.

 drivers/regulator/core.c         |   28 ++++++----------------------
 include/linux/regulator/driver.h |    2 --
 2 files changed, 6 insertions(+), 24 deletions(-)
drivers/regulator/core.c +6 -22
index 6b63159..32ba89b 100644
@@ -54,9 +54,7 @@ static LIST_HEAD(regulator_map_list);
 static bool has_full_constraints;
 static bool board_wants_dummy_regulator;
 
-#ifdef CONFIG_DEBUG_FS
 static struct dentry *debugfs_root;
-#endif
 
 /*
  * struct regulator_map

@@ -84,9 +82,7 @@ struct regulator {
 	char *supply_name;
 	struct device_attribute dev_attr;
 	struct regulator_dev *rdev;
-#ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
-#endif
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);

@@ -1142,12 +1138,10 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 			goto attr_err;
 	}
 
-#ifdef CONFIG_DEBUG_FS
 	regulator->debugfs = debugfs_create_dir(regulator->supply_name,
 						rdev->debugfs);
-	if (IS_ERR_OR_NULL(regulator->debugfs)) {
+	if (!regulator->debugfs) {
 		rdev_warn(rdev, "Failed to create debugfs directory\n");
-		regulator->debugfs = NULL;
 	} else {
 		debugfs_create_u32("uA_load", 0444, regulator->debugfs,
 				   &regulator->uA_load);

@@ -1156,7 +1150,6 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 		debugfs_create_u32("max_uV", 0444, regulator->debugfs,
 				   &regulator->max_uV);
 	}
-#endif
 
 	mutex_unlock(&rdev->mutex);
 	return regulator;

@@ -1365,9 +1358,7 @@ void regulator_put(struct regulator *regulator)
 	mutex_lock(&regulator_list_mutex);
 	rdev = regulator->rdev;
 
-#ifdef CONFIG_DEBUG_FS
 	debugfs_remove_recursive(regulator->debugfs);
-#endif
 
 	/* remove any sysfs entries */
 	if (regulator->dev) {

@@ -2710,11 +2701,9 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
 
 static void rdev_init_debugfs(struct regulator_dev *rdev)
 {
-#ifdef CONFIG_DEBUG_FS
 	rdev->debugfs = debugfs_create_dir(rdev_get_name(rdev), debugfs_root);
-	if (IS_ERR_OR_NULL(rdev->debugfs)) {
+	if (!rdev->debugfs) {
 		rdev_warn(rdev, "Failed to create debugfs directory\n");
-		rdev->debugfs = NULL;
 		return;
 	}
 

@@ -2722,7 +2711,6 @@ static void rdev_init_debugfs(struct regulator_dev *rdev)
 			   &rdev->use_count);
 	debugfs_create_u32("open_count", 0444, rdev->debugfs,
 			   &rdev->open_count);
-#endif
 }
 
 /**

@@ -2902,9 +2890,7 @@ void regulator_unregister(struct regulator_dev *rdev)
 		return;
 
 	mutex_lock(&regulator_list_mutex);
-#ifdef CONFIG_DEBUG_FS
 	debugfs_remove_recursive(rdev->debugfs);
-#endif
 	flush_work_sync(&rdev->disable_work.work);
 	WARN_ON(rdev->open_count);
 	unset_regulator_supplies(rdev);

@@ -3114,12 +3100,14 @@ static ssize_t supply_map_read_file(struct file *file, char __user *user_buf,
 
 	return ret;
 }
+#endif
 
 static const struct file_operations supply_map_fops = {
+#ifdef CONFIG_DEBUG_FS
 	.read = supply_map_read_file,
 	.llseek = default_llseek,
-};
 #endif
+};
 
 static int __init regulator_init(void)
 {

@@ -3127,16 +3115,12 @@ static int __init regulator_init(void)
 
 	ret = class_register(&regulator_class);
 
-#ifdef CONFIG_DEBUG_FS
 	debugfs_root = debugfs_create_dir("regulator", NULL);
-	if (IS_ERR_OR_NULL(debugfs_root)) {
+	if (!debugfs_root)
 		pr_warn("regulator: Failed to create debugfs directory\n");
-		debugfs_root = NULL;
-	}
 
 	debugfs_create_file("supply_map", 0444, debugfs_root, NULL,
 			    &supply_map_fops);
-#endif
 
 	regulator_dummy_init();
include/linux/regulator/driver.h +0 -2
index 4214b9a..c5dcada 100644
@@ -207,9 +207,7 @@ struct regulator_dev {
 
 	void *reg_data;		/* regulator_dev data */
 
-#ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
-#endif
 };
 
 struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#4 lore raw

Re: [PATCH] regulator: Simplify debugfs code

Quoted 3 lines · Stephen Boyd

Update the code to reflect this. We gain an extra dentry pointer per struct regulator and struct regulator_dev but that should be ok because most distros have debugfs compiled in anyway.

It'd be helpful if the changelog were a little less oblique about what the resulting changes were... either mention what the changes are or mention what the current state of the code is. Makes it much easier to work out if the patch is doing what it's supposed to. This was also an issue with the original patch, it wasn't at all clear what the actual intention was.

Anyway, I'll apply.