[PATCH] regulator: Fix and simplify debugfs support

[PATCH] regulator: Fix and simplify debugfs support

[Stephen Boyd]

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.

Instead of checking for an ERR_PTR check for NULL. This simplifies
the code and also fixes an error check that would never have
worked otherwise. While we're here modernize the code to use
S_IRUGO instead of 0444.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

If we're willing to sacrifice a pointer per rdev we can remove the
ifdefs and the compiler should be able to optimize away the dead
code.

 drivers/regulator/core.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e9a83f8..f20696d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1145,9 +1145,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 #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);
@@ -2712,9 +2711,8 @@ 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 (!rdev->debugfs) {
 		rdev_warn(rdev, "Failed to create debugfs directory\n");
-		rdev->debugfs = NULL;
 		return;
 	}
 
@@ -3129,13 +3127,10 @@ 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 (!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)))
+	else if (!debugfs_create_file("supply_map", S_IRUGO, debugfs_root,
+				       NULL, &supply_map_fops))
 		pr_warn("regulator: Failed to create supplies debugfs\n");
 #endif
 
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] regulator: Fix and simplify debugfs support

[Hillf Danton]

On Sun, Feb 19, 2012 at 3:28 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 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.
>

Unfortunately there is no decrement in #ifdefs observed:(

> Instead of checking for an ERR_PTR check for NULL. This simplifies
> the code and also fixes an error check that would never have
> worked otherwise. While we're here modernize the code to use
> S_IRUGO instead of 0444.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>
> If we're willing to sacrifice a pointer per rdev we can remove the
> ifdefs and the compiler should be able to optimize away the dead
> code.
>
>  drivers/regulator/core.c |   15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e9a83f8..f20696d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1145,9 +1145,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
>  #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);
> @@ -2712,9 +2711,8 @@ 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 (!rdev->debugfs) {
>                rdev_warn(rdev, "Failed to create debugfs directory\n");
> -               rdev->debugfs = NULL;
>                return;
>        }
>
> @@ -3129,13 +3127,10 @@ 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 (!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)))
> +       else if (!debugfs_create_file("supply_map", S_IRUGO, debugfs_root,
> +                                      NULL, &supply_map_fops))
>                pr_warn("regulator: Failed to create supplies debugfs\n");
>  #endif
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH] regulator: Fix and simplify debugfs support

[Mark Brown]

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

On Sat, Feb 18, 2012 at 11:28:25PM -0800, Stephen Boyd wrote:
> 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.

> Instead of checking for an ERR_PTR check for NULL. This simplifies
> the code and also fixes an error check that would never have
> worked otherwise. While we're here modernize the code to use
> S_IRUGO instead of 0444.

This was actually a deliberate decision to make the code more robust
against change - the IS_ERR_OR_NULL doesn't make the code any bigger but
it means it's less likely to break in the face of changes.

> If we're willing to sacrifice a pointer per rdev we can remove the
> ifdefs and the compiler should be able to optimize away the dead
> code.

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...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Fix and simplify debugfs support

[Stephen Boyd]

On 2/19/2012 6:12 PM, Mark Brown wrote:
> On Sat, Feb 18, 2012 at 11:28:25PM -0800, Stephen Boyd wrote:
>> 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.
>> Instead of checking for an ERR_PTR check for NULL. This simplifies
>> the code and also fixes an error check that would never have
>> worked otherwise. While we're here modernize the code to use
>> S_IRUGO instead of 0444.
> This was actually a deliberate decision to make the code more robust
> against change - the IS_ERR_OR_NULL doesn't make the code any bigger but
> it means it's less likely to break in the face of changes.

More robust? The debugfs code in the regulator core looks confused on 
what the return value is. Sometimes it's IS_ERR, sometimes it's NULL, 
sometimes it's both. Might as well clean it up to be consistent and proper.

>> If we're willing to sacrifice a pointer per rdev we can remove the
>> ifdefs and the compiler should be able to optimize away the dead
>> code.
> 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...

Ok. I'll send a patch to do that too.

How about a two part series with the erroneous error check fix in one 
and then the other stuff that isn't critical in another?

Re: [PATCH] regulator: Fix and simplify debugfs support

[Mark Brown]

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

On Sun, Feb 19, 2012 at 11:46:40PM -0800, Stephen Boyd wrote:
> On 2/19/2012 6:12 PM, Mark Brown wrote:

> >This was actually a deliberate decision to make the code more robust
> >against change - the IS_ERR_OR_NULL doesn't make the code any bigger but
> >it means it's less likely to break in the face of changes.

> More robust? The debugfs code in the regulator core looks confused
> on what the return value is. Sometimes it's IS_ERR, sometimes it's
> NULL, sometimes it's both. Might as well clean it up to be
> consistent and proper.

Well, the intention when I originally wrote it was to always check for
all possible error codes.  Looking at the code in mainline it's pretty
consistent, the class wide check and the per rdev checks are both open
coded IS_ERR_OR_NULL() and the per supply stuff uses the helper.  The
only case where we do a plain IS_ERR() is when we warn about not being
able to create supply_map and the consistent thing there would just be
to ignore the error since it's what we do for all the other individual
files.

This thing with returning NULL instead of an actual ERR_PTR() is pretty
unhelpful really, especially for something like debugfs where the caller
shouldn't care much if it works or not.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]