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@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?
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