arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL()

arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL()

[Russell King - ARM Linux]

So, I eliminated all but a very few of these from arch/arm, and I notice
today that there's a new couple of instances introduced by:

commit 6770b211432564c562c856d612b43bbd42e4ab5e
Author: Ruslan Bilovol <ruslan.bilovol@ti.com>
Date:   Thu Feb 14 13:55:24 2013 +0200

    ARM: OMAP2+: Export SoC information to userspace

    In some situations it is useful for userspace to
    know some SoC-specific information. For example,
    this may be used for deciding what kernel module to
    use or how to better configure some settings etc.
    This patch exports OMAP SoC information to userspace
    using existing in Linux kernel SoC infrastructure.

    This information can be read under
    /sys/devices/socX directory

    Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
    [tony@atomide.com: updated for multiplatform changes]
    Signed-off-by: Tony Lindgren <tony@atomide.com>

+       soc_dev = soc_device_register(soc_dev_attr);
+       if (IS_ERR_OR_NULL(soc_dev)) {
+               kfree(soc_dev_attr);
+               return;
+       }
+
+       parent = soc_device_to_device(soc_dev);
+       if (!IS_ERR_OR_NULL(parent))
+               device_create_file(parent, &omap_soc_attr);

This is nonsense.  For the first, IS_ERR() is sufficient.  For the second,
tell me what error checking is required in the return value of this
function:

struct device *soc_device_to_device(struct soc_device *soc_dev)
{
        return &soc_dev->dev;
}

when you've already determined that the passed soc_dev is a valid pointer.
If you read the comments against the prototype:

/**
 * soc_device_to_device - helper function to fetch struct device
 * @soc: Previously registered SoC device container
 */
struct device *soc_device_to_device(struct soc_device *soc);

if "soc" is valid, it means the "previously registered SoC device container"
must have succeeded and that can only happen if the struct device has been
registered.  Ergo, there will always be a valid struct device pointer for
any registered SoC device container.  Therefore, if soc_device_register()
succeeds, then the return value from soc_device_to_device() will always be
valid and no error checking of it is required.

Simples.  The rule as ever applies here: get to know the APIs your using
and don't fumble around in the dark hoping that you'll get this stuff
right.

Re: arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL()

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [130509 02:14]:
> So, I eliminated all but a very few of these from arch/arm, and I notice
> today that there's a new couple of instances introduced by:

Sorry I should have noticed that fnord, I had it in my muttrc but had
a an unnecessary \ in the expression so it did not work.  

Here's a patch to fix the issue.

Hmm maybe we could redefine IS_ERR_OR_NULL as error in some ARM header
as long as drivers don't include it?

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Thu, 9 May 2013 08:27:25 -0700
Subject: [PATCH] ARM: OMAP2+: Remove bogus IS_ERR_OR_NULL checking from id.c

Commit 6770b211 (ARM: OMAP2+: Export SoC information to userspace)
had some broken return value handling as noted by Russell King:

+       soc_dev = soc_device_register(soc_dev_attr);
+       if (IS_ERR_OR_NULL(soc_dev)) {
+               kfree(soc_dev_attr);
+               return;
+       }
+
+       parent = soc_device_to_device(soc_dev);
+       if (!IS_ERR_OR_NULL(parent))
+               device_create_file(parent, &omap_soc_attr);

This is nonsense.  For the first, IS_ERR() is sufficient.  For the second,
tell me what error checking is required in the return value of this
function:

struct device *soc_device_to_device(struct soc_device *soc_dev)
{
        return &soc_dev->dev;
}

when you've already determined that the passed soc_dev is a valid pointer.
If you read the comments against the prototype:

/**
 * soc_device_to_device - helper function to fetch struct device
 * @soc: Previously registered SoC device container
 */
struct device *soc_device_to_device(struct soc_device *soc);

if "soc" is valid, it means the "previously registered SoC device container"
must have succeeded and that can only happen if the struct device has been
registered.  Ergo, there will always be a valid struct device pointer for
any registered SoC device container.  Therefore, if soc_device_register()
succeeds, then the return value from soc_device_to_device() will always be
valid and no error checking of it is required.

