Re: [PATCH 1/4] gpiolib: introduce descriptor-based GPIO interface

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Jan 09, 2013 at 10:44:14AM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 09, 2013 at 10:35:22AM +0000, Arnd Bergmann wrote:
> > On Wednesday 09 January 2013, Alexandre Courbot wrote:
> > > On Tue, Jan 8, 2013 at 9:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > > Please avoid the use of IS_ERR_OR_NULL(), especially on interfaces you
> > > > introduce yourself. AFAICT, gpiod_get cannot return NULL, so you
> > > > should not check for that.
> > > 
> > > Sure - you sound like IS_ERR_OR_NULL() is generally considered evil,
> > 
> > Correct.
> > 
> > > may I ask why this is the case?
> > 
> > It's very hard to get right: either you are interested in the error code,
> > and then you don't have one in some cases, or you don't care but have
> > to check for it anyway. When you define a function, just make it clear
> > what the expected return values are, either NULL for error or a negative
> > ERR_PTR value, but not both.
> 
> Indeed, and any code which does this:
> 
> 	if (IS_ERR_OR_NULL(ptr))
> 		return PTR_ERR(ptr);
> 
> is buggy because on NULL it returns 0, which is generally accepted as being
> "success".

                oh = omap_hwmod_lookup(oh_name);
                if (!oh) {

        oh = omap_hwmod_lookup(oh_name);
        if (IS_ERR_OR_NULL(oh)) {

Does this function return NULL on errors or an errno-encoded-pointer on
errors?

        d = debugfs_create_dir("pm_debug", NULL);
        if (IS_ERR_OR_NULL(d))
                return PTR_ERR(d);

Well, covered above.  NULL is success here.

        err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
        if (err) {
                pr_err("%s: gpio_request failed: %d\n", __func__, err);
                return err;
        }

        gpio_direction_output(en_vdd_1v05, 1);

        regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
        if (IS_ERR_OR_NULL(regulator)) {
                pr_err("%s: regulator_get failed: %d\n", __func__,
                       (int)PTR_ERR(regulator));
                goto err_reg;
        }
...
err_reg:
        gpio_free(en_vdd_1v05);

        return err;
}

So 'err' here is never set.  when IS_ERR_OR_NULL evaluates true.
Setting 'err' to PTR_ERR(regulator) is not correct because a NULL return
sets 'err' to zero.  Same here:

        /* create property id */
        ret = ipp_create_id(&ctx->prop_idr, &ctx->prop_lock, c_node,
                &property->prop_id);
        if (ret) {
                DRM_ERROR("failed to create id.\n");
                goto err_clear;
        }
...
        c_node->start_work = ipp_create_cmd_work();
        if (IS_ERR_OR_NULL(c_node->start_work)) {
                DRM_ERROR("failed to create start work.\n");
                goto err_clear;
        }
        c_node->stop_work = ipp_create_cmd_work();
        if (IS_ERR_OR_NULL(c_node->stop_work)) {
                DRM_ERROR("failed to create stop work.\n");
                goto err_free_start;
        }
        c_node->event_work = ipp_create_event_work();
        if (IS_ERR_OR_NULL(c_node->event_work)) {
                DRM_ERROR("failed to create event work.\n");
                goto err_free_stop;
        }

We also have it cropping up as a way to 'verify' function arguments
are correct:

int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
                          struct gpd_timing_data *td)
{
        if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
                return -EINVAL;

And then we have this beauty in USB code:

        if (!IS_ERR_OR_NULL(udc->transceiver))
                (void) otg_set_peripheral(udc->transceiver->otg, NULL);
        else
                pullup_disable(udc);
...
        seq_printf(s,
                "UDC rev %d.%d, fifo mode %d, gadget %s\n"
                "hmc %d, transceiver %s\n",
                tmp >> 4, tmp & 0xf,
                fifo_mode,
                udc->driver ? udc->driver->driver.name : "(none)",
                HMC,
                udc->transceiver
                        ? udc->transceiver->label
                        : (cpu_is_omap1710()
                                ? "external" : "(none)"));

If udc->transceiver were to be an error code, the above will segfault or
access memory at the top of memory space.

These are just a few of the issues I've picked out at random from grepping
the kernel source for IS_ERR_OR_NULL().  Yes, there's some valid use cases
but the above are all horrid, buggy or down right wrong, and I wouldn't be
at all surprised if that was all too common.