Skip to content

Commit 9a95a22

Browse files
committed
cam_hal/ll_cam: avoid ISR queue use-after-free; fully quiesce DMA on stop
In PSRAM DMA mode the driver could dereference a deleted event queue and leave EOF interrupts armed after shutdown, leading to late ISRs and NPDs. Changes: - ll_cam_send_event(): return early if event_queue is NULL; use a local `woken` and propagate to caller via `hp` when non-NULL. - cam_deinit(): call cam_stop() before deleting queues; NULL out event_queue and frame_buffer_queue after vQueueDelete(). - ll_cam_stop(): disable all GDMA IN interrupts and clear them with a full W1C mask; stop CAM_START and halt link. - ll_cam_start(): clear all interrupts, then enable only IN_SUC_EOF. - ll_cam_dma_reset(): use 0xFFFFFFFF W1C mask for int_clr. - ISRs (VSYNC/DMA): use `hp` variable and portYIELD_FROM_ISR() if set. - cam_obj.state: mark as volatile. - Headers: update ll_cam_send_event() signature to `BaseType_t *hp`. Result: no null-pointer derefs or late interrupts after teardown; ISR wakeups propagate correctly and the DMA path restarts cleanly.
1 parent 89e8d4d commit 9a95a22

File tree

3 files changed

+31
-20
lines changed

3 files changed

+31
-20
lines changed

driver/cam_hal.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,15 @@ static bool cam_start_frame(int * frame_pos)
242242
return false;
243243
}
244244

245-
void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t * HPTaskAwoken)
245+
void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t *hp)
246246
{
247-
if (xQueueSendFromISR(cam->event_queue, (void *)&cam_event, HPTaskAwoken) != pdTRUE) {
247+
if (!cam->event_queue) {
248+
// ESP_DRAM_LOGD(TAG, "drop event: queue deleted");
249+
return;
250+
}
251+
252+
BaseType_t woken = pdFALSE;
253+
if (xQueueSendFromISR(cam->event_queue, &cam_event, &woken) != pdTRUE) {
248254
ll_cam_stop(cam);
249255
cam->state = CAM_STATE_IDLE;
250256
#if CAM_LOG_SPAM_EVERY_FRAME
@@ -255,6 +261,10 @@ void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType
255261
cam_event==CAM_IN_SUC_EOF_EVENT ? "EV-EOF-OVF" : "EV-VSYNC-OVF");
256262
#endif
257263
}
264+
265+
if (hp && woken) {
266+
*hp = pdTRUE;
267+
}
258268
}
259269

260270
//Copy fram from DMA dma_buffer to fram dma_buffer
@@ -633,15 +643,17 @@ esp_err_t cam_deinit(void)
633643
return ESP_FAIL;
634644
}
635645

636-
cam_stop();
646+
cam_stop(); // disable VSYNC and DMA before deleting queues
637647
if (cam_obj->task_handle) {
638648
vTaskDelete(cam_obj->task_handle);
639649
}
640650
if (cam_obj->event_queue) {
641651
vQueueDelete(cam_obj->event_queue);
652+
cam_obj->event_queue = NULL;
642653
}
643654
if (cam_obj->frame_buffer_queue) {
644655
vQueueDelete(cam_obj->frame_buffer_queue);
656+
cam_obj->frame_buffer_queue = NULL;
645657
}
646658

647659
ll_cam_deinit(cam_obj);

