How to complete userspace support for driver-specific API

[pabigot]

tl;dr: we have not fully solved #11993.

PR #18186 documents an approach to driver-specific APIs based on extended a standard driver API with additional methods only supported by a specific driver API. The requirements derive from #11993.

#12056 provided a macro to be used in system call verification to confirm that a driver-specific API call could be correctly called for a given device instance. This approach lacked documentation and tests, but was merged.

Since it was never used in-tree, #23407 proposes removing it, in part because the solution is based on comparing the device's init function against an available symbol, and #23407 eliminates the init function from the device structure.

In attempting to assess whether the API can be removed, I've attempted to update #17631 to work correctly with user mode, which requires adding syscall implementations for its specific API. This fails, because the syscall infrastructure doesn't support the architectural solution accepted in #18186.

To summarize the context:

• The DS3231 driver is a specialized counter, conforming to the counter API but using an API table that includes additional functions. A counter_driver_api instance appears at the start of the ds3231_driver_api.
• While counter_driver_api is a top-level driver API and so is declared with __subsystem which triggers synthesis of K_OBJ_DRIVER_COUNTER ds3231_driver_api is not a top-level API, and does not have its own K_OBJ_DRIVER_DS3231 object type. It cannot have such a type type, because the counter_foo() API is expected to work on devices that have ds3231_driver_api API tables.
• We need some way to validate the system calls that are specific to the driver, which requires a change to Z_SYSCALL_SPECIFIC_API() to verify that the device is associated with the base API (so that the base API functions validate) and it must use the API table that provides the extended functions.

A proposed solution is presented in #24537. I am not happy with it, particularly the dirty trick required to associate the base with extended API structures.

#17631 has been updated with a draft of what this would look like in a real driver. Initial testing is promising, but the test application requires a lot of work to become fully userspace compatible.

[pabigot]

@andrewboie please provide guidance.

[carlescufi]

@andrewboie we have kept this as an enhancement for now, but according to @pabigot it is currently not possible to give userspace support for driver-specific functions at all. If you could confirm that is the case, then we should change this into a bug.

[andrewboie]

@pabigot I'm really having trouble understanding why you need API struct abstractions for driver-specific APIs. I understand you invented this and am now having trouble with system calls.

The whole point of the API struct abstraction in our device model is so that many different drivers can all be programmed to the same subsystem interface.

What is the point of this API struct indirection when there is exactly ONE implementation, since these APIs are specific to one driver? This is totally unnecessary.

Defining a driver-specific syscall is easy. Here's an example. Let's say I want to add an API to the ns16550 driver:

In a header for ns16550 (not the main subsystem header):

__syscall int uart_ns16550_foo(struct device *dev, int bar);

In uart_ns16550.c:

int z_impl_uart_ns16550_foo(struct device *dev, int bar)
{
   .. do something ..
}

int z_vrfy_uart_ns16550_foo(struct device *dev, int bar)
{
    Z_OOPS(Z_SYSCALL_SPECIFIC_DRIVER(dev, Z_SYSCALL_DRIVER_UART, uart_ns16550_init)));

    return z_impl_uart_ns16550(dev, bar);
}

The verification function will not pass unless dev is a device instance of the UART subsystem, and its init function pointer matches uart_ns16550_init, guaranteeing that this is an instance of not just a uart driver, but an ns16550 driver.

What am I missing here? this works.

[pabigot]

I don't know that you're missing anything. What I was missing was this level of assistance back when I asked several times how to use the macro you provided, or when we discussed and got approved the solution documented in #18186. It's quite likely I was distracted by the solution that seemed natural to me (i.e., nested vtables).

I'll see if I can make this work. I do suspect the macro implementation needs to be fixed, though; it should not pass if it's a UART without also checking the init function.

[andrewboie]

What I was missing was this level of assistance back when I asked several times how to use the macro you provided

Peter I get literally hundreds of emails from GitHub a DAY. I do not read them all. If you can't figure out something you should try hitting me up on slack, or directly e-mailing me instead of whining like this.

when we discussed and got approved the solution documented in #18186.

I don't care who approved 18186. To be blunt: it belies a profound lack of understanding of why we have these vtables in the first place. To use them for singleton APIs is unnecessary boilerplate. I think it that PR should be deleted.

I do suspect the macro implementation needs to be fixed, though; it should not pass if it's a UART without also checking the init function.

It works just fine.
It returns an expression which first checks that the driver is the right type (the Z_SYSCALL_OBJ() part) and then checks that the dev->config->init function matches (the Z_SYSCALL_VERIFY_MSG() part). Did you not see where it checks the init function??? Help me out here

[pabigot]

#define Z_SYSCALL_SPECIFIC_DRIVER(_device, _dtype, _init_fn) \
        ({ \
                struct device *_dev = (struct device *)_device; \
                Z_SYSCALL_OBJ(_dev, _dtype) || \
                        Z_SYSCALL_VERIFY_MSG(_dev->config->init == _init_fn, \
                                             "init function mismatch"); \
        })

Does that not check the driver is the right type OR that the init function matches?

[andrewboie]

#define Z_SYSCALL_SPECIFIC_DRIVER(_device, _dtype, _init_fn) \
        ({ \
                struct device *_dev = (struct device *)_device; \
                Z_SYSCALL_OBJ(_dev, _dtype) || \
                        Z_SYSCALL_VERIFY_MSG(_dev->config->init == _init_fn, \
                                             "init function mismatch"); \
        })

Does that not check the driver is the right type OR that the init function matches?

They return 0 on success

[pabigot]

OK then. Thanks.