Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h instead
of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current drivers
to use errno.h codes instead of DEV_* is a feasible task.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

Regards,

Andre

Hi,

On Thu, 18 Feb 2016 17:47:56 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h instead
of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

Cool!
+1 from my side, mainly so we don't need to discuss the addition of new error codes ever again :).

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current drivers
to use errno.h codes instead of DEV_* is a feasible task.

I would say it's more about 3) and 4) really, as I've seen surprisingly often how 1) and 2) are not
always true. I've seen recently people getting confused about what to return and when, myself
included. In the end it doesn't matter from each .h file you get the errors from, as long as you
have clear guidelines for them, in my opinion.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a try with a).
If we get this two right, then we end up with both the guidelines for documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno codes will be as easy
as a few seds. So:

DEV_OK = 0
DEV_FAIL = (??)
DEV_INVALID_OP = -EPERM or -ENOTSUP (??)
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)
DEV_NOT_CONFIG = (??)
DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.


How bad does that look?!



Regards,
jesus

On Wed, 2 Mar 2016 16:03:22 -0300
Jesus Sanchez-Palencia <jesus.sanchez-palencia(a)intel.com> wrote:

Hi,

On Thu, 18 Feb 2016 17:47:56 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h instead
of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

Cool!
+1 from my side, mainly so we don't need to discuss the addition of new error codes ever again :).

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current drivers
to use errno.h codes instead of DEV_* is a feasible task.

I would say it's more about 3) and 4) really, as I've seen surprisingly often how 1) and 2) are not
always true. I've seen recently people getting confused about what to return and when, myself
included. In the end it doesn't matter from each .h file you get the errors from, as long as you
have clear guidelines for them, in my opinion.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a try with a).
If we get this two right, then we end up with both the guidelines for documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno codes will be as easy
as a few seds. So:

I forgot to mention that whichever guidelines come out of this thread, I will move to QMSI so we
are all aligned.

DEV_OK = 0
DEV_FAIL = (??)
DEV_INVALID_OP = -EPERM or -ENOTSUP (??)
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)
DEV_NOT_CONFIG = (??)
DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.


How bad does that look?!



Regards,
jesus

Hi Jesus,

On Wed, Mar 02, 2016, Jesus Sanchez-Palencia wrote:

DEV_OK = 0
DEV_FAIL = (??)
DEV_INVALID_OP = -EPERM or -ENOTSUP (??)
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN

-EACCES? EAGAIN doesn't seem like the best match here.

Johan

Hi Daniel,

On Wed, Mar 2, 2016 at 10:46 PM, Kalowsky, Daniel
<daniel.kalowsky(a)intel.com> wrote:

-----Original Message-----
From: Jesus Sanchez-Palencia [mailto:jesus.sanchez-palencia(a)intel.com]
Sent: Wednesday, March 2, 2016 11:03 AM
To: Guedes, Andre <andre.guedes(a)intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: RFC: Use error codes from errno.h

Hi,

On Thu, 18 Feb 2016 17:47:56 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h

instead

of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

Cool!
+1 from my side, mainly so we don't need to discuss the addition of new
error codes ever again :).

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current

drivers

to use errno.h codes instead of DEV_* is a feasible task.

I would say it's more about 3) and 4) really, as I've seen surprisingly often
how 1) and 2) are not
always true. I've seen recently people getting confused about what to return
and when, myself
included. In the end it doesn't matter from each .h file you get the errors
from, as long as you
have clear guidelines for them, in my opinion.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.


How bad does that look?!

--
Luiz Augusto von Dentz

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for
the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.

Hi everyone,


On Thu, 3 Mar 2016 13:33:16 -0500
Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote:

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

After all the feedback so far (thanks, btw!), the updated list now looks like:

DEV_OK = 0
DEV_FAIL = -EFAULT
DEV_INVALID_OP = -EPERM or -ENOTSUP or -ESRCH
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV or -ENXIO or -ENOTSUP
DEV_NOT_CONFIG = -EIO or -ENODEV
DEV_NOT_IMPLEMENTED = -ENOSYS


Which leaves us with only 3 left to be decided.
By comparing again the original description with the ones from errno.h, what about:

DEV_INVALID_OP = -ENOTSUP
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EIO


any better?


thanks,
jesus

Hi all,

Quoting Jesus Sanchez-Palencia (2016-03-02 16:03:22)

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

Yes, this is the idea.

DEV_OK = 0
DEV_FAIL = (??)
DEV_INVALID_OP = -EPERM or -ENOTSUP (??)
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)
DEV_NOT_CONFIG = (??)
DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.

Back then, I reviewed the error codes usages and this was what I had in mind:

DEV_OK = 0
DEV_FAIL = -EIO /* Input/output error */
DEV_INVALID_OP = -EINVAL /* Invalid argument */
DEV_INVALID_CONF = -EINVAL /* Invalid argument */
DEV_USED = -EBUSY /* Device or resource busy */
DEV_NO_ACCESS = -EACCES /* Permission denied */
DEV_NO_SUPPORT = -ENOTSUP /* Operation not supported */
DEV_NOT_CONFIG = -EPERM /* Operation not permitted */
DEV_NOT_IMPLEMENTED -ENOSYS /* Function not implemented */

The comments besides the error codes come from 'man errno'.

Regards,

Andre

On 4 Mar 2016, at 07:28, Jesus Sanchez-Palencia <jesus.sanchez-palencia(a)intel.com> wrote:

Hi again,


