Dear Larry,
Larry Finger wrote:
On 05/13/2015 05:08 PM, Luca Ceresoli wrote:
Several functions in this driver return a boolean: _SUCCESS = 1 on
success,
_FAIL = 0 on error, defined in
drivers/staging/rtl8188eu/include/osdep_service.h.
The common practice in the Linux kernel is to return 0 on success, a
negative
error code otherwise. This has the advantage that the return value also
describes the kind of error that happened, while a boolean squashes
all errors
down a unique value.
Change rtw_start_drv_threads() to return a proper 0-or-error value.
Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxx>
Cc: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
---
I think _SUCCESS and _FAIL should be totally eradicated and replaced with
proper error codes and my intent is to do it all over the codebase.
However, since that would be a massive change, I'm sending this small
patch to get some feedback. Should the idea be accepted, I'd go on.
I have no objections to the major change from those macros to simple 1
and 0.
This is not the change I propose. Let me try to explain better my idea.
Documentation/CodingStyle requires that if the name of a function is an
action it should return 0 for success and a negative error code in case
of failure (-EBUSY, -ENOMEM, -EPIPE etc). Functions returning a pointer
convey the same information via the ERR_PTR/PTR_ERR macros.
Predicate-like functions should return a "C-style boolean": 0 for false
and 1 for true.
I totally agree with this coding style rule. It's more expressive.
Currently a lot of functions in the rtl8188eu driver return _SUCCESS
(=1) or _FAIL (=0), but that happens also for action-like functions.
For example, in this patch I change the return value of
rtw_start_drv_threads(), which is an action.
I would like to change all action-like functions to comply with the
coding rules. My patch makes rtw_start_drv_threads() return 0 on
success or propagate the return value of kthread_run() on error, which
can be either -ENOMEM or -EINTR in the current kernel code.
Extended to the whole codebase, ultimately this means that when a
request fails, it may present a more informative error message.
BTW, in my hunt for _SUCCESS and _FAIL occurrences I would also change
the return value of predicate-like functions to return plain 1 and 0.
It's another step towards a full coding style compliance, but this one
does not change the actual behaviour. It would be simply a matter of
replacing _SUCCESS with 1 and _FAIL with 0, which is what the
preprocessor currently does.
I hope it's clearer now.
> Such changes would be more acceptable if you have the hardware
> and can test as well to eliminate any subtle side effects..
I do have an RTL8188EU USB WiFi adapter, so I can test it.
However I'm no WiFi guru, so I can use the adapter and see if it works.
I'm open to suggestions about more detailed tests I can do.
Also, there are other drivers in staging that look somewhat similar:
rtl8712 and rtl8723au. I don't have any of them (but I could buy one per
chip).
--
Luca
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
