On Thu, Apr 24, 2025 at 7:48 AM Su Hui <suhui@xxxxxxxxxxxx> 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
