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

From: atull
Date: Thu Sep 24 2015 - 16:53:44 EST

On Thu, 24 Sep 2015, Dan Carpenter wrote:

> Of course, the maintainer gets the last word regardless of what anyone
> else thinks.
> Generally, minimal code is better. Trying to future proof code is a
> waste of time because you can't predict what will happen in the future.
> It's way more likely that some pointer you never expected to be NULL
> will be NULL instead of the few checked at the beginning of a function.
> Adding useless code uses RAM and makes the function slower. It's a bit
> confusing for users as well because they will wonder when the NULL check
> is used. A lot of times this sort of error handling is a bit fake and
> what I mean is that it looks correct but the system will just crash in a
> later function.
> Also especially with a simple NULL dereferences like this theoretical
> one, it's better to just get the oops. It kills the module but you get
> a good message in the log and it's normally straight forward to debug.
> We spent a surprising amount of time discussing useless code. I made
> someone redo a patch yesterday because they had incomplete error
> handling for a situation which could never happen.
> regards,
> dan carpenter

Thanks for the discussion.

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.

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

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