[PATCH 10/10] staging: rtl8188eu: return an error code, not a boolean

[Luca Ceresoli]

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 at lucaceresoli.net>
>> Cc: Greg Kroah-Hartman <gregkh at linux.com>
>> Cc: Larry Finger <Larry.Finger at lwfinger.net>
>>
>> ---
>>
>> 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