Why does the HAL USB CDC driver call malloc in an ISR context?

[jschloss]

Since __malloc_lock has not been implemented by default, newlib's malloc / _malloc_r is not reentrant or thread safe (including ISR safe). This seems like a bug.

This will cause very bad interactions with user code use of malloc, including heap corruption. Additionally, most RTOS's heaps are not ISR-safe, so when using the freertos heap it causes a crash. Usually when using newlib & freertos I implement _mallor_r to use the rots heap, and provide __malloc_lock and __malloc_unlock so it is threadsafe. This is necessary so the C library internal allocations are threadsafe, but doing so causes a crash when the cdc driver attempts to allocate memory within the ISR.

Can this be changed to allow either a statically allocated buffer, a user provided buffer, or a thread-context dynamically allocated buffer? I have moved the classData it is trying to allocate to be static for now, but in general this needs to be allocated per usb instance, so a single one is not enough.

Perhaps a user provided buffer passed in at configuration time would be better.

[S.Ma]

static uint8_t USBD_CDC_Init (USBD_HandleTypeDef *pdev, uint8_t cfgidx)

...

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

IMOH avoiding using dynamic memory allocation is preffered (lower risks).

The USBB_CDC_Init is used as function pointer. This is where usually I create a double pointer which I call a "hook": The first pointer is the function, the second is the context, the pointer to the structure containing all what is needed by the function to operate, and is passed as parameter (as most HAL functions do)

[Pavel A.]

USBD_malloc is not the malloc. It is a different function. Implement it as your like.

-- pa

[jschloss]

Yes, my point is under FreeRTOS/newlib it is not really possible to provide a general ISR safe malloc, at least not without making a lot of compromises. I think this is similar for most platforms. I can't use either the C stdlib's malloc nor my RTOS's malloc.

The examples do default USBD_malloc to newlib's malloc, which is not a good idea. It likely works in the examples because there are no other allocations happening, but as soon as another context starts calling malloc (or printf, which will call malloc on first use, etc) there is risk of heap corruption. It seems like this needs to be more robust.

I could write my own heap implementation that is ISR safe, but that seems a bit unexpected to be necessary, especially for a single fixed size allocation. It would be very easy to pre-allocate this in a normal execution context.

[S.Ma]

This means:

• User may need to change this implementation
• The name of the function may be misleading
• Developper will scratch their head and feel like a "time bump" :sad_but_relieved_face: ?

[Bob S]

As @Pavel A.​ said, USBD_malloc is NOT malloc, at least not in the STM32L4xx libraries. It is a #define alias for USBD_static_malloc(), which simply returns a pointer to a statically-defined buffer large enough to hold one USBD_CDC_HandleTypeDef structure (under the presumption that USBD_CDC_Init() is only called once and only ever uses one CDC handle structure (reasonable assumptions for the way ST wrote their driver). So there are no re-entrancy or ISR issues with this particular malloc-like call.

[jschloss]

That is interesting - I am using the STM32H7 libraries (STM32Cube_FW_H7_V1.3.0), and in usbd_conf_template.h there is

#define USBD_malloc malloc

However, when I open STM32Cube_FW_L4_V1.13.0 and v1.11.0 's usbd_conf_template.h I also see

#define USBD_malloc malloc

Which version of the L4 middleware do you have? Maybe this was changed at some point? I'll download a few more and see if I can find one with USBD_static_malloc.

I do see USBD_static_malloc in the nucleo example projects in my L4 folders, but not in the library itself, or in the code that cube generated for me - I didn't start this project from an example, and cube didn't copy one over for me when I generated the startup code.

doing a ~/STM32Cube/Repository $ grep -r USBD_static_malloc only returns hits in the L4 examples too, none in the H7 example projects. Actually, looking at this more, I don't see any pre-generated USB examples in the H7 cube projects folder.

Maybe this is the source of my confusion - The default in the middleware folder is malloc (so I thought that would be ok), and I never looked at the L4 examples since I am using an H7 part, and everyone just "knows" that they need to use the USBD_static_malloc from the nucleo examples, haha.

[jschloss]

So basically what @S.Ma​  said, if the library doesn't actually want a full malloc implementation, it would be a lot less confusing if it wasn't called malloc, and didn't default to malloc in the templates. It was a bit worrying to me initially to provide a malloc that returned the same static array pointer, since without inspecting the library I wasn't sure it was really only called once.

I'll switch to something equivalent to the USBD_static_malloc in the L4 example code. Thanks for the help everyone!

[Bob S]

I'm using CubeMX 4.24.0 with the STM32Cube_FW_L4_V1.11.0 (current appears to be 1.13.0) - yes, almost a year old but I am also *very* paranoid about updating in the middle of a project. We used the Cube-generated code as a starting point, but have heavily modified it such that re-generating code using CubeX is no longer possible without an significant hand-merge process.

Hmmmm.... interestingly enough, when I look at the actual L4 library source, there is NO mention of ISBD_static_malloc, and USBD_malloc is defined as plain old "malloc" in "usbd_conf_template.h". I only see the "USBD_static_malloc" if I look in the CubeMX templates directory (on my Win10 PC here - C:\Program Files\STMicroelectronics\STM32Cube\STM32CubeMS\db\templates\usbdconf_l4_h.ftl).

[CMars.2]

Could somebody confirm this assumption?

"only ever uses one CDC handle structure (reasonable assumptions for the way ST wrote their driver)"

And if the CDC driver is designed to be single-instance anyway why all the hassle to reclaim memory to "platform code" for the user to provide instead of using a static buffer anyway?

The only reasonable option here seems like the driver can effectively be instantiated more than once at the same time and that the USBD_malloc could be static if only one instance is enabled per-application-design, and that it shall be dynamic (memory pool?) for applications where two CDC driver instances could be loaded at the same time.

Looking at the STM32F405V reference manual it looks like it can handle two USB ports (or at least it has different USB IRQs) which look like can be driven by this device library code, so theoretically it looks like you could have two simultaneous CDC connections.