kernel: Allow to use k_sem_take() without MULTITHREADING

[Olivier-ProGlove]

There is no thread queue if CONFIG_MULTITHREADING=n is set.

With the current implementation, _impl_k_sem_take() calls _pend_current_thread() that moves the current thread in the waiting queue.
But when there is no MULTITHREADING support, there is only one thread.

This change implements _impl_k_sem_take() as a busy loop when no MULTITHREADING support.
It means the semaphore would have to be released in the ISR to exit the loop.

This issue has been triggered while porting MCUBoot (no multithreading support) to my platform. Some drivers needed during the boot process were using semaphore.

Signed-off-by: Olivier Martin olivier.martin@proglove.com

[andyross]

So, you want to use semaphores so you can wait for an interrupt delivery? Why can't you just spin in the app?

This seems like a lot of code to do just that. CONFIG_MULTITHREADING is maybe poorly defined, but it's not really intended that IPC primitives work there. The idea was to have an absolutely minimal core of bootstrap code to permit writing things like bootloaders. Semaphores don't really fit that model.

[mike-scott]

@Olivier-ProGlove which driver is mcuboot using that causes the issue?

[codecov-io]

Codecov Report

Merging #8368 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8368   +/-   ##
=======================================
  Coverage   64.61%   64.61%           
=======================================
  Files         422      422           
  Lines       40284    40284           
  Branches     6800     6800           
=======================================
  Hits        26030    26030           
  Misses      11121    11121           
  Partials     3133     3133

Impacted Files	Coverage Δ
kernel/sem.c	91.66% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f3fea3...341870a. Read the comment docs.

[Olivier-ProGlove]

@andyross As you might know MCUBoot bootloader has been ported to Zephyr. And Zephyr drivers can be used by the bootloader without any changes which is really great! But MCUBoot does not support multi-threading.

When I ported MCUBoot to our platform, the bootloader was failing to start. After investigating the issue I discovered it was due to a semaphore used by one of our drivers.

At that time, I have not thought if the semaphore was relevant or not in our driver. But I was thinking semaphores could be used at other places in the code path and I did not want to redesign all our drivers. That the reason of my patch.

If we do not want to enable semaphore in a multithreaded environment then we should protect the semaphore API with:

#if !CONFIG_MULTITHREADING
(...)
#endif /* CONFIG_MULTITHREADING */

@mike-scott sorry I do not remember which drivers was it but I can check if you want even if I think it is one of our internal drivers.

[JoeHut]

@mike-scott Most of the flash drivers use semaphores. In our case it was the winbound driver (w25qxxdv) but if you do a grep for k_sem you can see that qmsi, sam0, nrf, stm32, mcux, nios2_qspi also use semaphores for syncing.

@carlescufi This is the patch we talked about on IRC

@andyross flash drivers are essential for bootloaders, so I think we either have to live with semaphores in the minimal core or the drivers have to be changed accordingly.

[nashif]

@JoeHut if there are no ram/rom constraints, you should be able to build mcuboot with multithreading, single thread was added just to minimise footprint. my concern is that this might not end with semaphores and other object will be pulled in to support other drivers, so single thread and the need for it becomes useless.

[andyross]

But semaphores don't actually work with CONFIG_MULTHREADING=n! They work only if the count never goes to zero, or if the count is only ever incremented from an ISR. And that's not a semaphore as understood by code written to a semaphore API. Nor is it very useful, compared to simple spin waiting.

I mean, the only value that MULTITHREADING=n has is size. And this is adding functionality to the image just to get something that you can't really get reliably just seems like the wrong tactic. Stated alternatively: if your flash drivers were written to assume OS synchronization primitives and threading, you probably need to be running them under an OS kernel.

The docs specifically tell you "Kernel objects will most probably not behave as expected, especially with regards to pending". I think this is a great example for why. Can you dig more carefully through your drivers, I think that's where you want to do the port to CONFIG_MULTITHREADING=n. They need to know they can't use a real semaphore.

(Finally: dollars to donuts says that all those semaphore calls in your drivers are doing is locking anyway, which you can just remove, right?)

[mbolivar]

@JoeHut if there are no ram/rom constraints, you should be able to build mcuboot with multithreading, single thread was added just to minimise footprint.

IMO it also has security advantages: less code == smaller attack surface.

[mbolivar]

(Finally: dollars to donuts says that all those semaphore calls in your drivers are doing is locking anyway, which you can just remove, right?)

Wouldn't that require different versions of the same drivers for use in MCUboot and within a multithreaded Zephyr application?

[andyross]

Wouldn't that require different versions of the same drivers for use in MCUboot and within a multithreaded Zephyr application?

#ifdef CONFIG_MULTITHREADING
        k_sem_take(&my_lock);
#endif

[JoeHut]

@andyross I get your point. The patch was a fix for us but I understand that it is not the proper way to go. Fine for me.
But this means that MCUBoot for Zephyr is broken, since it uses the flash API and all implementations in the Zephyr kernel use semaphores which might not behave as expected in the default settings.

If I get it right the alternatives are:

• Enable multithreading for MCUBoot by default
• Patch the flash drivers to work without multithreading and ensure that this will also be true in the future
• Apply this patch (which I understand is not preferable)

[mbolivar]

#ifdef CONFIG_MULTITHREADING
k_sem_take(&my_lock);
#endif

Is there a solution where semaphores degrade gracefully in the absence of multithreading, so MCUboot users don't have to audit all of their Zephyr drivers now and forever for this sort of thing?

[andrewboie]

#ifdef CONFIG_MULTITHREADING
        k_sem_take(&my_lock);
#endif

Would this work?
Seems like here we are using the semaphores to synchronize between the (single) thread running on the system, and interrupts. If we just ifdef these calls out does it still work properly?

Maybe we really should make sempahores work in a single-threaded environment since they also have use-case to synchronize with ISRs.

[andyross]

@andrewboie @mbolivar Yeah, it sort of depends on what the driver is doing with those semaphores. We can't have a general "semaphore" object, because that inherently implies threading. Two possibilities that have been discussed so far:

• The driver is using the semaphore as a "wake up userspace form an ISR" mechanism. Here you can replace k_sem_get() with a simple polling loop in the driver.

• The driver is just using a semaphore as a lock to block other userspace threads (lots and lots of our code uses semaphores as locks). There are no other threads, just #if it out.

All I'm saying is let's not try to fix this by fooling the driver into thinking semaphore actually work, because they don't.

[andyross]

Also @mbolivar: you absolutely have to audit any driver you use from a MULTITHREADING=n environment. It's not "Zephyr" at that point. The APIs don't work, or work partially, or were never tested. The docs specifically warn about that. No free lunch. :)

[nashif]

http://docs.zephyrproject.org/reference/kconfig/CONFIG_MULTITHREADING.html#cmdoption-arg-config-multithreading

Many drivers and subsystems will not work with this option; use only
  when you REALLY know what you are doing.

[mbolivar]

All I'm saying is let's not try to fix this by fooling the driver into thinking semaphore actually work, because they don't.

What would you recommend as a way for driver ISRs to unblock waiting code (be it one thread out of many or the hard loop if multithreading=n)?

Polling "works", but wouldn't power management be a concern?

[andyross]

The code submitted above is fundamentally just a busy loop too. If you need to wait for an ISR from user code, spinning is your only option (beyond writing some arch-specific code in the loop, of course). We have PM code as part of the idle thread, but of course there is no idle thread.

FWIW, I did a super-quick audit of the semaphore usage of the files under drivers/flash:

