drivers: Supporting driver-specific extensions to existing generic APIs

[pabigot]

I frequently run into cases where I want a specific driver to support capabilities not accessible through the standard driver API. Some of these might be added to the generic API, but some are too specific. Examples include:

• adc support calibration on Nordic SAADC peripheral
• gpio configure multiple pins simultaneously, as with Semtech SX150x
• ccs811 sensor manage calibration constants and update environmental state

Candidate approaches include:

• driver-specific functions exposed to user applications through a header, e.g. <drivers/sensor/ccs811.h>
• a generic IOCTL-like approach that would add a function pointer to struct device_config making the feature available on any driver through a common API, as with the power management commands (which would also require a header to document available commands and required data structures)

This issue is open to solicit perspectives on how this should be supported.

[pabigot]

See some discussion in slack.

[pabigot]

Here's an example of the approach I've taken for a complete rework of CCS811:

A new header ccs811.h is added to include/drivers/sensor. It includes documented data types for things like firmware version description, baseline calibration values, current environmental state. It also prototypes functions to manipulate those, e.g.:

int ccs811_configver_fetch(struct device *dev,
                           struct ccs811_configver_type *ptr);
int ccs811_baseline_fetch(struct device *dev);
int ccs811_baseline_update(struct device *dev, u16_t baseline);
int ccs811_envdata_update(struct device *dev,
                          const struct sensor_value *temperature,
                          const struct sensor_value *humidity);

User code would look like:

#include <drivers/sensor/ccs811.h>
// ... 
rc = ccs811_configver_fetch(dev, &cfgver);
if (0 == rc) {
  printf("CCS811 HW %02x; FW Boot %04x App %04x ; mode %02x\n",
         cfgver.hw_version, cfgver.fw_boot_version,
         cfgver.fw_app_version, cfgver.mode);
}

Because these functions are provided by the implementation of the ccs811 driver they have no impact on any other driver.

Because they're prototyped the arguments can be at least type-checked at build time, and clearly documented so their behavior is clear.

[pabigot]

And here's a rough outline of what it might take to use an IOCTL approach:

We'd need changes like this in include/device.h:

• struct device_config would have a new member, something like:

  int (* ioctl)(struct device *dev, u32_t request, ...);
  

  along with a generic wrapper:

  int device_ioctl(struct device *dev, u32_t request, ...)
  

• We'd need equivalents to at least the _IO, _IOR, _IOW, and _IORW macros in Linux
• We'd need a managed registry of IOCTL types (e.g. 'T' for terminal)
• We'd need a managed registry of IOCTL numbers (e.g. TIOCGSERIAL = 0x541E)

We'd still need the driver-specific headers to define the data structures, build up the IOCTL request constants, and document everything:

#define IOCTL_CCS811_GET_BASELINE _IOR('S', 0xCC01, u16_t)
#define IOCTL_CCS811_SET_ENVDATA _IOW('S', 0xCC02, struct ccs811_envdata)

The user code would end up being:

rc = device_ioctl(ccs811, IOCTL_CCS811_GET_BASELINE, &baseline);
rc = device_ioctl(ccs811, IOCTL_CCS811_SET_ENVDATA, &envdata);

In short, it could probably be done, but it'd be pretty hard to get right if we don't replicate the POSIX/Linux ioctl architecture, a whole lot of effort if we do, and pretty ugly to anybody who isn't an experienced POSIX developer.

[andyross]

This seems like significantly wasted work to me. The Linux model is "syscalls are rare and carefully curated and drivers need to follow these rules to poke holes in the abstraction for their own APIs".

The Zephyr model, so far, has been "drivers are responsible for exposing their own APIs via simple function calls, and for the special case of userspace we work to make it easy to expose those calls as automatically-generated system call traps".

Given the latter, all the ioctl() rigor doesn't seem to provide much value. Maybe the latter isn't the right model, I can see an argument there. But if we want to go there let's open up the "Zephyr's driver interface abstractions are a leaky mess" flame war and not start with ioctl. :)

[pfalcon]

@pabigot: Thanks for submitting this as a ticket. Let me admit then when I saw a post on Slack, I mostly saw keywords like "ioctl" vs "multiple functions". My immediate comment was that choosing to add multiple "virtual methods" to drivers vs single "ioctl-like vmethod", I'd recommend the latter.