target/esp32s3/ll_cam.c

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ void ll_cam_dma_print_state(cam_obj_t *cam)
6868
void ll_cam_dma_reset(cam_obj_t *cam)
6969
{
7070

71-
GDMA.channel[cam->dma_num].in.int_clr.val = ~0;
71+
GDMA.channel[cam->dma_num].in.int_clr.val = 0xFFFFFFFFU; // register is W1C; mask covers all sources
7272
GDMA.channel[cam->dma_num].in.int_ena.val = 0;
7373

7474
GDMA.channel[cam->dma_num].in.conf0.val = 0;
@@ -94,7 +94,7 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_vsync_isr(void *arg)
9494
{
9595
//DBG_PIN_SET(1);
9696
cam_obj_t *cam = (cam_obj_t *)arg;
97-
BaseType_t HPTaskAwoken = pdFALSE;
97+
BaseType_t hp = pdFALSE;
9898

9999
typeof(LCD_CAM.lc_dma_int_st) status = LCD_CAM.lc_dma_int_st;
100100
if (status.val == 0) {
@@ -104,10 +104,10 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_vsync_isr(void *arg)
104104
LCD_CAM.lc_dma_int_clr.val = status.val;
105105

106106
if (status.cam_vsync_int_st) {
107-
ll_cam_send_event(cam, CAM_VSYNC_EVENT, &HPTaskAwoken);
107+
ll_cam_send_event(cam, CAM_VSYNC_EVENT, &hp);
108108
}
109109

110-
if (HPTaskAwoken == pdTRUE) {
110+
if (hp == pdTRUE) {
111111
portYIELD_FROM_ISR();
112112
}
113113
//DBG_PIN_SET(0);
@@ -116,7 +116,7 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_vsync_isr(void *arg)
116116
static void CAMERA_ISR_IRAM_ATTR ll_cam_dma_isr(void *arg)
117117
{
118118
cam_obj_t *cam = (cam_obj_t *)arg;
119-
BaseType_t HPTaskAwoken = pdFALSE;
119+
BaseType_t hp = pdFALSE;
120120

121121
typeof(GDMA.channel[cam->dma_num].in.int_st) status = GDMA.channel[cam->dma_num].in.int_st;
122122
if (status.val == 0) {
@@ -126,20 +126,19 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_dma_isr(void *arg)
126126
GDMA.channel[cam->dma_num].in.int_clr.val = status.val;
127127

128128
if (status.in_suc_eof) {
129-
ll_cam_send_event(cam, CAM_IN_SUC_EOF_EVENT, &HPTaskAwoken);
129+
ll_cam_send_event(cam, CAM_IN_SUC_EOF_EVENT, &hp);
130130
}
131131

132-
if (HPTaskAwoken == pdTRUE) {
132+
if (hp == pdTRUE) {
133133
portYIELD_FROM_ISR();
134134
}
135135
}
136136

137137
bool IRAM_ATTR ll_cam_stop(cam_obj_t *cam)
138138
{
139-
if (cam->jpeg_mode || !cam->psram_mode) {
140-
GDMA.channel[cam->dma_num].in.int_ena.in_suc_eof = 0;
141-
GDMA.channel[cam->dma_num].in.int_clr.in_suc_eof = 1;
142-
}
139+
GDMA.channel[cam->dma_num].in.int_ena.val = 0;
140+
GDMA.channel[cam->dma_num].in.int_clr.val = 0xFFFFFFFFU; // register is W1C; mask covers all sources
141+
LCD_CAM.cam_ctrl1.cam_start = 0;
143142
GDMA.channel[cam->dma_num].in.link.stop = 1;
144143
return true;
145144
}
@@ -148,10 +147,10 @@ bool ll_cam_start(cam_obj_t *cam, int frame_pos)
148147
{
149148
LCD_CAM.cam_ctrl1.cam_start = 0;
150149

151-
if (cam->jpeg_mode || !cam->psram_mode) {
152-
GDMA.channel[cam->dma_num].in.int_clr.in_suc_eof = 1;
153-
GDMA.channel[cam->dma_num].in.int_ena.in_suc_eof = 1;
154-
}
150+
GDMA.channel[cam->dma_num].in.int_ena.val = 0;
151+
GDMA.channel[cam->dma_num].in.int_clr.val = 0xFFFFFFFFU; // register is W1C; mask covers all sources
152+
GDMA.channel[cam->dma_num].in.int_clr.in_suc_eof = 1; // redundant but explicit
153+
GDMA.channel[cam->dma_num].in.int_ena.in_suc_eof = 1; // only EOF interrupt is re-enabled
155154

156155
LCD_CAM.cam_ctrl1.cam_reset = 1;
157156
LCD_CAM.cam_ctrl1.cam_reset = 0;

target/private_include/ll_cam.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ typedef struct {
140140
#endif
141141
uint32_t fb_size;
142142

143-
cam_state_t state;
143+
volatile cam_state_t state;
144144
} cam_obj_t;
145145

146146

@@ -162,4 +162,4 @@ void ll_cam_dma_reset(cam_obj_t *cam);
162162
#endif
163163

164164
// implemented in cam_hal
165-
void ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t * HPTaskAwoken);
165+
void ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t *hp);

0 commit comments

Comments
 (0)