[PATCH v2 1/9] dm: core: Document the common error codes

[Sean Anderson <seanga2@gmail.com>]

On 3/23/21 1:40 AM, Simon Glass wrote:
> HI Sean,
> 
> On Tue, 23 Mar 2021 at 17:45, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 3/23/21 12:14 AM, Simon Glass wrote:
>>> Driver model uses quite strong conventions on error codes, but these are
>>> currently not clearly documented. Add a description of the commonly used
>>> errors.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Add a patch to document the common error codes
>>>
>>>    doc/driver-model/design.rst | 111 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 111 insertions(+)
>>>
>>> diff --git a/doc/driver-model/design.rst b/doc/driver-model/design.rst
>>> index 4e5cecbab6a..30a07bdf768 100644
>>> --- a/doc/driver-model/design.rst
>>> +++ b/doc/driver-model/design.rst
>>> @@ -900,6 +900,117 @@ Some special flags are used to determine whether to remove the device:
>>>    The dm_remove_devices_flags() function can be used to remove devices based on
>>>    their driver flags.
>>>
>>> +
>>> +Error codes
>>> +-----------
>>> +
>>> +Driver model tries to use errors codes in a consistent way, as follows:
>>> +
>>> +\-EAGAIN
>>> +   Try later, e.g. dependencies not ready
>>> +
>>> +\-EINVAL
>>> +   Invalid argument, such as `dev_read_...()` failed or any other
>>> +   devicetree-related access. Also used when a driver method is passed an
>>> +   argument it considers invalid or does not support.
>>> +
>>> +\-EIO
>>> +   Failed to perform I/O
>>
>> Do you mind providing a longer explanation here? What sort of IO
>> failures should return EIO instead of (e.g.) ETIMEDOUT? Would ECOMM as
>> used by the MMC subsystem be a good example of what to use EIO for in
>> new code?
> 
> I forgot about -ETIMEDOUT, will add.
> 
> How about:
> 
> Failed to perform an I/O operation. This is used when a local device
> (i.e. part of the SOC) does not work as expected. Use -EREMOTEIO for
> failures to talk to a separate device, e.g. over an I2C or SPI
> channel.
> 
>>
>>> +
>>> +\-ENODEV
>>> +   Do not bind the device. This should not be used to indicate an
>>> +   error probing the device or for any other purpose, lest driver model get
>>> +   confused. Using `-ENODEV` inside a driver method makes no sense, since
>>> +   clearly there is a device.
>>> +
>>> +\-ENOENT
>>> +   Entry or object not found
>>
>> Could we add some examples here? Off the top of my head, this is used
>> for missing device-tree properties/nodes, non-udevice devices (clocks,
>> pinctrl groups, etc.) and of course files and directories.
> 
> How about:
> 
> Entry or object not found. This is used when a device, file, directory
> cannot be found (e.g. when looked up by name), It can also indicate a
> missing devicetree subnode.

Sounds good.

>>
>>> +
>>> +\-ENOMEM
>>> +   Out of memory
>>> +
>>> +\-ENOSPC
>>> +   Ran out of space (e.g. in a buffer or limited-size array)
>>> +
>>> +\-ENOSYS
>>> +   Function not implemented. This is returned by uclasses where the driver does
>>> +   not implement a particular method. It can also be returned by drivers when
>>> +   a particular sub-method is not implemented. This is widely checked in the
>>> +   wider code base, where a feature may or may not be compiled into U-Boot. It
>>> +   indicates that the feature is not available, but this is often just normal
>>> +   operation. Please do not use -ENOSUPP. If an incorrect or unknown argument
>>> +   is provided to a method (e.g. an unknown clock ID), return -EINVAL.
>>> +
>>> +\-ENXIO
>>> +   Couldn't find device/address
>>
>> How does this differ from ENODEV and ENOENT?
> 
> How about:
> 
> Couldn't find device/address. This is used when a device or address
> could not be obtained or is not valid.

I think this still needs to be clarified. Both ENODEV and ENOENT may be
used to indicate a missing device. From what I have seen, this tends to
be used as a "third error code" when ENOENT is already used for some
purpose.

