Re: [PATCH] alpha: Use work_on_cpu() for cross-CPU RTC access

On Fri, May 29, 2026 at 1:07 AM Matt Turner <[email protected]> wrote:
>
> smp_call_function_single() runs its callback in IPI (hardirq)
> context. mc146818_set_time() and mc146818_get_time() take rtc_lock
> (spinlock_t), which is a sleeping lock on PREEMPT_RT, triggering
> a lockdep "Invalid wait context" splat on Marvel SMP.
>
> work_on_cpu() runs the callback in a kthread (process) context,
> which can acquire sleeping locks.
>
> Cc: [email protected]
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Matt Turner <[email protected]>
> ---
>  arch/alpha/kernel/rtc.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git ./arch/alpha/kernel/rtc.c ./arch/alpha/kernel/rtc.c
> index cfdf90bc8b3f..4ad5846a1d71 100644
> --- ./arch/alpha/kernel/rtc.c
> +++ ./arch/alpha/kernel/rtc.c
> @@ -15,6 +15,7 @@
>  #include <linux/bcd.h>
>  #include <linux/rtc.h>
>  #include <linux/platform_device.h>
> +#include <linux/workqueue.h>
>
>  #include "proto.h"
>
> @@ -155,11 +156,12 @@ union remote_data {
>         long retval;
>  };
>
> -static void
> +static long
>  do_remote_read(void *data)
>  {
>         union remote_data *x = data;
>         x->retval = alpha_rtc_read_time(NULL, x->tm);
> +       return 0;
>  }
>
>  static int
> @@ -168,17 +170,18 @@ remote_read_time(struct device *dev, struct rtc_time *tm)
>         union remote_data x;
>         if (smp_processor_id() != boot_cpuid) {
>                 x.tm = tm;
> -               smp_call_function_single(boot_cpuid, do_remote_read, &x, 1);
> +               work_on_cpu(boot_cpuid, do_remote_read, &x);
>                 return x.retval;
>         }
>         return alpha_rtc_read_time(NULL, tm);
>  }
>
> -static void
> +static long
>  do_remote_set(void *data)
>  {
>         union remote_data *x = data;
>         x->retval = alpha_rtc_set_time(NULL, x->tm);
> +       return 0;
>  }
>
>  static int
> @@ -187,7 +190,7 @@ remote_set_time(struct device *dev, struct rtc_time *tm)
>         union remote_data x;
>         if (smp_processor_id() != boot_cpuid) {
>                 x.tm = tm;
> -               smp_call_function_single(boot_cpuid, do_remote_set, &x, 1);
> +               work_on_cpu(boot_cpuid, do_remote_set, &x);
>                 return x.retval;
>         }
>         return alpha_rtc_set_time(NULL, tm);
> --
> 2.53.0
>


Hi Matt,

Very impressive works, thanks alot for taking the time to do this!

The overall approach makes sense for RT: smp_call_function_single()
runs the callback from the IPI path, so calling into mc146818 code that
takes rtc_lock is not valid once spinlock_t can sleep.

However, I don't think this should ignore the return value from
work_on_cpu(). work_on_cpu() returns fn(arg), so the callbacks can return
alpha_rtc_{read,set}_time() directly and remote_{read,set}_time() should
return work_on_cpu(...). That also avoids depending on x.retval if
work_on_cpu() itself fails.

Also, now that this path is intentionally process-context/sleepable, the
existing smp_processor_id() direct-call fast path deserves another look.
A task could test that it is on boot_cpuid and then migrate before the
direct alpha_rtc_*() call. If the access must be on boot_cpuid, either
always use work_on_cpu(boot_cpuid, ...) or protect the direct path
appropriately.

So I agree with the overall approach here. I wonder if we could simplify
the conversion by returning the alpha_rtc_{read,set}_time() result directly
from the work_on_cpu() callback and then returning work_on_cpu()
from remote_{read,set}_time(). Also, now that this path is intentionally
process-context/sleepable, do you think the existing smp_processor_id()
fast path is still safe against migration, or should we route the access
through work_on_cpu() unconditionally?

Regards

Magnus