Linus Torvalds [off-list ref] 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.
--
Krzysztof Halasa
On Fri, 2007-07-20 at 20:34 +0200, Krzysztof Halasa wrote:
Linus Torvalds [off-list ref] writes:
quoted
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/
Benjamin Herrenschmidt wrote:
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.
Not necessarily as simple as that -- you need to make sure you don't
pass something bogus to a sysfs_remove_blah() function at
unregister/unload time, if sysfs_create_blah() failed.
Certainly sysfs_foo() failure is often ignorable in the sense that you
want the driver to keep loading... but that does not imply that it is
strictly ignorable, if you also consider the associated cleanup code.
Jeff
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
Not necessarily as simple as that -- you need to make sure you don't
pass something bogus to a sysfs_remove_blah() function at
unregister/unload time, if sysfs_create_blah() failed.
Certainly sysfs_foo() failure is often ignorable in the sense that you
want the driver to keep loading... but that does not imply that it is
strictly ignorable, if you also consider the associated cleanup code.
It should be trivial enough to have sysfs_create_blah() do enough
initializations before it can fail so that sysfs_remove_blah() do the
right thing regardless.
It's actually a major PITA for a driver that creates a whole bunch of
sysfs files to have to track precisely which ones were created
successfully for the error path. If it's a single function, goto does
the trick but if for some reason it's not, it's really annoying.
Ben.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/