Fix-Point commented on code in PR #16342:
URL: https://github.com/apache/nuttx/pull/16342#discussion_r2081300472


##########
sched/wqueue/kwork_queue.c:
##########
@@ -109,39 +104,52 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s 
*wqueue,
 
   flags = spin_lock_irqsave(&wqueue->lock);
 
+  /* Ensure the work has been removed. */
+
+  retimer = work_available(work) ? false : work_remove(wqueue, work);
+
   /* Initialize the work structure. */
 
   work->worker = worker;   /* Work callback. non-NULL means queued */
   work->arg    = arg;      /* Callback argument */
   work->qtime  = expected; /* Expected time */
   work->period = period;   /* Periodical delay */
 
-  /* Insert to the pending list of the wqueue. */
-
   if (delay)
     {
+      /* Insert to the pending list of the wqueue. */
+
       if (work_insert_pending(wqueue, work))
         {
           /* Start the timer if the work is the earliest expired work. */
 
-          ret = wd_start_abstick(&wqueue->timer, work->qtime,
-                                 work_timer_expired, (wdparm_t)wqueue);
+          retimer = false;
+          wd_start_abstick(&wqueue->timer, work->qtime,
+                           work_timer_expired, (wdparm_t)wqueue);
         }
     }
   else
     {
+      /* Insert to the expired list of the wqueue. */
+
       list_add_tail(&wqueue->expired, &work->node);
-      wake = true;
+    }
+
+  if (retimer)
+    {
+      work_timer_reset(wqueue);
     }
 
   spin_unlock_irqrestore(&wqueue->lock, flags);
 
-  if (wake)
+  if (!delay)
     {
+      /* Immediately wake up the worker thread. */
+
       nxsem_post(&wqueue->sem);
     }
 
-  return ret;
+  return OK;

Review Comment:
   Done.



##########
sched/wqueue/kwork_cancel.c:
##########
@@ -60,56 +60,49 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue, 
bool sync,
 
   flags = spin_lock_irqsave(&wqueue->lock);
 
-  /* Check whether we own the work structure. */
-
   if (!work_available(work))
     {
-      bool is_head = list_is_head(&wqueue->pending, &work->node);
-
-      /* Seize the ownership from the work thread. */
-
-      work->worker = NULL;
-
-      list_delete(&work->node);
-
       /* If the head of the pending queue has changed, we should reset
        * the wqueue timer.
        */
 
-      if (is_head)
+      if (work_remove(wqueue, work))
         {
-          if (!list_is_empty(&wqueue->pending))
-            {
-              work = list_first_entry(&wqueue->pending, struct work_s, node);
-
-              ret = wd_start_abstick(&wqueue->timer, work->qtime,
-                                     work_timer_expired, (wdparm_t)wqueue);
-            }
-          else
-            {
-              wd_cancel(&wqueue->timer);
-            }
+          work_timer_reset(wqueue);
         }
     }
-  else if (!up_interrupt_context() && !sched_idletask() && sync)
+
+  /* Note that cancel_sync can not be called in the interrupt
+   * context and the idletask context.
+   */
+
+  if (sync)
     {
       int wndx;
 
+      sync_wait = NULL;

Review Comment:
   Fixed.



-- 
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.

To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org

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

Reply via email to