[Skiboot] [PATCH v7 15/22] fadump: Introduce new reboot type
hegdevasant at linux.vnet.ibm.com
Tue May 14 18:48:20 AEST 2019
On 05/14/2019 01:24 PM, Michael Neuling wrote:
> On Mon, 2019-05-13 at 20:55 +0530, Vasant Hegde wrote:
>> On 05/09/2019 09:08 AM, Michael Neuling wrote:
>>> On Sat, 2019-04-13 at 14:45 +0530, Vasant Hegde wrote:
>>>> Enhance reboot2 call to support FADUMP. Payload will call this interface
>>>> to initiate fadump.
>>>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>>>> core/platform.c | 7 ++++++-
>>>> doc/opal-api/opal-cec-reboot-6-116.rst | 7 +++++++
>>>> include/opal-api.h | 1 +
>>>> 3 files changed, 14 insertions(+), 1 deletion(-)
>>>> diff --git a/core/platform.c b/core/platform.c
>>>> index 570a4309a..5283fce0a 100644
>>>> --- a/core/platform.c
>>>> +++ b/core/platform.c
>>>> @@ -1,4 +1,4 @@
>>>> -/* Copyright 2013-2016 IBM Corp.
>>>> +/* Copyright 2013-2019 IBM Corp.
>>>> * Licensed under the Apache License, Version 2.0 (the "License");
>>>> * you may not use this file except in compliance with the License.
>>>> @@ -103,6 +103,11 @@ static int64_t opal_cec_reboot2(uint32_t reboot_type,
>>>> char *diag)
>>>> case OPAL_REBOOT_FULL_IPL:
>>>> disable_fast_reboot("full IPL reboot requested");
>>>> return opal_cec_reboot();
>>>> + case OPAL_REBOOT_OS_ERROR:
>>>> + prlog(PR_ERR, "Kernel requested for fadump\n");
>> Yep! PR_ERR is not nice. Changed it to PR_NOTICE so that log comes to console.
>>> the error message doesn't match the opal call name... This is a bit
> I'd like the call name and print to be consistent. The print says "kernel
> requested fadump", but it didn't. The kernel actually did a call saying
> The print should be something like "Reboot: OS reported error. Performing dump".
Yes. Makes sense. Will fix it.
>>>> + console_complete_flush();
>>>> + assert(false);
>>> Won't assert generate other prints we don't want?
>> You are right.. May be we should avoid unnecessary back traces. How about this
>> + if (platform.terminate)
>> + platform.terminate("Kernel requested for fadump");
>> + else
>> + assert(false);
>> Note that terminate path checks for MPIPL support and makes appropriate call.
> I think we can just fix all the terminate calls to do the right thing. Then
> maybe could just call _abort() here (which calls .terminate + fallback for us).
Yeah. assert() is bit confusing.
May be ew can just call _abort() here.. it will print opal backtraces to console.
May be that's fine.
> I think the assert() is just confusing me.
> Sorry to be a pain, but this code just seems a bit confusing in something that
> should be easy to make clear.
Agree. if /else condition in unnecessary.
More information about the Skiboot