[Skiboot] [PATCH v8 21/24] MPIPL: Invalidate dump

[Vasant Hegde]

On 06/28/2019 04:46 PM, Nicholas Piggin wrote:
> Vasant Hegde's on June 28, 2019 7:48 pm:
>> On 06/28/2019 07:17 AM, Nicholas Piggin wrote:
>>> Vasant Hegde's on June 17, 2019 3:10 am:
>>>> Post dump process, kernel sends FREE_PRESERVE_MEMEORY notification
>>>> to OPAL. OPAL will clear metadata section and tags.
>>>>
>>>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>>>> ---
>>>>    core/opal-dump.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/core/opal-dump.c b/core/opal-dump.c
>>>> index e53991784..555908114 100644
>>>> --- a/core/opal-dump.c
>>>> +++ b/core/opal-dump.c
>>>> @@ -309,6 +309,16 @@ static int64_t opal_mpipl_update(enum mpipl_ops ops,
>>>>    			prlog(PR_NOTICE, "Payload unregistered for MPIPL\n");
>>>>    		break;
>>>>    	case OPAL_MPIPL_FREE_PRESERVED_MEMORY:
>>>> +		/* Clear tags */
>>>> +		memset(&mpipl_tags, 0, (sizeof(u64) * MAX_MPIPL_TAGS));
>>>> +		max_tags = 0;
>>>> +		/* Release memory */
>>>> +		free(mpipl_opal_data);
>>>> +		mpipl_opal_data = NULL;
>>>> +		/* Clear MDRT table */
>>>> +		memset((void *)MDRT_TABLE_BASE, 0, MDRT_TABLE_SIZE);
>>>> +		/* Set MDRT count to max allocated count */
>>>> +		ntuple_mdrt->act_cnt = MDRT_TABLE_SIZE / sizeof(struct mdrt_table);
>>>
>>> Any particular reason you add this here in this patch rather than where
>>> the call type was defined?
>>
>> I have added patch based on dump workflow instead of adding everything in one patch.
>> (Prep work -> OPAL dump registration -> kernel API for registration -> handle
>> assert part ->
>>     trigger dump -> Post processing -> Invalidate dump).
>>
>> I thought this will make it easy for others to understand.
>>
> 
> I don't think it helps. Preferences how to break up patches are
> diverse and there doesn't seem to be a strong agreement, so I don't

Yeah. There is no standard and everyone has their own style.


> pick on it too much. Except that they should bisect and more or less
> result in a working system each step. So returning success from an
> API that is not yet implemented doesn't look right.

I have made sure patches are bisect able.  But you are right. Previous patch 
should throw UNSUPPORTED error instead of returning SUCCESS. Will fix it.

-Vasant