Re: [PATCH v11 3/4] add FPGA manager core

From: Dan Carpenter
Date: Fri Sep 25 2015 - 06:02:20 EST

On Thu, Sep 24, 2015 at 03:47:26PM -0500, atull wrote:
> Interesting. The amount of code bloat here compiles down to about two
> machine instructions (in two places). Actually a little more since I should
> be using IS_ERR_OR_NULL. But the main question is whether I should do
> it at all.

They kernel already has too many bogus checks for IS_ERR(). It's a very
common bug to check for IS_ERR() when you should be checking for NULL.

- foo = some_allocator();
+ foo = kmalloc();
if (IS_ERR(foo))
goto fail;

I have a static checker for "warn: 'foo' isn't an ERR_PTR" but I haven't
published it because too much code has impossible checks.

> The behaviour I should drive here is that the user will do their own error
> checking. After they get a pointer to a FPGA manager using
> of_fpga_mgr_get(), they should check it and not assume that
> fpga_mgr_firmware_load() will do it for them, i.e.
> mgr = of_fpga_mgr_get(mgr_node);
> if (IS_ERR(mgr))
> return PTR_ERR(mgr);
> fpga_mgr_firmware_load(mgr, flags, path);

I don't understand completely how of_fpga_mgr_get() ever returns NULL.
A lot of the of_ functions return ERR_PTRs if OF_ is compiled in but
they return NULL if it's not. I think this is so people can build with
COMPILE_TEST so we get more coverage with static analysis?

> I could take out these NULL pointer checks and it won't hurt anything unless
> someone is just using the functions badly, in which case: kablooey.

Linux devs are very good about doing error checking. An early kablooey
is what we want for people who don't.

Also if you provide a sanity check then Markus Elfring will remove all
the error checking in the callers. Don't do it.

dan carpenter

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at