Hi all,

It seems we don't have a consensus about what error code the device driver
should return in case a given driver API is not implemented. If we take a
look at suspend() and resume() functions, which are not implemented by some
device drivers, we can find three different return codes:
- DEV_OK: used in i2c_dw.c, i2c_atmel_sam3.c and gpio_sch.c
- DEV_INVALID_OP: used in pwm_dw.c, gpio_mmio.c and gpio_atmel_sam3.c
- DEV_NO_SUPPORT: used in i2c_qmsi.c and spi_qmsi.c

For these situations, a new error code (DEV_NOT_IMPLEMENTED) seems to be
applicable. If a new error code is not desired, we should agree on one
and use it in all device drivers.

Regards,

Andre

See [Peter]

On 02/12/2016 05:52 AM, Andre Guedes wrote:

Hi all,

It seems we don't have a consensus about what error code the device driver
should return in case a given driver API is not implemented. If we take a
look at suspend() and resume() functions, which are not implemented by some
device drivers, we can find three different return codes:
- DEV_OK: used in i2c_dw.c, i2c_atmel_sam3.c and gpio_sch.c
- DEV_INVALID_OP: used in pwm_dw.c, gpio_mmio.c and gpio_atmel_sam3.c
- DEV_NO_SUPPORT: used in i2c_qmsi.c and spi_qmsi.c

We definitely need to have one answer all drivers agree on.

I had an RFC where the suspend/resume functions move to the device
level and out of the device type specific APIs. This allows the PM
app/subsystem/entity to call the PM functions without needing to
know the type of the device.

I think I had agreement on the RFC but it fell by the wayside as we
were creeping up on the release date. I will get it rebased and
published again. Once we get agreement we can appoint all the
stuckies to update the drivers accordingly.

For these situations, a new error code (DEV_NOT_IMPLEMENTED) seems to be
applicable. If a new error code is not desired, we should agree on one
and use it in all device drivers.

In the new model the functions would be required to be implemented, the
RFC included a null implementation for drivers to use if they had no
work to do during suspend/resume. Still a topic for discussion but the
null functions can just return success meaning I have done everything
I need to or can do :-).

Regards,

Andre

Hi Peter,

Quoting Mitsis, Peter (2016-02-12 12:39:13)

[Peter] - Is there a reason why should not use the existing error code ENOSYS? From the description taken from http://www.gnu.org/software/libc/manual/html_node/Error-Codes.html (pasted below) I would think that it would fit our purposes.

I guess the main reason is that the device driver APIs follows the error code
format from include/device.h not the errno.h. Sure we can change all drivers
to use error code from errno.h, but I don't think this is desired ATM.

Thanks,

Andre

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-12 12:53:49)

Early on when we started the driver development, the suspend/resume functions were developed with the DEV_OK because as far as any OSPM policy is concerned, there should be no reason these devices would stop it. This was an not so nice way into fooling that it has happened.

Thanks for the background explanation.

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think
the error code DEV_INVALID_OP might be more suitable.

Anyways, if we move forward with DEV_NO_SUPPORT, I think we should fix the
comment in include/device.h since it looks a bit misleading. See below:

--[cut]--
#define DEV_NO_SUPPORT 6 /* Device type not supported */
--[cut]--

Thanks,

Andre

Hi Dirk,

Quoting Dirk Brandewie (2016-02-12 13:32:31)

In the new model the functions would be required to be implemented, the
RFC included a null implementation for drivers to use if they had no
work to do during suspend/resume. Still a topic for discussion but the
null functions can just return success meaning I have done everything
I need to or can do :-).

Yes, sure. If we can use this 'null' implementation in drivers that don't
properly implement the suspend/resume callbacks (yet), this will fix the
issue I described in the other email.

However, the suspend/resume callbacks were just an example to bring the
discussion about having a "not implemented"-wise error code in device.h.
I didn't review all drivers, but it seems such error code would be suitable
in other situations like the interrupt-related callbacks from the
gpio_pcal9535a.c driver. These callbacks are not implemented yet and we
use DEV_INVALID_OP.

