Skip to content

drivers: Move API driver structs to an iterable section #71773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 27, 2024
Merged

drivers: Move API driver structs to an iterable section #71773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b493b66
device: Add device subsystem enumeration script
May 21, 2023
b9a1bf9
device: Add DEVICE_API macros
pdgendt Apr 23, 2024
423972f
cmake: create CMake pre-load file for calling linker script generator
tejlmand Nov 18, 2024
c111c4e
device: export CMake pre-load from device subsystem enumeration script
tejlmand Nov 18, 2024
2165649
tests: kernel: device: Add API sections tests
pdgendt Apr 24, 2024
9b943d3
doc: kernel: drivers: Update with DEVICE_API macros
pdgendt May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ set(SYSCALL_LIST_H_TARGET syscall_list_h_target)
set(DRIVER_VALIDATION_H_TARGET driver_validation_h_target)
set(KOBJ_TYPES_H_TARGET kobj_types_h_target)
set(PARSE_SYSCALLS_TARGET parse_syscalls_target)
set(DEVICE_API_LD_TARGET device_api_ld_target)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in all other places:

Suggested change
set(DEVICE_API_LD_TARGET device_api_ld_target)
set(DRIVER_API_LD_TARGET driver_api_ld_target)

Just an example - see detailed comment for rationale.


define_property(GLOBAL PROPERTY PROPERTY_OUTPUT_FORMAT BRIEF_DOCS " " FULL_DOCS " ")
set_property( GLOBAL PROPERTY PROPERTY_OUTPUT_FORMAT elf32-little${ARCH}) # BFD format
Expand Down Expand Up @@ -882,6 +883,37 @@ add_custom_target(${DRIVER_VALIDATION_H_TARGET} DEPENDS ${DRV_VALIDATION})
include(${ZEPHYR_BASE}/cmake/kobj.cmake)
gen_kobj(KOBJ_INCLUDE_PATH)

# Generate sections for kernel device subsystems
set(
DEVICE_API_LD_SECTIONS
${CMAKE_CURRENT_BINARY_DIR}/include/generated/device-api-sections.ld
)

set(DEVICE_API_LINKER_SECTIONS_CMAKE
${CMAKE_CURRENT_BINARY_DIR}/include/generated/device-api-sections.cmake
)

add_custom_command(
OUTPUT ${DEVICE_API_LD_SECTIONS} ${DEVICE_API_LINKER_SECTIONS_CMAKE}
COMMAND
${PYTHON_EXECUTABLE}
${ZEPHYR_BASE}/scripts/build/gen_iter_sections.py
--alignment ${CONFIG_LINKER_ITERABLE_SUBALIGN}
--input ${struct_tags_json}
--tag __subsystem
--ld-output ${DEVICE_API_LD_SECTIONS}
--cmake-output ${DEVICE_API_LINKER_SECTIONS_CMAKE}
DEPENDS
${ZEPHYR_BASE}/scripts/build/gen_iter_sections.py
${struct_tags_json}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
)

add_custom_target(${DEVICE_API_LD_TARGET}
DEPENDS ${DEVICE_API_LD_SECTIONS}
${DEVICE_API_LINKER_SECTIONS_CMAKE}
)

# Add a pseudo-target that is up-to-date when all generated headers
# are up-to-date.

Expand Down Expand Up @@ -912,6 +944,7 @@ add_dependencies(zephyr_interface
${SYSCALL_LIST_H_TARGET}
${DRIVER_VALIDATION_H_TARGET}
${KOBJ_TYPES_H_TARGET}
${DEVICE_API_LD_TARGET}
)

