drivers: dma: esp32s3: Add DMA support for esp32s3

[LucasTambor]

Add GDMA support for esp32s3.
Remove suspend/resume since they are optional and do the same as start/stop.
Fix possible null pointer derreference.

[marekmatej]

maybe put files in test/drivers/dma/*.[conf,overlay] to separate commit?

[LucasTambor]

@marekmatej done ;)

[marekmatej]

You are freeing buf_temp which is allocated in the dma_esp32_config_tx_descriptor. Why cannot it be allocated for the driver lifetime?

[marekmatej]

you add trailing space

[marekmatej]

I think the tx_descriptor func. needs to free the buf_temp too

[teburd]

I missed that the heap is still being used here

[teburd]

Why are we trying to allocate on behalf of callers here, if an invalid buffer for DMA is being provided this seems like a bug that should be asserted on.

I'm a little confused by the code here to malloc/free a temporary buffer when that's not the case. Is there a particular scenario esp32 expects this to occur?

[LucasTambor]

We are freeing the channel's temporary buffer every time a new descriptor needs to be created. In case the original buffer is in a valid DMA region, the k_free can handle a NULL pointer without issue and no allocation is needed. In case it is in an invalid region, we create a temporary one to handle the transactions, freeing it and reallocating according the size required by the transactions. I'm trying to deal with both scenarios here, since the original buffer location depends on the application or driver using it.

Did I answer your question? I'm not shure haha

[teburd]

I guess what I'm suggesting is this is a bug then in the driver or application that should be caught. There's an RFC for managing buffers #57602

At the moment this would be, to my best knowledge, the only DMA driver that attempts to fix the buffer you asked a copy to by allocating from the heap. I would suggest pushing that issue up the stack as this is a surprising place to have that happen.

[LucasTambor]

Oh nice! I will follow up this RFC and contribute in whatever way I can. But since it is an ongoing discussion, should we block the merge of this commit? This strategy of dealing with buffer is around for quite a while, this commit only add support for esp32s3 and small fixes.

[teburd]

I'd point out that generally Zephyr is trying to follow misra, and in misra-c "dynamic allocation is not allowed" See our guidelines https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#main-rules

So I think this is something that needs to be revisited but today should be considered a programming error trying to ask DMA to work with a buffer that doesn't conform to DMA requirements.

[marekmatej]

LGTM