From 698cbe884ba0804ce63781fa64ea41dfa6fc842d Mon Sep 17 00:00:00 2001 From: Chen Chen Date: Wed, 19 Nov 2025 11:59:51 +0800 Subject: [PATCH] fix(ledc): fix potential null dereference issue & add test case --- components/esp_driver_ledc/src/ledc.c | 11 +++++- .../test_apps/ledc/main/test_ledc.c | 39 +++++++++++++++++++ .../include/driver/sdmmc_host.h | 2 +- components/hal/include/hal/ledc_hal.h | 19 +++++---- components/hal/include/hal/sdio_slave_hal.h | 10 ++--- components/hal/ledc_hal_iram.c | 5 --- 6 files changed, 66 insertions(+), 20 deletions(-) diff --git a/components/esp_driver_ledc/src/ledc.c b/components/esp_driver_ledc/src/ledc.c index 9dbaaa0a8f..d69e88dd11 100644 --- a/components/esp_driver_ledc/src/ledc.c +++ b/components/esp_driver_ledc/src/ledc.c @@ -1533,11 +1533,20 @@ exit: esp_err_t ledc_fade_func_install(int intr_alloc_flags) { LEDC_CHECK(s_ledc_fade_isr_handle == NULL, "fade function already installed", ESP_ERR_INVALID_STATE); + + ledc_mode_t speed_mode = LEDC_SPEED_MODE_MAX; + for (int i = 0; i < LEDC_SPEED_MODE_MAX; i++) { + if (p_ledc_obj[i] != NULL) { + speed_mode = i; + break; + } + } + //OR intr_alloc_flags with ESP_INTR_FLAG_IRAM because the fade isr is in IRAM return esp_intr_alloc_intrstatus( ETS_LEDC_INTR_SOURCE, intr_alloc_flags | ESP_INTR_FLAG_IRAM, - (uint32_t)ledc_hal_get_fade_end_intr_addr(&(p_ledc_obj[0]->ledc_hal)), + (uint32_t)ledc_hal_get_fade_end_intr_addr(&(p_ledc_obj[speed_mode]->ledc_hal)), LEDC_LL_FADE_END_INTR_MASK, ledc_fade_isr, NULL, diff --git a/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c b/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c index 1c92ef2c42..04fa4958a2 100644 --- a/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c +++ b/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c @@ -258,6 +258,45 @@ TEST_CASE("LEDC fade with step", "[ledc]") ledc_fade_func_uninstall(); } +#if SOC_LEDC_SUPPORT_HS_MODE +TEST_CASE("LEDC fade install with low speed mode only", "[ledc]") +{ + // This test verifies that ledc_fade_func_install works correctly when only + // LEDC_LOW_SPEED_MODE is initialized, without initializing LEDC_HIGH_SPEED_MODE. + // This tests the fix for NULL pointer dereference issue. + + // Only initialize low speed mode, do NOT initialize high speed mode + ledc_channel_config_t ledc_ch_config = initialize_channel_config(); + ledc_ch_config.speed_mode = LEDC_LOW_SPEED_MODE; + ledc_ch_config.duty = 0; + ledc_ch_config.channel = LEDC_CHANNEL_0; + ledc_ch_config.timer_sel = LEDC_TIMER_0; + + ledc_timer_config_t ledc_time_config = create_default_timer_config(); + ledc_time_config.speed_mode = LEDC_LOW_SPEED_MODE; + ledc_time_config.timer_num = LEDC_TIMER_0; + + // Initialize only low speed mode + TEST_ESP_OK(ledc_channel_config(&ledc_ch_config)); + TEST_ESP_OK(ledc_timer_config(&ledc_time_config)); + vTaskDelay(5 / portTICK_PERIOD_MS); + TEST_ESP_OK(ledc_fade_func_install(0)); + + // Verify that fade functionality works correctly with low speed mode + TEST_ESP_OK(ledc_set_fade_with_time(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0, 4000, 200)); + TEST_ESP_OK(ledc_fade_start(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0, LEDC_FADE_WAIT_DONE)); + TEST_ASSERT_EQUAL_INT32(4000, ledc_get_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0)); + + // Test fade down + TEST_ESP_OK(ledc_set_fade_with_time(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0, 0, 200)); + TEST_ESP_OK(ledc_fade_start(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0, LEDC_FADE_WAIT_DONE)); + TEST_ASSERT_EQUAL_INT32(0, ledc_get_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_0)); + + // Cleanup + ledc_fade_func_uninstall(); +} +#endif // SOC_LEDC_SUPPORT_HS_MODE + TEST_CASE("LEDC fast switching duty with fade_wait_done", "[ledc]") { const ledc_mode_t test_speed_mode = TEST_SPEED_MODE; diff --git a/components/esp_driver_sdmmc/include/driver/sdmmc_host.h b/components/esp_driver_sdmmc/include/driver/sdmmc_host.h index e88c8fa41d..037dbe83cf 100644 --- a/components/esp_driver_sdmmc/include/driver/sdmmc_host.h +++ b/components/esp_driver_sdmmc/include/driver/sdmmc_host.h @@ -14,7 +14,7 @@ #include "esp_err.h" #include "driver/sdmmc_types.h" #include "driver/sdmmc_default_configs.h" -#include "soc/gpio_num.h" +#include "driver/gpio.h" #ifdef __cplusplus extern "C" { diff --git a/components/hal/include/hal/ledc_hal.h b/components/hal/include/hal/ledc_hal.h index 04d9ba0f9f..c8cafc8449 100644 --- a/components/hal/include/hal/ledc_hal.h +++ b/components/hal/include/hal/ledc_hal.h @@ -396,14 +396,6 @@ void ledc_hal_get_fade_end_intr_status(ledc_hal_context_t *hal, uint32_t *intr_s */ void ledc_hal_clear_fade_end_intr_status(ledc_hal_context_t *hal, ledc_channel_t channel_num); -/** - * @brief Get the address of the fade end interrupt status register. - * - * @param hal Context of the HAL layer - * @return Pointer to the fade end interrupt status register. - */ -volatile void* ledc_hal_get_fade_end_intr_addr(ledc_hal_context_t *hal); - /** * @brief Get clock config of LEDC timer * @@ -415,6 +407,17 @@ volatile void* ledc_hal_get_fade_end_intr_addr(ledc_hal_context_t *hal); */ void ledc_hal_get_clk_cfg(ledc_hal_context_t *hal, ledc_timer_t timer_sel, ledc_clk_cfg_t *clk_cfg); +/** + * @brief Get the address of the fade end interrupt status register. + * + * @param hal Context of the HAL layer + * @return Pointer to the fade end interrupt status register. + */ +static inline volatile void* ledc_hal_get_fade_end_intr_addr(ledc_hal_context_t *hal) +{ + return ledc_ll_get_fade_end_intr_addr(hal->dev); +} + #endif //#if SOC_LEDC_SUPPORTED #ifdef __cplusplus diff --git a/components/hal/include/hal/sdio_slave_hal.h b/components/hal/include/hal/sdio_slave_hal.h index c7fc402ece..94588c26af 100644 --- a/components/hal/include/hal/sdio_slave_hal.h +++ b/components/hal/include/hal/sdio_slave_hal.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -153,7 +153,7 @@ extern "C" { #endif #if SOC_SDIO_SLAVE_SUPPORTED -/// Space used for each sending descriptor. Should initialize the sendbuf accoring to this size. +/// Space used for each sending descriptor. Should initialize the sendbuf according to this size. #define SDIO_SLAVE_SEND_DESC_SIZE sizeof(sdio_slave_hal_send_desc_t) @@ -239,7 +239,7 @@ typedef struct { /** * Initialize the HAL, should provide buffers to the context and configure the - * members before this funciton is called. + * members before this function is called. * * @param hal Context of the HAL layer. */ @@ -350,7 +350,7 @@ esp_err_t sdio_slave_hal_send_get_next_finished_arg(sdio_slave_context_t *hal, v * * @note Only call when the DMA is stopped! * @param hal Context of the HAL layer. - * @param out_arg Argument indiciating the buffer to send + * @param out_arg Argument indicating the buffer to send * @param out_return_cnt Space in the queue released after this descriptor is flushed. * @return * - ESP_ERR_INVALID_STATE: This function call be called only when the DMA is stopped. @@ -476,7 +476,7 @@ void sdio_slave_hal_recv_flush_one_buffer(sdio_slave_context_t *hal); /** * Enable some of the interrupts for the host. * - * @note May have concurrency issue wit the host or other tasks, suggest only use it during + * @note May have concurrency issue with the host or other tasks, suggest only use it during * initialization. * @param hal Context of the HAL layer. * @param mask Bitwise mask for the interrupts to enable. diff --git a/components/hal/ledc_hal_iram.c b/components/hal/ledc_hal_iram.c index 3a6279982c..804473132c 100644 --- a/components/hal/ledc_hal_iram.c +++ b/components/hal/ledc_hal_iram.c @@ -75,8 +75,3 @@ void ledc_hal_clear_fade_end_intr_status(ledc_hal_context_t *hal, ledc_channel_t { ledc_ll_clear_fade_end_intr_status(hal->dev, hal->speed_mode, channel_num); } - -volatile void* ledc_hal_get_fade_end_intr_addr(ledc_hal_context_t *hal) -{ - return ledc_ll_get_fade_end_intr_addr(hal->dev); -}