From c82e51cf79ea732a41454f6144513c08b8e4720e Mon Sep 17 00:00:00 2001 From: michael Date: Fri, 18 Aug 2017 15:15:47 +0800 Subject: [PATCH] fix(intr): always assign the same intr to a same source, disable the source only when all the handlers disabled. also document handlers sharing a same source. TW#13454, https://github.com/nodemcu/nodemcu-firmware/issues/1874 Breaking change: handles assigned to a same source should have the same flag now. --- components/esp32/include/esp_intr_alloc.h | 14 +- components/esp32/intr_alloc.c | 210 +++++++++++++++------- components/esp32/test/test_intr_alloc.c | 69 +++++++ docs/api-reference/system/intr_alloc.rst | 15 ++ 4 files changed, 244 insertions(+), 64 deletions(-) diff --git a/components/esp32/include/esp_intr_alloc.h b/components/esp32/include/esp_intr_alloc.h index d0ca1df90..ad121abb3 100644 --- a/components/esp32/include/esp_intr_alloc.h +++ b/components/esp32/include/esp_intr_alloc.h @@ -197,6 +197,11 @@ esp_err_t esp_intr_alloc_intrstatus(int source, int flags, uint32_t intrstatusre * Use an interrupt handle to disable the interrupt and release the resources * associated with it. * + * @note + * When the handler shares its source with other handlers, the interrupt status + * bits it's responsible for should be managed properly before freeing it. see + * ``esp_intr_disable`` for more details. + * * @param handle The handle, as obtained by esp_intr_alloc or esp_intr_alloc_intrstatus * * @return ESP_ERR_INVALID_ARG if handle is invalid, or esp_intr_free runs on another core than @@ -227,8 +232,13 @@ int esp_intr_get_intno(intr_handle_t handle); /** * @brief Disable the interrupt associated with the handle * - * @note For local interrupts (ESP_INTERNAL_* sources), this function has to be called on the - * CPU the interrupt is allocated on. Other interrupts have no such restriction. + * @note + * 1. For local interrupts (ESP_INTERNAL_* sources), this function has to be called on the + * CPU the interrupt is allocated on. Other interrupts have no such restriction. + * 2. When several handlers sharing a same interrupt source, interrupt status bits, which are + * handled in the handler to be disabled, should be masked before the disabling, or handled + * in other enabled interrupts properly. Miss of interrupt status handling will cause infinite + * interrupt calls and finally system crash. * * @param handle The handle, as obtained by esp_intr_alloc or esp_intr_alloc_intrstatus * diff --git a/components/esp32/intr_alloc.c b/components/esp32/intr_alloc.c index c51b348c9..7d8f205ca 100644 --- a/components/esp32/intr_alloc.c +++ b/components/esp32/intr_alloc.c @@ -168,7 +168,7 @@ struct non_shared_isr_arg_t { }; //Linked list of vector descriptions, sorted by cpu.intno value -static vector_desc_t *vector_desc_head; +static vector_desc_t *vector_desc_head = NULL; //This bitmask has an 1 if the int should be disabled when the flash is disabled. static uint32_t non_iram_int_mask[portNUM_PROCESSORS]; @@ -234,6 +234,32 @@ static vector_desc_t *get_desc_for_int(int intno, int cpu) } } +//Returns a vector_desc entry for an source, the cpu parameter is used to tell GPIO_INT and GPIO_NMI from different CPUs +static vector_desc_t * find_desc_for_source(int source, int cpu) +{ + vector_desc_t *vd=vector_desc_head; + while(vd!=NULL) { + if ( !(vd->flags & VECDESC_FL_SHARED) ) { + if ( vd->source == source && cpu == vd->cpu ) break; + } else if ( vd->cpu == cpu ) { + // check only shared vds for the correct cpu, otherwise skip + bool found = false; + shared_vector_desc_t *svd = vd->shared_vec_info; + assert(svd != NULL ); + while(svd) { + if ( svd->source == source ) { + found = true; + break; + } + svd = svd->next; + } + if ( found ) break; + } + vd=vd->next; + } + return vd; +} + esp_err_t esp_intr_mark_shared(int intno, int cpu, bool is_int_ram) { if (intno>31) return ESP_ERR_INVALID_ARG; @@ -286,11 +312,70 @@ static bool int_has_handler(int intr, int cpu) return (_xt_interrupt_table[intr*portNUM_PROCESSORS+cpu].handler != xt_unhandled_interrupt); } +static bool is_vect_desc_usable(vector_desc_t *vd, int flags, int cpu, int force) +{ + //Check if interrupt is not reserved by design + int x = vd->intno; + if (int_desc[x].cpuflags[cpu]==INTDESC_RESVD) { + ALCHLOG(TAG, "....Unusable: reserved"); + return false; + } + if (int_desc[x].cpuflags[cpu]==INTDESC_SPECIAL && force==-1) { + ALCHLOG(TAG, "....Unusable: special-purpose int"); + return false; + } + //Check if the interrupt level is acceptable + if (!(flags&(1<flags&VECDESC_FL_RESERVED) { + ALCHLOG(TAG, "....Unusable: reserved at runtime."); + return false; + } + + //Ints can't be both shared and non-shared. + assert(!((vd->flags&VECDESC_FL_SHARED)&&(vd->flags&VECDESC_FL_NONSHARED))); + //check if interrupt already is in use by a non-shared interrupt + if (vd->flags&VECDESC_FL_NONSHARED) { + ALCHLOG(TAG, "....Unusable: already in (non-shared) use."); + return false; + } + // check shared interrupt flags + if (vd->flags&VECDESC_FL_SHARED ) { + if (flags&ESP_INTR_FLAG_SHARED) { + bool in_iram_flag=((flags&ESP_INTR_FLAG_IRAM)!=0); + bool desc_in_iram_flag=((vd->flags&VECDESC_FL_INIRAM)!=0); + //Bail out if int is shared, but iram property doesn't match what we want. + if ((vd->flags&VECDESC_FL_SHARED) && (desc_in_iram_flag!=in_iram_flag)) { + ALCHLOG(TAG, "....Unusable: shared but iram prop doesn't match"); + return false; + } + } else { + //We need an unshared IRQ; can't use shared ones; bail out if this is shared. + ALCHLOG(TAG, "...Unusable: int is shared, we need non-shared."); + return false; + } + } else if (int_has_handler(x, cpu)) { + //Check if interrupt already is allocated by xt_set_interrupt_handler + ALCHLOG(TAG, "....Unusable: already allocated"); + return false; + } + + return true; +} //Locate a free interrupt compatible with the flags given. //The 'force' argument can be -1, or 0-31 to force checking a certain interrupt. //When a CPU is forced, the INTDESC_SPECIAL marked interrupts are also accepted. -static int get_free_int(int flags, int cpu, int force) +static int get_available_int(int flags, int cpu, int force, int source) { int x; int best=-1; @@ -299,69 +384,61 @@ static int get_free_int(int flags, int cpu, int force) //Default vector desc, for vectors not in the linked list vector_desc_t empty_vect_desc; memset(&empty_vect_desc, 0, sizeof(vector_desc_t)); + + //Level defaults to any low/med interrupt if (!(flags&ESP_INTR_FLAG_LEVELMASK)) flags|=ESP_INTR_FLAG_LOWMED; + ALCHLOG(TAG, "get_available_int: try to find existing. Cpu: %d, Source: %d", cpu, source); + vector_desc_t *vd = find_desc_for_source(source, cpu); + if ( vd ) { + // if existing vd found, don't need to search any more. + ALCHLOG(TAG, "get_avalible_int: existing vd found. intno: %d", vd->intno); + if ( force != -1 && force != vd->intno ) { + ALCHLOG(TAG, "get_avalible_int: intr forced but not matach existing. existing intno: %d, force: %d", vd->intno, force); + } else if ( !is_vect_desc_usable(vd, flags, cpu, force) ) { + ALCHLOG(TAG, "get_avalible_int: existing vd invalid."); + } else { + best = vd->intno; + } + return best; + } + if (force!=-1) { + ALCHLOG(TAG, "get_available_int: try to find force. Cpu: %d, Source: %d, Force: %d", cpu, source, force); + //if force assigned, don't need to search any more. + vd = find_desc_for_int(force, cpu); + if (vd == NULL ) { + //if existing vd not found, just check the default state for the intr. + empty_vect_desc.intno = force; + vd = &empty_vect_desc; + } + if ( is_vect_desc_usable(vd, flags, cpu, force) ) { + best = vd->intno; + } else { + ALCHLOG(TAG, "get_avalible_int: forced vd invalid."); + } + return best; + } + ALCHLOG(TAG, "get_free_int: start looking. Current cpu: %d", cpu); - //Iterate over the 32 possible interrupts + //No allocated handlers as well as forced intr, iterate over the 32 possible interrupts for (x=0; x<32; x++) { //Grab the vector_desc for this vector. - vector_desc_t *vd=find_desc_for_int(x, cpu); - if (vd==NULL) vd=&empty_vect_desc; - //See if we have a forced interrupt; if so, bail out if this is not it. - if (force!=-1 && force!=x) { - ALCHLOG(TAG, "Ignoring int %d: forced to %d", x, force); - continue; + vd=find_desc_for_int(x, cpu); + if (vd==NULL) { + empty_vect_desc.intno = x; + vd=&empty_vect_desc; } + ALCHLOG(TAG, "Int %d reserved %d level %d %s hasIsr %d", x, int_desc[x].cpuflags[cpu]==INTDESC_RESVD, int_desc[x].level, int_desc[x].type==INTTP_LEVEL?"LEVEL":"EDGE", int_has_handler(x, cpu)); - //Check if interrupt is not reserved by design - if (int_desc[x].cpuflags[cpu]==INTDESC_RESVD) { - ALCHLOG(TAG, "....Unusable: reserved"); - continue; - } - if (int_desc[x].cpuflags[cpu]==INTDESC_SPECIAL && force==-1) { - ALCHLOG(TAG, "....Unusable: special-purpose int"); - continue; - } - //Check if the interrupt level is acceptable - if (!(flags&(1<flags&VECDESC_FL_SHARED)) { - ALCHLOG(TAG, "....Unusable: already allocated"); - continue; - } - //Ints can't be both shared and non-shared. - assert(!((vd->flags&VECDESC_FL_SHARED)&&(vd->flags&VECDESC_FL_NONSHARED))); - //check if interrupt is reserved at runtime - if (vd->flags&VECDESC_FL_RESERVED) { - ALCHLOG(TAG, "....Unusable: reserved at runtime."); - continue; - } - //check if interrupt already is in use by a non-shared interrupt - if (vd->flags&VECDESC_FL_NONSHARED) { - ALCHLOG(TAG, "....Unusable: already in (non-shared) use."); - continue; - } + + if ( !is_vect_desc_usable(vd, flags, cpu, force) ) continue; + if (flags&ESP_INTR_FLAG_SHARED) { //We're allocating a shared int. - bool in_iram_flag=((flags&ESP_INTR_FLAG_IRAM)!=0); - bool desc_in_iram_flag=((vd->flags&VECDESC_FL_INIRAM)!=0); - //Bail out if int is shared, but iram property doesn't match what we want. - if ((vd->flags&VECDESC_FL_SHARED) && (desc_in_iram_flag!=in_iram_flag)) { - ALCHLOG(TAG, "....Unusable: shared but iram prop doesn't match"); - continue; - } + //See if int already is used as a shared interrupt. if (vd->flags&VECDESC_FL_SHARED) { //We can use this already-marked-as-shared interrupt. Count the already attached isrs in order to see @@ -396,11 +473,6 @@ static int get_free_int(int flags, int cpu, int force) } } } else { - //We need an unshared IRQ; can't use shared ones; bail out if this is shared. - if (vd->flags&VECDESC_FL_SHARED) { - ALCHLOG(TAG, "...Unusable: int is shared, we need non-shared."); - continue; - } //Seems this interrupt is feasible. Select it and break out of the loop; no need to search further. if (bestLevel>int_desc[x].level) { best=x; @@ -410,7 +482,7 @@ static int get_free_int(int flags, int cpu, int force) } } } - ALCHLOG(TAG, "get_free_int: using int %d", best); + ALCHLOG(TAG, "get_available_int: using int %d", best); //Okay, by now we have looked at all potential interrupts and hopefully have selected the best one in best. return best; @@ -508,7 +580,7 @@ esp_err_t esp_intr_alloc_intrstatus(int source, int flags, uint32_t intrstatusre portENTER_CRITICAL(&spinlock); int cpu=xPortGetCoreID(); //See if we can find an interrupt that matches the flags. - int intr=get_free_int(flags, cpu, force); + int intr=get_available_int(flags, cpu, force, source); if (intr==-1) { //None found. Bail out. portEXIT_CRITICAL(&spinlock); @@ -720,15 +792,29 @@ esp_err_t IRAM_ATTR esp_intr_disable(intr_handle_t handle) if (!handle) return ESP_ERR_INVALID_ARG; portENTER_CRITICAL(&spinlock); int source; + bool disabled = 1; if (handle->shared_vector_desc) { handle->shared_vector_desc->disabled=1; source=handle->shared_vector_desc->source; + + shared_vector_desc_t *svd=handle->vector_desc->shared_vec_info; + assert( svd != NULL ); + while( svd ) { + if ( svd->source == source && svd->disabled == 0 ) { + disabled = 0; + break; + } + svd = svd->next; + } } else { source=handle->vector_desc->source; } + if (source >= 0) { - //Disable using int matrix - intr_matrix_set(handle->vector_desc->cpu, source, INT_MUX_DISABLED_INTNO); + if ( disabled ) { + //Disable using int matrix + intr_matrix_set(handle->vector_desc->cpu, source, INT_MUX_DISABLED_INTNO); + } } else { //Disable using per-cpu regs if (handle->vector_desc->cpu!=xPortGetCoreID()) { diff --git a/components/esp32/test/test_intr_alloc.c b/components/esp32/test/test_intr_alloc.c index a5e6f0683..23d9504d1 100644 --- a/components/esp32/test/test_intr_alloc.c +++ b/components/esp32/test/test_intr_alloc.c @@ -228,3 +228,72 @@ TEST_CASE("Can allocate IRAM int only with an IRAM handler", "[esp32]") err = esp_intr_free(ih); TEST_ESP_OK(err); } + + +#include "soc/spi_struct.h" +typedef struct { + bool flag1; + bool flag2; + bool flag3; + bool flag4; +} intr_alloc_test_ctx_t; + +void IRAM_ATTR int_handler1(void* arg) +{ + intr_alloc_test_ctx_t* ctx=(intr_alloc_test_ctx_t*)arg; + ets_printf("handler 1 called.\n"); + if ( ctx->flag1 ) { + ctx->flag3 = true; + } else { + ctx->flag1 = true; + } + SPI2.slave.trans_done = 0; +} + +void IRAM_ATTR int_handler2(void* arg) +{ + intr_alloc_test_ctx_t* ctx = (intr_alloc_test_ctx_t*)arg; + ets_printf("handler 2 called.\n"); + if ( ctx->flag2 ) { + ctx->flag4 = true; + } else { + ctx->flag2 = true; + } +} + +TEST_CASE("allocate 2 handlers for a same source and remove the later one","[esp32]") +{ + intr_alloc_test_ctx_t ctx = {false, false, false, false }; + intr_handle_t handle1, handle2; + + //enable spi + DPORT_SET_PERI_REG_MASK(DPORT_PERIP_CLK_EN_REG, DPORT_SPI_CLK_EN ); + DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG, DPORT_SPI_RST); + + esp_err_t r; + r=esp_intr_alloc(ETS_SPI2_INTR_SOURCE, ESP_INTR_FLAG_SHARED, int_handler1, &ctx, &handle1); + TEST_ESP_OK(r); + //try an invalid assign first + r=esp_intr_alloc(ETS_SPI2_INTR_SOURCE, NULL, int_handler2, NULL, &handle2); + TEST_ASSERT_EQUAL_INT(r, ESP_ERR_NOT_FOUND ); + //assign shared then + r=esp_intr_alloc(ETS_SPI2_INTR_SOURCE, ESP_INTR_FLAG_SHARED, int_handler2, &ctx, &handle2); + TEST_ESP_OK(r); + SPI2.slave.trans_inten = 1; + + printf("trigger first time.\n"); + SPI2.slave.trans_done = 1; + + vTaskDelay(100); + TEST_ASSERT( ctx.flag1 && ctx.flag2 ); + + printf("remove intr 1.\n"); + r=esp_intr_free(handle2); + + printf("trigger second time.\n"); + SPI2.slave.trans_done = 1; + + vTaskDelay(500); + TEST_ASSERT( ctx.flag3 && !ctx.flag4 ); + printf("test passed.\n"); +} diff --git a/docs/api-reference/system/intr_alloc.rst b/docs/api-reference/system/intr_alloc.rst index 7fc20dfc8..d03630df9 100644 --- a/docs/api-reference/system/intr_alloc.rst +++ b/docs/api-reference/system/intr_alloc.rst @@ -83,6 +83,21 @@ It can also be useful to keep an interrupt handler in IRAM if it is called very Refer to the :ref:`SPI flash API documentation ` for more details. +Multiple Handlers Sharing A Source +---------------------------------- + +Several handlers can be assigned to a same source, given that all handlers are allocated using the ``ESP_INTR_FLAG_SHARED`` flag. +They'll be all allocated to the interrupt, which the source is attached to, and called sequentially when the source is active. +The handlers can be disabled and freed individually. The source is attached to the interrupt (enabled), if one or more handlers are enabled, otherwise detached. +A handler will never be called when disabled, while **its source may still be triggered** if any one of its handler enabled. + +Sources attached to non-shared interrupt do not support this feature. + +Though the framework support this feature, you have to use it *very carefully*. There usually exist 2 ways to stop a interrupt from being triggered: *disable the sourse* or *mask peripheral interrupt status*. +IDF only handles the enabling and disabling of the source itself, leaving status and mask bits to be handled by users. **Status bits should always be masked before the handler responsible for it is disabled, +or the status should be handled in other enabled interrupt properly**. You may leave some status bits unhandled if you just disable one of all the handlers without mask the status bits, which causes the interrupt being triggered infinitely, +and finally a system crash. + API Reference -------------