If the feature you care about is clearly specific to a particular hardware device, and this its driver, then your <drivers/sensor/ccs811.h> interface is absolutely right way to do. Its target audience it: people who work with a particular sensor, and who want to get all out of it (at the expense of making the application specific to a particular sensor, requiring substantial efforts to port to another sensor).

[pfalcon]

Now let me elaborate on what I had in mind when responding on Slake, for completeness.

We start not with a particular driver, we start with a subsystem, let it be sensor subsystem here too. Suppose, for every sensor we want to expose a means to get its accuracy. (I don't know how contrived that example is to you, for me, it's not at all.) So, how do we implement that? What we have is struct sensor_driver_api: https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sensor.h#L260 , which is a struct of function pointers aka vmethods. The whole "sensor API" is nothing but a set of dispatcher functions which call a vmethod for a particular device.

So, should we add something like sensor_get_accuracy_t get_accuracy;? That's the whole point of my argument - no, let's not do that/stop doing that, because it doesn't scale. Instead, let's add:

int (*sensor_ioctl)(int cmd, void *arg1, void *arg2);

there. And let's define

enum sensor_op {
sensor_get_accuracy,
...
};

I hope I clarified myself and it was somewhat related, and that I didn't cause too much confusion (again).

Comparing to your, @pabigot, example, key point is that we define subsys-specific "ioctl-like". By all means, let's avoid defining ioctl's on the level of struct device, that's far too polymorphic.

EDIT: And of course, subsys-level ioctl's make sense only if some feature really exists/is reusable across drivers, and if a common API can be reasonably devised for it. In all other cases, it's better to go for driver-adhoc functions like you show.

[pabigot]

@pfalcon yes, that's exactly it. While I appreciate in principle the goal of Zephyr to make applications as generic as possible by providing a common API, in the domain I'm working I know exactly what hardware and sensors I'm going to be using. It's...sub-optimal...for me to be dealing with temperatures and humidities expressed as 64-bit fixed-point values when the driver collects them together at a resolution of no more than 16 bits each. (Regarding sensor accuracy: I appreciate your point, but don't think that's going to scale--the current sensor_channel enumeration doesn't. I also don't think this is the place to discuss it in depth.)

@andyross I take from your comment you're not in favor of ioctl, which is my position as well. It's not clear to me whether you support (in principle) the driver-specific header and function approach, or believe driver-specific capabilities should be exposed through some other extension of a generic sensor, GPIO, or ADC API.

This arises because outside of clock control I can't see any API in include that isn't genericized to the point where features that distinguish a specific product can't be accessed. There are no driver ad-hoc functions for any sensor device, which astonishes me. I'd like to expose them in a way that's acceptable to the Zephyr community and am looking for architectural guidance.

[andyross]

I think my position summarizes as "I'd like to see some attention paid to firming up exactly how drivers expose APIs , but I don't think it should look like ioctl"

[andrewboie]

One thing about system calls. Right now all the driver system calls take a generic struct device pointer. The syscall code is able to distinguish what subsystem any given struct device belongs to; we have some build system logic which notes what API struct was assigned to the device at build time (i2c_driver_api, uart_driver_api, etc) and sets this in the kernel object metadata (K_OBJ_DRIVER_I2C, K_OBJ_DRIVER_UART, etc)

This allows us to enforce that appropriate calls are being made on a device at the subsystem level; the system will reject a system call made on a UART API if the device object isn't associated with the uart subsystem. This works because of the common interface between all devices of a particular subsystem type.

However, we don't have any facilities for distinguishing between individual driver instances at the moment. So if we were to do system calls that were truly driver specific, outside of the subsystem layer, and the only pointer passed into the API is of a generic struct device, we can say this driver belongs to the appropriate subsystem but currently no more than that, which would be problematic; for an API that only works on specific driver X of subsystem Y, we can only (curently) say that the device pointer belongs to subsystem Y and not that it is also an instance of driver X, it could also be some driver Z which also implements subsystem Y.