Simples.  The rule as ever applies here: get to know the APIs your using
and don't fumble around in the dark hoping that you'll get this stuff
right.

Fix it as noted by Russell.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 9bc5a18..1272c41 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -648,13 +648,12 @@ void __init omap_soc_device_init(void)
 	soc_dev_attr->revision = soc_rev;
 
 	soc_dev = soc_device_register(soc_dev_attr);
-	if (IS_ERR_OR_NULL(soc_dev)) {
+	if (IS_ERR(soc_dev)) {
 		kfree(soc_dev_attr);
 		return;
 	}
 
 	parent = soc_device_to_device(soc_dev);
-	if (!IS_ERR_OR_NULL(parent))
-		device_create_file(parent, &omap_soc_attr);
+	device_create_file(parent, &omap_soc_attr);
 }
 #endif /* CONFIG_SOC_BUS */

Re: arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL()

[Ruslan Bilovol]

Hi Russell,

On Thu, May 9, 2013 at 12:09 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> So, I eliminated all but a very few of these from arch/arm, and I notice
> today that there's a new couple of instances introduced by:
>
> commit 6770b211432564c562c856d612b43bbd42e4ab5e
> Author: Ruslan Bilovol <ruslan.bilovol@ti.com>
> Date:   Thu Feb 14 13:55:24 2013 +0200
>
>     ARM: OMAP2+: Export SoC information to userspace
>
>     In some situations it is useful for userspace to
>     know some SoC-specific information. For example,
>     this may be used for deciding what kernel module to
>     use or how to better configure some settings etc.
>     This patch exports OMAP SoC information to userspace
>     using existing in Linux kernel SoC infrastructure.
>
>     This information can be read under
>     /sys/devices/socX directory
>
>     Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
>     [tony@atomide.com: updated for multiplatform changes]
>     Signed-off-by: Tony Lindgren <tony@atomide.com>
>
> +       soc_dev = soc_device_register(soc_dev_attr);
> +       if (IS_ERR_OR_NULL(soc_dev)) {
> +               kfree(soc_dev_attr);
> +               return;
> +       }
> +
> +       parent = soc_device_to_device(soc_dev);
> +       if (!IS_ERR_OR_NULL(parent))
> +               device_create_file(parent, &omap_soc_attr);
>
> This is nonsense.  For the first, IS_ERR() is sufficient.  For the second,
> tell me what error checking is required in the return value of this
> function:
>
> struct device *soc_device_to_device(struct soc_device *soc_dev)
> {
>         return &soc_dev->dev;
> }
>
> when you've already determined that the passed soc_dev is a valid pointer.
> If you read the comments against the prototype:
>
> /**
>  * soc_device_to_device - helper function to fetch struct device
>  * @soc: Previously registered SoC device container
>  */
> struct device *soc_device_to_device(struct soc_device *soc);
>
> if "soc" is valid, it means the "previously registered SoC device container"
> must have succeeded and that can only happen if the struct device has been
> registered.  Ergo, there will always be a valid struct device pointer for
> any registered SoC device container.  Therefore, if soc_device_register()
> succeeds, then the return value from soc_device_to_device() will always be
> valid and no error checking of it is required.
>
> Simples.  The rule as ever applies here: get to know the APIs your using
> and don't fumble around in the dark hoping that you'll get this stuff
> right.

The interesting thing is that extra IS_ERR_OR_NULL checking was
introduced by author
of the SoC API in his implementation for the arm/mach-ux500 platform
and I used the same checking in my implementation when looked into his
implementation for reference.
Thanks for pointing on this, I will be more careful in the future.

Regards,
Ruslan

> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL()

[Russell King - ARM Linux]

