Driver APIs and Thread-Safety

[ldomaigne]

Introduction

The following RFC is the result of interactions with Zephyr adopters, some being documented on Discord .

As an application/product developer, I need to know which driver-APIs are thread-safe and which are not. Ideally, I don’t want to dig the driver code to figure this out.

I am aware that I might not have all relevant information, and that this RFC might need further refinement. The goal is to bring this topic back into awareness, and to take it to the architecture WG for discussion.

Problem description

There has been multiple RFC/PR/issues on the subject, many of which are still open. To name of few:

• Issue 18970
• PR 20959
• Issue 26073

Based on @carlescufi answer:

Unfortunately, we never really got very far in documenting this [thread-safety] to the user, and as you describe we have very high variability in the different drivers. There was an attempt here: https://github.com/zephyrproject-rtos/zephyr/pull/21678/files, but it's not quite the same

For a driver class, we should have a consistent behavior for the API interface.

Proposed change

We need to cope both with the existing code base, and with future development, while keeping the progress manageable. We therefore suggest the following approaches:

• A. Specify API Design guidelines wrt. to thread-safety applicable to any new driver class.
• B. Document existing driver classes.

Detailed RFC

A. API Design guidelines

• Specify guidelines/rules for new driver class with respect to thread safety. Update API Design Guidelines documentation accordingly.
• Ensure that these rules are enforced through automated or manual checks (code review).

B. Document existing driver classes

• Proof-of-concept: document on a few selected driver classes.
• gradual documentation: document class by class. Depending on the efforts involved, this might be done batch-wise across several Zephyr releases.

A. and B. can be completed independently. Task A ensures do the "right thing" with respect to thread-safety for any new development. Task B takes care of bridging any gaps in the current code base.

Proposed change (Detailed)

We follow the definition proposed by @pabigot for thread-safety:

Thread-safety: a function is thread-safe if its behavior is correct when invoked from multiple threads at the same time

Driver may choose the following design approaches:

• Fully Thread-Safe Drivers: All API functions are safe to call from any thread, and the driver handles all necessary locking internally. Pro: easy for user, safe. Cons: potential performance penalty.
• Externally Synchronized Drivers: The driver expects the caller to ensure that only one thread accesses the driver at a time, or to use external synchronization mechanisms. Pro: High performance possible. Cons: More complex for users.
• Hybrid Approaches: Some APIs may be thread-safe for certain operations but require external synchronization for others, especially during initialization or configuration. Pro: blanced, flexible. Cons: need to be documented accordingly.

For a driver class, this is more subtle. We should have a consistent behaviour for the API interface. This should be at least true for the mandatory APIs.

Following API meeting from April 2022, see this comment:

• Device APIs' thread-safety shall be enforced and documented per device class
• Thread safety guarantees shall be the same for all device classes that offer them (priority inheritance for example)
• Synchronization shall be disabled when CONFIG_MULTITHREADING is n

It is not clear from RFC 26073 if this decision is in place or not.

Dependencies

• A. impacts future development, and therefore has no dependencies.
• B. is about documenting existing "IS-state". We could be discover some inconsistency in some driver interface wrt. to thread-safety. Possible resolution would have to take place on a case-by-case basis.

Concerns and Unresolved Questions

• It is not clear if the decision from the API meeting mentioned above is being followed or not.
• Driver class mandatory should exhibit the same thread-safety behavio for mandatory API. TBD: how about the optional APIs ?
• There have been multiple RFCs, issues, and PRs on this subject, many of which remain open. The task is daunting, and we risk not completing it. This is likely a marathon, not a sprint!

Alternatives

See the RFC mentioned on the subject above. This RFC tries to keep the effort manageable in the long run, while ensuring steady progress/ROI.

[github-actions]

Hi @ldomaigne! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[bjarki-andreasen]

I believe we should go way simpler: Don't do any thread safety within any device driver (ISR context excluded), period. Why?

