Re: [PATCH 8/8] powerpc/rtas: consume retry statuses in sys_rtas()
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-03-23 09:45:21
Nathan Lynch via B4 Relay [off-list ref] writes:
From: Nathan Lynch <redacted> The kernel can handle retrying RTAS function calls in response to -2/990x in the sys_rtas() handler instead of relaying the intermediate status to user space.
This looks good in general. One query ...
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 47a2aa43d7d4..c330a22ccc70 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c@@ -1798,7 +1798,6 @@ static bool block_rtas_call(int token, int nargs, /* We assume to be passed big endian arguments */ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) { - struct pin_cookie cookie; struct rtas_args args; unsigned long flags; char *buff_copy, *errbuf = NULL;@@ -1866,20 +1865,25 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) buff_copy = get_errorlog_buffer(); - raw_spin_lock_irqsave(&rtas_lock, flags); - cookie = lockdep_pin_lock(&rtas_lock); + do { + struct pin_cookie cookie; - rtas_args = args; - do_enter_rtas(&rtas_args); - args = rtas_args; + raw_spin_lock_irqsave(&rtas_lock, flags); + cookie = lockdep_pin_lock(&rtas_lock); - /* A -1 return code indicates that the last command couldn't - be completed due to a hardware error. */ - if (be32_to_cpu(args.rets[0]) == -1) - errbuf = __fetch_rtas_last_error(buff_copy); + rtas_args = args; + do_enter_rtas(&rtas_args); + args = rtas_args; - lockdep_unpin_lock(&rtas_lock, cookie); - raw_spin_unlock_irqrestore(&rtas_lock, flags); + /* + * Handle error record retrieval before releasing the lock. + */ + if (be32_to_cpu(args.rets[0]) == -1) + errbuf = __fetch_rtas_last_error(buff_copy); + + lockdep_unpin_lock(&rtas_lock, cookie); + raw_spin_unlock_irqrestore(&rtas_lock, flags); + } while (rtas_busy_delay(be32_to_cpu(args.rets[0])));
rtas_busy_delay_early() has the successive_ext_delays case that will break out eventually. But if we keep getting plain RTAS_BUSY back from RTAS I *think* this loop will never terminate? To avoid that, and just as good manners, I think we should have a fatal_signal_pending() check, and if that returns true we bail out of the syscall with -EINTR ? cheers