[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #1912: Fix up_interrupt_context() for SMP

[GitBox <gi...@apache.org>]

masayuki2009 commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r496328310



##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
 
 bool up_interrupt_context(void)
 {
-  return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+  irqstate_t flags = up_irq_save();

Review comment:
       > Just to clarify, it is non-atomic in the SMP case because CURRENT_REGS expands to:
   > 
   > ```
   > #  define CURRENT_REGS (g_current_regs[up_cpu_index()])
   >```
   
   @patacongo 
   
   Thanks for the comments.
   
   Yes, what I noticed at a Wi-Fi streaming aging test with Spresense was
   NuttX stopped with DEBUGASSERT at the following code.
   
   ```
   nxplayer> [ 4916.595630] cxd56_configure: ERROR: Unknown feature unit: 2048
   [ 4916.598621] cxd56_configure: ERROR: Unknown feature unit: 4
   [ 4916.604175] cxd56_configure: ERROR: Unknown feature unit: 16
   [ 4982.154812] cxd56_start_dma: Underrun
   [ 4982.205165] up_assert: Assertion failed CPU1 at file:semaphore/sem_wait.c line: 78 task: gs2200m
   
   int nxsem_wait(FAR sem_t *sem)                                                                                                                                                                
   {                                                                                                                                                                                             
     FAR struct tcb_s *rtcb = this_task();                                                                                                                                                       
     irqstate_t flags;                                                                                                                                                                           
     int ret = -EINVAL;                                                                                                                                                                          
                                                                                                                                                                                                 
     /* This API should not be called from interrupt handlers */                                                                                                                                 
                                                                                                                                                                                                 
   =>  DEBUGASSERT(sem != NULL && up_interrupt_context() == false);    
   ````
   
   Actually, the sem was not NULL but g_current_regs were the following values.
   
   ```
   (gdb) p g_current_regs
   $1 = {0x0, 0xd05df54}
   ```
   
   Here, the DEBUGASSERT() happened on CPU1, and it was not in the interrupt context.
   However, CPU0 was in the interrupt context, so I found that the task was moved from
   CPU0 to CPU1 during calling up_interruput_context() in nxsem_wait().
   
   With this PR, a Wi-Fi streaming aging test is still running over 12hrs.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org