In the pass, the stack info for IDLE thread is just used for debugging purpose, so the priority is low, but we need boost this to high priority since TLS(errno) depend on it now
On Fri, May 22, 2020 at 8:25 PM David Sidrane <david.sidr...@nscdg.com> wrote: > > This is super confusing to me. > > The todo list says. > > Status: Open > Priority: Low. Things are working OK the way they are. But the > design > could be improved and made a little more efficient with > this > change. > > So it sounds like no big deal. Hardfaulting on set_errno() it is a big > deal. > > > Did we destroy active acrhs with a partial implementation and not realize > it? TLS(errno) implementation is fully completed, it's an arch problem not the core OS issue. > > Why was this work not done on a branch and tested? > This work initially do on a branch and merge into master after finish. > Where is the process? Where is the PMC agreeing on these sweeping changes? PR follow the process, the contributor sumbit the path, PMC review the code and approve. Here is the email: https://lists.apache.org/thread.html/r76ac7aa434cf982acd4c7227ede7221ad7a09b9235a99c188f716199%40%3Cdev.nuttx.apache.org%3E > > David > > > > -----Original Message----- > From: Xiang Xiao [mailto:xiaoxiang781...@gmail.com] > Sent: Friday, May 22, 2020 4:51 AM > To: dev@nuttx.apache.org > Subject: Re: Arm Kinetis: set_errno in nx_start with CONFIG_TLS_ALIGNED not > set > > The new implementation of set_errno require all thread(include IDLE) > initialize the stack related fields in tcb_s correctly. But many > chipset forget to setup them for IDLE thread. TODO contain an item > described this: > Task: IDLE THREAD TCB SETUP > Description: There are issues with setting IDLE thread stacks: > > 1. One problem is stack-related data in the IDLE threads TCB. > A solution might be to standardize the use of > g_idle_topstack. > That you could add initialization like this in nx_start: > > @@ -344,6 +347,11 @@ void nx_start(void) > g_idleargv[1] = NULL; > g_idletcb.argv = g_idleargv; > > + /* Set the IDLE task stack size */ > + > + g_idletcb.cmn.adj_stack_size = > CONFIG_IDLETHREAD_STACKSIZE; > + g_idletcb.cmn.stack_alloc_ptr = (void > *)(g_idle_topstack - CONFIG_IDLETHREAD_STACKSIZE); > + > /* Then add the idle task's TCB to the head of > the ready to run list */ > > dq_addfirst((FAR dq_entry_t *)&g_idletcb, (FAR > dq_queue_t *)&g_readytorun); > > The g_idle_topstack variable is available for almost > all architectures: > > $ find . -name *.h | xargs grep g_idle_top > ./arm/src/common/up_internal.h:EXTERN const uint32_t > g_idle_topstack; > ./avr/src/avr/avr.h:extern uint16_t g_idle_topstack; > ./avr/src/avr32/avr32.h:extern uint32_t g_idle_topstack; > ./hc/src/common/up_internal.h:extern uint16_t > g_idle_topstack; > ./mips/src/common/up_internal.h:extern uint32_t > g_idle_topstack; > ./misoc/src/lm32/lm32.h:extern uint32_t g_idle_topstack; > ./renesas/src/common/up_internal.h:extern uint32_t > g_idle_topstack; > ./renesas/src/m16c/chip.h:extern uint32_t > g_idle_topstack; /* Start of the heap */ > ./risc-v/src/common/up_internal.h:EXTERN uint32_t > g_idle_topstack; > ./x86/src/common/up_internal.h:extern uint32_t > g_idle_topstack; > > That omits these architectures: sh1, sim, xtensa, z16, > z80, > ez80, and z8. All would have to support this common > global variable. > > Also, the stack itself may be 8-, 16-, or 32-bits wide, > depending upon the architecture and do have differing > alignment requirements. > > 2. Another problem is colorizing that stack to use with > stack usage monitoring logic. There is logic in some > start functions to do this in a function called > go_nx_start. > It is available in these architectures: > > ./arm/src/efm32/efm32_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/kinetis/kinetis_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/sam34/sam_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/samv7/sam_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/stm32/stm32_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/stm32f7/stm32_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/stm32l4/stm32l4_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/tms570/tms570_boot.c:static void > go_nx_start(void *pv, unsigned int nbytes) > ./arm/src/xmc4/xmc4_start.c:static void > go_nx_start(void *pv, unsigned int nbytes) > So, it become a block issue and need immedately fix now. > > On Fri, May 22, 2020 at 7:40 PM Schock, Johannes - NIVUS GmbH > <johannes.sch...@nivus.com> wrote: > > > > Hello, > > I think there's a problem with set_errno() in nx_start(), when > > CONFIG_TLS_ALIGNED is not set: > > I disabled SERIAL_CONSOLE and DEV_CONSOLE, because I wanted to test > > CDCACM_CONSOLE. Now calling fs_fdopen() in group_setupstreams() leads to a > > call of set_errno(). > > But since tls_get_info() returns NULL pointer for stack_alloc_ptr the > > board goes into hardfault. > > For the CONFIG_TLS_ALIGNED case it would return the actual stack pointer > > from arm_getsp() which accesses the stack pointer directly. > > I think thread local storage for nx_start() should be handled differently, > > or the stack_alloc_ptr should be set to a sane value. > > I attach my config for reference, perhaps I'm missing something. > > > > Regards, Johannes > > > >
