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

From: Russell King - ARM Linux
Date: Wed Jan 09 2013 - 06:10:56 EST


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@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/