[PATCH v2 0/2] regulator: core: Fix error checking and messages

[PATCH v2 0/2] regulator: core: Fix error checking and messages

[Geert Uytterhoeven]

	Hi all,

This patch series corrects an error check, fixes error messages when
debugfs is not enabled, and improves debugfs error handling in the
regulator core.

Changes compared to v1:
  - Split in two patches,
  - Improve rationale.

Thanks for your comments!

Geert Uytterhoeven (2):
  regulator: core: Fix more error checking for debugfs_create_dir()
  regulator: core: Streamline debugfs operations

 drivers/regulator/core.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/2] regulator: core: Fix more error checking for debugfs_create_dir()

[Geert Uytterhoeven]

In case of failure, debugfs_create_dir() does not return NULL, but an
error pointer.  Most incorrect error checks were fixed, but the one in
create_regulator() was forgotten.

Fix the remaining error check.

Fixes: 2bf1c45be3b8f3a3 ("regulator: Fix error checking for debugfs_create_dir")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Split off from "regulator: core: Streamline debugfs operations".
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 698ab7f5004bf6b7..ad8baf65f63e369b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1911,7 +1911,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 
 	if (err != -EEXIST)
 		regulator->debugfs = debugfs_create_dir(supply_name, rdev->debugfs);
-	if (!regulator->debugfs) {
+	if (IS_ERR(regulator->debugfs))
 		rdev_dbg(rdev, "Failed to create debugfs directory\n");
 	} else {
 		debugfs_create_u32("uA_load", 0444, regulator->debugfs,
-- 
2.34.1

Re: [PATCH v2 1/2] regulator: core: Fix more error checking for debugfs_create_dir()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Wed, May 24, 2023 at 02:22:17PM +0200, Geert Uytterhoeven wrote:
> In case of failure, debugfs_create_dir() does not return NULL, but an
> error pointer.  Most incorrect error checks were fixed, but the one in
> create_regulator() was forgotten.

This breaks the build:

/build/stage/linux/drivers/regulator/core.c:1916:11: error: expected identifier 
or ‘(’ before ‘else’
 1916 |         } else {
      |           ^~~~
/build/stage/linux/drivers/regulator/core.c:1933:9: error: expected identifier o
r ‘(’ before ‘if’
 1933 |         if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) &&
      |         ^~
/build/stage/linux/drivers/regulator/core.c:1937:9: error: expected identifier o
r ‘(’ before ‘return’
 1937 |         return regulator;
      |         ^~~~~~
/build/stage/linux/drivers/regulator/core.c:1938:1: error: expected identifier or ‘(’ before ‘}’ token
 1938 | }
      | ^

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] regulator: core: Streamline debugfs operations

[Geert Uytterhoeven]

If CONFIG_DEBUG_FS is not set:

    regulator: Failed to create debugfs directory
    ...
    regulator-dummy: Failed to create debugfs directory

As per the comments for debugfs_create_dir(), errors returned by this
function should be expected, and ignored:

 * If debugfs is not enabled in the kernel, the value -%ENODEV will be
 * returned.
 *
 * NOTE: it's expected that most callers should _ignore_ the errors returned
 * by this function. Other debugfs functions handle the fact that the "dentry"
 * passed to them could be an error and they don't crash in that case.
 * Drivers should generally work fine even if debugfs fails to init anyway.

Adhere to the debugfs spirit, and streamline all operations by:
  1. Demoting the importance of the printed error messages to debug
     level, like is already done in create_regulator(),
  2. Further ignoring any returned errors, as by design, all debugfs
     functions are no-ops when passed an error pointer.

Fixes: 2bf1c45be3b8f3a3 ("regulator: Fix error checking for debugfs_create_dir")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Spin off error check fix into a separate patch,
  - Improve rationale.
---
 drivers/regulator/core.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ad8baf65f63e369b..d8e1caaf207e108f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1913,17 +1913,15 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 		regulator->debugfs = debugfs_create_dir(supply_name, rdev->debugfs);
 	if (IS_ERR(regulator->debugfs))
 		rdev_dbg(rdev, "Failed to create debugfs directory\n");
-	} else {
-		debugfs_create_u32("uA_load", 0444, regulator->debugfs,
-				   &regulator->uA_load);
-		debugfs_create_u32("min_uV", 0444, regulator->debugfs,
-				   &regulator->voltage[PM_SUSPEND_ON].min_uV);
-		debugfs_create_u32("max_uV", 0444, regulator->debugfs,
-				   &regulator->voltage[PM_SUSPEND_ON].max_uV);
-		debugfs_create_file("constraint_flags", 0444,
-				    regulator->debugfs, regulator,
-				    &constraint_flags_fops);
-	}
+
+	debugfs_create_u32("uA_load", 0444, regulator->debugfs,
+			   &regulator->uA_load);
+	debugfs_create_u32("min_uV", 0444, regulator->debugfs,
+			   &regulator->voltage[PM_SUSPEND_ON].min_uV);
+	debugfs_create_u32("max_uV", 0444, regulator->debugfs,
+			   &regulator->voltage[PM_SUSPEND_ON].max_uV);
+	debugfs_create_file("constraint_flags", 0444, regulator->debugfs,
+			    regulator, &constraint_flags_fops);
 
 	/*
 	 * Check now if the regulator is an always on regulator - if
@@ -5256,10 +5254,8 @@ static void rdev_init_debugfs(struct regulator_dev *rdev)
 	}
 
 	rdev->debugfs = debugfs_create_dir(rname, debugfs_root);
-	if (IS_ERR(rdev->debugfs)) {
-		rdev_warn(rdev, "Failed to create debugfs directory\n");
-		return;
-	}
+	if (IS_ERR(rdev->debugfs))
+		rdev_dbg(rdev, "Failed to create debugfs directory\n");
 
 	debugfs_create_u32("use_count", 0444, rdev->debugfs,
 			   &rdev->use_count);
@@ -6179,7 +6175,7 @@ static int __init regulator_init(void)
 
 	debugfs_root = debugfs_create_dir("regulator", NULL);
 	if (IS_ERR(debugfs_root))
-		pr_warn("regulator: Failed to create debugfs directory\n");
+		pr_debug("regulator: Failed to create debugfs directory\n");
 
 #ifdef CONFIG_DEBUG_FS
 	debugfs_create_file("supply_map", 0444, debugfs_root, NULL,
-- 
2.34.1

Re: [PATCH v2 0/2] regulator: core: Fix error checking and messages

[Mark Brown]

On Wed, 24 May 2023 14:22:16 +0200, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> This patch series corrects an error check, fixes error messages when
> debugfs is not enabled, and improves debugfs error handling in the
> regulator core.
> 
> Changes compared to v1:
>   - Split in two patches,
>   - Improve rationale.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] regulator: core: Fix more error checking for debugfs_create_dir()
      commit: 2715bb11cfff964aa33946847f9527cfbd4874f5
[2/2] regulator: core: Streamline debugfs operations
      commit: 08880713ceec023dd94d634f1e8902728c385939

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark