cancel
Showing results for 
Search instead for 
Did you mean: 

Does USB device code for STM32 really allocate dynamic memory from interrupts?

freddie_chopin
Associate III
Posted on January 13, 2017 at 18:29

Today in one of my projects I've enabled a new feature of my RTOS (

http://distortos.org/

) which ensures some functions are not used in interrupts. This checks - among others - for any use of mutexes from interrupts, as it is a logical error to do that (first of all - interrupts cannot ''block'' and additionally using mutexes in interrupts violates the concept of ''ownership'').

You may imagine how surprised I was when the critical error was raised even before the application would fully start... See this call stack:

0690X000006062lQAA.png

It turns out that ST's code for USB device (CDC, audio, HID, DFU, MSC) in STM32 happily allocates dynamic memory in interrupts... The path is like this:

  • OTG_FS_IRQHandler() interrupt,

  • HAL_PCD_IRQHandler(),

  • HAL_PCD_SetupStageCallback(),

  • USBD_LL_SetupStage(),

  • USBD_StdDevReq(),

  • USBD_SetConfig(),

  • USBD_SetClassConfig(),

  • indirect call of a function stored in ''Init'' field of USBD_ClassTypeDef struct.

The ''Init'' pointer in 

USBD_ClassTypeDef struct is initialized with an address of functions like 

USBD_CDC_Init(), and these function - almost right at the beginning - call malloc() via USBD_malloc() macro...

As malloc() is generally _NOT_ reentrant, it has to be protected from concurrent access from multiple contexts/threads. If a real-time operating system is used, this protection is usually implemented with a mutex or pausing the scheduler (preventing context switches). In single threaded applications malloc() is usually not protected at all, because most of the people are reasonable enough and don't even consider using malloc() in interrupts. In both of these cases adding ST's USB device code sets the application on a path to certain failure if it actually uses dynamic allocation anywhere else:

  • when locking is done with mutexes in a multithreaded system - any attempt to block in interrupt handler (like locking a locked mutex used to serialize access to malloc()) is undefined behaviour and will most likely severely corrupt the system, 

    worse - as mutexes serializing access to malloc() are usually recursive, such call can lead to a typical race condition, causing heap corruption,

  • when locking is done by pausing the scheduler in a multithreaded system - interrupts are not affected by that locking mechanism, malloc() may be re-entered from two different contexts, leading to heap corruption,

  • in a single-threaded application with no locking - an allocation in interrupt during an allocation in main thread will lead to heap corruption.

This whole discussion obviously also applies to free(), which is called in interrupt context via at least 3 different code paths...

#malloc #memory-corruption #race-condition #heap #rtos #usb
21 REPLIES 21
Ernesto B
Associate II
Posted on February 05, 2017 at 17:45

Hi there, any thoughts about this question?

valentin
Senior
Posted on February 07, 2017 at 20:21

Wow, I had no idea about malloc on interrupts. I hadn't used it in them yet - based on a gut feeling. I guess that feeling was right

This makes me worry about using the ST HAL usb code though. Because the init function is called once the user plugs in the usb cable, this means that in theory, malloc can be called at any time from within an ISR. And I am using malloc/free in some of my threads (mutex protected).

If anybody comes up with a solution, please let me know / post it here. I need to make sure that my finished product is stable at all times!

Maybe I can protect it using taskEnterCritical / ExitCritical instead? Doesn't that macro temporarily disable interrupts as well? (FreeRTOS).

Edit: yes, according to FreeRTOS docs, taskEnter/ExitCritical() macros would be a workaround here. They disable (mask) all interrupts below priority 5 (per default) and thus also disable any other context switching.

Unfortunately, that doesn't help with distortos, I presume? Maybe you can implement a similar critical section macro that disables all context switches AND interrupts temporarily?

Posted on February 19, 2017 at 09:58

Hello

Roehrig.Valentin

Valentin wrote:

If anybody comes up with a solution, please let me know / post it here. I need to make sure that my finished product is stable at all times!

Maybe I can protect it using taskEnterCritical / ExitCritical instead? Doesn't that macro temporarily disable interrupts as well? (FreeRTOS).

Edit: yes, according to FreeRTOS docs, taskEnter/ExitCritical() macros would be a workaround here. They disable (mask) all interrupts below priority 5 (per default) and thus also disable any other context switching.

You can do that, but this is more like a 'work around' than a solution. In my project I've just replaced malloc with static allocation - I know how many device classes will be active at any time, so this is not an issue. The problem here is that such approach should be the default one, not the other way around...

Valentin wrote:

Unfortunately, that doesn't help with distortos, I presume? Maybe you can implement a similar critical section macro that disables all context switches AND interrupts temporarily?

There are objects for creating critical sections - distortos::InterruptMaskingLock - which uses RAII pattern. And you could use that to protect malloc, but - as I've said above - this is just a work around of the original problem. There are things you just don't do, and allocating memory from the interrupt handler is one of them. Doing that kinda ruins the main concept of the RTOS - predictable behaviour with low latency. Blocking all interrupts to allocate memory is obviously not going to lower the latency or the jitter - quite the opposite...

Posted on May 03, 2017 at 08:25

I agree - original code is not good. Allocation in real-time (or more precisely high-availability) systems must be avoided not only in interrupts, but totally not used after the initialization phase. This must be followed quite strictly by 'libraries', 'frameworks', 'stacks' and all kind of shared components (reusable ones) - because in other way those components will 'pollute' the final application. 

There are ways to avoid using heap at all, at least in the 'component' level and keep it out in the main (integration/application/project) level - if needed. 

I might be wrong but locking interrupts may solve the problem only if we use it globally for all heap operations - if 'task' level code is running and starts an allocation, then it is preempted by usb interrupt inside the malloc, we come to the same conflict. Another way to solve this is to use multiple heaps for conflicting parts - e.g. separate task heap and interrupt heap. And going this way we can find the real solution - to correctly implement static allocation outside of the interrupts.

valentin
Senior
Posted on May 16, 2017 at 05:44

******, now I'm also stuck at that problem:

Using FreeRtos, it now fires the OTG_HS_IRQHandler() interrupt after calling MX_USB_DEVICE_Init();

That results in a failed assert because I overloaded malloc() to make it thread safe:

void* malloc(size_t size) {
 return pvPortMalloc(size);
}�?�?�?

See here:

0690X00000606mrQAA.png

Does anyone know a workaround? Can I preallocate the memory for the usb struct in USBD_CDC_Init()?

pdev->pClassData = USBD_malloc(sizeof (USBD_CDC_HandleTypeDef));�?

anonymous.8
Senior II
Posted on June 05, 2017 at 23:10

Take a look at UM1734 - STM32Cube USB Device Library User Manual.

At the end it discusses how to use static memory allocation in your device class init functions.

Posted on June 05, 2017 at 23:32

Thanks for the hint, I will take a look at that.

I've had it changed to static allocation now anyway.

I replaced this in usbd_cdc.c:

// pdev->pClassData = USBD_malloc(sizeof (USBD_CDC_HandleTypeDef)); //fix: alocate statically
 pdev->pClassData = &ClassData;�?�?

And added this in usbd_cdc.h:

/* fix: allocate memory statically */
USBD_CDC_HandleTypeDef ClassData;�?�?

Problem solved.

@ST: Would it be possible to maybe introduce a define somewhere e.g. in mxconstants.h about dynamic vs static allocation here?

I see that dynamic allocation is intriguing so that the memory is only used after the usb cable is actually plugged in.

But together with FreeRTOS is would be better to have it default to static allocation IMHO.

Ant M
Associate II
Posted on October 27, 2017 at 17:10

I agree that It really should be static by default, or maybe put user code areas in the relevant file?

Posted on January 19, 2018 at 23:42

This is a solution with limited value. How would your solution deal with a case when you use both USB peripherals as devices? Or if you have more than one device class?