Re: [git patches] two warning fixes

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2007-07-20 at 20:34 +0200, Krzysztof Halasa wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > More people *should* generally ask themselves: "was the warning worth it?" 
> > and then, if the answer is "no", they shouldn't add code, they should 
> > remove the thing that causes the warning in the first place.
> 
> Sure. If a routine uses must_check yet its return value may be
> safely ignored then that must_check is simply misplaced and should
> be removed. It does not mean all must_checks are bad - each of them
> isn't bad unless one can demonstrate it is.
> 
> Back to sysfs_create_bin_file() - if one can demonstrate a caller
> can safely ignore the return value (which, it seems, is the
> case), then exactly this very must_check should be removed

Typically, the EDID creation in radeonfb :-)

In fact, I'm not even sure there's -any- user of those sysfs files. I
added them back then to allow distros to extract the EDID infos that
were probed by radeonfb to properly configure the X server (because on
some machines, the EDID is coming from the firmware/BIOS, not from DDC,
and X can't get at it). I don't know if they ever used them.

In any case, it doesn't make sense to abort initialization of the driver
if for some reasons those files can't be created (for example, the core
fbdev starts exposing EDID files, radeonfb isn't properly updated, name
clash, error). Aborting the initialization will make sure that on some
machines such as powermacs with radeon, whatever error is displayed will
never be seen by the user.

That's a typical, but I have plenty more.

For example, the powermac thermal control drivers. They work pretty well
by themselves. They also expose via sysfs all the current values, fan
speeds, temps ,etc... for the sake of whoever wants to do a GUI or
"monitor" what's going on, but that is not critical to the operation of
the driver. Thus, failure to create those files is not critical.

I have plenty other examples.

Thus, we have two choices here:

 - The simple one: sysfs_create_blah() displays a warning when it fails
and has no must_check

 - The one that adds code everywhere (the current one):
sysfs_create_blah() returns an error, has much_check, and thus all
callers like I described abvoe need to add code to test it and print a
warning. Lots of added .text and .data for little benefit.

Cheers,
Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/