xtensa/esp32: Fix the issue of WiFi internal malloc from PSRAM

[cwespressif]

Summary

SPIRAM is not DMA-capable, while WiFi internal malloc from heap must be DMA-capable, We must avoid internal malloc from the heap of PSRAM

Impact

If WiFi internal malloc from PSRAM, it will cause the system block.

Testing

ESP32

[yamt]

• size == 0 case is better to be left to kmm_realloc
• standard realloc is supposed to preserve the old memory in case of a failure. does this wifi_osi_funcs_t variant have a different semantics?

[cwespressif]

The size == 0 case means that the size of the new malloc is 0,
In the esp_realloc_internal processing of wifi_osi_funcs_t, you can refer to:
https://github.com/espressif/esp-idf/blob/master/components/heap/heap_caps.c#L318-L321

[yamt]

the esp-idf implementation does not seem to free the given ptr in case of error.
eg. https://github.com/espressif/esp-idf/blob/master/components/heap/heap_caps.c#L323-L327
it matches my expectation for realloc().
i guess this implementation should do the same.
unfortunately it seems ~impossible to implement it for !CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP case though.

[cwespressif]

Yes, as the heap management of NuttX and esp-idf is too different, this part of the modification is too big, now temporarily free the given ptr in case of error.

[yamt]

but doesn't it cause double-free sooner or later, as the library would think the ptr still needs to be freed?

[yamt]

maybe you can do something like the following instead of kmm_realloc. how do you think?

new_ptr = kmm_malloc(new_size);
if (esp32_ptr_extram(new_ptr)) {
    kmm_free(new_ptr);
    return NULL;
}
old_size = malloc_usable_size(old_ptr);
memcpy(new_ptr, old_ptr, min(old_size, new_size));
kmm_free(old_ptr);
return new_ptr

[cwespressif]

@yamt The library will check ptr and then free it, there is a risk of double-free and I also think the above code instead of kmm_realloc is more reasonable, I have modified it, thank you.

[yamt]

i guess you want to check esp32_ptr_extram on the result of kmm_realloc.

[cwespressif]

I want to make sure that the old memory pointer is not in external RAM (PSRAM), although this is unlikely to happen.

[cwespressif]

kmm_realloc also needs to be checked by esp32_ptr_extram, I have modified it, thank you for your reminder.

[acassis]

@cwespressif is everything done? May we merge it now?

[cwespressif]

@cwespressif is everything done? May we merge it now?

@acassis I think it's OK, but I still need to wait to see if @yamt has any comments or suggestions ?

[yamt]

the esp-idf implementation does not seem to free the given ptr in case of error.
eg. https://github.com/espressif/esp-idf/blob/master/components/heap/heap_caps.c#L323-L327
it matches my expectation for realloc().
i guess this implementation should do the same.
unfortunately it seems ~impossible to implement it for !CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP case though.

[yamt]

do you mean p, not ptr?

[cwespressif]

It's p, sorry for the mistake.

[Ouss4]

@yamt do you have any more concerns?

[yamt]

for size == 0 case, you should free ptr.

i don't think you need to check esp32_ptr_extram(old_ptr). the old_ptr will be freed on successful realloc anyway.

[cwespressif]

Done

[yamt]

lgtm. thank you