From 1bb727c66e0b0f99ea55ad06684eed8808fdac0a Mon Sep 17 00:00:00 2001 From: Hu Rui Date: Wed, 15 Jul 2026 17:26:21 +0800 Subject: [PATCH] fix(uhci): rx fsm race condition and buffer size check Closes https://github.com/espressif/esp-idf/issues/18819 Closes https://github.com/espressif/esp-idf/issues/18820 --- .../esp_driver_uart/include/driver/uhci.h | 13 +- .../include/driver/uhci_types.h | 2 + components/esp_driver_uart/src/uhci.c | 214 +++++++++--------- components/esp_driver_uart/src/uhci_private.h | 3 + .../test_apps/uhci/main/test_uhci.c | 43 +++- docs/en/api-reference/peripherals/uhci.rst | 6 +- docs/zh_CN/api-reference/peripherals/uhci.rst | 8 +- 7 files changed, 175 insertions(+), 114 deletions(-) diff --git a/components/esp_driver_uart/include/driver/uhci.h b/components/esp_driver_uart/include/driver/uhci.h index fd466ab9249..bbb8f761f89 100644 --- a/components/esp_driver_uart/include/driver/uhci.h +++ b/components/esp_driver_uart/include/driver/uhci.h @@ -22,7 +22,7 @@ typedef struct { uart_port_t uart_port; /*!< UART port that connect to UHCI controller */ size_t tx_trans_queue_depth; /*!< Depth of internal transfer queue, increase this value can support more transfers pending in the background */ size_t max_transmit_size; /*!< Maximum transfer size in one transaction, in bytes. This decides the number of DMA nodes will be used for each transaction */ - size_t max_receive_internal_mem; /*!< Internal DMA usage memory. Each DMA node can point to a maximum of x bytes (depends on chip). This value determines the number of DMA nodes used for each transaction. When your transfer size is large enough, it is recommended to set this value greater than x to facilitate efficient ping-pong operations, such as 2 * x. */ + size_t max_receive_internal_mem; /*!< Expected maximum buffer size for uhci_receive(). This value determines the number of descriptors in the receive DMA chain. Each DMA descriptor can reference a buffer of up to X bytes (depending on the chip). For large transfers, at least two descriptors are recommended for ping-pong operation. */ size_t dma_burst_size; /*!< DMA burst size, in bytes. Set to 0 to disable data burst. Otherwise, use a power of 2. */ size_t max_packet_receive; /*!< Max receive size, auto stop receiving after reach this value, only valid when `length_eof` set true */ @@ -72,15 +72,16 @@ esp_err_t uhci_new_controller(const uhci_controller_config_t *config, uhci_contr * `uhci_new_controller()`. * @param[out] read_buffer Pointer to the buffer where the received data will be stored. * The buffer must be pre-allocated by the caller. - * @param[in] buffer_size The size of read buffer. + * @param[in] buffer_size The size of read buffer. Should generally not exceed `uhci_controller_config_t.max_receive_internal_mem`. * * @note @note The function is non-blocking, it just mounts the user buffer to the DMA. * The return from the function doesn't mean a finished receive. You need to register corresponding * callback function to get notification. * * @return - * - `ESP_OK`: Data successfully received and written to the buffer. - * - `ESP_ERR_INVALID_ARG`: Invalid arguments (e.g., null buffer or invalid controller handle). + * - `ESP_OK`: The driver is ready for data reception. + * - `ESP_ERR_INVALID_STATE`: The controller is not in enable state. + * - `ESP_ERR_INVALID_ARG`: Invalid arguments (e.g., invalid controller handle, null buffer, invalid buffer size). */ esp_err_t uhci_receive(uhci_controller_handle_t uhci_ctrl, uint8_t *read_buffer, size_t buffer_size); @@ -95,13 +96,15 @@ esp_err_t uhci_receive(uhci_controller_handle_t uhci_ctrl, uint8_t *read_buffer, * @param[in] write_buffer Pointer to the buffer containing the data to be transmitted. * The buffer must remain valid until the transmission is complete. * @param[in] write_size The number of bytes to transmit from the buffer. + * Must not exceed `uhci_controller_config_t.max_transmit_size`. * * @note The function is an non-blocking api, which means this function will return immediately. You can * get corresponding event from callbacks. * * @return * - `ESP_OK`: Data successfully queued for transmission. - * - `ESP_ERR_INVALID_ARG`: Invalid arguments (e.g., null buffer, invalid handle, or zero `write_size`). + * - `ESP_ERR_INVALID_ARG`: Invalid arguments (e.g., null buffer, invalid handle, zero `write_size`, or + * `write_size` exceeds `max_transmit_size`). */ esp_err_t uhci_transmit(uhci_controller_handle_t uhci_ctrl, uint8_t *write_buffer, size_t write_size); diff --git a/components/esp_driver_uart/include/driver/uhci_types.h b/components/esp_driver_uart/include/driver/uhci_types.h index 4d7b19d56af..8826f47d3fa 100644 --- a/components/esp_driver_uart/include/driver/uhci_types.h +++ b/components/esp_driver_uart/include/driver/uhci_types.h @@ -46,6 +46,8 @@ typedef bool (*uhci_tx_done_callback_t)(uhci_controller_handle_t uhci_ctrl, cons /** * @brief UHCI RX Done Event Data Structure + * + * @note When an abnormal EOF occurs, `data` will be NULL and `recv_size` will be 0. */ typedef struct { const uint8_t *data; /*!< Pointer to the received data buffer. Data pointed to by this pointer is typically only guaranteed to be readable during the callback. If you need to use it after callback returns, copy it to external buffer first or refer to advanced zero-copy usage. */ diff --git a/components/esp_driver_uart/src/uhci.c b/components/esp_driver_uart/src/uhci.c index bf445a071b5..7e6f8464e95 100644 --- a/components/esp_driver_uart/src/uhci.c +++ b/components/esp_driver_uart/src/uhci.c @@ -108,78 +108,73 @@ static bool uhci_gdma_rx_callback_done(gdma_channel_handle_t dma_chan, gdma_even { bool need_yield = false; uhci_controller_handle_t uhci_ctrl = (uhci_controller_handle_t) user_data; - bool is_buf_from_psram = esp_ptr_external_ram(uhci_ctrl->rx_dir.buffer_pointers[uhci_ctrl->rx_dir.node_index]); - size_t cache_line = uhci_ctrl->rx_dir.cache_line; - // If the data is not all received, handle it in not normal_eof block. Otherwise, in eof block. - if (!event_data->flags.normal_eof) { - size_t rx_size = uhci_ctrl->rx_dir.buffer_size_per_desc_node[uhci_ctrl->rx_dir.node_index]; - uhci_rx_event_data_t evt_data = { + + // Prevent any spurious interrupts after EOF. + if (atomic_load(&uhci_ctrl->rx_dir.rx_fsm) != UHCI_RX_FSM_RUN) { + return false; + } + + if (event_data->flags.abnormal_eof || event_data->flags.normal_eof) { + // An EOF signal does not automatically stop the DMA transfer, so we need to stop it manually. + gdma_stop(uhci_ctrl->rx_dir.dma_chan); + // stop() cannot prevent already prefetched DMA descriptors from being processed. + // A reset() is required to fully halt the DMA engine and eliminate any subsequent spurious interrupts. + gdma_reset(uhci_ctrl->rx_dir.dma_chan); + } + + uhci_rx_event_data_t evt_data = {0}; + + if (!event_data->flags.abnormal_eof) { + const size_t cache_line = uhci_ctrl->rx_dir.cache_line; + + size_t rx_size, sync_size; + if (!event_data->flags.normal_eof) { + rx_size = uhci_ctrl->rx_dir.buffer_size_per_desc_node[uhci_ctrl->rx_dir.node_index]; + sync_size = rx_size; + } else { + rx_size = gdma_link_count_buffer_size_till_eof(uhci_ctrl->rx_dir.dma_link, uhci_ctrl->rx_dir.node_index); + sync_size = UHCI_ALIGN_UP(rx_size, cache_line); // round up to the next cache line + } + + evt_data = (uhci_rx_event_data_t) { .data = uhci_ctrl->rx_dir.buffer_pointers[uhci_ctrl->rx_dir.node_index], .recv_size = rx_size, - .flags.totally_received = false, + .flags.totally_received = event_data->flags.normal_eof, }; - if (is_buf_from_psram) { + if (esp_ptr_external_ram(evt_data.data)) { esp_psram_mspi_mb(); } + // DMA just finished writing the node's buffer. Because the descriptor link is circular, // the same buffer region gets overwritten on every loop. On targets where the buffer is - // backed by a cache, the cache is not snooped by DMA, so the CPU must invalidate the range - // before reading, otherwise it will return stale data from a previous loop. + // backed by a cache, the CPU must invalidate the range before reading, otherwise it will + // return stale data from a previous loop. if (cache_line > 0) { - // The per-node buffer base is aligned to cache_line (see uhci_receive), and rx_size here - // equals buffer_size_per_desc_node[] which is also a multiple of cache_line. - esp_cache_msync((void *)evt_data.data, rx_size, ESP_CACHE_MSYNC_FLAG_DIR_M2C); + esp_cache_msync((void *)evt_data.data, sync_size, ESP_CACHE_MSYNC_FLAG_DIR_M2C); } - if (uhci_ctrl->rx_dir.on_rx_trans_event) { - need_yield |= uhci_ctrl->rx_dir.on_rx_trans_event(uhci_ctrl, &evt_data, uhci_ctrl->user_data); + } + + if (event_data->flags.abnormal_eof || event_data->flags.normal_eof) { + uhci_ctrl->rx_dir.node_index = 0; + +#if CONFIG_PM_ENABLE + // release power manager lock + if (uhci_ctrl->pm_lock) { + esp_pm_lock_release(uhci_ctrl->pm_lock); } +#endif + atomic_store(&uhci_ctrl->rx_dir.rx_fsm, UHCI_RX_FSM_ENABLE); + } else { uhci_ctrl->rx_dir.node_index++; // Go back to 0 as its a circle descriptor link if (uhci_ctrl->rx_dir.node_index >= uhci_ctrl->rx_dir.rx_num_dma_nodes) { uhci_ctrl->rx_dir.node_index = 0; } - - } else { - // eof event - size_t rx_size = gdma_link_count_buffer_size_till_eof(uhci_ctrl->rx_dir.dma_link, uhci_ctrl->rx_dir.node_index); - uhci_rx_event_data_t evt_data = { - .data = uhci_ctrl->rx_dir.buffer_pointers[uhci_ctrl->rx_dir.node_index], - .recv_size = rx_size, - .flags.totally_received = true, - }; - - if (is_buf_from_psram) { - esp_psram_mspi_mb(); - } - // release power manager lock - if (uhci_ctrl->pm_lock) { - esp_pm_lock_release(uhci_ctrl->pm_lock); - } - - // Same reasoning as the partial branch. rx_size here may not be a multiple of cache_line - // because transfer can end mid-buffer on a UART idle EOF, so round up to the next cache - // line (esp_cache_msync's M2C direction requires aligned size and doesn't accept the - // UNALIGNED flag). The extra bytes still belong to the user buffer so invalidating them - // is harmless. - if (cache_line > 0) { - size_t sync_size = (rx_size + cache_line - 1) & ~(cache_line - 1); - esp_cache_msync((void *)evt_data.data, sync_size, ESP_CACHE_MSYNC_FLAG_DIR_M2C); - } - if (uhci_ctrl->rx_dir.on_rx_trans_event) { - need_yield |= uhci_ctrl->rx_dir.on_rx_trans_event(uhci_ctrl, &evt_data, uhci_ctrl->user_data); - } - - // Stop the transaction when EOF is detected. In case for length EOF, there is no further more callback to be invoked. - gdma_stop(uhci_ctrl->rx_dir.dma_chan); - gdma_reset(uhci_ctrl->rx_dir.dma_chan); - - atomic_store(&uhci_ctrl->rx_dir.rx_fsm, UHCI_RX_FSM_ENABLE); - uhci_ctrl->rx_dir.node_index = 0; } - if (event_data->flags.abnormal_eof) { - esp_rom_printf(DRAM_STR("An abnormal eof on uhci detected\n")); + if (uhci_ctrl->rx_dir.on_rx_trans_event) { + need_yield |= uhci_ctrl->rx_dir.on_rx_trans_event(uhci_ctrl, &evt_data, uhci_ctrl->user_data); } return need_yield; @@ -305,59 +300,73 @@ static void uhci_do_transmit(uhci_controller_handle_t uhci_ctrl, uhci_transactio esp_err_t uhci_receive(uhci_controller_handle_t uhci_ctrl, uint8_t *read_buffer, size_t buffer_size) { ESP_RETURN_ON_FALSE(uhci_ctrl, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - ESP_RETURN_ON_FALSE((read_buffer != NULL), ESP_ERR_INVALID_ARG, TAG, "read buffer null"); + ESP_RETURN_ON_FALSE(read_buffer != NULL && buffer_size > 0, ESP_ERR_INVALID_ARG, TAG, "read buffer null or buffer size is 0"); - uint32_t mem_cache_line_size = esp_ptr_external_ram(read_buffer) ? uhci_ctrl->ext_mem_cache_line_size : uhci_ctrl->int_mem_cache_line_size; + uhci_rx_fsm_t expected_fsm = UHCI_RX_FSM_ENABLE; + ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&uhci_ctrl->rx_dir.rx_fsm, &expected_fsm, UHCI_RX_FSM_RUN_WAIT), ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); + esp_err_t ret = ESP_OK; + + const uint32_t mem_cache_line_size = esp_ptr_external_ram(read_buffer) ? uhci_ctrl->ext_mem_cache_line_size : uhci_ctrl->int_mem_cache_line_size; // Must take cache line into consideration for C2M operation. - uint32_t max_alignment_needed = UHCI_MAX(UHCI_MAX(uhci_ctrl->rx_dir.int_mem_align, uhci_ctrl->rx_dir.ext_mem_align), mem_cache_line_size); + const uint32_t max_alignment_needed = UHCI_MAX(UHCI_MAX(uhci_ctrl->rx_dir.int_mem_align, uhci_ctrl->rx_dir.ext_mem_align), mem_cache_line_size); + uhci_ctrl->rx_dir.cache_line = mem_cache_line_size; // Align the read_buffer pointer to mem_cache_line_size if (max_alignment_needed > 0 && (((uintptr_t)read_buffer) & (max_alignment_needed - 1)) != 0) { uintptr_t aligned_address = ((uintptr_t)read_buffer + max_alignment_needed - 1) & ~(max_alignment_needed - 1); size_t offset = aligned_address - (uintptr_t)read_buffer; - ESP_RETURN_ON_FALSE(buffer_size > offset, ESP_ERR_INVALID_ARG, TAG, "buffer size too small to align"); + ESP_GOTO_ON_FALSE(buffer_size > offset, ESP_ERR_INVALID_ARG, err, TAG, "buffer size too small to align"); read_buffer = (uint8_t *)aligned_address; buffer_size -= offset; } - uhci_ctrl->rx_dir.cache_line = mem_cache_line_size; - uhci_rx_fsm_t expected_fsm = UHCI_RX_FSM_ENABLE; - ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&uhci_ctrl->rx_dir.rx_fsm, &expected_fsm, UHCI_RX_FSM_RUN_WAIT), ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); - - size_t node_count = uhci_ctrl->rx_dir.rx_num_dma_nodes; - + const size_t node_count = uhci_ctrl->rx_dir.rx_num_dma_nodes; // Initialize the mount configurations for each DMA node, making sure every node is properly aligned. size_t usable_size = (max_alignment_needed == 0) ? buffer_size : (buffer_size / max_alignment_needed) * max_alignment_needed; size_t base_size = (max_alignment_needed == 0) ? usable_size / node_count : (usable_size / node_count / max_alignment_needed) * max_alignment_needed; size_t remaining_size = usable_size - (base_size * node_count); - gdma_buffer_mount_config_t mount_configs[node_count]; - memset(mount_configs, 0, node_count * sizeof(gdma_buffer_mount_config_t)); + { + gdma_buffer_mount_config_t mount_configs[node_count]; + memset(mount_configs, 0, node_count * sizeof(gdma_buffer_mount_config_t)); - for (size_t i = 0; i < node_count; i++) { - uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] = base_size; - uhci_ctrl->rx_dir.buffer_pointers[i] = read_buffer; - size_t buffer_alignment = esp_ptr_internal(read_buffer) ? uhci_ctrl->rx_dir.int_mem_align : uhci_ctrl->rx_dir.ext_mem_align; - // Distribute the remaining size to the first few nodes - if (remaining_size >= max_alignment_needed) { - uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] += max_alignment_needed; - remaining_size -= max_alignment_needed; + for (size_t i = 0; i < node_count; i++) { + uhci_ctrl->rx_dir.buffer_pointers[i] = read_buffer; + // Distribute the remaining size to the first few nodes + if (remaining_size >= max_alignment_needed) { + uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] = base_size + max_alignment_needed; + remaining_size -= max_alignment_needed; + } else { + uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] = base_size; + } + ESP_GOTO_ON_FALSE(uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] != 0 && uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] <= DMA_DESCRIPTOR_BUFFER_MAX_SIZE, + ESP_ERR_INVALID_ARG, err, TAG, "buffer_size is too small or too large"); + + size_t buffer_alignment = esp_ptr_internal(read_buffer) ? uhci_ctrl->rx_dir.int_mem_align : uhci_ctrl->rx_dir.ext_mem_align; + mount_configs[i] = (gdma_buffer_mount_config_t) { + .buffer = read_buffer, + .buffer_alignment = buffer_alignment, + .length = uhci_ctrl->rx_dir.buffer_size_per_desc_node[i], + .flags = { + .mark_final = GDMA_FINAL_LINK_TO_DEFAULT, + } + }; + ESP_LOGD(TAG, "The DMA node %d has %d byte", i, uhci_ctrl->rx_dir.buffer_size_per_desc_node[i]); + + read_buffer += uhci_ctrl->rx_dir.buffer_size_per_desc_node[i]; } - mount_configs[i] = (gdma_buffer_mount_config_t) { - .buffer = read_buffer, - .buffer_alignment = buffer_alignment, - .length = uhci_ctrl->rx_dir.buffer_size_per_desc_node[i], - .flags = { - .mark_final = GDMA_FINAL_LINK_TO_DEFAULT, - } - }; - ESP_LOGD(TAG, "The DMA node %d has %d byte", i, uhci_ctrl->rx_dir.buffer_size_per_desc_node[i]); - ESP_RETURN_ON_FALSE(uhci_ctrl->rx_dir.buffer_size_per_desc_node[i] != 0, ESP_ERR_INVALID_STATE, TAG, "Allocate dma node length is 0, please reconfigure the buffer_size"); - read_buffer += uhci_ctrl->rx_dir.buffer_size_per_desc_node[i]; + ESP_GOTO_ON_ERROR(gdma_link_mount_buffers(uhci_ctrl->rx_dir.dma_link, 0, mount_configs, node_count, NULL), err, TAG, "DMA link mount buffers failed"); + + // Invalidate cache before DMA starts to ensure no dirty cache lines. + // All DMA nodes (mount_configs) share the same contiguous user buffer, so checking mount_configs[0].buffer is sufficient. + bool need_cache_sync = esp_ptr_internal(mount_configs[0].buffer) ? (uhci_ctrl->int_mem_cache_line_size > 0) : (uhci_ctrl->ext_mem_cache_line_size > 0); + if (need_cache_sync) { + ESP_GOTO_ON_ERROR(esp_cache_msync(mount_configs[0].buffer, usable_size, ESP_CACHE_MSYNC_FLAG_DIR_M2C), err, TAG, "cache sync failed"); + } } // acquire power manager lock @@ -365,27 +374,23 @@ esp_err_t uhci_receive(uhci_controller_handle_t uhci_ctrl, uint8_t *read_buffer, esp_pm_lock_acquire(uhci_ctrl->pm_lock); } - gdma_link_mount_buffers(uhci_ctrl->rx_dir.dma_link, 0, mount_configs, node_count, NULL); - - // Invalidate cache before DMA starts to ensure no dirty cache lines. - // All DMA nodes (mount_configs) share the same contiguous user buffer, so checking mount_configs[0].buffer is sufficient. - bool need_cache_sync = esp_ptr_internal(mount_configs[0].buffer) ? (uhci_ctrl->int_mem_cache_line_size > 0) : (uhci_ctrl->ext_mem_cache_line_size > 0); - if (need_cache_sync) { - ESP_RETURN_ON_ERROR(esp_cache_msync(mount_configs[0].buffer, usable_size, ESP_CACHE_MSYNC_FLAG_DIR_M2C), TAG, "cache sync failed"); - } - atomic_store(&uhci_ctrl->rx_dir.rx_fsm, UHCI_RX_FSM_RUN); gdma_reset(uhci_ctrl->rx_dir.dma_chan); gdma_start(uhci_ctrl->rx_dir.dma_chan, gdma_link_get_head_addr(uhci_ctrl->rx_dir.dma_link)); return ESP_OK; +err: + atomic_store(&uhci_ctrl->rx_dir.rx_fsm, UHCI_RX_FSM_ENABLE); + return ret; } esp_err_t uhci_transmit(uhci_controller_handle_t uhci_ctrl, uint8_t *write_buffer, size_t write_size) { ESP_RETURN_ON_FALSE(uhci_ctrl, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - ESP_RETURN_ON_FALSE((write_buffer != NULL), ESP_ERR_INVALID_ARG, TAG, "write buffer null"); + ESP_RETURN_ON_FALSE((write_buffer != NULL) && (write_size > 0), ESP_ERR_INVALID_ARG, TAG, "write buffer null or write size is 0"); + ESP_RETURN_ON_FALSE(write_size <= uhci_ctrl->tx_dir.max_transmit_size, ESP_ERR_INVALID_ARG, TAG, + "write size %zu exceeds max_transmit_size %zu", write_size, uhci_ctrl->tx_dir.max_transmit_size); size_t alignment = 0; size_t cache_line_size = 0; @@ -432,20 +437,29 @@ esp_err_t uhci_del_controller(uhci_controller_handle_t uhci_ctrl) { ESP_RETURN_ON_FALSE(uhci_ctrl, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - if (uhci_ctrl->rx_dir.rx_fsm != UHCI_RX_FSM_ENABLE) { + uhci_rx_fsm_t expected_rx = UHCI_RX_FSM_ENABLE; + if (!atomic_compare_exchange_strong(&uhci_ctrl->rx_dir.rx_fsm, &expected_rx, UHCI_RX_FSM_DELETE)) { ESP_LOGE(TAG, "RX transaction is not finished, delete controller failed"); return ESP_ERR_INVALID_STATE; } - if (uhci_ctrl->tx_dir.tx_fsm != UHCI_TX_FSM_ENABLE) { + uhci_tx_fsm_t expected_tx = UHCI_TX_FSM_ENABLE; + if (!atomic_compare_exchange_strong(&uhci_ctrl->tx_dir.tx_fsm, &expected_tx, UHCI_TX_FSM_DELETE)) { ESP_LOGE(TAG, "TX transaction is not finished, delete controller failed"); + atomic_store(&uhci_ctrl->rx_dir.rx_fsm, UHCI_RX_FSM_ENABLE); // rollback return ESP_ERR_INVALID_STATE; } + // Ensure that all interrupts (GDMA callbacks) have completed and that no further callbacks can be + // triggered before releasing the resources. + ESP_RETURN_ON_ERROR(uhci_gdma_deinitialize(uhci_ctrl), TAG, "deinitialize uhci dma channel failed"); + UHCI_RCC_ATOMIC() { uhci_ll_enable_bus_clock(uhci_ctrl->uhci_num, false); } + uhci_hal_deinit(&uhci_ctrl->hal); + for (int i = 0; i < UHCI_TRANS_QUEUE_MAX; i++) { if (uhci_ctrl->tx_dir.trans_queues[i]) { vQueueDeleteWithCaps(uhci_ctrl->tx_dir.trans_queues[i]); @@ -467,10 +481,6 @@ esp_err_t uhci_del_controller(uhci_controller_handle_t uhci_ctrl) ESP_RETURN_ON_ERROR(esp_pm_lock_delete(uhci_ctrl->pm_lock), TAG, "delete rx pm_lock failed"); } - ESP_RETURN_ON_ERROR(uhci_gdma_deinitialize(uhci_ctrl), TAG, "deinitialize uhci dam channel failed"); - - uhci_hal_deinit(&uhci_ctrl->hal); - s_uhci_platform.controller[uhci_ctrl->uhci_num] = NULL; heap_caps_free(uhci_ctrl); @@ -512,6 +522,8 @@ esp_err_t uhci_new_controller(const uhci_controller_config_t *config, uhci_contr ESP_GOTO_ON_FALSE(uhci_ctrl->tx_dir.trans_queues[i], ESP_ERR_NO_MEM, err, TAG, "no mem for transaction queue"); } + uhci_ctrl->tx_dir.max_transmit_size = config->max_transmit_size; + uhci_ctrl->tx_dir.trans_desc_pool = heap_caps_calloc(config->tx_trans_queue_depth, sizeof(uhci_transaction_desc_t), UHCI_MEM_ALLOC_CAPS); ESP_GOTO_ON_FALSE(uhci_ctrl->tx_dir.trans_desc_pool, ESP_ERR_NO_MEM, err, TAG, "no mem for transaction desc pool"); uhci_transaction_desc_t *p_trans_desc = NULL; diff --git a/components/esp_driver_uart/src/uhci_private.h b/components/esp_driver_uart/src/uhci_private.h index e48906e3deb..c3c8a757045 100644 --- a/components/esp_driver_uart/src/uhci_private.h +++ b/components/esp_driver_uart/src/uhci_private.h @@ -54,6 +54,7 @@ typedef enum { UHCI_TX_FSM_ENABLE, /**< FSM is enabling the UHCI system. */ UHCI_TX_FSM_RUN_WAIT, /**< FSM is waiting to transition to the running state. */ UHCI_TX_FSM_RUN, /**< FSM is in the running state, actively handling UHCI operations. */ + UHCI_TX_FSM_DELETE, /**< FSM is claimed by uhci_del_controller() for teardown, no new transaction is accepted. */ } uhci_tx_fsm_t; typedef enum { @@ -68,6 +69,7 @@ typedef enum { UHCI_RX_FSM_ENABLE, /**< FSM is enabling the UHCI system. */ UHCI_RX_FSM_RUN_WAIT, /**< FSM is waiting to transition to the running state. */ UHCI_RX_FSM_RUN, /**< FSM is in the running state, actively handling UHCI operations. */ + UHCI_RX_FSM_DELETE, /**< FSM is claimed by uhci_del_controller() for teardown, no new transaction is accepted. */ } uhci_rx_fsm_t; typedef struct { @@ -81,6 +83,7 @@ typedef struct { size_t int_mem_align; // Alignment for internal memory size_t ext_mem_align; // Alignment for external memory atomic_int num_trans_inflight; // Indicates the number of transactions that are undergoing but not recycled to ready_queue + size_t max_transmit_size; // per-transaction max size in bytes, from config->max_transmit_size; the DMA node pool is sized for this } uhci_tx_dir; typedef struct { diff --git a/components/esp_driver_uart/test_apps/uhci/main/test_uhci.c b/components/esp_driver_uart/test_apps/uhci/main/test_uhci.c index 2253533cb9d..6b4270a6b3e 100644 --- a/components/esp_driver_uart/test_apps/uhci/main/test_uhci.c +++ b/components/esp_driver_uart/test_apps/uhci/main/test_uhci.c @@ -73,6 +73,47 @@ TEST_CASE("UHCI controller install-uninstall test", "[uhci]") TEST_ESP_OK(uhci_del_controller(uhci_ctrl)); } +TEST_CASE("UHCI receive/transmit reject invalid buffer size", "[uhci]") +{ + uhci_controller_config_t uhci_cfg = { + .uart_port = EX_UART_NUM, + .tx_trans_queue_depth = 3, + .max_receive_internal_mem = 2 * 1024, + .max_transmit_size = 2 * 1024, + .dma_burst_size = 32, + .rx_eof_flags.idle_eof = 1, + }; + + uhci_controller_handle_t uhci_ctrl; + TEST_ESP_OK(uhci_new_controller(&uhci_cfg, &uhci_ctrl)); + + const size_t oversize = 5 * 1024; + uint8_t *big_buf = heap_caps_calloc(1, oversize, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + TEST_ASSERT_NOT_NULL(big_buf); + + // -------- uhci_receive -------- + + // buffer_size == 0 must be rejected. + TEST_ESP_ERR(ESP_ERR_INVALID_ARG, uhci_receive(uhci_ctrl, big_buf, 0)); + + // Excessively large buffers must be rejected. + TEST_ESP_ERR(ESP_ERR_INVALID_ARG, uhci_receive(uhci_ctrl, big_buf, oversize)); + + // -------- uhci_transmit -------- + + // write_size == 0 must be rejected. + TEST_ESP_ERR(ESP_ERR_INVALID_ARG, uhci_transmit(uhci_ctrl, big_buf, 0)); + + // A single write_size exceeding max_transmit_size must be rejected. + TEST_ESP_ERR(ESP_ERR_INVALID_ARG, uhci_transmit(uhci_ctrl, big_buf, oversize)); + + free(big_buf); + + // The controller must still be in a clean, deletable state after all the rejected + // calls above, which confirms the RX/TX FSMs were correctly rolled back on error. + TEST_ESP_OK(uhci_del_controller(uhci_ctrl)); +} + typedef enum { UHCI_EVT_PARTIAL_DATA, UHCI_EVT_EOF, @@ -148,7 +189,7 @@ static void uhci_receive_test(void *arg) if (evt == UHCI_EVT_EOF) { disp_buf(receive_data, ctx->receive_size); for (int i = 0; i < ctx->receive_size; i++) { - TEST_ASSERT(receive_data[i] == (uint8_t)i); + TEST_ASSERT_EQUAL(receive_data[i], (uint8_t)i); } printf("Received size: %d\n", ctx->receive_size); break; diff --git a/docs/en/api-reference/peripherals/uhci.rst b/docs/en/api-reference/peripherals/uhci.rst index 9d04e182fd8..afbd76a279f 100644 --- a/docs/en/api-reference/peripherals/uhci.rst +++ b/docs/en/api-reference/peripherals/uhci.rst @@ -52,7 +52,7 @@ If the configurations in :cpp:type:`uhci_controller_config_t` is specified, user uhci_controller_config_t uhci_cfg = { .uart_port = EX_UART_NUM, // Connect uart port to UHCI hardware. .tx_trans_queue_depth = 30, // Queue depth of transaction queue. - .max_receive_internal_mem = 10 * 1024, // internal memory usage, for more information, please refer to API reference. + .max_receive_internal_mem = 10 * 1024, // Expected max uhci_receive() buffer size; also sizes the RX DMA descriptor chain. For large transfers, configure it so that at least two descriptors are available for ping-pong operation. .max_transmit_size = 10 * 1024, // Maximum transfer size in one transaction, in bytes. .dma_burst_size = 32, // Burst size. .rx_eof_flags.idle_eof = 1, // When to trigger a end of frame event, you can choose `idle_eof`, `rx_brk_eof`, `length_eof`, for more information, please refer to API reference. @@ -165,7 +165,7 @@ Data can be received via UHCI as follows: } } -In the API :cpp:func:`uhci_receive` interface, the parameter `read_buffer` is a buffer that must be provided by the user, and parameter `buffer_size` represents the size of the buffer supplied by the user. In the configuration structure of the UHCI controller, the parameter :cpp:member:`uhci_controller_config_t::max_receive_internal_mem` specifies the desired size of the internal DMA working space. The software allocates a certain number of DMA nodes based on this working space size. These nodes form a circular linked list. +In the API :cpp:func:`uhci_receive` interface, the parameter ``read_buffer`` is a buffer that must be provided by the user, and parameter ``buffer_size`` represents the size of the buffer supplied by the user. ``buffer_size`` should generally not exceed :cpp:member:`uhci_controller_config_t::max_receive_internal_mem`. When a node is filled, but the reception has not yet completed, the event :cpp:member:`uhci_event_callbacks_t::on_rx_trans_event` will be triggered, accompanied by :cpp:member:`uhci_rx_event_data_t::flags::totally_received` set to 0. When all the data has been fully received, the :cpp:member:`uhci_event_callbacks_t::on_rx_trans_event` event will be triggered again with :cpp:member:`uhci_rx_event_data_t::flags::totally_received` set to 1. @@ -173,7 +173,7 @@ This mechanism allows the user to achieve continuous and fast reception using a .. note:: - The parameter `read_buffer` of :cpp:func:`uhci_receive` cannot be freed until receive finishes. + The parameter ``read_buffer`` of :cpp:func:`uhci_receive` cannot be freed until receive finishes. Uninstall UHCI controller ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/zh_CN/api-reference/peripherals/uhci.rst b/docs/zh_CN/api-reference/peripherals/uhci.rst index 5a671f12bea..e7ddd93624e 100644 --- a/docs/zh_CN/api-reference/peripherals/uhci.rst +++ b/docs/zh_CN/api-reference/peripherals/uhci.rst @@ -52,8 +52,8 @@ UHCI 控制器需要通过 :cpp:type:`uhci_controller_config_t` 进行配置。 uhci_controller_config_t uhci_cfg = { .uart_port = EX_UART_NUM, // 将指定 UART 端口连接到 UHCI 硬件 .tx_trans_queue_depth = 30, // 发送队列的队列深度 - .max_receive_internal_mem = 10 * 1024, // 内部接收内存大小，更多信息请参考 API 注释。 - .max_transmit_size = 10 * 1024, // 单次传输的最大传输量，单位是字节 + .max_receive_internal_mem = 10 * 1024, // uhci_receive() 期望的最大缓冲区大小，同时决定 RX DMA 描述符链长度。对于较大的传输，建议将该值配置为至少会分配两个描述符，以便进行乒乓操作。 + .max_transmit_size = 10 * 1024, // 一次传输事务中的最大总字节数 .dma_burst_size = 32, // 突发传输大小 .rx_eof_flags.idle_eof = 1, // 结束帧的条件，用户可以选择 `idle_eof`, `rx_brk_eof` 和 `length_eof`, 关于更多信息请参考 API 注释. }; @@ -165,9 +165,9 @@ RX 事件数据在 :cpp:type:`uhci_rx_event_data_t` 中定义： } } -在 API :cpp:func:`uhci_receive` 接口中，参数 ``read_buffer`` 是用户必须提供的缓冲区，参数 ``buffer_size`` 表示用户提供的缓冲区大小。在 UHCI 控制器的配置结构中，参数 :cpp:member:`uhci_controller_config_t::max_receive_internal_mem` 指定了内部 DMA 工作空间的期望大小。软件将根据此工作空间大小分配一定数量的 DMA 节点，这些节点形成一个循环链表。 +在 API :cpp:func:`uhci_receive` 接口中，参数 ``read_buffer`` 是用户必须提供的缓冲区，参数 ``buffer_size`` 表示用户提供的缓冲区大小。``buffer_size`` 一般不得超过 :cpp:member:`uhci_controller_config_t::max_receive_internal_mem`。 -当一个节点被填满，但接收尚未完成时，将触发 :cpp:member:`uhci_event_callbacks_t::on_rx_trans_event` 事件，且 :cpp:member:`uhci_rx_event_data_t::flags::totally_received` 的值为 0。 当所有数据接收完成时，该事件将再次被触发，并且 :cpp:member:`uhci_rx_event_data_t::flags::totally_received` 的值为 1。 +当一个节点被填满，但接收尚未完成时，将触发 :cpp:member:`uhci_event_callbacks_t::on_rx_trans_event` 事件，且 :cpp:member:`uhci_rx_event_data_t::flags::totally_received` 的值为 0。当所有数据接收完成时，该事件将再次被触发，并且 :cpp:member:`uhci_rx_event_data_t::flags::totally_received` 的值为 1。 此机制允许用户使用相对较小的缓冲区实现连续且快速的接收，而无需分配与接收总数据量相等大小的缓冲区。