From: Nathan Lynch <nathanl@linux.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
Nicholas Piggin <npiggin@gmail.com>,
Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Tyrel Datwyler <tyreld@linux.ibm.com>,
Nick Child <nnac123@linux.ibm.com>,
Andrew Donnellan <ajd@linux.ibm.com>,
Scott Cheloha <cheloha@linux.ibm.com>,
Laurent Dufour <ldufour@linux.ibm.com>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 8/8] powerpc/rtas: consume retry statuses in sys_rtas()
Date: Thu, 23 Mar 2023 08:40:04 -0500 [thread overview]
Message-ID: <87jzz77fij.fsf@linux.ibm.com> (raw)
In-Reply-To: <87pm8zu7ij.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org> writes:
>> From: Nathan Lynch <nathanl@linux.ibm.com>
>>
>> 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 ...
>
>> 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?
Yes, but if this happens, then there is a serious bug in Linux or
RTAS. The only time I've seen something like that on PowerVM is when
Linux corrupted internal RTAS state by not serializing calls correctly.
rtas_busy_delay_early() has a bail-out heuristic, not for RTAS_BUSY, but
for extended delay statuses (990x), which I suspect happen rarely (if
ever) that early. That's there in order to allow boot to proceed and
hopefully get useful messages out in a truly unexpected circumstance.
That said...
> 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 ?
That probably makes sense. In its current state, I could see
this patch preventing or delaying OS shutdown in situations where it
wouldn't have occurred before.
I think I would want the bailout condition in this case to be
(fatal_signal_pending() && retries > some_threshold), to reduce the
likelihood of non-"stuck" operations from being left unfinished. And it
should dump a stack trace.
next prev parent reply other threads:[~2023-03-23 13:41 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 21:33 [PATCH 0/8] RTAS changes for 6.4 Nathan Lynch via B4 Relay
2023-03-06 21:33 ` Nathan Lynch
2023-03-06 21:33 ` [PATCH 1/8] powerpc/rtas: ensure 8-byte alignment for struct rtas_args Nathan Lynch
2023-03-06 21:33 ` Nathan Lynch via B4 Relay
2023-03-23 4:00 ` Andrew Donnellan
2023-03-06 21:33 ` [PATCH 2/8] powerpc/rtas: use memmove for potentially overlapping buffer copy Nathan Lynch
2023-03-06 21:33 ` Nathan Lynch via B4 Relay
2023-03-23 4:09 ` Andrew Donnellan
2023-03-06 21:33 ` [PATCH 3/8] powerpc/rtas: rtas_call_unlocked() kerneldoc Nathan Lynch via B4 Relay
2023-03-06 21:33 ` Nathan Lynch
2023-03-23 4:15 ` Andrew Donnellan
2023-03-06 21:33 ` [PATCH 4/8] powerpc/rtas: fix miswording in rtas_function kerneldoc Nathan Lynch via B4 Relay
2023-03-06 21:33 ` Nathan Lynch
2023-03-23 0:17 ` Andrew Donnellan
2023-03-06 21:33 ` [PATCH 5/8] powerpc/rtas: rename va_rtas_call_unlocked() to va_rtas_call() Nathan Lynch
2023-03-06 21:33 ` Nathan Lynch via B4 Relay
2023-03-23 4:17 ` Andrew Donnellan
2023-03-23 16:11 ` Nathan Lynch
2023-03-29 12:24 ` Michael Ellerman
2023-03-06 21:33 ` [PATCH 6/8] powerpc/rtas: lockdep annotations Nathan Lynch
2023-03-06 21:33 ` Nathan Lynch via B4 Relay
2023-03-23 6:01 ` Andrew Donnellan
2023-03-06 21:33 ` [PATCH 7/8] powerpc/rtas: warn on unsafe argument to rtas_call_unlocked() Nathan Lynch via B4 Relay
2023-03-06 21:33 ` Nathan Lynch
2023-03-23 4:25 ` Andrew Donnellan
2023-03-23 12:17 ` Nathan Lynch
2023-03-24 0:56 ` Nathan Lynch
2023-03-29 12:20 ` Michael Ellerman
2023-03-29 16:23 ` Nathan Lynch
2023-03-06 21:33 ` [PATCH 8/8] powerpc/rtas: consume retry statuses in sys_rtas() Nathan Lynch via B4 Relay
2023-03-06 21:33 ` Nathan Lynch
2023-03-23 6:26 ` Andrew Donnellan
2023-03-23 19:39 ` Nathan Lynch
2023-03-23 9:44 ` Michael Ellerman
2023-03-23 13:40 ` Nathan Lynch [this message]
2024-01-25 15:55 ` Christophe Leroy
2024-01-25 16:33 ` Nathan Lynch
2024-01-25 16:46 ` Christophe Leroy
2024-01-25 17:23 ` Nathan Lynch
2023-04-06 1:09 ` (subset) [PATCH 0/8] RTAS changes for 6.4 Michael Ellerman
2023-04-26 12:12 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87jzz77fij.fsf@linux.ibm.com \
--to=nathanl@linux.ibm.com \
--cc=ajd@linux.ibm.com \
--cc=cheloha@linux.ibm.com \
--cc=christophe.leroy@csgroup.eu \
--cc=ldufour@linux.ibm.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=nnac123@linux.ibm.com \
--cc=npiggin@gmail.com \
--cc=tyreld@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.