On Fri, May 10, 2013 at 12:50:42PM +0300, Ruslan Bilovol wrote:
> Hi Russell,
> 
> On Thu, May 9, 2013 at 12:09 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > So, I eliminated all but a very few of these from arch/arm, and I notice
> > today that there's a new couple of instances introduced by:
> >
> > commit 6770b211432564c562c856d612b43bbd42e4ab5e
> > Author: Ruslan Bilovol <ruslan.bilovol@ti.com>
> > Date:   Thu Feb 14 13:55:24 2013 +0200
> >
> >     ARM: OMAP2+: Export SoC information to userspace
> >
> >     In some situations it is useful for userspace to
> >     know some SoC-specific information. For example,
> >     this may be used for deciding what kernel module to
> >     use or how to better configure some settings etc.
> >     This patch exports OMAP SoC information to userspace
> >     using existing in Linux kernel SoC infrastructure.
> >
> >     This information can be read under
> >     /sys/devices/socX directory
> >
> >     Signed-off-by: Ruslan Bilovol <ruslan.bilovol@ti.com>
> >     [tony@atomide.com: updated for multiplatform changes]
> >     Signed-off-by: Tony Lindgren <tony@atomide.com>
> >
> > +       soc_dev = soc_device_register(soc_dev_attr);
> > +       if (IS_ERR_OR_NULL(soc_dev)) {
> > +               kfree(soc_dev_attr);
> > +               return;
> > +       }
> > +
> > +       parent = soc_device_to_device(soc_dev);
> > +       if (!IS_ERR_OR_NULL(parent))
> > +               device_create_file(parent, &omap_soc_attr);
> >
> > This is nonsense.  For the first, IS_ERR() is sufficient.  For the second,
> > tell me what error checking is required in the return value of this
> > function:
> >
> > struct device *soc_device_to_device(struct soc_device *soc_dev)
> > {
> >         return &soc_dev->dev;
> > }
> >
> > when you've already determined that the passed soc_dev is a valid pointer.
> > If you read the comments against the prototype:
> >
> > /**
> >  * soc_device_to_device - helper function to fetch struct device
> >  * @soc: Previously registered SoC device container
> >  */
> > struct device *soc_device_to_device(struct soc_device *soc);
> >
> > if "soc" is valid, it means the "previously registered SoC device container"
> > must have succeeded and that can only happen if the struct device has been
> > registered.  Ergo, there will always be a valid struct device pointer for
> > any registered SoC device container.  Therefore, if soc_device_register()
> > succeeds, then the return value from soc_device_to_device() will always be
> > valid and no error checking of it is required.
> >
> > Simples.  The rule as ever applies here: get to know the APIs your using
> > and don't fumble around in the dark hoping that you'll get this stuff
> > right.
> 
> The interesting thing is that extra IS_ERR_OR_NULL checking was
> introduced by author
> of the SoC API in his implementation for the arm/mach-ux500 platform
> and I used the same checking in my implementation when looked into his
> implementation for reference.
> Thanks for pointing on this, I will be more careful in the future.

Well, the whole reasoning here is that IS_ERR_OR_NULL() is far too easy
to get wrong - there are too many of this kind of crap in the kernel:

	foo = some_func();
	if (IS_ERR_OR_NULL(foo))
		return PTR_ERR(foo);

which is wrong, because if foo _is_ NULL, the function doesn't return an
error, it returns success instead.  Of course, if some_func() never ever
returns NULL in the first place, that can't happen, but then the additional
test there for a NULL pointer is, to put it bluntly, total bollocks.

Instead, what it shows is that the author of the code hasn't been bothered
to really check what the return conditions of the API actually are - and
yes, this kind of crap really has happened.  Where APIs return NULL on
failure and the author has used code similar to the above...

So, the general rule is... if you see IS_ERR_OR_NULL(), it probably is a
bug just waiting to happen in some way.

IS_ERR_OR_NULL() is not a subsitute for putting thought in.
IS_ERR_OR_NULL() is a trap for the unwary kernel programmer.

And there's moves to eliminate it from the kernel - I have a patch prepared
which has a lot of support to mark IS_ERR_OR_NULL() deprecated... that can't
go in until we've eliminated most of the current (probably incorrect) uses.