Re: [PATCHv2 08/29] x86/tdx: Handle in-kernel MMIO

[Josh Poimboeuf]

On Tue, Jan 25, 2022 at 01:08:21AM +0300, Kirill A. Shutemov wrote:
> > The return code conventions are still all mismatched and confusing:
> > 
> > - Most tdx_handle_*() handlers return bool (success == true)
> > 
> > - tdx_handle_mmio() returns int (success > 0)
> 
> Right, all tdx_handle_* are consistent: success > 0.

Non-zero success is not the same as above-zero success.  The behavior is
not interchangeable.

> > - tdx_mmio*() helpers return bool (success == false)
> 
> And what is wrong with that? Why do you mix functions that called in
> different contexts and expect them to have matching semantics?

Why would you expect the reader of the code to go investigate the weird
return semantics of every called function?

And "success == false" is just plain confusing, I haven't seen that one.

> > I still don't see any benefit in arbitrarily mixing three different
> > return conventions, none of which matches the typical kernel style for
> > returning errors, unless the goal is to confuse the reader and invite
> > bugs.
> 
> Okay, we have an disagreement here.
> 
> I picked a way to communicate function result as I see best fits the
> situation. It is a judgement call.
> 
> I will adjust code if maintainers see it differently from me. But until
> then I don't see anything wrong here.
> 
> > There is precedent in traps.c for some handle_*() functions to return
> > bool (success == true), so if the goal is to align with that
> > semi-convention, that's ok.  But at the very least, please do it
> > consistently:
> > 
> >   - change tdx_mmio*() to return true on success;
> > 
> >   - change tdx_handle_mmio() to return bool, with 'len' passed as an
> >     argument.
> 
> Hard no.
> 
> Returning a value via passed argument is the last resort for cases when
> more than one value has to be returned. In this case the function is
> perfectly capable to communicate result via single return value.
> 
> I don't see a reason to complicate the code to satisfy some "typical
> kernel style".

It's a convention for a reason.

> > Or, even better, just change them all to return 0 on success like 99+%
> > of error-returning kernel functions.
> 
> Citation needed. 99+% looks like an overstatement to me.

>From Documentation/process/coding-style.rst:

16) Function return values and names
------------------------------------

Functions can return values of many different kinds, and one of the
most common is a value indicating whether the function succeeded or
failed.  Such a value can be represented as an error-code integer
(-Exxx = failure, 0 = success) or a ``succeeded`` boolean (0 = failure,
non-zero = success).

Mixing up these two sorts of representations is a fertile source of
difficult-to-find bugs.  If the C language included a strong distinction
between integers and booleans then the compiler would find these mistakes
for us... but it doesn't.  To help prevent such bugs, always follow this
convention::

	If the name of a function is an action or an imperative command,
	the function should return an error-code integer.  If the name
	is a predicate, the function should return a "succeeded" boolean.

For example, ``add work`` is a command, and the add_work() function returns 0
for success or -EBUSY for failure.  In the same way, ``PCI device present`` is
a predicate, and the pci_dev_present() function returns 1 if it succeeds in
finding a matching device or 0 if it doesn't.

-- 
Josh