Thanks,

Andre

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-17 05:09:07)

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a

DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.

Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.

I think ENOSYS would perfectly match what we want here since it means
"Function not implemented" (as Peter pointed in the other reply).

Speaking of which, I was talking to Dirk in the other day and we both agreed
that it would be a good idea we use errno.h codes instead of DEV_* at the
driver's layer. The main points 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.

What do you think?

I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.

Yes, this is the challenge. I volunteer to review and fix what we have
upstream. However, we have to pay attention for this kind of things
while reviewing new patches on Gerrit to ensure they are merged with
the proper return codes.

Thanks,

Andre

Hi,


On Thu, 18 Feb 2016 09:11:59 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-17 05:09:07)

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a

DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.

Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.

I think ENOSYS would perfectly match what we want here since it means
"Function not implemented" (as Peter pointed in the other reply).

Speaking of which, I was talking to Dirk in the other day and we both agreed
that it would be a good idea we use errno.h codes instead of DEV_* at the
driver's layer. The main points 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.

What do you think?

Just so we avoid mixing two different topics, it looks like this deserves a separate RFC.

The original problem I raised on that code review a few weeks ago -- not having a defined return
code to state that a given driver API impl is dummy -- seems to have had some conclusion reached
here already. Looks like adding DEV_NOT_IMPLEMENTED has some appeal in the end. I also support that
meanwhile the errno idea is not discussed.

I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.

Yes, this is the challenge. I volunteer to review and fix what we have
upstream. However, we have to pay attention for this kind of things
while reviewing new patches on Gerrit to ensure they are merged with
the proper return codes.

Guidelines, folks. Let's make sure this is documented somewhere so we can just copy&paste a link on
reviews afterwards and people can refer to it when coding.

Thanks for preparing this RFC, by the way!

Regards,
jesus

Thanks,

Andre

Hi Jesus,

Quoting Jesus Sanchez-Palencia (2016-02-18 12:42:07)

Just so we avoid mixing two different topics, it looks like this deserves a separate RFC.

Yes. I already had this in mind and I'm going to send the errno.h RFC later
today.

I was basically replying Daniel's comment and took the opportunity to get an
early feedback from him.

Regards,

Andre

On 02/18/2016 06:42 AM, Jesus Sanchez-Palencia wrote:

Hi,


On Thu, 18 Feb 2016 09:11:59 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-17 05:09:07)

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a

DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.

Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.

I think ENOSYS would perfectly match what we want here since it means
"Function not implemented" (as Peter pointed in the other reply).

Speaking of which, I was talking to Dirk in the other day and we both agreed
that it would be a good idea we use errno.h codes instead of DEV_* at the
driver's layer. The main points 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.

A good first step is to redefine DEV_* in terms of -E* error codes :-)

Adding the include of errno.h and the removal of DEV_* can happen
at our leisure :-) The apps will be getting a consistent set of error
codes in any case.

What do you think?

Just so we avoid mixing two different topics, it looks like this deserves a separate RFC.

The original problem I raised on that code review a few weeks ago -- not having a defined return
code to state that a given driver API impl is dummy -- seems to have had some conclusion reached
here already. Looks like adding DEV_NOT_IMPLEMENTED has some appeal in the end. I also support that
meanwhile the errno idea is not discussed.

I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.

Yes, this is the challenge. I volunteer to review and fix what we have
upstream. However, we have to pay attention for this kind of things
while reviewing new patches on Gerrit to ensure they are merged with
the proper return codes.

Agreed

Guidelines, folks. Let's make sure this is documented somewhere so we can just copy&paste a link on
reviews afterwards and people can refer to it when coding.

Great idea! This will help with readability of driver code and if the
app developer actually tries to decode the error they will know where
to get the *meaning*

Thanks for preparing this RFC, by the way!

Regards,
jesus

Thanks,

Andre