There are two code styles for the lock in alarmtimer, guard() and
spin_{lock,unlock}_irqsave(). Switch all these to guard() to make code
neater.
Signed-off-by: Su Hui <suhui@nfschina.com>
---
kernel/time/alarmtimer.c | 84 ++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 47 deletions(-)
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index e5450a77ada9..920a3544d0cd 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -70,12 +70,10 @@ static DEFINE_SPINLOCK(rtcdev_lock);
*/
struct rtc_device *alarmtimer_get_rtcdev(void)
{
- unsigned long flags;
struct rtc_device *ret;
- spin_lock_irqsave(&rtcdev_lock, flags);
- ret = rtcdev;
- spin_unlock_irqrestore(&rtcdev_lock, flags);
+ scoped_guard(spinlock_irqsave, &rtcdev_lock)
+ ret = rtcdev;
return ret;
}
@@ -83,7 +81,6 @@ EXPORT_SYMBOL_GPL(alarmtimer_get_rtcdev);
static int alarmtimer_rtc_add_device(struct device *dev)
{
- unsigned long flags;
struct rtc_device *rtc = to_rtc_device(dev);
struct platform_device *pdev;
int ret = 0;
@@ -101,22 +98,21 @@ static int alarmtimer_rtc_add_device(struct device *dev)
if (!IS_ERR(pdev))
device_init_wakeup(&pdev->dev, true);
- spin_lock_irqsave(&rtcdev_lock, flags);
- if (!IS_ERR(pdev) && !rtcdev) {
- if (!try_module_get(rtc->owner)) {
+ scoped_guard(spinlock_irqsave, &rtcdev_lock) {
+ if (!IS_ERR(pdev) && !rtcdev) {
+ if (!try_module_get(rtc->owner)) {
+ ret = -1;
+ break;
+ }
+
+ rtcdev = rtc;
+ /* hold a reference so it doesn't go away */
+ get_device(dev);
+ pdev = NULL;
+ } else {
ret = -1;
- goto unlock;
}
-
- rtcdev = rtc;
- /* hold a reference so it doesn't go away */
- get_device(dev);
- pdev = NULL;
- } else {
- ret = -1;
}
-unlock:
- spin_unlock_irqrestore(&rtcdev_lock, flags);
platform_device_unregister(pdev);
@@ -198,7 +194,7 @@ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer)
struct alarm *alarm = container_of(timer, struct alarm, timer);
struct alarm_base *base = &alarm_bases[alarm->type];
- scoped_guard (spinlock_irqsave, &base->lock)
+ scoped_guard(spinlock_irqsave, &base->lock)
alarmtimer_dequeue(base, alarm);
if (alarm->function)
@@ -230,15 +226,15 @@ static int alarmtimer_suspend(struct device *dev)
ktime_t min, now, expires;
int i, ret, type;
struct rtc_device *rtc;
- unsigned long flags;
struct rtc_time tm;
- spin_lock_irqsave(&freezer_delta_lock, flags);
- min = freezer_delta;
- expires = freezer_expires;
- type = freezer_alarmtype;
- freezer_delta = 0;
- spin_unlock_irqrestore(&freezer_delta_lock, flags);
+ scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
+ min = freezer_delta;
+ expires = freezer_expires;
+ type = freezer_alarmtype;
+ freezer_delta = 0;
+ }
+
rtc = alarmtimer_get_rtcdev();
/* If we have no rtcdev, just return */
@@ -251,9 +247,8 @@ static int alarmtimer_suspend(struct device *dev)
struct timerqueue_node *next;
ktime_t delta;
- spin_lock_irqsave(&base->lock, flags);
- next = timerqueue_getnext(&base->timerqueue);
- spin_unlock_irqrestore(&base->lock, flags);
+ scoped_guard(spinlock_irqsave, &base->lock)
+ next = timerqueue_getnext(&base->timerqueue);
if (!next)
continue;
delta = ktime_sub(next->expires, base->get_ktime());
@@ -352,13 +347,13 @@ EXPORT_SYMBOL_GPL(alarm_init);
void alarm_start(struct alarm *alarm, ktime_t start)
{
struct alarm_base *base = &alarm_bases[alarm->type];
- unsigned long flags;
- spin_lock_irqsave(&base->lock, flags);
- alarm->node.expires = start;
- alarmtimer_enqueue(base, alarm);
- hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS);
- spin_unlock_irqrestore(&base->lock, flags);
+ scoped_guard(spinlock_irqsave, &base->lock) {
+ alarm->node.expires = start;
+ alarmtimer_enqueue(base, alarm);
+ hrtimer_start(&alarm->timer, alarm->node.expires,
+ HRTIMER_MODE_ABS);
+ }
trace_alarmtimer_start(alarm, base->get_ktime());
}
@@ -381,13 +376,11 @@ EXPORT_SYMBOL_GPL(alarm_start_relative);
void alarm_restart(struct alarm *alarm)
{
struct alarm_base *base = &alarm_bases[alarm->type];
- unsigned long flags;
- spin_lock_irqsave(&base->lock, flags);
+ guard(spinlock_irqsave)(&base->lock);
hrtimer_set_expires(&alarm->timer, alarm->node.expires);
hrtimer_restart(&alarm->timer);
alarmtimer_enqueue(base, alarm);
- spin_unlock_irqrestore(&base->lock, flags);
}
EXPORT_SYMBOL_GPL(alarm_restart);
@@ -401,14 +394,13 @@ EXPORT_SYMBOL_GPL(alarm_restart);
int alarm_try_to_cancel(struct alarm *alarm)
{
struct alarm_base *base = &alarm_bases[alarm->type];
- unsigned long flags;
int ret;
- spin_lock_irqsave(&base->lock, flags);
- ret = hrtimer_try_to_cancel(&alarm->timer);
- if (ret >= 0)
- alarmtimer_dequeue(base, alarm);
- spin_unlock_irqrestore(&base->lock, flags);
+ scoped_guard(spinlock_irqsave, &base->lock) {
+ ret = hrtimer_try_to_cancel(&alarm->timer);
+ if (ret >= 0)
+ alarmtimer_dequeue(base, alarm);
+ }
trace_alarmtimer_cancel(alarm, base->get_ktime());
return ret;
@@ -479,7 +471,6 @@ EXPORT_SYMBOL_GPL(alarm_forward_now);
static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
{
struct alarm_base *base;
- unsigned long flags;
ktime_t delta;
switch(type) {
@@ -498,13 +489,12 @@ static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
delta = ktime_sub(absexp, base->get_ktime());
- spin_lock_irqsave(&freezer_delta_lock, flags);
+ guard(spinlock_irqsave)(&freezer_delta_lock);
if (!freezer_delta || (delta < freezer_delta)) {
freezer_delta = delta;
freezer_expires = absexp;
freezer_alarmtype = type;
}
- spin_unlock_irqrestore(&freezer_delta_lock, flags);
}
/**
--
2.30.2On Thu, Apr 24, 2025 at 10:48:20PM +0800, Su Hui wrote:
> @@ -230,15 +226,15 @@ static int alarmtimer_suspend(struct device *dev)
> ktime_t min, now, expires;
> int i, ret, type;
> struct rtc_device *rtc;
> - unsigned long flags;
> struct rtc_time tm;
>
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - min = freezer_delta;
> - expires = freezer_expires;
> - type = freezer_alarmtype;
> - freezer_delta = 0;
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> + scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
> + min = freezer_delta;
> + expires = freezer_expires;
> + type = freezer_alarmtype;
> + freezer_delta = 0;
> + }
> +
>
Don't add the extra blank line here.
> rtc = alarmtimer_get_rtcdev();
> /* If we have no rtcdev, just return */
regards,
dan carpenter
On 2025/4/25 21:04, Dan Carpenter wrote:
> On Thu, Apr 24, 2025 at 10:48:20PM +0800, Su Hui wrote:
>> @@ -230,15 +226,15 @@ static int alarmtimer_suspend(struct device *dev)
>> ktime_t min, now, expires;
>> int i, ret, type;
>> struct rtc_device *rtc;
>> - unsigned long flags;
>> struct rtc_time tm;
>>
>> - spin_lock_irqsave(&freezer_delta_lock, flags);
>> - min = freezer_delta;
>> - expires = freezer_expires;
>> - type = freezer_alarmtype;
>> - freezer_delta = 0;
>> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
>> + scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
>> + min = freezer_delta;
>> + expires = freezer_expires;
>> + type = freezer_alarmtype;
>> + freezer_delta = 0;
>> + }
>> +
>>
> Don't add the extra blank line here.
Will update in v2 patch, thanks for the suggestion.
Su Hui
On Thu, Apr 24, 2025 at 7:48 AM Su Hui <suhui@nfschina.com> wrote:
>
> There are two code styles for the lock in alarmtimer, guard() and
> spin_{lock,unlock}_irqsave(). Switch all these to guard() to make code
> neater.
>
Thanks for sending this out! A few comments below.
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index e5450a77ada9..920a3544d0cd 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -70,12 +70,10 @@ static DEFINE_SPINLOCK(rtcdev_lock);
> */
> struct rtc_device *alarmtimer_get_rtcdev(void)
> {
> - unsigned long flags;
> struct rtc_device *ret;
>
> - spin_lock_irqsave(&rtcdev_lock, flags);
> - ret = rtcdev;
> - spin_unlock_irqrestore(&rtcdev_lock, flags);
> + scoped_guard(spinlock_irqsave, &rtcdev_lock)
> + ret = rtcdev;
>
> return ret;
This seems like it could be simplified further to just:
{
guard(spinlock_irqsave, &rtcdev_lock);
return rtcdev;
}
No?
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - min = freezer_delta;
> - expires = freezer_expires;
> - type = freezer_alarmtype;
> - freezer_delta = 0;
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> + scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
> + min = freezer_delta;
> + expires = freezer_expires;
> + type = freezer_alarmtype;
> + freezer_delta = 0;
> + }
I'm not necessarily opposed, but I'm not sure we're gaining much here.
> @@ -352,13 +347,13 @@ EXPORT_SYMBOL_GPL(alarm_init);
> void alarm_start(struct alarm *alarm, ktime_t start)
> {
> struct alarm_base *base = &alarm_bases[alarm->type];
> - unsigned long flags;
>
> - spin_lock_irqsave(&base->lock, flags);
> - alarm->node.expires = start;
> - alarmtimer_enqueue(base, alarm);
> - hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS);
> - spin_unlock_irqrestore(&base->lock, flags);
> + scoped_guard(spinlock_irqsave, &base->lock) {
> + alarm->node.expires = start;
> + alarmtimer_enqueue(base, alarm);
> + hrtimer_start(&alarm->timer, alarm->node.expires,
> + HRTIMER_MODE_ABS);
> + }
Similarly, this just seems more like churn, than making the code
particularly more clear.
Overall, there's a few nice cleanups in this one, but there's also
some that I'd probably leave be. I personally don't see
straightforward explicit lock/unlocks as an anti-patern, but the guard
logic definitely helps cleanup some of the uglier goto unlock
patterns, which is a nice benefit. One argument I can see for pushing
to switch even the simple lock/unlock usage, is that having both
models used makes the code less consistent, and adds mental load to
the reader, but there's a lot of complex locking that can't be done
easily with guard() so I don't know if we will ever be able to excise
all the explicit lock/unlock calls, and the extra indentation for
those scoped_guard sections can cause readability problems on its own
as well.
thanks
-john
On Thu, Apr 24 2025 at 16:59, John Stultz wrote:
> On Thu, Apr 24, 2025 at 7:48 AM Su Hui <suhui@nfschina.com> wrote:
>> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
>> + scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
>> + min = freezer_delta;
>> + expires = freezer_expires;
>> + type = freezer_alarmtype;
>> + freezer_delta = 0;
>> + }
>
> I'm not necessarily opposed, but I'm not sure we're gaining much here.
>> @@ -352,13 +347,13 @@ EXPORT_SYMBOL_GPL(alarm_init);
>> void alarm_start(struct alarm *alarm, ktime_t start)
>> {
>> struct alarm_base *base = &alarm_bases[alarm->type];
>> - unsigned long flags;
>>
>> - spin_lock_irqsave(&base->lock, flags);
>> - alarm->node.expires = start;
>> - alarmtimer_enqueue(base, alarm);
>> - hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS);
>> - spin_unlock_irqrestore(&base->lock, flags);
>> + scoped_guard(spinlock_irqsave, &base->lock) {
>> + alarm->node.expires = start;
>> + alarmtimer_enqueue(base, alarm);
>> + hrtimer_start(&alarm->timer, alarm->node.expires,
>> + HRTIMER_MODE_ABS);
>> + }
>
> Similarly, this just seems more like churn, than making the code
> particularly more clear.
I disagree. scoped_guard() is actually superior as it makes it
visually entirely clear what the actual scope of the spin lock protected
code is. That's the whole point.
Especially in alarm_suspend() this would end up with a mix of scoped
guards and open coded spinlock regions. That's obstructing the reading
flow.
I'll bring them back for consistency when applying the series.
Thanks,
tglx
On 2025/4/25 07:59, John Stultz wrote:
> On Thu, Apr 24, 2025 at 7:48 AM Su Hui <suhui@nfschina.com> wrote:
>> There are two code styles for the lock in alarmtimer, guard() and
>> spin_{lock,unlock}_irqsave(). Switch all these to guard() to make code
>> neater.
>>
> Thanks for sending this out! A few comments below.
>
>> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
>> index e5450a77ada9..920a3544d0cd 100644
>> --- a/kernel/time/alarmtimer.c
>> +++ b/kernel/time/alarmtimer.c
>> @@ -70,12 +70,10 @@ static DEFINE_SPINLOCK(rtcdev_lock);
>> */
>> struct rtc_device *alarmtimer_get_rtcdev(void)
>> {
>> - unsigned long flags;
>> struct rtc_device *ret;
>>
>> - spin_lock_irqsave(&rtcdev_lock, flags);
>> - ret = rtcdev;
>> - spin_unlock_irqrestore(&rtcdev_lock, flags);
>> + scoped_guard(spinlock_irqsave, &rtcdev_lock)
>> + ret = rtcdev;
>>
>> return ret;
> This seems like it could be simplified further to just:
> {
> guard(spinlock_irqsave, &rtcdev_lock);
> return rtcdev;
> }
>
> No?
Yes, it's better. I can update this in v2.
>> - spin_lock_irqsave(&freezer_delta_lock, flags);
>> - min = freezer_delta;
>> - expires = freezer_expires;
>> - type = freezer_alarmtype;
>> - freezer_delta = 0;
>> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
>> + scoped_guard(spinlock_irqsave, &freezer_delta_lock) {
>> + min = freezer_delta;
>> + expires = freezer_expires;
>> + type = freezer_alarmtype;
>> + freezer_delta = 0;
>> + }
> I'm not necessarily opposed, but I'm not sure we're gaining much here.
I can remove this in v2.
>
>> @@ -352,13 +347,13 @@ EXPORT_SYMBOL_GPL(alarm_init);
>> void alarm_start(struct alarm *alarm, ktime_t start)
>> {
>> struct alarm_base *base = &alarm_bases[alarm->type];
>> - unsigned long flags;
>>
>> - spin_lock_irqsave(&base->lock, flags);
>> - alarm->node.expires = start;
>> - alarmtimer_enqueue(base, alarm);
>> - hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS);
>> - spin_unlock_irqrestore(&base->lock, flags);
>> + scoped_guard(spinlock_irqsave, &base->lock) {
>> + alarm->node.expires = start;
>> + alarmtimer_enqueue(base, alarm);
>> + hrtimer_start(&alarm->timer, alarm->node.expires,
>> + HRTIMER_MODE_ABS);
>> + }
> Similarly, this just seems more like churn, than making the code
> particularly more clear.
I can remove this in v2 too.
> Overall, there's a few nice cleanups in this one, but there's also
> some that I'd probably leave be. I personally don't see
> straightforward explicit lock/unlocks as an anti-patern, but the guard
> logic definitely helps cleanup some of the uglier goto unlock
> patterns, which is a nice benefit. One argument I can see for pushing
> to switch even the simple lock/unlock usage, is that having both
> models used makes the code less consistent, and adds mental load to
> the reader, but there's a lot of complex locking that can't be done
> easily with guard() so I don't know if we will ever be able to excise
> all the explicit lock/unlock calls, and the extra indentation for
> those scoped_guard sections can cause readability problems on its own
> as well.
Understand, thanks for your suggestions!
I will send a v2 patch to update these points later if there is no more
other
suggestion.
Su Hui
© 2016 - 2026 Red Hat, Inc.