Message-ID: <20150924211332.GA29890@amd>
Date:	Thu, 24 Sep 2015 23:13:32 +0200
From:	Pavel Machek <pavel@...x.de>
To:	atull <atull@...nsource.altera.com>
Cc:	Dan Carpenter <dan.carpenter@...cle.com>, mark.rutland@....com,
	linux-doc@...r.kernel.org, rubini@...dd.com,
	pantelis.antoniou@...sulko.com, linux-kernel@...r.kernel.org,
	hpa@...or.com, s.trumtrar@...gutronix.de,
	devel@...verdev.osuosl.org, sameo@...ux.intel.com, nico@...aro.org,
	iws@...o.caltech.edu, michal.simek@...inx.com, kyle.teske@...com,
	jgunthorpe@...idianresearch.com, grant.likely@...aro.org,
	davidb@...eaurora.org, linus.walleij@...aro.org, cesarb@...arb.net,
	devicetree@...r.kernel.org, jason@...edaemon.net,
	pawel.moll@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, broonie@...nel.org, philip@...ister.org,
	Petr Cvek <petr.cvek@....cz>, akpm@...ux-foundation.org,
	monstr@...str.eu, gregkh@...uxfoundation.org, balbi@...com,
	davem@...emloft.net, robh+dt@...nel.org, rob@...dley.net,
	Josh Cartwright <joshc@...com>, dinguyen@...nsource.altera.com,
	delicious.quinoa@...il.com, m.chehab@...sung.com
Subject: Re: [PATCH v11 3/4] add FPGA manager core

Hi!

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

2 instructions is not that bad, do whatever is easier for you. These
patches received enough bikeshed painting.

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists