reentrancy protection in counter drivers?

[pabigot]

While implementing a new counter driver for the Maxim DS3231, I noticed that support for safe use in a multithreaded environment is pretty much missing: except in qmsi where it's optional nothing prevents one thread from being swapped out in mid operation, or invocation from an interrupt.

Is this a problem?

[pabigot]

Let me give a little more context for this:

I'm working on a driver for the DS3231. This is an RTC, so to Zephyr it's a counter.

It's also an I2C device, so access to it will require blocking operations, and so at least some of its internal state must be protected in case a read-modify-write operation with I2C is in progress from one thread when another tries to read the counter. So this driver will require reentrancy protection.

Also this may be the first counter driver where reading the counter is a blocking operation with an uncontrollable duration. The counter cannot be read from an ISR, though there will be an extended function that supports back-calculating the corresponding value given a capture of the cycle counter.

[nordic-krch]

i would say that for function like counter_read it should be on counter side to handle reading from multiple contexts. For other function i'm not sure. If counter has one 'owner' (the one that configures, starts and stops the counter) then he should be able to manage the access.

I think that counter_set_channel_alarm is missing -EBUSY when channel is occupied. Integrity within api calls and interrupt should be handled internally by the driver (probably none is doing it now).

I would prefer to not use os infrastructure in the driver which would prevent calls from interrupt context. I've already seen it in couple of places and one particular case (nrf internal temperature sensor as sensor device) made me trouble.

[pabigot]

My main point was that in the past Zephyr has followed a policy that drivers shall be re-entrant (#1960). The counter API does not document that it's an exception, but no implementation is, in fact, re-entrant.

If a counter has multiple channels it may have a single owner, but the individual channels may be used by different subsystems that cannot easily coordinate their changes (some of which may be to the counter itself). Then the counter driver would have to protect against conflicts.

I would prefer to not use os infrastructure in the driver which would prevent calls from interrupt context. I've already seen it in couple of places and one particular case (nrf internal temperature sensor as sensor device) made me trouble.

I sympathize with this position but it's not sustainable if the counter API is to be used with out-of-SOC capabilities like RTC chips.

One possibility would be a documentation fix, stating that reading a counter from interrupt context is acceptable unless the specific driver documents that it does not support it. In that case the driver should return a documented error code like -EDEADLK. (Except that the API doesn't allow for failure to read the counter, which is a problem with the external counter hardware where reads may well fail.)

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.