Fix Memory leak in addApbChangeCallback() (espressif#3560)

* `ledcWriteTone()` added a `apbcallback()` evertime the tone value was non zero.  
* `addApbChangeCallback()` did not detect duplicate callbacks.
* changed the apbcallback list to a double link to support roll forward, roll back execution.  This made the sequences of clock change callback start with the newest registered -> to oldest on the `before` then oldest -> newest after the clock change.  This made the UART debug log output have minimal gibberish during the clock change.
* change how the UART callback handled the MUTEX because if any `apbchangeCallback()` executed a `log_x()` a deadlock would occur.

This fixes espressif#3555

Author: stickbreaker

cores/esp32/esp32-hal-cpu.c

@@ -28,6 +28,7 @@
 #include "esp32-hal-cpu.h"
 
 typedef struct apb_change_cb_s {
+        struct apb_change_cb_s * prev;
         struct apb_change_cb_s * next;
         void * arg;
         apb_change_cb_t cb;
@@ -53,9 +54,19 @@ static void triggerApbChangeCallback(apb_change_ev_t ev_type, uint32_t old_apb,
     initApbChangeCallback();
     xSemaphoreTake(apb_change_lock, portMAX_DELAY);
     apb_change_t * r = apb_change_callbacks;
-    while(r != NULL){
-        r->cb(r->arg, ev_type, old_apb, new_apb);
-        r=r->next;
+    if( r != NULL ){
+        if(ev_type == APB_BEFORE_CHANGE )
+            while(r != NULL){
+                r->cb(r->arg, ev_type, old_apb, new_apb);
+                r=r->next;
+            }
+        else { // run backwards through chain
+            while(r->next != NULL) r = r->next; // find first added
+            while( r != NULL){
+                r->cb(r->arg, ev_type, old_apb, new_apb);
+                r=r->prev;
+            }
+        }
     }
     xSemaphoreGive(apb_change_lock);
 }
@@ -68,25 +79,28 @@ bool addApbChangeCallback(void * arg, apb_change_cb_t cb){
         return false;
     }
     c->next = NULL;
+    c->prev = NULL;
     c->arg = arg;
     c->cb = cb;
     xSemaphoreTake(apb_change_lock, portMAX_DELAY);
     if(apb_change_callbacks == NULL){
         apb_change_callbacks = c;
     } else {
         apb_change_t * r = apb_change_callbacks;
-        if(r->cb != cb || r->arg != arg){
-            while(r->next){
-                r = r->next;
-                if(r->cb == cb && r->arg == arg){
-                    free(c);
-                    goto unlock_and_exit;
-                }
-            }
-            r->next = c;
+        // look for duplicate callbacks
+        while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next;
+        if (r) {
+            log_e("duplicate func=%08X arg=%08X",c->cb,c->arg);
+            free(c);
+            xSemaphoreGive(apb_change_lock);
+            return false;
+        }
+        else {
+            c->next = apb_change_callbacks;
+            apb_change_callbacks-> prev = c;
+            apb_change_callbacks = c;
         }
     }
-unlock_and_exit:
     xSemaphoreGive(apb_change_lock);
     return true;
 }
@@ -95,24 +109,21 @@ bool removeApbChangeCallback(void * arg, apb_change_cb_t cb){
     initApbChangeCallback();
     xSemaphoreTake(apb_change_lock, portMAX_DELAY);
     apb_change_t * r = apb_change_callbacks;
-    if(r == NULL){
+    // look for matching callback
+    while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next;
+    if ( r == NULL ) {
+        log_e("not found func=%08X arg=%08X",cb,arg);
         xSemaphoreGive(apb_change_lock);
         return false;
-    }
-    if(r->cb == cb && r->arg == arg){
-        apb_change_callbacks = r->next;
-        free(r);
-    } else {
-        while(r->next && (r->next->cb != cb || r->next->arg != arg)){
-            r = r->next;
         }
-        if(r->next == NULL || r->next->cb != cb || r->next->arg != arg){
-            xSemaphoreGive(apb_change_lock);
-            return false;
+    else {
+        // patch links
+        if(r->prev) r->prev->next = r->next;
+        else { // this is first link
+           apb_change_callbacks = r->next;
         }
-        apb_change_t * c = r->next;
-        r->next = c->next;
-        free(c);
+        if(r->next) r->next->prev = r->prev;
+        free(r);
     }
     xSemaphoreGive(apb_change_lock);
     return true;
@@ -186,7 +197,7 @@ bool setCpuFrequencyMhz(uint32_t cpu_freq_mhz){
         //Update REF_TICK (uncomment if REF_TICK is different than 1MHz)
         //if(conf.freq_mhz < 80){
         //    ESP_REG(APB_CTRL_XTAL_TICK_CONF_REG) = conf.freq_mhz / (REF_CLK_FREQ / MHZ) - 1;
-        //}
+        // }
         //Update APB Freq REG
         rtc_clk_apb_freq_update(apb);
         //Update esp_timer divisor

cores/esp32/esp32-hal-ledc.c

@@ -28,7 +28,7 @@
 #else
 #define LEDC_MUTEX_LOCK()    do {} while (xSemaphoreTake(_ledc_sys_lock, portMAX_DELAY) != pdPASS)
 #define LEDC_MUTEX_UNLOCK()  xSemaphoreGive(_ledc_sys_lock)
-xSemaphoreHandle _ledc_sys_lock;
+xSemaphoreHandle _ledc_sys_lock = NULL;
 #endif
 
 /*
@@ -55,27 +55,35 @@ xSemaphoreHandle _ledc_sys_lock;
 
 static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb, uint32_t new_apb){
     if(ev_type == APB_AFTER_CHANGE && old_apb != new_apb){
-        uint32_t iarg = (uint32_t)arg;
-        uint8_t chan = iarg;
-        uint8_t group=(chan/8), timer=((chan/2)%4);
+        uint16_t iarg = *(uint16_t*)arg;
+        uint8_t chan = 0;
         old_apb /= 1000000;
         new_apb /= 1000000;
-        if(LEDC_TIMER(group, timer).conf.tick_sel){
-            LEDC_MUTEX_LOCK();
-            uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider;
-            uint32_t div_num = (new_apb * old_div) / old_apb;
-            if(div_num > LEDC_DIV_NUM_HSTIMER0_V){
-                new_apb = REF_CLK_FREQ / 1000000;
-                div_num = (new_apb * old_div) / old_apb;
-                if(div_num > LEDC_DIV_NUM_HSTIMER0_V) {
-                    div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible
+        while(iarg){ // run though all active channels, adjusting timing configurations
+            if(iarg & 1) {// this channel is active
+                uint8_t group=(chan/8), timer=((chan/2)%4);
+                if(LEDC_TIMER(group, timer).conf.tick_sel){
+                    LEDC_MUTEX_LOCK();
+                    uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider;
+                    uint32_t div_num = (new_apb * old_div) / old_apb;
+                    if(div_num > LEDC_DIV_NUM_HSTIMER0_V){
+                        div_num = ((REF_CLK_FREQ /1000000) * old_div) / old_apb;
+                        if(div_num > LEDC_DIV_NUM_HSTIMER0_V) {
+                            div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible
+                        }
+                        LEDC_TIMER(group, timer).conf.tick_sel = 0;
+                    } else if(div_num < 256) {
+                        div_num = 256;//highest clock possible
+                    }
+                    LEDC_TIMER(group, timer).conf.clock_divider = div_num;
+                    LEDC_MUTEX_UNLOCK();
+                }
+                else {
+                    log_d("using REF_CLK chan=%d",chan);
                 }
-                LEDC_TIMER(group, timer).conf.tick_sel = 0;
-            } else if(div_num < 256) {
-                div_num = 256;//highest clock possible
             }
-            LEDC_TIMER(group, timer).conf.clock_divider = div_num;
-            LEDC_MUTEX_UNLOCK();
+            iarg = iarg >> 1;
+            chan++;
         }
     }
 }
@@ -85,11 +93,14 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo
 {
     uint8_t group=(chan/8), timer=((chan/2)%4);
     static bool tHasStarted = false;
+    static uint16_t _activeChannels = 0;
     if(!tHasStarted) {
         tHasStarted = true;
         DPORT_SET_PERI_REG_MASK(DPORT_PERIP_CLK_EN_REG, DPORT_LEDC_CLK_EN);
         DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG, DPORT_LEDC_RST);
         LEDC.conf.apb_clk_sel = 1;//LS use apb clock
+        addApbChangeCallback((void*)&_activeChannels, _on_apb_change);
+
 #if !CONFIG_DISABLE_HAL_LOCKS
         _ledc_sys_lock = xSemaphoreCreateMutex();
 #endif
@@ -105,8 +116,7 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo
     LEDC_TIMER(group, timer).conf.rst = 1;//This bit is used to reset timer the counter will be 0 after reset.
     LEDC_TIMER(group, timer).conf.rst = 0;
     LEDC_MUTEX_UNLOCK();
-    uint32_t iarg = chan;
-    addApbChangeCallback((void*)iarg, _on_apb_change);
+    _activeChannels |= (1 << chan); // mark as active for APB callback
 }
 
 //max div_num 0x3FFFF (262143)

cores/esp32/esp32-hal-uart.c

@@ -217,7 +217,6 @@ uart_t* uartBegin(uint8_t uart_nr, uint32_t baudrate, uint32_t config, int8_t rx
     if(txPin != -1) {
         uartAttachTx(uart, txPin, inverted);
     }
-
     addApbChangeCallback(uart, uart_on_apb_change);
     return uart;
 }
@@ -377,18 +376,21 @@ static void uart_on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old
         uart->dev->int_clr.val = 0xffffffff;
         // read RX fifo
         uint8_t c;
-        BaseType_t xHigherPriorityTaskWoken;
+   //     BaseType_t xHigherPriorityTaskWoken;
         while(uart->dev->status.rxfifo_cnt != 0 || (uart->dev->mem_rx_status.wr_addr != uart->dev->mem_rx_status.rd_addr)) {
             c = uart->dev->fifo.rw_byte;
-            if(uart->queue != NULL && !xQueueIsQueueFullFromISR(uart->queue)) {
-                xQueueSendFromISR(uart->queue, &c, &xHigherPriorityTaskWoken);
+            if(uart->queue != NULL ) {
+                xQueueSend(uart->queue, &c, 1); //&xHigherPriorityTaskWoken);
             }
         }
+        UART_MUTEX_UNLOCK();
+ 
         // wait TX empty
         while(uart->dev->status.txfifo_cnt || uart->dev->status.st_utx_out);
     } else {
         //todo:
         // set baudrate
+        UART_MUTEX_LOCK();
         uint32_t clk_div = (uart->dev->clk_div.div_int << 4) | (uart->dev->clk_div.div_frag & 0x0F);
         uint32_t baud_rate = ((old_apb<<4)/clk_div);
         clk_div = ((new_apb<<4)/baud_rate);