linux-kernel - Re: checkpatch.pl warning for "return" with value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8859753b-c7c9-78ec-20cc-ce424b436b13@amd.com>
Date:   Fri, 17 Apr 2020 14:52:17 -0400
From:   Luben Tuikov <luben.tuikov@....com>
To:     Joe Perches <joe@...ches.com>, Andy Whitcroft <apw@...onical.com>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: checkpatch.pl warning for "return" with value

On 2020-04-17 1:32 p.m., Joe Perches wrote:
> On Fri, 2020-04-17 at 13:20 -0400, Luben Tuikov wrote:
>> Hi guys,
>>
>> I get this warning:
>>
>> :32: WARNING: else is not generally useful after a break or return
>> #32: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_job.c:55:
>> +		return 0;
>> +	} else {
>>
>> for the following code, at the bottom of a function:
>>
>> 	if (amdgpu_device_should_recover_gpu(ring->adev)) {
>> 		amdgpu_device_gpu_recover(ring->adev, job);
>> 		return 0;
>> 	} else {
>> 		drm_sched_suspend_timeout(&ring->sched);
>> 		return 1;
>> 	}
>> }
>>
>> It seems like a false positive--I mean, if the else branch was
>> taken, we'd return a different result.
> 
> There is an existing checkpatch exception for single line
> if/else returns
> like:
> 
> 	if (foo)
> 		return bar;
> 	else
> 		return baz;
> 
> because that's a pretty common code style.

Yes, that's true--and that's what I have above, except
the braces.

> 
> But I personally don't think that your example fits the
> same style.

Why? Linguistically, it doesn't matter if a compound
statement is used or a single one. (I mean, one could
use a comma "," too... :-) )

> 
> I think when unexpected condition should be separated from
> the expected condition which should typically be the last
> block of a function like:

I couldn't understand this sentence because of the "when"
and "which".

> 
> 
> 	if (<atypical_condition>) {
> 		...;
> 		return <atypical_result>;
> 	}
> 
> 	...;
> 	return <typical_result>;
> }
> 
> If you want to code it, and it works, go ahead, but I
> won't attempt it because I think it's not appropriate.

For error checking, when the 2nd ellipsis is
a long, long body of the function, so that the error
checking is done at the top, then long body,
then at the bottom we return some computed value.
But in the case I have above, it's a compact if-else
at the bottom of the function.

In the example I gave above, there is no "typical" or
"atypical" condition--it's just checking a condition
and deciding what to do, all at the bottom of
a function. (And that condition, samples
an external stimuli, which cannot be predicted.)

Also checking and returning from a function, doesn't
always have to be binary. It could be,

	if (A) {
		...;
		return X;
	} else if (B) {
		...;
		return Y;
	} else {
		...;
		return Z;
	}
}

And interestingly, checkpatch.pl doesn't complain for
the triplet above. But if I remove condition B, above,
it does complain.

Since we're returning a different result and since
it works fine with a triplet, could you fix the binary
case above to not complain?

You already seem to have an exception for it, as you stated
above.

Regards,
Luben