Now if we did the abstraction at the subsystem level, I think we can handle this fairly easily. So like for the sensor API, add an additional ioctl method (or whatever you like to call it). If a particular driver doesn't have any capabilities beyond what is offered in the base API, this API pointer can be set to NULL. Each driver that did offer extra capabilities would have to do checks to ensure that the requested ioctl operation is supported, and such ioctl operation types would be shared across all drivers of that subsystem.

Note that I have no particular attachment to any of the ideas being proposed in this thread, just thought I'd mention this caveat. Not saying it's impossible to support out-of-subsystem calls either, we'd just have to solve the problem of correctly enforcing that appropriate device pointers are being passed in (but it may be a lot of hassle to do so.)

[pabigot]

@andrewboie Thanks, this exposes more detail I was missing related to verification and userspace, the design criteria that led to the current architecture, and the breadth of Zephyr's "here, let me do that for you" code generation framework. I think the user-level API I've suggested is still a good solution, but underneath it might have to be accomplished using a generic extension function that supports this checking. I have some ideas and will try them when it comes time to submit some of these enhancements as PRs.

[andrewboie]

we'd just have to solve the problem of correctly enforcing that appropriate device pointers are being passed in (but it may be a lot of hassle to do so.)

Thinking about this some more, it might not be terrible to also uniquely identify drivers. It may not be much hassle to determine that given a generic device pointer, to associate with a specific driver. Right now the build system logic stores the type of the device->driver_api object in the kernel object metadata, but we could additionally register the device->driver_data which is specific to the driver implementation. So we don't necessary need to rule this out. It might actually be better this way.

it might have to be accomplished using a generic extension function that supports this checking

The problem I see with a subsystem-level extension function, that takes as a parameter some enumerated type of capabilities declared subsystem-wide in some switch statement, is that I'm not seeing much difference in that, and just adding new API pointers for these capabilities to the subsystem, which are set to NULL for devices that do not support them.

I have some ideas and will try them when it comes time to submit some of these enhancements as PRs.

I'd suggest sending some high-level proposals here before you write a lot of code and send a PR. The discussion may need to iterate a few times before everyone is happy.

[andrewboie]

This commit here is a quick-and-dirty proof-of-concept on doing a syscall specific to a driver:

andrewboie@3a6fbf9

Output of samples/hello_world on qemu_x86:

Hello World! qemu_x86
Hello from user mode!
Hello from device 0x0040c1c0, _impl_uart_ns16550_specific_syscall(42)

This is very hacked together and a real implementation would not register the dev_data structs as kernel objects, but you get the idea.

If the feature you care about is clearly specific to a particular hardware device, and this its driver, then your <drivers/sensor/ccs811.h> interface is absolutely right way to do. Its target audience it: people who work with a particular sensor, and who want to get all out of it (at the expense of making the application specific to a particular sensor, requiring substantial efforts to port to another sensor).

I concur, if it's really specific to the HW then just declare a syscall in the driver specific header, and the plumbing would look similar to the POC above.

If we have a capability which is supported by many, but not all devices, then I think we should just add it to the API and set the API pointer to NULL for devices that don't support it.

I think my position summarizes as "I'd like to see some attention paid to firming up exactly how drivers expose APIs , but I don't think it should look like ioctl"`

@andyross fair enough, but what should it look like?

[pfalcon]

@andrewboie

The problem I see with a subsystem-level extension function, that takes as a parameter some enumerated type of capabilities declared subsystem-wide in some switch statement, is that I'm not seeing much difference in that, and just adding new API pointers for these capabilities to the subsystem, which are set to NULL for devices that do not support them.

Well, I do see. In a general case, there can be a hundred such "additional functions", which of course unlikely would be implemented by all drivers, wasting memory (even if RO) on all these sparse pointer arrays. Next - it's highly dependent on the actual functions of course - but usually it's possible to amortize some operations of similar functions by combining them into one. That would apply to syscall validation functions - even if there's a only dozen of them, each validating device pointer, then merging them into one, validating that pointer once, even dispatching on "command code" should lead to the overall code savings. And of course, I assume each new syscall is not free, and we should target having less syscalls overall, rather than more.

So, as was pretty clearly noted #11917, usage of syscalls is not mandatory. But even folks who aren't interested in userspace/syscalls should be aware that they're integral part of Zephyr, and should try to "design for syscalls", even if the actual implementation won't be done by them, but by someone else later.