anchao commented on code in PR #17640:
URL: https://github.com/apache/nuttx/pull/17640#discussion_r2642387984
##########
sched/wdog/wd_gettime.c:
##########
@@ -54,14 +54,13 @@
sclock_t wd_gettime(FAR struct wdog_s *wdog)
{
- sclock_t delay;
+ sclock_t delay = 0;
- if (wdog == NULL || !WDOG_ISACTIVE(wdog))
+ if (wdog && WDOG_ISACTIVE(wdog))
{
- return 0;
+ delay = (sclock_t)(wdog->expired - clock_systime_ticks());
+ delay = delay < 0 ? 0 : delay;
Review Comment:
```suggestion
delay = delay >= 0 ? delay : 0;
```
##########
sched/wdog/wd_cancel.c:
##########
@@ -61,34 +61,33 @@
int wd_cancel(FAR struct wdog_s *wdog)
{
irqstate_t flags;
- bool head;
+ bool reassess = false;
+ int ret = -EINVAL;
- flags = spin_lock_irqsave(&g_wdspinlock);
+ flags = enter_critical_section();
/* Make sure that the watchdog is valid and still active. */
- if (wdog == NULL || !WDOG_ISACTIVE(wdog))
+ if (wdog != NULL && WDOG_ISACTIVE(wdog))
Review Comment:
move wdog == NULL check out of lock
##########
sched/wdog/wd_start.c:
##########
@@ -265,62 +258,63 @@ int wd_start_abstick(FAR struct wdog_s *wdog, clock_t
ticks,
wdentry_t wdentry, wdparm_t arg)
{
irqstate_t flags;
- bool reassess = false;
+ bool reassess = false;
+ int ret = -EINVAL;
/* Verify the wdog and setup parameters */
- if (wdog == NULL || wdentry == NULL)
+ if (wdog != NULL && wdentry != NULL)
Review Comment:
why not return here directly?
##########
sched/wdog/wd_cancel.c:
##########
@@ -61,34 +61,33 @@
int wd_cancel(FAR struct wdog_s *wdog)
{
irqstate_t flags;
- bool head;
+ bool reassess = false;
+ int ret = -EINVAL;
- flags = spin_lock_irqsave(&g_wdspinlock);
+ flags = enter_critical_section();
/* Make sure that the watchdog is valid and still active. */
- if (wdog == NULL || !WDOG_ISACTIVE(wdog))
+ if (wdog != NULL && WDOG_ISACTIVE(wdog))
{
- spin_unlock_irqrestore(&g_wdspinlock, flags);
- return -EINVAL;
- }
+ /* Prohibit timer interactions with the timer queue until the
+ * cancellation is complete
+ */
- sched_note_wdog(NOTE_WDOG_CANCEL, (FAR void *)wdog->func,
- (FAR void *)(uintptr_t)wdog->expired);
+ reassess = list_is_head(&g_wdactivelist, &wdog->node);
- /* Prohibit timer interactions with the timer queue until the
- * cancellation is complete
- */
+ /* Now, remove the watchdog from the timer queue */
- head = list_is_head(&g_wdactivelist, &wdog->node);
+ list_delete_fast(&wdog->node);
- /* Now, remove the watchdog from the timer queue */
+ /* Mark the watchdog inactive */
- list_delete(&wdog->node);
+ wdog->func = NULL;
- spin_unlock_irqrestore(&g_wdspinlock, flags);
+ ret = OK;
Review Comment:
could we remove return value of wd_cancel ? Whether a timer is in the queue
or not, the return value is meaningless to the caller.
##########
sched/wdog/wd_gettime.c:
##########
@@ -54,14 +54,13 @@
sclock_t wd_gettime(FAR struct wdog_s *wdog)
{
- sclock_t delay;
+ sclock_t delay = 0;
- if (wdog == NULL || !WDOG_ISACTIVE(wdog))
+ if (wdog && WDOG_ISACTIVE(wdog))
Review Comment:
race condition here?
##########
sched/wdog/wd_cancel.c:
##########
@@ -61,34 +61,33 @@
int wd_cancel(FAR struct wdog_s *wdog)
{
irqstate_t flags;
- bool head;
+ bool reassess = false;
+ int ret = -EINVAL;
- flags = spin_lock_irqsave(&g_wdspinlock);
+ flags = enter_critical_section();
Review Comment:
why switch back to critical section?
##########
sched/wdog/wd_start.c:
##########
@@ -354,37 +348,35 @@ clock_t wd_timer(clock_t ticks, bool noswitches)
{
FAR struct wdog_s *wdog;
irqstate_t flags;
- sclock_t ret;
+ clock_t ret = CLOCK_MAX;
/* Check if the watchdog at the head of the list is ready to run */
if (!noswitches)
{
- wd_expiration(ticks);
+ ret = wd_expiration(ticks);
}
-
- flags = spin_lock_irqsave(&g_wdspinlock);
-
- /* Return the delay for the next watchdog to expire */
-
- if (list_is_empty(&g_wdactivelist))
+ else
{
- spin_unlock_irqrestore(&g_wdspinlock, flags);
- return CLOCK_MAX;
- }
+ flags = enter_critical_section();
Review Comment:
```suggestion
ret = CLOCK_MAX;
flags = enter_critical_section();
```
--
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]