On Thu, 3 Mar 2016 16:17:45 -0300
Jesus Sanchez-Palencia <jesus.sanchez-palencia(a)intel.com> wrote:

Hi everyone,


On Thu, 3 Mar 2016 13:33:16 -0500
Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote:

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */

After some more f2f feedback, André's reply (thanks!), and merging with previous
state/feedback we now have:

DEV_OK = 0
DEV_FAIL = -EIO
DEV_INVALID_OP = -ENOTSUP
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EPERM
DEV_NOT_IMPLEMENTED = -ENOSYS


Time to move this to gerrit for proper reviews, right?!

Yes please,

Thanks.
Anas

thanks,
jesus

Hi,

Quoting Nashif, Anas (2016-03-04 09:34:45)

DEV_OK = 0
DEV_FAIL = -EIO
DEV_INVALID_OP = -ENOTSUP
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EPERM
DEV_NOT_IMPLEMENTED = -ENOSYS


Time to move this to gerrit for proper reviews, right?!

Yes please,

Nice, let's move on with this convention. I'll send patches to gerrit soon.

Thanks all,

Andre

Hi all,

Quoting Andre Guedes (2016-03-04 10:55:24)

Hi,

Quoting Nashif, Anas (2016-03-04 09:34:45)

DEV_OK = 0
DEV_FAIL = -EIO
DEV_INVALID_OP = -ENOTSUP
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EPERM
DEV_NOT_IMPLEMENTED = -ENOSYS


Time to move this to gerrit for proper reviews, right?!

Yes please,

Nice, let's move on with this convention. I'll send patches to gerrit soon.

I just sent a patchset with topic "errno-drivers" implementing what we have
discussed in this RFC. The patchset basically redefines DEV_* using -E* codes
and replaces all occurrences of DEV_* by -E* at the driver layer.

Regards,

Andre

Hi all,

Quoting Andre Guedes (2016-03-03 16:43:24)

Quoting Jesus Sanchez-Palencia (2016-03-02 16:03:22)

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

Yes, this is the idea.

Do we have a consensus about this?

After the "errno-drivers" patchset, DEV_* codes will be used only at 'board'
and 'arch' layers. Actually, all occurrences of DEV_* codes in board/ are
from pinmux drivers. These drivers are going to be landed in drivers/ soon
(patches are under review on gerrit) so they will already be replaced by -E*
codes. This means that only a few files from arch/ will be using DEV_* codes.

If we have a consensus, the next steps are:
1) Replace DEV_* occurrences in arch/ (and boards/ if applicable);
2) Deprecate DEV_* (just add a comment in device.h saying: DEV_* are deprecate,
use errno.h codes instead.
3) Remove DEV_* codes.

Since 3) might break external applications, we should apply it during a major
release I guess.

Regards,

Andre

Hi, trying the webmail reply for a first time. Be gentle.

Hi all,

Quoting Andre Guedes (2016-03-03 16:43:24)

Do we have a consensus about this?

After the "errno-drivers" patchset, DEV_* codes will be used only at
'board'
and 'arch' layers. Actually, all occurrences of DEV_* codes in board/ are
from pinmux drivers. These drivers are going to be landed in drivers/ soon
(patches are under review on gerrit) so they will already be replaced by -E*
codes. This means that only a few files from arch/ will be using DEV_* codes.

If we have a consensus, the next steps are:
1) Replace DEV_* occurrences in arch/ (and boards/ if applicable);

+1

2) Deprecate DEV_* (just add a comment in device.h saying: DEV_* are deprecate,
use errno.h codes instead.

+1 I would argue this should step 1. This can be and should be merged in before the close of the release window this week. So get this done ASAP.

3) Remove DEV_* codes.

Since 3) might break external applications, we should apply it during a major
release I guess.

I'd wait until the start of the next release cycle to make this change. It will give a long window of opportunity to update.

Hi Daniel, thanks for your feedback!

See some comments inline:

Quoting Dan Kalowsky (2016-03-21 14:10:43)

Hi, trying the webmail reply for a first time. Be gentle.

Hi all,

Quoting Andre Guedes (2016-03-03 16:43:24)

Do we have a consensus about this?

After the "errno-drivers" patchset, DEV_* codes will be used only at
'board'
and 'arch' layers. Actually, all occurrences of DEV_* codes in board/ are
from pinmux drivers. These drivers are going to be landed in drivers/ soon
(patches are under review on gerrit) so they will already be replaced by -E*
codes. This means that only a few files from arch/ will be using DEV_* codes.

If we have a consensus, the next steps are:
1) Replace DEV_* occurrences in arch/ (and boards/ if applicable);

+1

Patch sent to gerrit.

2) Deprecate DEV_* (just add a comment in device.h saying: DEV_* are deprecate,
use errno.h codes instead.

+1 I would argue this should step 1. This can be and should be merged in before the close of the release window this week. So get this done ASAP.

Fine by me. Patch sent to gerrit. There is no dependency so it can be merged
before any other patch.

3) Remove DEV_* codes.

Since 3) might break external applications, we should apply it during a major
release I guess.

I'd wait until the start of the next release cycle to make this change. It will give a long window of opportunity to update.

Fine by me too. I also sent a patch removing the DEV_* codes to gerrit, but
we can wait until the next merge window is open to apply it.

BTW, while we don't remove DEV_* codes, we have to be extra careful to avoid
new code is merged using DEV_* instead of -E*. I just noticed this occurred
with aon counters and gpio_stm32 drivers. Patches fixing this on gerrit.

Regards,

Andre