Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clock_timekeeping: remove enter_critical_section in sched/clock/clock_timekeeping.c #15136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+13 −8
Open

clock_timekeeping: remove enter_critical_section in sched/clock/clock_timekeeping.c #15136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions sched/clock/clock_timekeeping.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ static struct timespec g_clock_wall_time;
static uint64_t g_clock_last_counter;
static uint64_t g_clock_mask;
static long g_clock_adjust;
static spinlock_t g_clock_lock = SP_UNLOCKED;

/****************************************************************************
* Private Functions
Expand All @@ -72,7 +73,7 @@ static int clock_get_current_time(FAR struct timespec *ts,
time_t sec;
int ret;

flags = enter_critical_section();
flags = spin_lock_irqsave(&g_clock_lock);

ret = up_timer_gettick(&counter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ret = up_timer_gettick(&counter);
ret = up_timer_gettick(&counter); <---

same issue with #15129, how to protect lower half data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is no clear specification that requires up_timer_gettick to be called within a critical_section. Currently, there are numerous calls to up_timer_gettick that are not within a critical section. For example, clock_systime_ticks calls up_timer_gettick, but clock_systime_ticks itself is not within a critical_section in many places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key issue is that the scope of impact of your code before and after modification is different. You cannot guarantee that the modification will not bring side effects within the visible range. If you insist on making changes, please ignore my comments，I will not spend time reviewing similar submissions in the future.

if (ret < 0)
Expand All @@ -96,7 +97,7 @@ static int clock_get_current_time(FAR struct timespec *ts,
ts->tv_sec = base->tv_sec + sec;

errout_in_critical_section:
leave_critical_section(flags);
spin_unlock_irqrestore(&g_clock_lock, flags);
return ret;
}

Expand All @@ -123,7 +124,7 @@ int clock_timekeeping_set_wall_time(FAR const struct timespec *ts)
uint64_t counter;
int ret;

flags = enter_critical_section();
flags = spin_lock_irqsave(&g_clock_lock);

ret = up_timer_gettick(&counter);
if (ret < 0)
Expand All @@ -137,7 +138,7 @@ int clock_timekeeping_set_wall_time(FAR const struct timespec *ts)
g_clock_last_counter = counter;

errout_in_critical_section:
leave_critical_section(flags);
spin_unlock_irqrestore(&g_clock_lock, flags);
return ret;
}

Expand Down Expand Up @@ -188,7 +189,7 @@ int adjtime(FAR const struct timeval *delta, FAR struct timeval *olddelta)
return -1;
}

flags = enter_critical_section();
flags = spin_lock_irqsave(&g_clock_lock);

adjust_usec = delta->tv_sec * USEC_PER_SEC + delta->tv_usec;

Expand All @@ -199,7 +200,7 @@ int adjtime(FAR const struct timeval *delta, FAR struct timeval *olddelta)

g_clock_adjust = adjust_usec;

leave_critical_section(flags);
spin_unlock_irqrestore(&g_clock_lock, flags);

return OK;
}
Expand All @@ -217,7 +218,7 @@ void clock_update_wall_time(void)
time_t sec;
int ret;

flags = enter_critical_section();
flags = spin_lock_irqsave(&g_clock_lock);

ret = up_timer_gettick(&counter);
if (ret < 0)
Expand Down Expand Up @@ -271,7 +272,7 @@ void clock_update_wall_time(void)
g_clock_last_counter = counter;

errout_in_critical_section:
leave_critical_section(flags);
spin_unlock_irqrestore(&g_clock_lock, flags);
}

/****************************************************************************
Expand All @@ -280,6 +281,9 @@ void clock_update_wall_time(void)

void clock_inittimekeeping(FAR const struct timespec *tp)
{
irqstate_t flags;

flags = spin_lock_irqsave(&g_clock_lock);
up_timer_getmask(&g_clock_mask);

if (tp)
Expand All @@ -292,6 +296,7 @@ void clock_inittimekeeping(FAR const struct timespec *tp)
}

up_timer_gettick(&g_clock_last_counter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

spin_unlock_irqrestore(&g_clock_lock, flags);
}

#endif /* CONFIG_CLOCK_TIMEKEEPING */
Loading