[PATCH 0/3] time: some cleanup for jiffies and alarmtimer

[PATCH 0/3] time: some cleanup for jiffies and alarmtimer

[Su Hui]

There are some small cleanup for jiffies.c and alarmtimer.c.
Compile test only.

Su Hui (3):
  time/jiffies: change register_refined_jiffies() to void __init
  alarmtimer: remove dead return value in clock2alarm()
  alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

 include/linux/jiffies.h  |  2 +-
 kernel/time/alarmtimer.c | 90 ++++++++++++++++++----------------------
 kernel/time/jiffies.c    |  5 +--
 3 files changed, 42 insertions(+), 55 deletions(-)

-- 
2.30.2

[PATCH 1/3] time/jiffies: change register_refined_jiffies() to void __init

[Su Hui]

register_refined_jiffies() is only used in setup code and always return 0.
Mark it to __init to save some bytes and change it to void.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 include/linux/jiffies.h | 2 +-
 kernel/time/jiffies.c   | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 0ea8c9887429..91b20788273d 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -59,7 +59,7 @@
 /* LATCH is used in the interval timer and ftape setup. */
 #define LATCH ((CLOCK_TICK_RATE + HZ/2) / HZ)	/* For divider */
 
-extern int register_refined_jiffies(long clock_tick_rate);
+extern void register_refined_jiffies(long clock_tick_rate);
 
 /* TICK_USEC is the time between ticks in usec assuming SHIFTED_HZ */
 #define TICK_USEC ((USEC_PER_SEC + HZ/2) / HZ)
diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
index bc4db9e5ab70..34eeacac2253 100644
--- a/kernel/time/jiffies.c
+++ b/kernel/time/jiffies.c
@@ -75,13 +75,11 @@ struct clocksource * __init __weak clocksource_default_clock(void)
 
 static struct clocksource refined_jiffies;
 
-int register_refined_jiffies(long cycles_per_second)
+void __init register_refined_jiffies(long cycles_per_second)
 {
 	u64 nsec_per_tick, shift_hz;
 	long cycles_per_tick;
 
-
-
 	refined_jiffies = clocksource_jiffies;
 	refined_jiffies.name = "refined-jiffies";
 	refined_jiffies.rating++;
@@ -100,5 +98,4 @@ int register_refined_jiffies(long cycles_per_second)
 	refined_jiffies.mult = ((u32)nsec_per_tick) << JIFFIES_SHIFT;
 
 	__clocksource_register(&refined_jiffies);
-	return 0;
 }
-- 
2.30.2

[PATCH 2/3] alarmtimer: remove dead return value in clock2alarm()

[Su Hui]

'clockid' only can be ALARM_REALTIME and ALARM_BOOTTIME. It's impossible
to return -1 and callers never check the value of -1.

Only alarm_clock_get_timespec(), alarm_clock_get_ktime(),
alarm_timer_create() and alarm_timer_nsleep() call clock2alarm(). These
callers using clockid_to_kclock() to get 'struct k_clock', this ensures
clock2alarm() never returns -1.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 kernel/time/alarmtimer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 0ddccdff119a..e5450a77ada9 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -515,9 +515,9 @@ static enum alarmtimer_type clock2alarm(clockid_t clockid)
 {
 	if (clockid == CLOCK_REALTIME_ALARM)
 		return ALARM_REALTIME;
-	if (clockid == CLOCK_BOOTTIME_ALARM)
-		return ALARM_BOOTTIME;
-	return -1;
+
+	/* CLOCK_BOOTTIME_ALARM case */
+	return ALARM_BOOTTIME;
 }
 
 /**
-- 
2.30.2

[PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[Su Hui]

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

Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[John Stultz]

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

Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[Su Hui]

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

Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[Dan Carpenter]

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.

>  	rtc = alarmtimer_get_rtcdev();
>  	/* If we have no rtcdev, just return */

regards,
dan carpenter

Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[Su Hui]

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

Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

[Thomas Gleixner]

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