Copilot commented on code in PR #20652:
URL: https://github.com/apache/nuttx/pull/20652#discussion_r4266868088
##########
sched/wqueue/kwork_thread.c:
##########
@@ -624,6 +604,31 @@ int work_queue_priority(int qid)
return work_queue_priority_wq(work_qid2wq(qid));
}
+/****************************************************************************
+ * Name: work_initialize
+ *
+ * Description:
+ * Set up the kernel work queues before any work can be queued. The
+ * queues live in .bss; only the self-referencing list heads and the
+ * thread count differ from the zero state.
+ *
+ ****************************************************************************/
+
+void work_initialize(void)
+{
+#ifdef CONFIG_SCHED_HPWORK
+ list_initialize(&g_hpwork.wq.expired);
+ list_initialize(&g_hpwork.wq.pending);
+ g_hpwork.wq.nthreads = CONFIG_SCHED_HPNTHREADS;
+#endif
+
+#ifdef CONFIG_SCHED_LPWORK
+ list_initialize(&g_lpwork.wq.expired);
+ list_initialize(&g_lpwork.wq.pending);
+ g_lpwork.wq.nthreads = CONFIG_SCHED_LPNTHREADS;
+#endif
+}
Review Comment:
Now that `g_hpwork`/`g_lpwork` are `.bss`-zeroed, this relies on non-list
fields (notably the `SP_UNLOCKED` spinlock state and any semaphore initializer
invariants) being equivalent to all-zeros. That assumption is not guaranteed by
the prior code (which explicitly used `SP_UNLOCKED` / `SEM_INITIALIZER(0)`). To
preserve correctness across architectures/configs, explicitly initialize those
fields in `work_initialize()` (e.g., set the lock to `SP_UNLOCKED` and call the
appropriate semaphore init routine used elsewhere in NuttX) rather than
depending on `.bss` layout.
##########
sched/task/task_spawnparms.c:
##########
@@ -227,7 +227,14 @@ int spawn_execattrs(pid_t pid, FAR const posix_spawnattr_t
*attr)
param.sched_ss_init_budget.tv_sec = attr->budget.tv_sec;
param.sched_ss_init_budget.tv_nsec = attr->budget.tv_nsec;
#endif
+#if CONFIG_RR_INTERVAL > 0 || defined(CONFIG_SCHED_SPORADIC)
ret = nxsched_set_scheduler(pid, attr->policy, ¶m);
+#else
+ /* SCHED_FIFO is the only policy, so only the priority can change */
Review Comment:
The comment says `SCHED_FIFO` is the only policy, but the code accepts
`SCHED_OTHER` as well. If `SCHED_OTHER` is intentionally treated as an alias
for FIFO in FIFO-only builds, please update the comment to state that
explicitly (or normalize `SCHED_OTHER` to `SCHED_FIFO` before applying params).
Otherwise, restrict acceptance to `SCHED_FIFO` to keep behavior/comment
consistent.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
