Re: [RFC] Device: device_get_binding() returns NULL if device fails initialization


Daniel Leung <daniel.leung@...>
 

On Wed, Apr 06, 2016 at 02:02:17PM +0000, Kalowsky, Daniel wrote:
-----Original Message-----
From: Tomasz Bursztyka [mailto:tomasz.bursztyka(a)linux.intel.com]
Sent: Wednesday, April 6, 2016 12:00 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] Re: [RFC] Device: device_get_binding() returns NULL if
device fails initialization

Hi Daniel,

That's actually a good point.

And that reminds me, that most of our drivers do not set driver API to
NULL if they fail.
But they do return an explicit code which no one care about:
see _sys_device_do_config_level() in kernel/nanokernel/device.c

I think there you can add also a tiny test:

if (device->init(info) < 0) {
info->driver_api = NULL;
}
Good point Tomasz. Might need to be part of the RFC patch that Daniel posted.
The original discussion was actually started about getting rid of returns of
initialization functions. The idea was that the kernel could not really do
anything when a driver failed initialization, so why not getting rid of returns
to save a few bytes. The burden of determining whether it is a critical error
during init is up to the driver. If there is a critical error, the driver
should assert (and setting driver_api to NULL).

There are situations where there are non-critical erros during initialization.
For example, the I2C driver, failure to set default configuration is not
critical. As long as the developer sets it later, the driver can still function.
However, with the above code, this non-critical error will cause driver_api
to be NULL, which prevents its usage at all. The driver writer should know
whether an error prevents the device from functioning at all. So it should be
up to driver writer to set driver_api to NULL. One can argue that a non-critical
error should not return anything other than 0. But then it is not passing on
the information that there is a non-critical error (though the kernel does not
really care).

(actually, naming in this function should be reversed: struct device
should be "device" and
struct device_config should be "info" or "config", even better. But I am
diverging from subject)

It's better to let device.c doing it, instead of making sure all driver
behaves well on that aspect.
As long as they return a relevant status, it will be fine.
Correct. If the OS is going to depend upon a feature/function being set to
indicate a specific status, the OS should be doing this not the device driver.
Although it doesn't hurt if the device driver does it as well :-)
(see above.)


Tomasz

Problem Statement:
Currently, there is no way to know if a driver fails initialization.

Problem Description:
Zephyr currently does not provide a way to check programmatically
whether a device can be used. The device_get_binding() always
returns a valid device struct (if such device driver instance
exists). This causes a bit of headache when debugging apps as
the developer assumes the devices are ready to be used.

Solution:
The solution was actually proposed in [1] a while ago. The idea
is to piggy-back onto driver_api pointer. If the pointer is NULL,
the device driver has failed in its own initialization function.
The driver_api is set to NULL (or in other words, never set to
the actual API struct). When device_get_binding() sess that
the driver_api is NULL for a particular device struct, it returns
NULL to the caller, and thus preventing its use in the app.
Since this is a binary state, instead of creating another variable
inside device struct, this uses driver_api to avoid unnecessarily
enlarging the ROM size. Since this is a simple change, a patch
has been created in [2].

[1]
https://lists.zephyrproject.org/archives/list/devel(a)lists.zephyrproject.org/
message/MZB5PYBSRHV3NIEHJYXYQVLTPFIIHPB3/
[2] https://gerrit.zephyrproject.org/r/1261


----------
Daniel Leung

Join devel@lists.zephyrproject.org to automatically receive all group messages.