>>
>>> +
>>> +\-EPERM
>>> +   This is -1 so some older code may use it as a generic error. This indicates
>>> +   that an operation is not permitted, e.g. a security violation or policy
>>> +   constraint. It is returned internally when binding devices before relocation,
>>> +   if the device is not marked for pre-relocation use.
>>> +
>>> +\-EPFNOSUPPORT
>>> +   Missing uclass. This is deliberately an uncommon error code so that it can
>>> +   easily be distinguished. If you see this very early in U-Boot, it means that
>>> +   a device exists with a particular uclass but the uclass does not (mostly
>>> +   likely because it is not compiled in). Enable DEBUG in uclass.c or lists.c
>>> +   to see which uclass ID or driver is causing the problem.
>>> +
>>> +\-EREMOTEIO
>>> +   Cannot talk to peripheral, e.g. i2c
>>
>> How does this differ from EIO or ECOMM?
> 
> This indicates an error in talking to a peripheral over a comms link,
> such as I2C or SPI. It might indicate that the device is not present
> or is not responding as expected.

Should ECOMM be used in new code? The current users are the MMC
subsystem for when there is a CRC error, and dm_i2c_ops.xfer for
unsupported speeds (though no one seems to implement that).

--Sean

> 
>>
>>> +
>>> +Less common ones:
>>> +
>>> +\-EKEYREJECTED
>>> +   Attempt to remove a device which does not match the removal flags. See
>>> +   device_remove().
>>> +
>>> +\-EILSEQ
>>> +   Devicetree read failure, specifically trying to read a string index which
>>> +   does not exist, in a string-listg property
>>> +
>>> +\-ENOEXEC
>>> +   Attempt to use a uclass method on a device not in that uclass. This is
>>> +   seldom checked at present, since it is generally a programming error and a
>>> +   waste of code space. A DEBUG-only check would be useful here.
>>> +
>>> +\-ENODATA
>>> +   Devicetree read error, where a property exists but has no data associated
>>> +   with it
>>> +
>>> +\-EOVERFLOW
>>> +   Devicetree read error, where the property is longer than expected
>>> +
>>> +\-EPROBE_DEFER
>>> +   Attempt to remove a non-vital device when the removal flags indicate that
>>> +   only vital devices should be removed
>>> +
>>> +\-ERANGE
>>> +   Returned by regmap functions when arguments are out of range. This can be
>>> +   useful for disinguishing regmap errors from other errors obtained while
>>> +   probing devices.
>>> +
>>> +Drivers should use the same conventions so that things function as expected.
>>> +In particular, if a driver fails to probe, or a uclass operation fails, the
>>> +error code is the primary way to indicate what actually happened.
>>> +
>>> +Printing error messages in drivers is discouraged due to code size bloat and
>>> +since it can result in messages appearing in normal operation. For example, if
>>> +a command tries two different devices and uses whichever one probes correctly,
>>> +we don't want an error message displayed, even if the command itself might show
>>> +a warning or informational message.
>>
>> So should errors while probing always be DEBUG?
> 
> Ideally.
> 
>>
>> Should misconfiguration (e.g. missing a requried devicetree property)
>> and device errors be logged differently?
> 
> To me that is -EINVAL since it indicates the devicetree node is
> invalid for the device.
> 
>>
>> Thanks for documenting this. It is useful to know what the "official"
>> stance on different return codes is, especially when existing code uses
>> everything.
> 
> Yes, Tom and Marek poked me about it.
> 
> Still some grey areas and inconsistencies. But once this is figured
> out I will send v3.
> 
> Regards,
> Simon
> 
>>
>> --Sean
>>
>>> +
>>> +Error messages can be logged using `log_msg_ret()`, so that enabling
>>> +`CONFIG_LOG` and `CONFIG_LOG_ERROR_RETURN` shows a trace of error codes returned
>>> +through the call stack. That can be a handy way of quickly figuring out where
>>> +an error occurred. Get into the habit of return errors with
>>> +`return log_msg_ret("here", ret)` instead of just `return ret`. The string
>>> +just needs to be long enough to find in a single function, since a log record
>>> +stores (and can print with `CONFIG_LOGF_FUNC`) the function where it was
>>> +generated.
>>> +
>>> +
>>>    Data Structures
>>>    ---------------
>>>
>>>
>>