drivers: dma: stm32: add null callback checking

[ttwards]

drivers: dma: stm32: check for null dma_callback before enabling irqs

The STM32 DMA driver did not check if the optional dma_callback was set in dma_stm32_configure(). This could lead to a hard fault when an interrupt occurs and the callback is NULL, even though the API allows the callback to be omitted.

This patch modifies dma_stm32_configure() to enable transfer and half-transfer interrupts only if a valid dma_callback is supplied in config. This prevents NULL-function pointer dereference if callbacks are not used.

Fixes #97454

[etienne-lms]

Nitpicking on coding guidelines, otherwise imeplementation LGTM.

Regarding the commit message, I think you should rephrase the last sentence that does not really describe the change IMHO.
My suggestion:

 drivers: dma: stm32: add null callback checking
 
-The STM32 DMA driver did not check if the optional dma_callback was set
+Fix the STM32 DMA driver did not check if the optional dma_callback was set
 in dma_stm32_configure(). This could lead to a hard fault when an
 interrupt occurs and the callback is NULL, even though the API allows
 the callback to be omitted.

-This patch modifies dma_stm32_configure() to enable transfer and
-half-transfer interrupts only if a valid dma_callback is supplied in
-config. This prevents NULL-function pointer dereference if callbacks
-are not used.
-
 Fixes: ...
 Signed-off-by: ...

[etienne-lms]

Nitpicking on coding style here and below:

Suggested change

	if (stream->dma_callback) {
	stream->dma_callback(dev, stream->user_data,
	callback_arg, DMA_STATUS_BLOCK);
	}
	if (stream->dma_callback != NULL) {
	stream->dma_callback(dev, stream->user_data,
	callback_arg, DMA_STATUS_BLOCK);
	}

[AUtech-dev]

Thanks for the PR!
I can confirm that the current state fixes the hard fault.

However, I am a little bit concerned about the performance impact of the interrupts if the callback is not used:
The half transfer isr only clears the isr flag with dma_stm32_clear_ht() and does nothing more. Not requesting the interrupt at all (by not enabling it in dma_stm32_configure) would not change the system behavior but saves some cpu cycles - from my point of view.

In case of a cyclic dma tranfser (stream->cyclic == true) the transfer complete isr has the same behavior.

From my point of view this diff would improve performance:

--- a/drivers/dma/dma_stm32.c
+++ b/drivers/dma/dma_stm32.c
@@ -491,10 +501,13 @@
 	DMA_InitStruct.PeriphRequest = config->dma_slot;
 #endif
 	LL_DMA_Init(dma, dma_stm32_id_to_stream(id), &DMA_InitStruct);
 
-	LL_DMA_EnableIT_TC(dma, dma_stm32_id_to_stream(id));
+	/* Enable transfer complete isr if in non-cyclic mode or a callback is requested*/
+	if (!stream->cyclic || stream->dma_callback) {
+		LL_DMA_EnableIT_TC(dma, dma_stm32_id_to_stream(id));
+	}
 
-	/* Enable Half-Transfer irq if circular mode is enabled */
-	if (stream->cyclic) {
+	/* Enable Half-Transfer irq if circular mode is enabled and a callback is requested */
+	if (stream->cyclic && stream->dma_callback) {
 		LL_DMA_EnableIT_HT(dma, dma_stm32_id_to_stream(id));
 	}

Background: In my application I have a dma for RX and TX data with a quite fast timing and really need only one interrupt because both data paths are handled together.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[AUtech-dev]

Tested this on hardware (stm32u073Xc). Works fine.

[etienne-lms]

@ttwards, note that tabulations are 8 space character wide in Zephyr.
CI compliance check also complains on mixes of tab and spaces here and there.

For info, C source files can use up to 100char/line so feel free to use this range without line escapes when applicable.

[ttwards]

I understand, but two callings of stream->dma_callback reach 100~110 chars.
Shoud I use tab only?