[Skiboot] [PATCH v7 15/22] fadump: Introduce new reboot type

[Vasant Hegde]

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");
>>>
>>> PR_INFO
>>
>> 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
>>> confusing
> 
> 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
> OS_ERROR.
> 
> 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
>> one?
>>
>> +               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.

Agree.

> 
> 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.

-Vasant