Change DCT read access to lock()/unlock() or read-with-copy

[avtolstoy]

Do not merge. Review only. Superseded by #1320

Problem

When using dct_read_app_data the returned pointer might become invalid in case a different thread writes into DCT causing a DCT swap.

Photon/P1 should have a critical section around flash operations and around DCT operations.

Solution

This branched is based on #1295 and then rebased on #1289.

• Changes DCT read access to lock()/unlock() or read-with-copy. New methods introduced: dct_read_app_data_lock(), dct_read_app_data_unlock(), dct_read_app_data_copy().
• Adds atomic flag mutex for DCT read/write operations on Photon/P1 (see PR#8 in WICED repo)
• Adds atomic flag mutex for flash operations on Photon/P1 (see PR#8 in WICED repo)

NOTE: Photon/P1 bootloader builds should overflow. It's intended, as we are removing DCT implementation out of it and importing it from system-part2 instead in https://github.com/spark/firmware/tree/feature/bootloader_dct

Steps to Test

N/A

Example App

N/A

References

• photon-wiced: PR#8
• v2 dcd format and tests #1295
• WPA/WPA2 Enterprise support [ch1317] #1289

---

Completeness

• User is totes amazing for contributing!
• Contributor has signed CLA (Info here)
• Problem and Solution clearly stated
• Run unit/integration/application tests on device
• Added documentation
• Added to CHANGELOG.md after merging (add links to docs and issues)

Enhancement

• [PR #1307] [Photon/P1] New version of WICED adds CRC checking to the DCT implementation so that write errors are detected. Added a critical section around flash operations and around DCT operations to make them thread safe.

[sergeuz]

Let's simplify this:

return ((const char*)wiced_dct_get_current_address(DCT_APP_SECTION) + offset);

I know this function is going to be deprecated, but still, how about adding a critical section here, just to avoid concurrent initialization/swapping of the DCTs?

[sergeuz]

I would split this into two functions or at least add a comment to improve readability

fetch_device_private_key(1); // Lock key data
fetch_device_private_key(0); // Unlock key data

[sergeuz]

dct_read_app_data_unlock() is missing here. I think we need to implement a RAII-based locker for DCT and avoid using dct_read_app_data_lock/unlock() in C++ code whenever possible

[sergeuz]

code is dereferenced after unlocking DCT, later in this function:

if (code[0] == 0xFF || code[0] == 0)
...

[sergeuz]

Is it safe that a lot of WICED's stuff gets called while DCT is locked?

[sergeuz]

Unnecessary copying

[sergeuz]

Should we add HAL_IsISR() check here?

[sergeuz]

How about adding a separate function, e.g. init_device_public_key(), and making fetch_device_public_key() to call it internally? These calls look weird :)

[sergeuz]

I would like to see this AtomicFlagMutex implementation supporting recursive locking, possibly in next versions of the firmware. For example:

void lock() {
    acquire_flag();
    if (!mutex_is_initialized && mutex_can_be_initialized) { // Heap is initialized, etc.
        init_recursive_mutex();
        mutex_is_initialized = true;
    }
    if (mutex_is_initialized) {
        lock_mutex();
        release_flag();
    }
}

void unlock() {
    if (mutex_is_initialized) {
        unlock_mutex();
    } else {
        release_flag();
    }
}

[avtolstoy]

How about something like this?

template <typename R, R(*spin)()>
class AtomicRecursiveMutex {
    std::atomic<std::thread::id> owner;
    std::atomic_int refcount{0};
public:
    int lock() {
        SPARK_ASSERT(!HAL_IsISR());
        std::thread::id no_id = std::thread::id();
        std::thread::id self = std::this_thread::get_id();
        while ((no_id != std::thread::id() && no_id != self) || !owner.compare_exchange_weak(no_id, self)) {
            if (no_id == self) {
                // To avoid spinning
                break;
            }
            else if (no_id != std::thread::id()) {
                no_id = std::thread::id();
            }
            spin();
        }
        return ++refcount;
    }

    int unlock() {
        SPARK_ASSERT(!HAL_IsISR());
        SPARK_ASSERT(owner.load() == std::this_thread::get_id());
        int c = --refcount;
        SPARK_ASSERT(c >= 0);
        if (c == 0) {
            // Unlock
            owner.store(std::thread::id());
        }
        return c;
    }
};

We can also add additional assertions to wiced_dct_lock/wiced_dct_unlock that will help us track issues with such an implementation.

wiced_result_t wiced_dct_lock(int exclusive)
{
    SPARK_ASSERT(!HAL_IsISR());
    int refcount = dctLock.lock();
    SPARK_ASSERT(!exclusive || refcount == 1);
    return WICED_SUCCESS;
}

wiced_result_t wiced_dct_unlock(int exclusive)
{
    SPARK_ASSERT(!HAL_IsISR());
    int refcount = dctLock.unlock();
    SPARK_ASSERT(!exclusive || refcount == 0);
    return WICED_SUCCESS;
}

exclusive would be set to 1 when writing to dct, as we cannot guarantee data validity after a dct sector swap.

[sergeuz]

Nice! 👍 I've just checked other places where we use non-copying variant of wiced_dct_read_lock(), and haven't found anything suspicious.

[technobly]

Closing due to changes being in released/v0.7.0-rc.1