• soc_flash_mcux uses a semaphore as a simple lock, you can drop that (it also somewhat dubiously uses the taken semaphore as a way to implement "write protection" by blocking anyone who tries. Isn't that supposed to set some kind of hardware mode, not just block local writers?!)

• soc_flash_nrf has a simple lock. It also has a "sem_sync" that it uses when CONFIG_SOC_FLASH_NRF_RADIO_SYNC is defined that pulls in a whole "ticker" abstraction from the bluetooth subsystem. This submode will never work in !MULTITHREADING, though the rest of the driver would be fine.

• soc_flash_qmsi uses a single semaphore to lock access around QMSI calls. Remove

• spi_flash_w25?!@#!@#[1] likewise just uses it as a lock

• flash_sam0 ditto

• flash_stm32 yup

• soc_flash_nios2_qspi this too

So it seems to me that this problem should just go away without need for polling nor semaphore API abuse.

[1] -ELINENOISEINFILENAME, couldn't read

[oliviermartin]

@andyross I get your point, that is a fair statement. But in this case, let's disable all kernel API that will not be supported in MULTITHREADING=n environment. Actually looking at kernel.h most of the code could be interpreted as multithread only.

Or we enable multithreading in MCUBoot (https://github.com/runtimeco/mcuboot/blob/master/boot/zephyr/prj.conf#L32) to prevent this kind of patch in the future in Zephyr's project:

[mbolivar]

The code submitted above is fundamentally just a busy loop too. If you need to wait for an ISR from user code, spinning is your only option (beyond writing some arch-specific code in the loop, of course). We have PM code as part of the idle thread, but of course there is no idle thread.

I suppose my question was really about the availability of a single kernel API for waiting on an ISR that could be used in both multithreaded and non-multithreaded mode (since the flash drivers must work in both situations) that wouldn't imply adding a bunch of ifdefs to stitch things together.

soc_flash_mcux uses a semaphore as a simple lock, you can drop that (it also somewhat dubiously uses the taken semaphore as a way to implement "write protection" by blocking anyone who tries. Isn't that supposed to set some kind of hardware mode, not just block local writers?!)

For your reference:

#5961

IOW, there was nothing there before; this is a band-aid to limp along until someone from NXP has time to fully implement the flash API.

soc_flash_nrf has a simple lock.

I think you mean sem_lock. That's a semaphore, which is needed when the driver is run with in multithreaded mode so the thread can schedule out if it's unavailable, no? Which gets back to my question about what kernel API could be used that would be safe in both contexts, and not requiring a bunch of ifdeffery. E.g. if semaphores acting as locks could degrade to busy loops if multithreading = n in some way.

It also has a "sem_sync" that it uses when CONFIG_SOC_FLASH_NRF_RADIO_SYNC is defined that pulls in a whole "ticker" abstraction from the bluetooth subsystem. This submode will never work in !MULTITHREADING, though the rest of the driver would be fine.

There's no radio in use to sync with when running this driver in MCUboot. There is later on when the chain-loaded zephyr image boots up and wants to use the same driver at the same time as communicating via Bluetooth. Hence, the ...NRF_RADIO_SYNC knob is n when building the driver for MCUboot, and y in applications that need it for BT.

So I'm not sure how that applies.

soc_flash_qmsi uses a single semaphore to lock access around QMSI calls. Remove

spi_flash_w25?!@#!@#[1] likewise just uses it as a lock

flash_sam0 ditto

flash_stm32 yup

soc_flash_nios2_qspi this too

Sorry If I'm being dense, but it seems like removing the locks outright and replacing them with busy loops means that flash drivers will spin unnecessarily when multithreading is enabled. What am I missing here?

[andyross]

@mbolivar : I think you might be conflating the two cases:

If you have some C code that needs to "wait" for an IRQ to be delivered before returning: obviously you can't return out of the function, so you either have to context switch away to do something else, or you have to spin. No other options (except maybe spinning + using some arch-specific mechanism to gate power like asm("hlt") or whatever). But none of the flash drivers need to do that.

Instead, all they need to so is use the semaphore as a mutex to prevent other threads on the system from coming along and stomping on the flash operation while it's in progress (I'm guessing this is done with a semaphore and not irq_lock() because flash operations are comparatively slow.

But in !MULTITHREADING, there are no other threads. You don't replace those with busy loops, you replace them with no code at all, because there is no one for you to lock against.

[hongshui3000]

There is no thread queue if CONFIG_MULTITHREADING=nis set.

Using Event Queue Polling and Status Triggering (State Machines) May Be Easier to Implement

Not knowing whether this mechanism will be provided？

[carlescufi]

No other options (except maybe spinning + using some arch-specific mechanism to gate power like asm("hlt") or whatever)

Yes so that is what is done in some of our drivers. The point is whether we want to provide a "spinning + flag" mechanism for managing this:

k_sem_take()
{
    while(!sem.flag) {
         _WFE(); / HLT(); / ...
    }
}
k_sem_give()
{
     sem.flag = 1;
     // no need to do anything else, the interrupt firing will unblock WFE/HLT, etc.
}

[carlescufi]

@andyross will k_fifo_get( K_NO_WAIT) and k_fifo_put() work with CONFIG_MULTITHREADING=n? I was just testing this now and I am not sure if this is at fault but I see an issue there.

Context:
https://github.com/runtimeco/mcuboot/blob/master/boot/zephyr/serial_adapter.c

CC @nvlsianpu

[nvlsianpu]

@carlescufi I have come to the same suspicions as mcuboot serial-recovery mode stops responding since #8370 was merged.

[andyross]

@carlescufi I'd strongly expect the k_queue/fifo/lifo behavior would be the same as semaphores (which are just queues without data, semantically). You can't have a working k_fifo() without threads. You could (in theory) have one that was never empty, or one that was only ever inserted to from an ISR. Whether the code works or not is unknown.

As far as the need for a "power-friendly wait for ISR" API, I'm not opposed. I just don't like abusing the semaphore API to provide it. Note again that the flash drivers don't need that, though.

[carlescufi]

@andyross understood. We will replace the k_fifo_ functions with sys_slist_ ones then. Singly linked lists do work without threads I hope :)

[andyross]

Thinking about this on the "new API" level (which would require modifying drivers of course):

The "locking" needed by the flash drivers can be solved with a variant semaphore API. Consider:

#define k_sem_lock k_sem_take
#define k_sem_unlock k_sem_give

These implement a semaphore with restricted semantics (Of course in
practice you'd implement these as inlines with assertions to enforce
the rules)

• Initial count must be 1
• Lock/unlock calls must occur in non-nested pairs in the same thread

That API can be supported in both MULTITHREADING and !MULTITHREADING modes, because we're guaranteed it's a noop when there are no other threads to contend on the lock.

A "wait for ISR" API would still not be mappable directly to a sempahore, you'd have to write new code and select it via #ifdef. But the calls could be simple, e.g. (the naming here is deliberately designed to look like an x86 MONITOR/MWAIT, but lots of archs have similar tricks):

struct k_monitor { /* arch-specific */ };

/**
 * Busy-wait (ideally in an architecturally-defined low power state)
 * for another CPU or ISR to call k_signal_monitor(), then returns.
 * May return spuriously (for example, after unrelated interrupts) and
 * must be used in a loop.
 */
void k_monitor_wait(struct k_monitor *monitor);

/**
 * Signal any waiters on the address (either on this CPU in a preempted
 * context or on another CPU) to return from k_monitor_wait()
 */
void k_signal_monitor(struct k_monitor *monitor);

[nashif]

can this be closed now?

[JoeHut]

From our side, yes. I think it still should be documented in MCUboot that the selected drivers (e.g. flash, uart for serial recovery) have to be reviewed that they work without multithreading.

[nvlsianpu]

This sounds like dependency on MULTITHREADING should be added to any drivers which requiress multithreding.

[oliviermartin]

I am not sure we found a solution for this issue. As far as I can see the status of non-threaded firmware is undefined with Zehpyr. And the documentation says MULTITHREADING=n is risky: http://docs.zephyrproject.org/reference/kconfig/CONFIG_MULTITHREADING.html.

We know there are issues when non-threaded firmwares use some/most Zephyr kernel API. MCUBoot seems to be the bootloader solution chosen by Zephyr project at the moment. MCUBoot uses MULTITHREADING=n.

What should be the message for MCUBoot? "We do not guarantee MCUBoot Zephyr's backend being fonctional. Do not use MCUBoot Zephyr's backend on production device without being aware of MULTITHREADING=n limitation and having reviewed the code in consequence".

See @carlescufi recent issue "Right now this is completely preventing proper use of MCUboot.": #8393 (comment)

I see three possible solutions:

• Use MULTITHREADING=y for MCUBoot Zephyr's backend
• Disable Zephyr kernel API that are not suitable for MULTITHREADING=n
• Add a big warning next to MCUBoot Zephyr's backend

[andyross]

Some of this is semantics. There will never be a generic solution to this issue where you get to have MULTITHREADING=n and still use whatever APIs you want. If you want APIs, you use Zephyr.

But for all the APIs that have actually been discussed, there are IMHO very acceptable solutions available (see above). I'd be happy to work on something like that, or someone else could. Neither the locking nor monitor features are large or complicated (though implementing monitor in a non-toy way requires platform-specific assembly).

But that analaysis needs to be repeated for every new driver or feature you want to suck into MULTITHREADING=n. No free lunch.

[pizi-nordic]

I agree with @oliviermartin, that we should disable all API which cannot be used without threads when CONFIG_MULTITHREADING=n in order to avoid such issues in future.

[oliviermartin]

Be aware SPI uses semaphore: https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/spi/spi_context.h#L27
As soon as you use an external SPI-Flash in MCUBoot (or other non-MULTITHREADING project) you will be affected by this issue.

[nashif]

• Use MULTITHREADING=y for MCUBoot Zephyr's backend

I think we will go for this option and remove the single thread support which was experimental to start with, see #9808

[Olivier-ProGlove]

Thanks a lot @nashif for following on this pull-request 👍