add_custom_command(
Expand Down
21 changes: 16 additions & 5 deletions cmake/linker/armlink/target.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ macro(configure_linker_script linker_script_gen linker_pass_define)
set(STEERING_FILE_ARG)
set(STEERING_C_ARG)
set(linker_pass_define_list ${linker_pass_define})
set(cmake_linker_script_settings
${PROJECT_BINARY_DIR}/include/generated/ld_script_settings_${linker_pass_define}.cmake
)

if("LINKER_ZEPHYR_FINAL" IN_LIST linker_pass_define_list)
set(STEERING_FILE ${CMAKE_CURRENT_BINARY_DIR}/armlink_symbol_steering.steer)
Expand All @@ -26,21 +29,29 @@ macro(configure_linker_script linker_script_gen linker_pass_define)
set(STEERING_C_ARG "-DSTEERING_C=${STEERING_C}")
endif()

file(GENERATE OUTPUT ${cmake_linker_script_settings} CONTENT
"set(FORMAT \"$<TARGET_PROPERTY:linker,FORMAT>\" CACHE INTERNAL \"\")\n
set(ENTRY \"$<TARGET_PROPERTY:linker,ENTRY>\" CACHE INTERNAL \"\")\n
set(MEMORY_REGIONS \"$<TARGET_PROPERTY:linker,MEMORY_REGIONS>\" CACHE INTERNAL \"\")\n
set(GROUPS \"$<TARGET_PROPERTY:linker,GROUPS>\" CACHE INTERNAL \"\")\n
set(SECTIONS \"$<TARGET_PROPERTY:linker,SECTIONS>\" CACHE INTERNAL \"\")\n
set(SECTION_SETTINGS \"$<TARGET_PROPERTY:linker,SECTION_SETTINGS>\" CACHE INTERNAL \"\")\n
set(SYMBOLS \"$<TARGET_PROPERTY:linker,SYMBOLS>\" CACHE INTERNAL \"\")\n
"
)
add_custom_command(
OUTPUT ${linker_script_gen}
${STEERING_FILE}
${STEERING_C}
COMMAND ${CMAKE_COMMAND}
-C ${DEVICE_API_LINKER_SECTIONS_CMAKE}
-C ${cmake_linker_script_settings}
-DPASS="${linker_pass_define}"
-DMEMORY_REGIONS="$<TARGET_PROPERTY:linker,MEMORY_REGIONS>"
-DGROUPS="$<TARGET_PROPERTY:linker,GROUPS>"
-DSECTIONS="$<TARGET_PROPERTY:linker,SECTIONS>"
-DSECTION_SETTINGS="$<TARGET_PROPERTY:linker,SECTION_SETTINGS>"
-DSYMBOLS="$<TARGET_PROPERTY:linker,SYMBOLS>"
${STEERING_FILE_ARG}
${STEERING_C_ARG}
-DOUT_FILE=${CMAKE_CURRENT_BINARY_DIR}/${linker_script_gen}
-P ${ZEPHYR_BASE}/cmake/linker/armlink/scatter_script.cmake
DEPENDS ${DEVICE_API_LD_TARGET}
)

if("LINKER_ZEPHYR_FINAL" IN_LIST linker_pass_define_list)
Expand Down
25 changes: 17 additions & 8 deletions cmake/linker/ld/target.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,31 @@ endif()
# NOTE: ${linker_script_gen} will be produced at build-time; not at configure-time
macro(configure_linker_script linker_script_gen linker_pass_define)
set(extra_dependencies ${ARGN})
set(cmake_linker_script_settings
${PROJECT_BINARY_DIR}/include/generated/ld_script_settings_${linker_pass_define}.cmake
)

if(CONFIG_CMAKE_LINKER_GENERATOR)
file(GENERATE OUTPUT ${cmake_linker_script_settings} CONTENT
"set(FORMAT \"$<TARGET_PROPERTY:linker,FORMAT>\" CACHE INTERNAL \"\")\n
set(ENTRY \"$<TARGET_PROPERTY:linker,ENTRY>\" CACHE INTERNAL \"\")\n
set(MEMORY_REGIONS \"$<TARGET_PROPERTY:linker,MEMORY_REGIONS>\" CACHE INTERNAL \"\")\n
set(GROUPS \"$<TARGET_PROPERTY:linker,GROUPS>\" CACHE INTERNAL \"\")\n
set(SECTIONS \"$<TARGET_PROPERTY:linker,SECTIONS>\" CACHE INTERNAL \"\")\n
set(SECTION_SETTINGS \"$<TARGET_PROPERTY:linker,SECTION_SETTINGS>\" CACHE INTERNAL \"\")\n
set(SYMBOLS \"$<TARGET_PROPERTY:linker,SYMBOLS>\" CACHE INTERNAL \"\")\n
"
)
add_custom_command(
OUTPUT ${linker_script_gen}
COMMAND ${CMAKE_COMMAND}
-C ${DEVICE_API_LINKER_SECTIONS_CMAKE}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in principle not fond of relying on outer scoped CMake variables inside macros or functions.

However the configure_linker_script is an old macro which has been doing so from early days.

Also relying on DEVICE_API_LINKER_SECTIONS_CMAKE is similar in nature to common-rom-kernel-devices.ld relying on the device-api-sections.ld (CMake var: DEVICE_API_LD_SECTIONS) file created by CMake through calling gen_iter_sections.py.
https://github.com/zephyrproject-rtos/zephyr/blob/d8399fc9e03719439dbb63924ca5f5b2f26f8fa4/include/zephyr/linker/common-rom/common-rom-kernel-devices.ld#L106

-C ${cmake_linker_script_settings}
-DPASS="${linker_pass_define}"
-DFORMAT="$<TARGET_PROPERTY:linker,FORMAT>"
-DENTRY="$<TARGET_PROPERTY:linker,ENTRY>"
-DMEMORY_REGIONS="$<TARGET_PROPERTY:linker,MEMORY_REGIONS>"
-DGROUPS="$<TARGET_PROPERTY:linker,GROUPS>"
-DSECTIONS="$<TARGET_PROPERTY:linker,SECTIONS>"
-DSECTION_SETTINGS="$<TARGET_PROPERTY:linker,SECTION_SETTINGS>"
-DSYMBOLS="$<TARGET_PROPERTY:linker,SYMBOLS>"
-DOUT_FILE=${CMAKE_CURRENT_BINARY_DIR}/${linker_script_gen}
-P ${ZEPHYR_BASE}/cmake/linker/ld/ld_script.cmake
)
DEPENDS ${DEVICE_API_LD_TARGET}
)
else()
set(template_script_defines ${linker_pass_define})
list(TRANSFORM template_script_defines PREPEND "-D")
Expand Down
4 changes: 2 additions & 2 deletions cmake/linker/linker_script_common.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ function(create_section)
set(INDEX 100)
set(settings_single "ALIGN;ANY;FIRST;KEEP;OFFSET;PRIO;SECTION;SORT")
set(settings_multi "FLAGS;INPUT;PASS;SYMBOLS")
foreach(settings ${SECTION_SETTINGS})
foreach(settings ${SECTION_SETTINGS} ${DEVICE_API_SECTION_SETTINGS})
if("${settings}" MATCHES "^{(.*)}$")
cmake_parse_arguments(SETTINGS "" "${settings_single}" "${settings_multi}" ${CMAKE_MATCH_1})

Expand Down Expand Up @@ -652,7 +652,7 @@ foreach(group ${GROUPS})
endif()
endforeach()

foreach(section ${SECTIONS})
foreach(section ${SECTIONS} ${DEVICE_API_SECTIONS})
if("${section}" MATCHES "^{(.*)}$")
create_section(${CMAKE_MATCH_1} SYSTEM ${new_system})
endif()
Expand Down
37 changes: 21 additions & 16 deletions doc/kernel/drivers/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,24 @@ The following APIs for device drivers are provided by :file:`device.h`. The APIs
are intended for use in device drivers only and should not be used in
applications.

:c:func:`DEVICE_DEFINE()`
:c:macro:`DEVICE_DEFINE()`
Create device object and related data structures including setting it
up for boot-time initialization.

:c:func:`DEVICE_NAME_GET()`
:c:macro:`DEVICE_NAME_GET()`
Converts a device identifier to the global identifier for a device
object.

:c:func:`DEVICE_GET()`
:c:macro:`DEVICE_GET()`
Obtain a pointer to a device object by name.

:c:func:`DEVICE_DECLARE()`
:c:macro:`DEVICE_DECLARE()`
Declare a device object. Use this when you need a forward reference
to a device that has not yet been defined.

:c:macro:`DEVICE_API()`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:c:macro:`DEVICE_API()`
:c:macro:`DRIVER_API()`

See the inconsistency with the explanatory text, too.

Wrap a driver API declaration to assign it to its respective linker section.

.. _device_struct:

Driver Data Structures
Expand All @@ -97,8 +100,8 @@ split into read-only and runtime-mutable parts. At a high level we have:
struct device {
const char *name;
const void *config;
const void *api;
void * const data;
const void *api;
void * const data;
};

The ``config`` member is for read-only configuration data set at build time. For
Expand All @@ -122,36 +125,38 @@ Most drivers will be implementing a device-independent subsystem API.
Applications can simply program to that generic API, and application
code is not specific to any particular driver implementation.

If all driver API instances are assigned to their respective API linker section
use :c:macro:`DEVICE_API_IS()` to verify the API's type.

A subsystem API definition typically looks like this:

.. code-block:: C

typedef int (*subsystem_do_this_t)(const struct device *dev, int foo, int bar);
typedef void (*subsystem_do_that_t)(const struct device *dev, void *baz);

struct subsystem_api {
__subsystem struct subsystem_driver_api {
Copy link

@ghost ghost Oct 1, 2024 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance this could be __driver_api __subsystem and driver_subsystem_api for the new approach? Should be easy to add this to parse_syscalls.py:

  • This has probably always been confusing a misnomer as our users tend to call subsystem colloquially what is below the subsys folder and we had device.api before that. We could maybe use this opportunity to make nomenclature gradually more consistent w/o a treewide change? UPDATE: We have driver subsystem APIs and "higher" (in need of a better term) subsystem APIs. So we should probably call all occurrences DRIVER_SUBSYSTEM_API or at least have the common understanding that DRIVER_API is a specialization of a subsystem API (i.e. hence why I propose to combine the __driver_api __subsystem tags here).
  • Introduces a soft migration path w/o forcing everybody into the new approach - scripting could rely on a unique tag and existing __subsystem markers would not have to be changed, especially not for out-of-tree solutions.

Side notes:

  • Actually it should be device.driver_api as this is not the API of the device but of the driver that has been assigned to the device. This is probably what has caused the naming confusion in the first place.
  • parse_syscalls.py should be renamed to parse_macro_tags.py or similar and all syscall-specific concerns should be extracted into its own artifact (class, module, whatever is appropriate).
  • The subsys folder should urgently be renamed to distinguish it from driver subsystems and avoid the common confusion that driver APIs are not exposing a subsystem, too, which I also fell for. I can't think of a good name right now, but I'll keep my eyes open.

UPDATED to account for the proper definition(s) of subsystem, see the related discussion in #71090.

subsystem_do_this_t do_this;
subsystem_do_that_t do_that;
};

static inline int subsystem_do_this(const struct device *dev, int foo, int bar)
{
struct subsystem_api *api;
__ASSERT_NO_MSG(DEVICE_API_IS(subsystem, dev));

api = (struct subsystem_api *)dev->api;
return api->do_this(dev, foo, bar);
return DEVICE_API_GET(subsystem, dev)->do_this(dev, foo, bar);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just hiding that everything is a void*, which again wouldn't actually be a void* if we had proper typing of the devices

Copy link
Member

@cfriedt cfriedt Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessarily type checking per-se (any pointer can be cast to one of the expected type to get around type checking at the compiler level).

It's more like ensuring that a Thing is in fact a Thing by checking that a given Thing pointer is a member of the array of all Things, which is pretty hard to argue with (in terms of static RTOS behaviour).

Drivers loaded at runtime really should also get the same kind of check as a kind of minimal security practice.

Currently with userspace, the kernel validates all device API pointer arguments as being readable or writeable memory regions. A similar check is done for permissions on kernel objects. Somehow we completely bypass any checks whatsoever for (arguably) the most important pointer in a device API call (the const struct device *dev), which makes it trivial to trigger faults (or worse - not trigger faults but have unpredictable side-effects) in syscall context.

Do we already support drivers loaded at runtime? If so, I think that means they implicitly rely on this (IMHO) hole in security.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more like ensuring that a Thing is in fact a Thing by checking that a given Thing pointer is a member of the array of all Things, which is pretty hard to argue with (in terms of static RTOS behaviour).

Right, which could be done by the compiler if we had proper types instead of void*'s, which is the right place to do this. Unless your talking about userspace where you have untrusted code perhaps? In that case sure, but then this sort of thing should be done in z_vrfy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely userspace, and agree about z_vrfy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding runtime loadable drivers, we are already boxed in quite substantionally. We don't have a way of storing the devicetree in ROM, so drivers have to be compiled in, and "switched between" at runtime. Given that these drivers are compiled in, their APIs would be placed in the correct API sections as well :) Unless by runtime loadable you mean "truly" runtime loadable, I don't see this PR as any more of a box :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear here, my main complaint is this is kicking the can down the road of using proper types a compiler will yell about and instead leaving this to a runtime check that I guess might blow up at any random time while firmware is running.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this PR as an improvement to the status quo, not really a new feature. Kicking the can down the road would IMO imply we are breaking the status quo with an incomplete, short-term solution, which I just don't think describes this PR.

}

static inline void subsystem_do_that(const struct device *dev, void *baz)
{
struct subsystem_api *api;
__ASSERT_NO_MSG(DEVICE_API_IS(subsystem, dev));

api = (struct subsystem_api *)dev->api;
api->do_that(dev, baz);
DEVICE_API_GET(subsystem, dev)->do_that(dev, baz);
}

A driver implementing a particular subsystem will define the real implementation
of these APIs, and populate an instance of subsystem_api structure:
of these APIs, and populate an instance of subsystem_driver_api structure using
the :c:macro:`DEVICE_API()` wrapper:

.. code-block:: C

Expand All @@ -165,9 +170,9 @@ of these APIs, and populate an instance of subsystem_api structure:
...
}

static struct subsystem_api my_driver_api_funcs = {
static DEVICE_API(subsystem, my_driver_api_funcs) = {
.do_this = my_driver_do_this,
.do_that = my_driver_do_that
.do_that = my_driver_do_that,
};

The driver would then pass ``my_driver_api_funcs`` as the ``api`` argument to
Expand Down
39 changes: 39 additions & 0 deletions include/zephyr/device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,8 +1195,47 @@ device_get_dt_nodelabels(const struct device *dev)

DT_FOREACH_STATUS_OKAY_NODE(Z_MAYBE_DEVICE_DECLARE_INTERNAL)

/** @brief Expands to the full type. */
#define Z_DEVICE_API_TYPE(_class) _CONCAT(_class, _driver_api)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the TYPE suffix if anyhow possible. Type is so overloaded with meaning...

Suggested change
#define Z_DEVICE_API_TYPE(_class) _CONCAT(_class, _driver_api)
#define Z_DRIVER_API_SECTION_NAME(_driver_api_name) _CONCAT(_driver_api_name, _driver_api)


/** @endcond */

/**
* @brief Wrapper macro for declaring device API structs inside iterable sections.
*
* @param _class The device API class.
Copy link

@ghost ghost Oct 3, 2024 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param _class The device API class.
* @param _driver_api_name The name of the driver subsystem API. Usually this is the device class (adc, uart, i2c, ...) of the devices being exposed by this driver subsystem. If you define complex drivers that may wrap devices of different classes (rtc, stepper, ieee802154_radio), during migration (usb vs. usb_next) or if you implement a custom API for the same device class (adc_<vendor/project>), then we recommend to pre-/suffix the device class or name the driver subsystem according to its purpose to designate the API variant uniquely.

@gmarull It turns out that this is one thing where we have differing opinions. Can you see my point why we cannot always equate device classes and APIs precisely although they are of course very closely related?

I assume you don't have really strong feelings about placing the macros inside devices vs. drivers? I do - so it would be great if we could go that way.

* @param _name The API instance name.
*/
#define DEVICE_API(_class, _name) const STRUCT_SECTION_ITERABLE(Z_DEVICE_API_TYPE(_class), _name)

/**
* @brief Expands to the pointer of a device's API for a given class.
*
* @param _class The device API class.
* @param _dev The device instance pointer.
*
* @return the pointer to the device API.
*/
#define DEVICE_API_GET(_class, _dev) ((const struct Z_DEVICE_API_TYPE(_class) *)_dev->api)

/**
* @brief Macro that evaluates to a boolean that can be used to check if
* a device is of a particular class.
*
* @param _class The device API class.
* @param _dev The device instance pointer.
*
* @retval true If the device is of the given class
* @retval false If the device is not of the given class
*/
#define DEVICE_API_IS(_class, _dev) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still find this macro misleading, maybe DEVICE_CLASS_IS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer the DEVICE_API common prefix, anyone else pro/con the current naming?

Copy link

@ghost ghost Oct 1, 2024 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of both? I propose to call this "DRIVER_API" or "DRIVER_SUBSYSTEM_API" consistently. And we must not refer to _class either: s/_class/_driver_api_name/g

IMO we have:

                                             subsystem API
                                                  ^
device <- n-to-m -> device class <- n-to-m -> driver API
   ^-----------------^---1-to-n---> driver <--n-to-1---^

Explanation:

  • In my understanding, anything named device... is some kind of hardware abstraction, anything named driver... is some kind of software abstraction. Therefore I'd define based on existing Zephyr usage...
  • A device abstracts a specific hardware programming model (e.g. a specific kind of peripheral usually of one vendor).
  • A device class abstracts common features and programming models across devices, a single device can belong to several device classes. This may be a controversial definition as in the context of a single OS, there will often be a single API per device class (see next point, though).
  • A driver subsystem API abstracts a driver programming model. Zephyr (more so than Linux) may want to support several driver subsystem APIs per device or device class in parallel (custom vs. in-tree, in-tree during migrations, see old vs. new USB API, perspectively even co-existing in one build). A single device subsystem API may abstract over several device classes (see counter vs. rtc devices for RTC).
  • A driver abstracts software that bridges the impedance mismatch between a device or device class and a driver API. This means that we can (but don't have to) support up to n x m drivers for n devices/device classes and m driver APIs.
  • A subsystem is any user-facing Zephyr software module identified by an abstract subsystem API. The driver subsystem API entity is a specialization of the subsystem API entity (i.e. subsystem API <- 1-to-0...1 -> driver API).
  • The type suffix should be generally avoided IMO. It matches on too many occasions and therefore provokes misunderstandings rather than good inter-subjective associations.

Given these definitions, what we're trying to name here would be a "driver subsystem API" if I'm not mistaken. I'm fine shortening this to "driver API" as long as we keep in mind that a "driver API" is a (as opposed to has a) subsystem API.

UPDATED to account for the proper definition of subsystem. See #71090.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually thinking more about this, there is a 1-to-1 relation between a device and its driver API. It's an opaque type (which I'm trying to figure out at runtime with this PR) but can only serve one purpose.

You're not allowed to pass the same device instance to different API layers, multi-purpose drivers (your n-to-m ?) need to instantiate multiple devices each with a different API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, every driver implementation is creating devices instances (DEVICE_DT_DEFINE) passing the opaque api type, so for me, DEVICE_API is more suited to passed as an argument there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I've not questioned that. That's beside the point, though.

({ \
STRUCT_SECTION_START_EXTERN(Z_DEVICE_API_TYPE(_class)); \
STRUCT_SECTION_END_EXTERN(Z_DEVICE_API_TYPE(_class)); \
(DEVICE_API_GET(_class, _dev) < STRUCT_SECTION_END(Z_DEVICE_API_TYPE(_class)) && \
DEVICE_API_GET(_class, _dev) >= STRUCT_SECTION_START(Z_DEVICE_API_TYPE(_class))); \
})

#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 2 additions & 0 deletions include/zephyr/linker/common-rom/common-rom-kernel-devices.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,5 @@
#include <zephyr/linker/device-deps.ld>
} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)
#endif /* !CONFIG_DEVICE_DEPS_DYNAMIC */

#include <device-api-sections.ld>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <device-api-sections.ld>
#include <driver-api-sections.ld>

73 changes: 73 additions & 0 deletions scripts/build/gen_iter_sections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#!/usr/bin/env python3
#
# Copyright (c) 2023 Bjarki Arge Andreasen
#
# SPDX-License-Identifier: Apache-2.0
"""
Script to generate iterable sections from JSON encoded dictionary containing lists of items.
"""

import argparse
import json


def get_tagged_items(filepath: str, tag: str) -> list:
with open(filepath) as fp:
return json.load(fp)[tag]


def gen_ld(filepath: str, items: list, alignment: int):
with open(filepath, "w") as fp:
for item in items:
fp.write(f"ITERABLE_SECTION_ROM({item}, {alignment})\n")


def gen_cmake(filepath: str, items: list, alignment: int):
with open(filepath, "w") as fp:
for item in items:
fp.write(
f'list(APPEND sections "{{NAME\\;{item}_area\\;'
+ 'GROUP\\;RODATA_REGION\\;'
+ f'SUBALIGN\\;{alignment}\\;'
+ 'NOINPUT\\;TRUE}")\n'
)
fp.write(
f'list(APPEND section_settings "{{SECTION\\;{item}_area\\;'
+ 'SORT\\;NAME\\;'
+ 'KEEP\\;TRUE\\;'
+ f'INPUT\\;._{item}.static.*\\;'
+ f'SYMBOLS\\;_{item}_list_start\\;_{item}_list_end}}")\n'
)
fp.write('set(DEVICE_API_SECTIONS "${sections}" CACHE INTERNAL "")\n')
fp.write('set(DEVICE_API_SECTION_SETTINGS "${section_settings}" CACHE INTERNAL "")\n')


def parse_args() -> argparse.Namespace:
parser = argparse.ArgumentParser(
description=__doc__,
formatter_class=argparse.RawDescriptionHelpFormatter,
allow_abbrev=False,
)

parser.add_argument("-i", "--input", required=True, help="Path to input list of tags")
parser.add_argument("-a", "--alignment", required=True, help="Iterable section alignment")
parser.add_argument("-t", "--tag", required=True, help="Tag to generate iterable sections for")
parser.add_argument("-l", "--ld-output", required=True, help="Path to output linker file")
parser.add_argument(
"-c", "--cmake-output", required=True, help="Path to CMake linker script inclusion file"
)

return parser.parse_args()


def main():
args = parse_args()

items = get_tagged_items(args.input, args.tag)

gen_ld(args.ld_output, items, args.alignment)
gen_cmake(args.cmake_output, items, args.alignment)


if __name__ == "__main__":
main()
1 change: 1 addition & 0 deletions tests/kernel/device/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ CONFIG_ZTEST=y
CONFIG_TEST_USERSPACE=y
CONFIG_PM_DEVICE=y
CONFIG_PM_DEVICE_RUNTIME=y
CONFIG_APPLICATION_DEFINED_SYSCALL=y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise the __syscall and __subsystem tags aren't found for the abstract driver, needed for the gen_iter_sections.py script.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be redundant when using zephyr_syscall_header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to build without it results in:

In file included from /home/pdgendt/dev/zephyr-main/zephyr/tests/kernel/device/src/main.c:14:
/home/pdgendt/dev/zephyr-main/zephyr/tests/kernel/device/src/abstract_driver.h:46:10: fatal error: syscalls/abstract_driver.h: No such file or directory
   46 | #include <syscalls/abstract_driver.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could remove the zephyr_syscall_header call in cmake though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I think it's a good idea to decouple syscall-specific logic from the generic "tag parser" python script. See my proposal to rename that script to what it is actually used for - part of the same problem.

Loading
Loading