Skip to content

Commit a94e277

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 10b08b6 commit a94e277

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
@@ -205,9 +205,15 @@ static bool cam_start_frame(int * frame_pos)
205205
return false;
206206
}
207207

208-
void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t * HPTaskAwoken)
208+
void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType_t *hp)
209209
{
210-
if (xQueueSendFromISR(cam->event_queue, (void *)&cam_event, HPTaskAwoken) != pdTRUE) {
210+
if (!cam->event_queue) {
211+
// ESP_DRAM_LOGD(TAG, "drop event: queue deleted");
212+
return;
213+
}
214+
215+
BaseType_t woken = pdFALSE;
216+
if (xQueueSendFromISR(cam->event_queue, &cam_event, &woken) != pdTRUE) {
211217
ll_cam_stop(cam);
212218
cam->state = CAM_STATE_IDLE;
213219
#if CAM_LOG_SPAM_EVERY_FRAME
@@ -218,6 +224,10 @@ void IRAM_ATTR ll_cam_send_event(cam_obj_t *cam, cam_event_t cam_event, BaseType
218224
cam_event==CAM_IN_SUC_EOF_EVENT ? "EV-EOF-OVF" : "EV-VSYNC-OVF");
219225
#endif
220226
}
227+
228+
if (hp && woken) {
229+
*hp = pdTRUE;
230+
}
221231
}
222232

223233
//Copy fram from DMA dma_buffer to fram dma_buffer
@@ -596,15 +606,17 @@ esp_err_t cam_deinit(void)
596606
return ESP_FAIL;
597607
}
598608

599-
cam_stop();
609+
cam_stop(); // disable VSYNC and DMA before deleting queues
600610
if (cam_obj->task_handle) {
601611
vTaskDelete(cam_obj->task_handle);
602612
}
603613
if (cam_obj->event_queue) {
604614
vQueueDelete(cam_obj->event_queue);
615+
cam_obj->event_queue = NULL;
605616
}
606617
if (cam_obj->frame_buffer_queue) {
607618
vQueueDelete(cam_obj->frame_buffer_queue);
619+
cam_obj->frame_buffer_queue = NULL;
608620
}
609621

610622
ll_cam_deinit(cam_obj);

target/esp32s3/ll_cam.c

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

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

7373
GDMA.channel[cam->dma_num].in.conf0.val = 0;
@@ -93,7 +93,7 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_vsync_isr(void *arg)
9393
{
9494
//DBG_PIN_SET(1);
9595
cam_obj_t *cam = (cam_obj_t *)arg;
96-
BaseType_t HPTaskAwoken = pdFALSE;
96+
BaseType_t hp = pdFALSE;
9797

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

105105
if (status.cam_vsync_int_st) {
106-
ll_cam_send_event(cam, CAM_VSYNC_EVENT, &HPTaskAwoken);
106+
ll_cam_send_event(cam, CAM_VSYNC_EVENT, &hp);
107107
}
108108

109-
if (HPTaskAwoken == pdTRUE) {
109+
if (hp == pdTRUE) {
110110
portYIELD_FROM_ISR();
111111
}
112112
//DBG_PIN_SET(0);
@@ -115,7 +115,7 @@ static void CAMERA_ISR_IRAM_ATTR ll_cam_vsync_isr(void *arg)
115115
static void CAMERA_ISR_IRAM_ATTR ll_cam_dma_isr(void *arg)
116116
{
117117
cam_obj_t *cam = (cam_obj_t *)arg;
118-
BaseType_t HPTaskAwoken = pdFALSE;
118+
BaseType_t hp = pdFALSE;
119119

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

127127
if (status.in_suc_eof) {
128-
ll_cam_send_event(cam, CAM_IN_SUC_EOF_EVENT, &HPTaskAwoken);
128+
ll_cam_send_event(cam, CAM_IN_SUC_EOF_EVENT, &hp);
129129
}
130130

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

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

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

155154
LCD_CAM.cam_ctrl1.cam_reset = 1;
156155
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)