• Most, if not all devices can not be shared safely. They are the lowest level construct, and as such, have the least amount of
  knowledge of how they are being used. Best they can do is prevent a single API call from being reentered, something that
  almost never happens outside of bus transfer APIs
• Busses which are to be shared, need to have all APIs synced, like i2c_configure, followed by an i2c_transfer. Drivers can't do
  anything here but pray they where called in appropriate order by appropriate context.
• Case in point: To share busses, we now have RTIO, which removes all needs for sync from device drivers (addressing the i2c sync issue above)
• Sync is probably the biggest source of evil, nasty bugs, better to move it to a higher layer, where it can be reused and only optionally included.

Drivers should be the dumbest, simplest part of the system, if application chooses to use threads, application gets to deal with it, this allows drivers to be reused for bootloaders, makes them way less buggy, and forces applications and subsystems to actually deal with sync, no hoping that a driver can somewhat survive being bashed with API calls from all angles :D

[carlescufi]

@ldomaigne How about next week (June 10th) for this topic?

[ldomaigne]

@ldomaigne How about next week (June 10th) for this topic?

@carlescufi : doesn't work for me, but the week after (June 17th) does.

[carlescufi]

@ldomaigne How about next week (June 10th) for this topic?

@carlescufi : doesn't work for me, but the week after (June 17th) does.

Settled then @ldomaigne, the 17th.

[ldomaigne]

Hi @bjarki-andreasen ,

thanks for your comments, very insightful.

The main motivation of this RFC is to get consistency across the drivers, and to communicate it to the user. It's not about making everything thread-safe (perhaps the RFC title chosen is somewhat unfortunate...).

I believe we should go way simpler: Don't do any thread safety within any device driver (ISR context excluded), period. Why?

Yeah, that's the option "externally synchronized driver" mentioned in the proposal.

I had the feeling that certain drivers class would be "naturally" thread safe. And so we could go a mixed-approach (but still, making sure it's consistent across the driver class). It's possibly just plain wrong.

I have one question about your proposal. Are there any implications if user mode threads are used?

[bjarki-andreasen]

Hi @bjarki-andreasen ,

thanks for your comments, very insightful.

The main motivation of this RFC is to get consistency across the drivers, and to communicate it to the user. It's not about making everything thread-safe (perhaps the RFC title chosen is somewhat unfortunate...).

I believe we should go way simpler: Don't do any thread safety within any device driver (ISR context excluded), period. Why?

Yeah, that's the option "externally synchronized driver" mentioned in the proposal.

I had the feeling that certain drivers class would be "naturally" thread safe. And so we could go a mixed-approach (but still, making sure it's consistent across the driver class). It's possibly just plain wrong.

I have one question about your proposal. Are there any implications if user mode threads are used?

Not any the drivers can do anything about :) We would need to rework our syscalls A LOT to prevent concurrent access to hardware from usermode, like adding some "ownership" model where usermode would need to "reserve" some hardware before calling any device driver APIs through syscalls, something like

/* get handle that can be used with "safe" syscalls */
int dev_handle = device_user_reserve(DEVICE_DT_GET(DT_NODELABEL(rtc)));
/* use handle with special "safe" syscall which checks ownership and calls rtc_get_time() internally */
int ret = user_rtc_get_time(dev_handle, struct rtc_time *timeptr);
/* let go of device for some other thread to use device */
device_user_release(dev_handle);

I think this goes ways beyond what device drivers should need to deal with :)

[ldomaigne]

I think this goes ways beyond what device drivers should need to deal with :)

Yeah, I agree.
It's OK to shift that responsibility to the application. :)

For products subject to safety requirements (the word "safety" here has to be understood as in "safety working group"), it means we have to do something about it. While certain applications will have specific requirements and implement their own sync mechanisms, I was wondering if some common mechanisms could be provided for the most common use cases. (Just a thought at that point, I don't have the answer yet).