linux-kernel - Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANDhNCq0yOXRF+6_JaMYo98o5uKP_af+UXJcJmzuFvX63RdTaA@mail.gmail.com>
Date: Thu, 24 Apr 2025 16:59:53 -0700
From: John Stultz <jstultz@...gle.com>
To: Su Hui <suhui@...china.com>
Cc: tglx@...utronix.de, sboyd@...nel.org, linux-kernel@...r.kernel.org, 
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 3/3] alarmtimer: switch spin_{lock,unlock}_irqsave() to guard()

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